From 2dcfb4e3b8b07bb2b5419cad6fa324e5e585c3da Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Sat, 8 Jul 2017 16:45:06 -0500 Subject: [PATCH] Send ICE updates immediately. Now that SN changes do not block incoming messages, the caller does not need to enqueue outgoing ICE updates. However this introduces the possibility that the call recipient could recieve an ICE update before their peerConnectionClient is set up - so now we ensure that call service waits for it's peerConnectionClient before processing incoming ICE updates. // FREEBIE --- Signal/src/call/CallService.swift | 166 +++++++++++++++++------------- 1 file changed, 94 insertions(+), 72 deletions(-) diff --git a/Signal/src/call/CallService.swift b/Signal/src/call/CallService.swift index 5bf9c9e0b..a14f2c19d 100644 --- a/Signal/src/call/CallService.swift +++ b/Signal/src/call/CallService.swift @@ -157,19 +157,14 @@ protocol CallServiceObserver: class { } } - /** - * In the process of establishing a connection between the clients (ICE process) we must exchange ICE updates. - * Because this happens via Signal Service it's possible the callee user has not accepted any change in the caller's - * identity. In which case *each* ICE update would cause an "identity change" warning on the callee's device. Since - * this could be several messages, the caller stores all ICE updates until receiving positive confirmation that the - * callee has received a message from us. This positive confirmation comes in the form of the callees `CallAnswer` - * message. - */ - var sendIceUpdatesImmediately = true - var pendingIceUpdateMessages = [OWSCallIceUpdateMessage]() - // Used to coordinate promises across delegate methods - var fulfillCallConnectedPromise: (() -> Void)? + private var fulfillCallConnectedPromise: (() -> Void)? + + // Used by waitForPeerConnectionClient to make sure any received + // ICE messages wait until the peer connection client is set up. + private var fulfillPeerConnectionClientPromise: (() -> Void)? + private var rejectPeerConnectionClientPromise: ((Error) -> Void)? + private var peerConnectionClientPromise: Promise? weak var localVideoTrack: RTCVideoTrack? { didSet { @@ -265,9 +260,6 @@ protocol CallServiceObserver: class { self.call = call - sendIceUpdatesImmediately = false - pendingIceUpdateMessages = [] - let callRecord = TSCall(timestamp: NSDate.ows_millisecondTimeStamp(), withCallNumber: call.remotePhoneNumber, callType: RPRecentCallTypeOutgoingIncomplete, in: call.thread) callRecord.save() call.callRecord = callRecord @@ -290,6 +282,7 @@ protocol CallServiceObserver: class { let peerConnectionClient = PeerConnectionClient(iceServers: iceServers, delegate: self, callDirection: .outgoing, useTurnOnly: useTurnOnly) Logger.debug("\(self.TAG) setting peerConnectionClient in \(#function) for call: \(call.identifiersForLogs)") self.peerConnectionClient = peerConnectionClient + self.fulfillPeerConnectionClientPromise?() return peerConnectionClient.createOffer() }.then { (sessionDescription: HardenedRTCSessionDescription) -> Promise in @@ -356,19 +349,6 @@ protocol CallServiceObserver: class { return } - // Now that we know the recipient trusts our identity, we no longer need to enqueue ICE updates. - self.sendIceUpdatesImmediately = true - - if pendingIceUpdateMessages.count > 0 { - Logger.error("\(self.TAG) Sending \(pendingIceUpdateMessages.count) pendingIceUpdateMessages") - - let callMessage = OWSOutgoingCallMessage(thread: thread, iceUpdateMessages: pendingIceUpdateMessages) - let sendPromise = messageSender.sendCallMessage(callMessage).catch { error in - Logger.error("\(self.TAG) failed to send ice updates in \(#function) with error: \(error)") - } - sendPromise.retainUntilComplete() - } - guard let peerConnectionClient = self.peerConnectionClient else { handleFailedCall(failedCall: call, error: CallError.assertionError(description: "peerConnectionClient was unexpectedly nil in \(#function)")) return @@ -556,6 +536,7 @@ protocol CallServiceObserver: class { Logger.debug("\(self.TAG) setting peerConnectionClient in \(#function) for: \(newCall.identifiersForLogs)") let peerConnectionClient = PeerConnectionClient(iceServers: iceServers, delegate: self, callDirection: .incoming, useTurnOnly: useTurnOnly) self.peerConnectionClient = peerConnectionClient + self.fulfillPeerConnectionClientPromise?() let offerSessionDescription = RTCSessionDescription(type: .offer, sdp: callerSessionDescription) let constraints = RTCMediaConstraints(mandatoryConstraints: nil, optionalConstraints: nil) @@ -616,30 +597,33 @@ protocol CallServiceObserver: class { * Remote client (could be caller or callee) sent us a connectivity update */ public func handleRemoteAddedIceCandidate(thread: TSContactThread, callId: UInt64, sdp: String, lineIndex: Int32, mid: String) { - AssertIsOnMainThread() - Logger.info("\(TAG) called \(#function)") + waitForPeerConnectionClient().then { () -> Void in + AssertIsOnMainThread() - guard let call = self.call else { - Logger.warn("ignoring remote ice update for thread: \(thread.uniqueId) since there is no current call. Call already ended?") - return - } + guard let call = self.call else { + Logger.warn("ignoring remote ice update for thread: \(thread.uniqueId) since there is no current call. Call already ended?") + return + } - guard call.signalingId == callId else { - Logger.warn("ignoring remote ice update for thread: \(thread.uniqueId) due to callId mismatch. Call already ended?") - return - } + guard call.signalingId == callId else { + Logger.warn("ignoring remote ice update for thread: \(thread.uniqueId) due to callId mismatch. Call already ended?") + return + } - guard thread.contactIdentifier() == call.thread.contactIdentifier() else { - Logger.warn("ignoring remote ice update for thread: \(thread.uniqueId) due to thread mismatch. Call already ended?") - return - } + guard thread.contactIdentifier() == call.thread.contactIdentifier() else { + Logger.warn("ignoring remote ice update for thread: \(thread.uniqueId) due to thread mismatch. Call already ended?") + return + } - guard let peerConnectionClient = self.peerConnectionClient else { - Logger.warn("ignoring remote ice update for thread: \(thread.uniqueId) since there is no current peerConnectionClient. Call already ended?") - return - } + guard let peerConnectionClient = self.peerConnectionClient else { + Logger.warn("ignoring remote ice update for thread: \(thread.uniqueId) since there is no current peerConnectionClient. Call already ended?") + return + } - peerConnectionClient.addRemoteIceCandidate(RTCIceCandidate(sdp: sdp, sdpMLineIndex: lineIndex, sdpMid: mid)) + peerConnectionClient.addRemoteIceCandidate(RTCIceCandidate(sdp: sdp, sdpMLineIndex: lineIndex, sdpMid: mid)) + }.catch { error in + Logger.error("\(self.TAG) in \(#function) failed with error: \(error)") + }.retainUntilComplete() } /** @@ -665,19 +649,8 @@ protocol CallServiceObserver: class { let iceUpdateMessage = OWSCallIceUpdateMessage(callId: call.signalingId, sdp: iceCandidate.sdp, sdpMLineIndex: iceCandidate.sdpMLineIndex, sdpMid: iceCandidate.sdpMid) - if self.sendIceUpdatesImmediately { - Logger.info("\(TAG) in \(#function). Sending immediately.") - let callMessage = OWSOutgoingCallMessage(thread: call.thread, iceUpdateMessage: iceUpdateMessage) - let sendPromise = self.messageSender.sendCallMessage(callMessage) - sendPromise.retainUntilComplete() - } else { - // For outgoing messages, we wait to send ice updates until we're sure client received our call message. - // e.g. if the client has blocked our message due to an identity change, we'd otherwise - // bombard them with a bunch *more* undecipherable messages. - Logger.info("\(TAG) in \(#function). Enqueing for later.") - self.pendingIceUpdateMessages.append(iceUpdateMessage) - return - } + let callMessage = OWSOutgoingCallMessage(thread: call.thread, iceUpdateMessage: iceUpdateMessage) + self.messageSender.sendCallMessage(callMessage).retainUntilComplete() } /** @@ -1165,6 +1138,49 @@ protocol CallServiceObserver: class { // MARK: Helpers + private func waitForPeerConnectionClient() -> Promise { + AssertIsOnMainThread() + + guard self.peerConnectionClient == nil else { + // peerConnectionClient already set + return Promise(value: ()) + } + + if self.peerConnectionClientPromise == nil { + createPeerConnectionClientPromise() + } + + guard let peerConnectionClientPromise = self.peerConnectionClientPromise else { + return Promise(error: CallError.assertionError(description: "failed to create peerConnectionClientPromise")) + } + + return peerConnectionClientPromise + } + + private func createPeerConnectionClientPromise() { + AssertIsOnMainThread() + + guard self.peerConnectionClientPromise == nil else { + Logger.error("expected peerConnectionClientPromise to be nil") + return + } + + guard self.fulfillPeerConnectionClientPromise == nil else { + Logger.error("expected fulfillPeerConnectionClientPromise to be nil") + return + } + + guard self.rejectPeerConnectionClientPromise == nil else { + Logger.error("expected rejectPeerConnectionClientPromise to be nil") + return + } + + let (promise, fulfill, reject) = Promise.pending() + self.fulfillPeerConnectionClientPromise = fulfill + self.rejectPeerConnectionClientPromise = reject + self.peerConnectionClientPromise = promise + } + /** * RTCIceServers are used when attempting to establish an optimal connection to the other party. SignalService supplies * a list of servers, plus we have fallback servers hardcoded in the app. @@ -1247,21 +1263,27 @@ protocol CallServiceObserver: class { Logger.debug("\(TAG) in \(#function)") - localVideoTrack = nil - remoteVideoTrack = nil - isRemoteVideoEnabled = false + self.localVideoTrack = nil + self.remoteVideoTrack = nil + self.isRemoteVideoEnabled = false PeerConnectionClient.stopAudioSession() - peerConnectionClient?.terminate() + self.peerConnectionClient?.terminate() Logger.debug("\(TAG) setting peerConnectionClient in \(#function)") - peerConnectionClient = nil - - call?.removeAllObservers() - call = nil - sendIceUpdatesImmediately = true - Logger.info("\(TAG) clearing pendingIceUpdateMessages") - pendingIceUpdateMessages = [] - fulfillCallConnectedPromise = nil + self.peerConnectionClient = nil + + self.call?.removeAllObservers() + self.call = nil + self.fulfillCallConnectedPromise = nil + + // In case we're still waiting on the peer connection setup somewhere, we need to reject it to avoid a memory leak. + // There is no harm in rejecting a previously fulfilled promise. + if let rejectPeerConnectionClientPromise = self.rejectPeerConnectionClientPromise { + rejectPeerConnectionClientPromise(CallError.obsoleteCall(description: "Terminating call")) + } + self.rejectPeerConnectionClientPromise = nil + self.fulfillPeerConnectionClientPromise = nil + self.peerConnectionClientPromise = nil } // MARK: - CallObserver