From a5dac751b087c15730b533f2e4e12e86d3ec5f4e Mon Sep 17 00:00:00 2001 From: David Baker Date: Mon, 13 Nov 2017 17:51:59 +0000 Subject: [PATCH 1/6] BREAKING CHANGE: Remove send_event_error Reverts https://github.com/matrix-org/matrix-js-sdk/pull/378 This swallowed all errors from sendEvent, breaking the ICE candidate retrying. react-sdk no longer listens for send_event_error so I think it's best to just remove this. --- src/webrtc/call.js | 47 +--------------------------------------------- 1 file changed, 1 insertion(+), 46 deletions(-) diff --git a/src/webrtc/call.js b/src/webrtc/call.js index a44e18ad6..33fcfdfc0 100644 --- a/src/webrtc/call.js +++ b/src/webrtc/call.js @@ -24,47 +24,6 @@ const DEBUG = true; // set true to enable console logging. // events: hangup, error(err), replaced(call), state(state, oldState) -/** - * Fires when the MatrixCall encounters an error when sending a Matrix event. - *

- * This is required to allow errors, which occur during sending of events, to bubble up. - * (This is because call.js does a hangup when it encounters a normal `error`, which in - * turn could lead to an UnknownDeviceError.) - *

- * To deal with an UnknownDeviceError when trying to send events, the application should let - * users know that there are new devices in the encrypted room (into which the event was - * sent) and give the user the options to resend unsent events or cancel them. Resending - * is done using {@link module:client~MatrixClient#resendEvent} and cancelling can be done by using - * {@link module:client~MatrixClient#cancelPendingEvent}. - *

- * MatrixCall will not do anything in response to an error that causes `send_event_error` - * to be emitted with the exception of sending `m.call.candidates`, which is retried upon - * failure when ICE candidates are being sent. This happens during call setup. - * - * @event module:webrtc/call~MatrixCall#"send_event_error" - * @param {Error} err The error caught from calling client.sendEvent in call.js. - * @example - * matrixCall.on("send_event_error", function(err){ - * console.error(err); - * }); - */ - -/** - * Fires whenever an error occurs when call.js encounters an issue with setting up the call. - *

- * The error given will have a code equal to either `MatrixCall.ERR_LOCAL_OFFER_FAILED` or - * `MatrixCall.ERR_NO_USER_MEDIA`. `ERR_LOCAL_OFFER_FAILED` is emitted when the local client - * fails to create an offer. `ERR_NO_USER_MEDIA` is emitted when the user has denied access - * to their audio/video hardware. - * - * @event module:webrtc/call~MatrixCall#"error" - * @param {Error} err The error raised by MatrixCall. - * @example - * matrixCall.on("error", function(err){ - * console.error(err.code, err); - * }); - */ - /** * Construct a new Matrix Call. * @constructor @@ -972,11 +931,7 @@ const setState = function(self, state) { * @return {Promise} */ const sendEvent = function(self, eventType, content) { - return self.client.sendEvent(self.roomId, eventType, content).catch( - (err) => { - self.emit('send_event_error', err); - }, - ); + return self.client.sendEvent(self.roomId, eventType, content); }; const sendCandidate = function(self, content) { From 8fcf55d7615e3f1d8e4119a2b423f30b0521ab23 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 15 Nov 2017 10:56:57 +0000 Subject: [PATCH 2/6] BREAKING CHANGE: Fixes for unknown device errors * If we can't send an invite due to unknown devices, abort the call. * Don't transition to the `invite_sent` state until the invite has actually sent. * Add a specific error code for failure due to unknown devices. * Don't send ICE candidate messages if the call has ended. * Add an `event` property to errors from `sendEvent` so that the caller can resend or cancel the event. --- src/client.js | 3 +++ src/webrtc/call.js | 38 +++++++++++++++++++++++++++++++------- 2 files changed, 34 insertions(+), 7 deletions(-) diff --git a/src/client.js b/src/client.js index 1da561e75..c70c1397f 100644 --- a/src/client.js +++ b/src/client.js @@ -1127,6 +1127,9 @@ function _sendEvent(client, room, event, callback) { try { _updatePendingEventStatus(room, event, EventStatus.NOT_SENT); event.error = err; + // also put the event object on the error: the caller will need this + // to resend or cancel the event + err.event = event; if (callback) { callback(err); diff --git a/src/webrtc/call.js b/src/webrtc/call.js index 33fcfdfc0..1a92b6c9a 100644 --- a/src/webrtc/call.js +++ b/src/webrtc/call.js @@ -1,5 +1,6 @@ /* Copyright 2015, 2016 OpenMarket Ltd +Copyright 2017 New Vector Ltd Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -82,6 +83,12 @@ MatrixCall.ERR_LOCAL_OFFER_FAILED = "local_offer_failed"; */ MatrixCall.ERR_NO_USER_MEDIA = "no_user_media"; +/* + * Error code used when a call event failed to send + * because unknown devices were present in the room + */ +MatrixCall.ERR_UNKNOWN_DEVICES = "unknown_devices"; + utils.inherits(MatrixCall, EventEmitter); /** @@ -416,6 +423,8 @@ MatrixCall.prototype._replacedBy = function(newCall) { * @param {boolean} suppressEvent True to suppress emitting an event. */ MatrixCall.prototype.hangup = function(reason, suppressEvent) { + if (this.state == 'ended') return; + debuglog("Ending call " + this.callId); terminate(this, "local", reason, !suppressEvent); const content = { @@ -630,6 +639,9 @@ MatrixCall.prototype._gotLocalIceCandidate = function(event) { "Got local ICE " + event.candidate.sdpMid + " candidate: " + event.candidate.candidate, ); + + if (this.state == 'ended') return; + // As with the offer, note we need to make a copy of this object, not // pass the original: that broke in Chrome ~m43. const c = { @@ -712,14 +724,26 @@ MatrixCall.prototype._gotLocalOffer = function(description) { }, lifetime: MatrixCall.CALL_TIMEOUT_MS, }; - sendEvent(self, 'm.call.invite', content); + sendEvent(self, 'm.call.invite', content).then(() => { + setState(self, 'invite_sent'); + setTimeout(function() { + if (self.state == 'invite_sent') { + self.hangup('invite_timeout'); + } + }, MatrixCall.CALL_TIMEOUT_MS); + }).catch((error) => { + self.client.cancelPendingEvent(error.event); + terminate(self, "local", "unknown_devices", false); + self.emit( + "error", + callError( + MatrixCall.ERR_UNKNOWN_DEVICES, + "Unknown devices present in the room", + ), + ); + throw error; + }); - setTimeout(function() { - if (self.state == 'invite_sent') { - self.hangup('invite_timeout'); - } - }, MatrixCall.CALL_TIMEOUT_MS); - setState(self, 'invite_sent'); }, function() { debuglog("Error setting local description!"); }); From a48a88c312368c2290f04ed1ddbeeb9ccac85ec5 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 15 Nov 2017 17:18:47 +0000 Subject: [PATCH 3/6] Don't send a hangup on user media failure We won't have sent the invite anyway. Also termainate before we fire the error event so the call is 'ended' when the event handlers fire (which means if they try to hang up it's also ignored) --- src/webrtc/call.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/webrtc/call.js b/src/webrtc/call.js index 1a92b6c9a..9f2d8a907 100644 --- a/src/webrtc/call.js +++ b/src/webrtc/call.js @@ -767,6 +767,7 @@ MatrixCall.prototype._getLocalOfferFailed = function(error) { * @param {Object} error */ MatrixCall.prototype._getUserMediaFailed = function(error) { + terminate(this, "local", 'user_media_failed', false); this.emit( "error", callError( @@ -775,7 +776,6 @@ MatrixCall.prototype._getUserMediaFailed = function(error) { "does this app have permission?", ), ); - this.hangup("user_media_failed"); }; /** From 9e2bb5b37bb80a66c7712f0f701e8eefb90b20ee Mon Sep 17 00:00:00 2001 From: David Baker Date: Thu, 16 Nov 2017 16:29:45 +0000 Subject: [PATCH 4/6] Allow answer to be called again after failing * Store the answer we generate so if we fail to send it, we can try to send it again (doing the same again doesn't work as webrtc is in the wrong state). * Don't send ICE candidates if the call is ringing: queue them up so we can send them later if we manage to actually send the answer. --- src/webrtc/call.js | 40 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 37 insertions(+), 3 deletions(-) diff --git a/src/webrtc/call.js b/src/webrtc/call.js index 9f2d8a907..4a1c41b4d 100644 --- a/src/webrtc/call.js +++ b/src/webrtc/call.js @@ -70,6 +70,8 @@ function MatrixCall(opts) { this.mediaPromises = Object.create(null); this.screenSharingStream = null; + + this._answerContent = null; } /** The length of time a call can be ringing for. */ MatrixCall.CALL_TIMEOUT_MS = 60000; @@ -375,6 +377,11 @@ MatrixCall.prototype.answer = function() { debuglog("Answering call %s of type %s", this.callId, this.type); const self = this; + if (self._answerContent) { + self._sendAnswer(); + return; + } + if (!this.localAVStream && !this.waitForLocalAVStream) { this.webRtc.getUserMedia( _getUserMediaVideoContraints(this.type), @@ -561,6 +568,27 @@ MatrixCall.prototype._maybeGotUserMediaForInvite = function(stream) { setState(self, 'create_offer'); }; +MatrixCall.prototype._sendAnswer = function(stream) { + sendEvent(this, 'm.call.answer', this._answerContent).then(() => { + setState(this, 'connecting'); + // If this isn't the first time we've tried to send the answer, + // we may have candidates queued up, so send them now. + _sendCandidateQueue(this); + }).catch((error) => { + // We've failed to answer: back to the ringing state + setState(this, 'ringing'); + this.client.cancelPendingEvent(error.event); + this.emit( + "error", + callError( + MatrixCall.ERR_UNKNOWN_DEVICES, + "Unknown devices present in the room", + ), + ); + throw error; + }); +}; + /** * Internal * @private @@ -609,7 +637,7 @@ MatrixCall.prototype._maybeGotUserMediaForAnswer = function(stream) { self.peerConn.createAnswer(function(description) { debuglog("Created answer: " + description); self.peerConn.setLocalDescription(description, function() { - const content = { + self._answerContent = { version: 0, call_id: self.callId, answer: { @@ -617,8 +645,7 @@ MatrixCall.prototype._maybeGotUserMediaForAnswer = function(stream) { type: self.peerConn.localDescription.type, }, }; - sendEvent(self, 'm.call.answer', content); - setState(self, 'connecting'); + self._sendAnswer(); }, function() { debuglog("Error setting local description!"); }, constraints); @@ -962,6 +989,13 @@ const sendCandidate = function(self, content) { // Sends candidates with are sent in a special way because we try to amalgamate // them into one message self.candidateSendQueue.push(content); + + // Don't send the ICE candidates yet if the call is in the ringing state: this + // means we tried to pick (ie. started generating candidates) and then failed to + // send the answer and went back to the ringing state. Queue up the candidates + // to send if we sucessfully send the answer. + if (self.state == 'ringing') return; + if (self.candidateSendTries === 0) { setTimeout(function() { _sendCandidateQueue(self); From 8bf92d84db0cc1843708f08d6eb57935d536d1c6 Mon Sep 17 00:00:00 2001 From: David Baker Date: Thu, 16 Nov 2017 18:12:09 +0000 Subject: [PATCH 5/6] oops - didn't mean to remove that bit of doc --- src/webrtc/call.js | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/webrtc/call.js b/src/webrtc/call.js index 4a1c41b4d..71272f1d0 100644 --- a/src/webrtc/call.js +++ b/src/webrtc/call.js @@ -25,6 +25,22 @@ const DEBUG = true; // set true to enable console logging. // events: hangup, error(err), replaced(call), state(state, oldState) +/** + * Fires whenever an error occurs when call.js encounters an issue with setting up the call. + *

+ * The error given will have a code equal to either `MatrixCall.ERR_LOCAL_OFFER_FAILED` or + * `MatrixCall.ERR_NO_USER_MEDIA`. `ERR_LOCAL_OFFER_FAILED` is emitted when the local client + * fails to create an offer. `ERR_NO_USER_MEDIA` is emitted when the user has denied access + * to their audio/video hardware. + * + * @event module:webrtc/call~MatrixCall#"error" + * @param {Error} err The error raised by MatrixCall. + * @example + * matrixCall.on("error", function(err){ + * console.error(err.code, err); + * }); + */ + /** * Construct a new Matrix Call. * @constructor From 26e28ed6875d738f11a08bef6d9530c3d3d844d6 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 17 Nov 2017 11:48:56 +0000 Subject: [PATCH 6/6] Don't spuriously send unknown devices error Send a sensible error message for other errors. --- src/webrtc/call.js | 44 +++++++++++++++++++++++++++++--------------- 1 file changed, 29 insertions(+), 15 deletions(-) diff --git a/src/webrtc/call.js b/src/webrtc/call.js index 71272f1d0..0a44eee1e 100644 --- a/src/webrtc/call.js +++ b/src/webrtc/call.js @@ -107,6 +107,18 @@ MatrixCall.ERR_NO_USER_MEDIA = "no_user_media"; */ MatrixCall.ERR_UNKNOWN_DEVICES = "unknown_devices"; +/* + * Error code usewd when we fail to send the invite + * for some reason other than there being unknown devices + */ +MatrixCall.ERR_SEND_INVITE = "send_invite"; + +/* + * Error code usewd when we fail to send the answer + * for some reason other than there being unknown devices + */ +MatrixCall.ERR_SEND_ANSWER = "send_answer"; + utils.inherits(MatrixCall, EventEmitter); /** @@ -594,13 +606,14 @@ MatrixCall.prototype._sendAnswer = function(stream) { // We've failed to answer: back to the ringing state setState(this, 'ringing'); this.client.cancelPendingEvent(error.event); - this.emit( - "error", - callError( - MatrixCall.ERR_UNKNOWN_DEVICES, - "Unknown devices present in the room", - ), - ); + + let code = MatrixCall.ERR_SEND_ANSWER; + let message = "Failed to send answer"; + if (error.name == 'UnknownDeviceError') { + code = MatrixCall.ERR_UNKNOWN_DEVICES; + message = "Unknown devices present in the room"; + } + this.emit("error", callError(code, message)); throw error; }); }; @@ -775,15 +788,16 @@ MatrixCall.prototype._gotLocalOffer = function(description) { } }, MatrixCall.CALL_TIMEOUT_MS); }).catch((error) => { + let code = MatrixCall.ERR_SEND_INVITE; + let message = "Failed to send invite"; + if (error.name == 'UnknownDeviceError') { + code = MatrixCall.ERR_UNKNOWN_DEVICES; + message = "Unknown devices present in the room"; + } + self.client.cancelPendingEvent(error.event); - terminate(self, "local", "unknown_devices", false); - self.emit( - "error", - callError( - MatrixCall.ERR_UNKNOWN_DEVICES, - "Unknown devices present in the room", - ), - ); + terminate(self, "local", code, false); + self.emit("error", callError(code, message)); throw error; });