From ea39b69f65b59ac86e047d21606fcb0c5b0c4a07 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 26 Feb 2021 21:25:52 +0000 Subject: [PATCH] Don't ignore ICE candidates received before offer/answer The main bug here was a race on the callee side because we await-ed on setRemoteDescription before setting the opponent party ID, and while we were await-ing, the callEventHandler could give us candidate events which we'd duly ignore because we thought the party ID didn't match. This also meant that any candidates that arrived before the answer would have been ignored. Save these up by party ID and then add the ones from the party ID that we pick once the answer comes in. Also fix the confusion on party IDs where we weren't sure whether we hadn't picked an opponent or we'd picked an opponent without a party ID. It's now undefined for the former and null for the latter, as it claims to be in the comment. --- spec/unit/webrtc/call.spec.ts | 58 +++++++++++++++ src/webrtc/call.ts | 135 ++++++++++++++++++++++------------ 2 files changed, 145 insertions(+), 48 deletions(-) diff --git a/spec/unit/webrtc/call.spec.ts b/spec/unit/webrtc/call.spec.ts index 06da02502..7336a7371 100644 --- a/spec/unit/webrtc/call.spec.ts +++ b/spec/unit/webrtc/call.spec.ts @@ -190,4 +190,62 @@ describe('Call', function() { // Hangup to stop timers call.hangup(CallErrorCode.UserHangup, true); }); + + it('should add candidates received before answer if party ID is correct', async function() { + await call.placeVoiceCall(); + call.peerConn.addIceCandidate = jest.fn(); + + call.onRemoteIceCandidatesReceived({ + getContent: () => { + return { + version: 1, + call_id: call.callId, + party_id: 'the_correct_party_id', + candidates: [ + { + candidate: 'the_correct_candidate', + sdpMid: '', + }, + ], + }; + }, + }); + + call.onRemoteIceCandidatesReceived({ + getContent: () => { + return { + version: 1, + call_id: call.callId, + party_id: 'some_other_party_id', + candidates: [ + { + candidate: 'the_wrong_candidate', + sdpMid: '', + }, + ], + }; + }, + }); + + expect(call.peerConn.addIceCandidate.mock.calls.length).toBe(0); + + await call.onAnswerReceived({ + getContent: () => { + return { + version: 1, + call_id: call.callId, + party_id: 'the_correct_party_id', + answer: { + sdp: DUMMY_SDP, + }, + }; + }, + }); + + expect(call.peerConn.addIceCandidate.mock.calls.length).toBe(1); + expect(call.peerConn.addIceCandidate).toHaveBeenCalledWith({ + candidate: 'the_correct_candidate', + sdpMid: '', + }); + }); }); diff --git a/src/webrtc/call.ts b/src/webrtc/call.ts index bcca5268d..1eac9eaf5 100644 --- a/src/webrtc/call.ts +++ b/src/webrtc/call.ts @@ -251,8 +251,6 @@ export class MatrixCall extends EventEmitter { private localAVStream: MediaStream; private inviteOrAnswerSent: boolean; private waitForLocalAVStream: boolean; - // XXX: This is either the invite or answer from remote... - private msg: any; // XXX: I don't know why this is called 'config'. private config: MediaStreamConstraints; private successor: MatrixCall; @@ -284,6 +282,11 @@ export class MatrixCall extends EventEmitter { private makingOffer: boolean; private ignoreOffer: boolean; + // If candidates arrive before we've picked an opponent (which, in particular, + // will happen if the opponent sends candidates eagerly before the user answers + // the call) we buffer them up here so we can then add the ones from the party we pick + private remoteCandidateBuffer = new Map(); + constructor(opts: CallOpts) { super(); this.roomId = opts.roomId; @@ -291,9 +294,6 @@ export class MatrixCall extends EventEmitter { this.type = null; this.forceTURN = opts.forceTURN; this.ourPartyId = this.client.deviceId; - // We compare this to null to checks the presence of a party ID: - // make sure it's null, not undefined - this.opponentPartyId = null; // Array of Objects with urls, username, credential keys this.turnServers = opts.turnServers || []; if (this.turnServers.length === 0 && this.client.isFallbackICEServerAllowed()) { @@ -541,11 +541,16 @@ export class MatrixCall extends EventEmitter { * @param {MatrixEvent} event The m.call.invite event */ async initWithInvite(event: MatrixEvent) { - this.msg = event.getContent(); + const invite = event.getContent(); this.direction = CallDirection.Inbound; + this.peerConn = this.createPeerConnection(); + // we must set the party ID before await-ing on anything: the call event + // handler will start giving us more call events (eg. candidates) so if + // we haven't set the party ID, we'll ignore them. + this.chooseOpponent(event); try { - await this.peerConn.setRemoteDescription(this.msg.offer); + await this.peerConn.setRemoteDescription(invite.offer); } catch (e) { logger.debug("Failed to set remote description", e); this.terminate(CallParty.Local, CallErrorCode.SetRemoteDescription, false); @@ -564,13 +569,6 @@ export class MatrixCall extends EventEmitter { this.type = this.remoteStream.getTracks().some(t => t.kind === 'video') ? CallType.Video : CallType.Voice; this.setState(CallState.Ringing); - this.opponentVersion = this.msg.version; - if (this.opponentVersion !== 0) { - // ignore party ID in v0 calls: party ID isn't a thing until v1 - this.opponentPartyId = this.msg.party_id || null; - } - this.opponentCaps = this.msg.capabilities || {}; - this.opponentMember = event.sender; if (event.getLocalAge()) { setTimeout(() => { @@ -584,7 +582,7 @@ export class MatrixCall extends EventEmitter { } this.emit(CallEvent.Hangup); } - }, this.msg.lifetime - event.getLocalAge()); + }, invite.lifetime - event.getLocalAge()); } } @@ -596,7 +594,6 @@ export class MatrixCall extends EventEmitter { // perverse as it may seem, sometimes we want to instantiate a call with a // hangup message (because when getting the state of the room on load, events // come in reverse order and we want to remember that a call has been hung up) - this.msg = event.getContent(); this.setState(CallState.Ended); } @@ -1030,37 +1027,33 @@ export class MatrixCall extends EventEmitter { return; } - if (!this.partyIdMatches(ev.getContent())) { - logger.info( - `Ignoring candidates from party ID ${ev.getContent().party_id}: ` + - `we have chosen party ID ${this.opponentPartyId}`, - ); - return; - } - const cands = ev.getContent().candidates; if (!cands) { logger.info("Ignoring candidates event with no candidates!"); return; } - for (const cand of cands) { - if ( - (cand.sdpMid === null || cand.sdpMid === undefined) && - (cand.sdpMLineIndex === null || cand.sdpMLineIndex === undefined) - ) { - logger.debug("Ignoring remote ICE candidate with no sdpMid or sdpMLineIndex"); - return; - } - logger.debug("Got remote ICE " + cand.sdpMid + " candidate: " + cand.candidate); - try { - this.peerConn.addIceCandidate(cand); - } catch (err) { - if (!this.ignoreOffer) { - logger.info("Failed to add remore ICE candidate", err); - } - } + const fromPartyId = ev.getContent().version === 0 ? null : ev.getContent().party_id || null; + + if (this.opponentPartyId === undefined) { + // we haven't picked an opponent yet so save the candidates + logger.info(`Bufferring ${cands.length} candidates until we pick an opponent`); + const bufferedCands = this.remoteCandidateBuffer.get(fromPartyId) || []; + bufferedCands.push(...cands); + this.remoteCandidateBuffer.set(fromPartyId, bufferedCands); + return; } + + if (!this.partyIdMatches(ev.getContent())) { + logger.info( + `Ignoring candidates from party ID ${ev.getContent().party_id}: ` + + `we have chosen party ID ${this.opponentPartyId}`, + ); + + return; + } + + this.addIceCandidates(cands); } /** @@ -1072,7 +1065,7 @@ export class MatrixCall extends EventEmitter { return; } - if (this.opponentPartyId !== null) { + if (this.opponentPartyId !== undefined) { logger.info( `Ignoring answer from party ID ${event.getContent().party_id}: ` + `we already have an answer/reject from ${this.opponentPartyId}`, @@ -1080,12 +1073,7 @@ export class MatrixCall extends EventEmitter { return; } - this.opponentVersion = event.getContent().version; - if (this.opponentVersion !== 0) { - this.opponentPartyId = event.getContent().party_id || null; - } - this.opponentCaps = event.getContent().capabilities || {}; - this.opponentMember = event.sender; + this.chooseOpponent(event); this.setState(CallState.Connecting); @@ -1702,9 +1690,60 @@ export class MatrixCall extends EventEmitter { private partyIdMatches(msg): boolean { // They must either match or both be absent (in which case opponentPartyId will be null) - const msgPartyId = msg.party_id || null; + // Also we ignore party IDs on the invite/offer if the version is 0, so we must do the same + // here and use null if the version is 0 (woe betide any opponent sending messages in the + // same call with different versions) + const msgPartyId = msg.version === 0 ? null : msg.party_id || null; return msgPartyId === this.opponentPartyId; } + + // Commits to an opponent for the call + // ev: An invite or answer event + private chooseOpponent(ev: MatrixEvent) { + // I choo-choo-choose you + const msg = ev.getContent(); + + this.opponentVersion = msg.version; + if (this.opponentVersion === 0) { + // set to null to indicate that we've chosen an opponent, but because + // they're v0 they have no party ID (even if they sent one, we're ignoring it) + this.opponentPartyId = null; + } else { + // set to their party ID, or if they're naughty and didn't send one despite + // not being v0, set it to null to indicate we picked an opponent with no + // party ID + this.opponentPartyId = msg.party_id || null; + } + this.opponentCaps = msg.capabilities || {}; + this.opponentMember = ev.sender; + + const bufferedCands = this.remoteCandidateBuffer.get(this.opponentPartyId); + if (bufferedCands) { + logger.info(`Adding ${bufferedCands.length} buffered candidates for opponent ${this.opponentPartyId}`); + this.addIceCandidates(bufferedCands); + } + this.remoteCandidateBuffer = null; + } + + private addIceCandidates(cands: RTCIceCandidate[]) { + for (const cand of cands) { + if ( + (cand.sdpMid === null || cand.sdpMid === undefined) && + (cand.sdpMLineIndex === null || cand.sdpMLineIndex === undefined) + ) { + logger.debug("Ignoring remote ICE candidate with no sdpMid or sdpMLineIndex"); + return; + } + logger.debug("Got remote ICE " + cand.sdpMid + " candidate: " + cand.candidate); + try { + this.peerConn.addIceCandidate(cand); + } catch (err) { + if (!this.ignoreOffer) { + logger.info("Failed to add remore ICE candidate", err); + } + } + } + } } function setTracksEnabled(tracks: Array, enabled: boolean) {