From 98caeb6a03b2a7369853525b78626caad37a5aaf Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Fri, 3 Feb 2017 10:25:03 -0500 Subject: [PATCH 1/3] Be even more cautious when tearing down a PeerConnectionClient. // FREEBIE --- Signal/src/call/CallService.swift | 62 +++++++++++++++++----- Signal/src/call/PeerConnectionClient.swift | 5 +- 2 files changed, 51 insertions(+), 16 deletions(-) diff --git a/Signal/src/call/CallService.swift b/Signal/src/call/CallService.swift index f5bfd868f..ee7ec3f80 100644 --- a/Signal/src/call/CallService.swift +++ b/Signal/src/call/CallService.swift @@ -281,6 +281,7 @@ protocol CallServiceObserver: class { peerConnectionClient.createSignalingDataChannel() assert(self.peerConnectionClient == nil, "Unexpected PeerConnectionClient instance") + Logger.debug("\(self.TAG) setting peerConnectionClient in \(#function)") self.peerConnectionClient = peerConnectionClient return self.peerConnectionClient!.createOffer() @@ -428,6 +429,7 @@ protocol CallServiceObserver: class { // FIXME for first time call recipients I think we'll see mic/camera permission requests here, // even though, from the users perspective, no incoming call is yet visible. assert(self.peerConnectionClient == nil, "Unexpected PeerConnectionClient instance") + Logger.debug("\(self.self.TAG) setting peerConnectionClient in \(#function)") self.peerConnectionClient = PeerConnectionClient(iceServers: iceServers, delegate: self) let offerSessionDescription = RTCSessionDescription(type: .offer, sdp: callerSessionDescription) @@ -921,18 +923,28 @@ protocol CallServiceObserver: class { /** * The connection has been established. The clients can now communicate. */ - internal func peerConnectionClientIceConnected(_ peerconnectionClient: PeerConnectionClient) { + internal func peerConnectionClientIceConnected(_ peerConnectionClient: PeerConnectionClient) { AssertIsOnMainThread() + if peerConnectionClient != self.peerConnectionClient { + Logger.debug("\(self.TAG) \(#function) Ignoring event from obsolete peerConnectionClient") + return + } + self.handleIceConnected() } /** * The connection failed to establish. The clients will not be able to communicate. */ - internal func peerConnectionClientIceFailed(_ peerconnectionClient: PeerConnectionClient) { + internal func peerConnectionClientIceFailed(_ peerConnectionClient: PeerConnectionClient) { AssertIsOnMainThread() + if peerConnectionClient != self.peerConnectionClient { + Logger.debug("\(self.TAG) \(#function) Ignoring event from obsolete peerConnectionClient") + return + } + self.handleFailedCall(error: CallError.disconnected) } @@ -941,31 +953,51 @@ protocol CallServiceObserver: class { * reach the local client via the internet. The delegate must shuttle these IceCandates to the other (remote) client * out of band, as part of establishing a connection over WebRTC. */ - internal func peerConnectionClient(_ peerconnectionClient: PeerConnectionClient, addedLocalIceCandidate iceCandidate: RTCIceCandidate) { + internal func peerConnectionClient(_ peerConnectionClient: PeerConnectionClient, addedLocalIceCandidate iceCandidate: RTCIceCandidate) { AssertIsOnMainThread() + if peerConnectionClient != self.peerConnectionClient { + Logger.debug("\(self.TAG) \(#function) Ignoring event from obsolete peerConnectionClient") + return + } + self.handleLocalAddedIceCandidate(iceCandidate) } /** * Once the peerconnection is established, we can receive messages via the data channel, and notify the delegate. */ - internal func peerConnectionClient(_ peerconnectionClient: PeerConnectionClient, received dataChannelMessage: OWSWebRTCProtosData) { + internal func peerConnectionClient(_ peerConnectionClient: PeerConnectionClient, received dataChannelMessage: OWSWebRTCProtosData) { AssertIsOnMainThread() + if peerConnectionClient != self.peerConnectionClient { + Logger.debug("\(self.TAG) \(#function) Ignoring event from obsolete peerConnectionClient") + return + } + self.handleDataChannelMessage(dataChannelMessage) } - internal func peerConnectionClient(_ peerconnectionClient: PeerConnectionClient, didUpdateLocal videoTrack: RTCVideoTrack?) { + internal func peerConnectionClient(_ peerConnectionClient: PeerConnectionClient, didUpdateLocal videoTrack: RTCVideoTrack?) { AssertIsOnMainThread() + if peerConnectionClient != self.peerConnectionClient { + Logger.debug("\(self.TAG) \(#function) Ignoring event from obsolete peerConnectionClient") + return + } + self.localVideoTrack = videoTrack self.fireDidUpdateVideoTracks() } - internal func peerConnectionClient(_ peerconnectionClient: PeerConnectionClient, didUpdateRemote videoTrack: RTCVideoTrack?) { + internal func peerConnectionClient(_ peerConnectionClient: PeerConnectionClient, didUpdateRemote videoTrack: RTCVideoTrack?) { AssertIsOnMainThread() + if peerConnectionClient != self.peerConnectionClient { + Logger.debug("\(self.TAG) \(#function) Ignoring event from obsolete peerConnectionClient") + return + } + self.remoteVideoTrack = videoTrack self.fireDidUpdateVideoTracks() } @@ -1028,21 +1060,27 @@ protocol CallServiceObserver: class { Logger.debug("\(TAG) in \(#function)") - PeerConnectionClient.stopAudioSession() - peerConnectionClient?.terminate() - - peerConnectionClient = nil + self.localVideoTrack = nil + self.remoteVideoTrack = nil + self.fireDidUpdateVideoTracks() localVideoTrack = nil remoteVideoTrack = nil isRemoteVideoEnabled = false + + var peerConnectionClientCopy = peerConnectionClient + Logger.debug("\(TAG) setting peerConnectionClient in \(#function)") + peerConnectionClient = nil + + PeerConnectionClient.stopAudioSession() + peerConnectionClientCopy?.terminate() + peerConnectionClientCopy = nil + call?.removeAllObservers() call = nil thread = nil incomingCallPromise = nil sendIceUpdatesImmediately = true pendingIceUpdateMessages = [] - - fireDidUpdateVideoTracks() } // MARK: - CallObserver diff --git a/Signal/src/call/PeerConnectionClient.swift b/Signal/src/call/PeerConnectionClient.swift index 405798fcd..1a6d3dcdc 100644 --- a/Signal/src/call/PeerConnectionClient.swift +++ b/Signal/src/call/PeerConnectionClient.swift @@ -423,10 +423,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD AssertIsOnMainThread() Logger.debug("\(TAG) in \(#function)") - delegate.peerConnectionClient(self, didUpdateLocal: nil) - delegate.peerConnectionClient(self, didUpdateRemote: nil) - - PeerConnectionClient.signalingQueue.async { + PeerConnectionClient.signalingQueue.sync { assert(self.peerConnection != nil) self.terminateInternal() } From 732144c9edd318b012ec9c8380fac43b65aac2b4 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Fri, 3 Feb 2017 10:51:40 -0500 Subject: [PATCH 2/3] Respond to CR. // FREEBIE --- Signal/src/call/CallService.swift | 48 +++++++++++++++++-------------- 1 file changed, 27 insertions(+), 21 deletions(-) diff --git a/Signal/src/call/CallService.swift b/Signal/src/call/CallService.swift index ee7ec3f80..d32535f0c 100644 --- a/Signal/src/call/CallService.swift +++ b/Signal/src/call/CallService.swift @@ -271,7 +271,7 @@ protocol CallServiceObserver: class { return Promise(error: CallError.assertionError(description: errorDescription)) } - return getIceServers().then() { iceServers -> Promise in + return getIceServers().then { iceServers -> Promise in Logger.debug("\(self.TAG) got ice servers:\(iceServers)") let peerConnectionClient = PeerConnectionClient(iceServers: iceServers, delegate: self) @@ -285,13 +285,13 @@ protocol CallServiceObserver: class { self.peerConnectionClient = peerConnectionClient return self.peerConnectionClient!.createOffer() - }.then() { (sessionDescription: HardenedRTCSessionDescription) -> Promise in - return self.peerConnectionClient!.setLocalSessionDescription(sessionDescription).then() { + }.then { (sessionDescription: HardenedRTCSessionDescription) -> Promise in + return self.peerConnectionClient!.setLocalSessionDescription(sessionDescription).then { let offerMessage = OWSCallOfferMessage(callId: call.signalingId, sessionDescription: sessionDescription.sdp) let callMessage = OWSOutgoingCallMessage(thread: thread, offerMessage: offerMessage) return self.messageSender.sendCallMessage(callMessage) } - }.catch() { error in + }.catch { error in Logger.error("\(self.TAG) placing call failed with error: \(error)") if let callError = error as? CallError { @@ -339,7 +339,7 @@ protocol CallServiceObserver: class { let sessionDescription = RTCSessionDescription(type: .answer, sdp: sessionDescription) _ = peerConnectionClient.setRemoteSessionDescription(sessionDescription).then { Logger.debug("\(self.TAG) successfully set remote description") - }.catch() { error in + }.catch { error in if let callError = error as? CallError { self.handleFailedCall(error: callError) } else { @@ -425,7 +425,7 @@ protocol CallServiceObserver: class { incomingCallPromise = firstly { return getIceServers() - }.then() { (iceServers: [RTCIceServer]) -> Promise in + }.then { (iceServers: [RTCIceServer]) -> Promise in // FIXME for first time call recipients I think we'll see mic/camera permission requests here, // even though, from the users perspective, no incoming call is yet visible. assert(self.peerConnectionClient == nil, "Unexpected PeerConnectionClient instance") @@ -437,14 +437,14 @@ protocol CallServiceObserver: class { // Find a sessionDescription compatible with my constraints and the remote sessionDescription return self.peerConnectionClient!.negotiateSessionDescription(remoteDescription: offerSessionDescription, constraints: constraints) - }.then() { (negotiatedSessionDescription: HardenedRTCSessionDescription) in + }.then { (negotiatedSessionDescription: HardenedRTCSessionDescription) in Logger.debug("\(self.TAG) set the remote description") let answerMessage = OWSCallAnswerMessage(callId: newCall.signalingId, sessionDescription: negotiatedSessionDescription.sdp) let callAnswerMessage = OWSOutgoingCallMessage(thread: thread, answerMessage: answerMessage) return self.messageSender.sendCallMessage(callAnswerMessage) - }.then() { + }.then { Logger.debug("\(self.TAG) successfully sent callAnswerMessage") let (promise, fulfill, _) = Promise.pending() @@ -458,7 +458,7 @@ protocol CallServiceObserver: class { self.fulfillCallConnectedPromise = fulfill return race(promise, timeout) - }.catch() { error in + }.catch { error in if let callError = error as? CallError { self.handleFailedCall(error: callError) } else { @@ -773,9 +773,9 @@ protocol CallServiceObserver: class { // If the call hasn't started yet, we don't have a data channel to communicate the hang up. Use Signal Service Message. let hangupMessage = OWSCallHangupMessage(callId: call.signalingId) let callMessage = OWSOutgoingCallMessage(thread: thread, hangupMessage: hangupMessage) - _ = self.messageSender.sendCallMessage(callMessage).then() { + _ = self.messageSender.sendCallMessage(callMessage).then { Logger.debug("\(self.TAG) successfully sent hangup call message to \(thread)") - }.catch() { error in + }.catch { error in Logger.error("\(self.TAG) failed to send hangup call message to \(thread) with error: \(error)") } @@ -926,7 +926,7 @@ protocol CallServiceObserver: class { internal func peerConnectionClientIceConnected(_ peerConnectionClient: PeerConnectionClient) { AssertIsOnMainThread() - if peerConnectionClient != self.peerConnectionClient { + guard peerConnectionClient == self.peerConnectionClient else { Logger.debug("\(self.TAG) \(#function) Ignoring event from obsolete peerConnectionClient") return } @@ -940,7 +940,7 @@ protocol CallServiceObserver: class { internal func peerConnectionClientIceFailed(_ peerConnectionClient: PeerConnectionClient) { AssertIsOnMainThread() - if peerConnectionClient != self.peerConnectionClient { + guard peerConnectionClient == self.peerConnectionClient else { Logger.debug("\(self.TAG) \(#function) Ignoring event from obsolete peerConnectionClient") return } @@ -956,7 +956,7 @@ protocol CallServiceObserver: class { internal func peerConnectionClient(_ peerConnectionClient: PeerConnectionClient, addedLocalIceCandidate iceCandidate: RTCIceCandidate) { AssertIsOnMainThread() - if peerConnectionClient != self.peerConnectionClient { + guard peerConnectionClient == self.peerConnectionClient else { Logger.debug("\(self.TAG) \(#function) Ignoring event from obsolete peerConnectionClient") return } @@ -970,7 +970,7 @@ protocol CallServiceObserver: class { internal func peerConnectionClient(_ peerConnectionClient: PeerConnectionClient, received dataChannelMessage: OWSWebRTCProtosData) { AssertIsOnMainThread() - if peerConnectionClient != self.peerConnectionClient { + guard peerConnectionClient == self.peerConnectionClient else { Logger.debug("\(self.TAG) \(#function) Ignoring event from obsolete peerConnectionClient") return } @@ -981,7 +981,7 @@ protocol CallServiceObserver: class { internal func peerConnectionClient(_ peerConnectionClient: PeerConnectionClient, didUpdateLocal videoTrack: RTCVideoTrack?) { AssertIsOnMainThread() - if peerConnectionClient != self.peerConnectionClient { + guard peerConnectionClient == self.peerConnectionClient else { Logger.debug("\(self.TAG) \(#function) Ignoring event from obsolete peerConnectionClient") return } @@ -993,7 +993,7 @@ protocol CallServiceObserver: class { internal func peerConnectionClient(_ peerConnectionClient: PeerConnectionClient, didUpdateRemote videoTrack: RTCVideoTrack?) { AssertIsOnMainThread() - if peerConnectionClient != self.peerConnectionClient { + guard peerConnectionClient == self.peerConnectionClient else { Logger.debug("\(self.TAG) \(#function) Ignoring event from obsolete peerConnectionClient") return } @@ -1013,7 +1013,7 @@ protocol CallServiceObserver: class { return firstly { return accountManager.getTurnServerInfo() - }.then() { turnServerInfo -> [RTCIceServer] in + }.then { turnServerInfo -> [RTCIceServer] in Logger.debug("\(self.TAG) got turn server urls: \(turnServerInfo.urls)") return turnServerInfo.urls.map { url in @@ -1060,13 +1060,19 @@ protocol CallServiceObserver: class { Logger.debug("\(TAG) in \(#function)") - self.localVideoTrack = nil - self.remoteVideoTrack = nil - self.fireDidUpdateVideoTracks() localVideoTrack = nil remoteVideoTrack = nil isRemoteVideoEnabled = false + // We make a local copy of peerConnectionClient and then clear + // the peerConnectionClient property immediately. This class' + // PeerConnectionClientDelegate methods exit early if their + // peerConnectionClient argument doesn't match the peerConnectionClient + // property. + // + // In practice this won't matter, since PeerConnectionClient invokes + // its delegate methods asynchronously, but we don't want to bake that + // assumption into this logic. var peerConnectionClientCopy = peerConnectionClient Logger.debug("\(TAG) setting peerConnectionClient in \(#function)") peerConnectionClient = nil From 6e390d40b75996bff9141a3774a6f66adb301bdd Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Fri, 3 Feb 2017 11:29:40 -0500 Subject: [PATCH 3/3] Respond to CR. // FREEBIE --- Signal/src/call/CallService.swift | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/Signal/src/call/CallService.swift b/Signal/src/call/CallService.swift index d32535f0c..1471ca2d0 100644 --- a/Signal/src/call/CallService.swift +++ b/Signal/src/call/CallService.swift @@ -1064,23 +1064,11 @@ protocol CallServiceObserver: class { remoteVideoTrack = nil isRemoteVideoEnabled = false - // We make a local copy of peerConnectionClient and then clear - // the peerConnectionClient property immediately. This class' - // PeerConnectionClientDelegate methods exit early if their - // peerConnectionClient argument doesn't match the peerConnectionClient - // property. - // - // In practice this won't matter, since PeerConnectionClient invokes - // its delegate methods asynchronously, but we don't want to bake that - // assumption into this logic. - var peerConnectionClientCopy = peerConnectionClient + PeerConnectionClient.stopAudioSession() + peerConnectionClient?.terminate() Logger.debug("\(TAG) setting peerConnectionClient in \(#function)") peerConnectionClient = nil - PeerConnectionClient.stopAudioSession() - peerConnectionClientCopy?.terminate() - peerConnectionClientCopy = nil - call?.removeAllObservers() call = nil thread = nil