From d9bcd563b19ca2e8b69c7729290d2d2007eaa9e4 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Fri, 3 Feb 2017 16:54:34 -0500 Subject: [PATCH 1/4] Avoid possible deadlock in PeerConnectionClient. // FREEBIE --- Signal/src/call/CallService.swift | 8 ++---- Signal/src/call/PeerConnectionClient.swift | 27 ++++++++++++------- .../test/call/PeerConnectionClientTest.swift | 3 +-- 3 files changed, 21 insertions(+), 17 deletions(-) diff --git a/Signal/src/call/CallService.swift b/Signal/src/call/CallService.swift index 0cf4cb6a5..1ef4cb966 100644 --- a/Signal/src/call/CallService.swift +++ b/Signal/src/call/CallService.swift @@ -283,11 +283,7 @@ protocol CallServiceObserver: class { return getIceServers().then { iceServers -> Promise in Logger.debug("\(self.TAG) got ice servers:\(iceServers)") - let peerConnectionClient = PeerConnectionClient(iceServers: iceServers, delegate: self) - - // When placing an outgoing call, it's our responsibility to create the DataChannel. Recipient will not have - // to do this explicitly. - peerConnectionClient.createSignalingDataChannel() + let peerConnectionClient = PeerConnectionClient(iceServers: iceServers, delegate: self, callType: .Outgoing) assert(self.peerConnectionClient == nil, "Unexpected PeerConnectionClient instance") Logger.debug("\(self.TAG) setting peerConnectionClient in \(#function)") @@ -439,7 +435,7 @@ protocol CallServiceObserver: class { // 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) + self.peerConnectionClient = PeerConnectionClient(iceServers: iceServers, delegate: self, callType: .Incoming) let offerSessionDescription = RTCSessionDescription(type: .offer, sdp: callerSessionDescription) let constraints = RTCMediaConstraints(mandatoryConstraints: nil, optionalConstraints: nil) diff --git a/Signal/src/call/PeerConnectionClient.swift b/Signal/src/call/PeerConnectionClient.swift index 1a6d3dcdc..8dc63b664 100644 --- a/Signal/src/call/PeerConnectionClient.swift +++ b/Signal/src/call/PeerConnectionClient.swift @@ -64,6 +64,11 @@ protocol PeerConnectionClientDelegate: class { */ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelDelegate { + enum CallType { + case Incoming + case Outgoing + } + let TAG = "[PeerConnectionClient]" enum Identifiers: String { case mediaStream = "ARDAMS", @@ -113,7 +118,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD private var remoteVideoTrack: RTCVideoTrack? private var cameraConstraints: RTCMediaConstraints - init(iceServers: [RTCIceServer], delegate: PeerConnectionClientDelegate) { + init(iceServers: [RTCIceServer], delegate: PeerConnectionClientDelegate, callType: CallType) { AssertIsOnMainThread() self.iceServers = iceServers @@ -139,21 +144,25 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD delegate: self) createAudioSender() createVideoSender() + + if callType == .Outgoing { + // When placing an outgoing call, it's our responsibility to create the DataChannel. + // Recipient will not have to do this explicitly. + createSignalingDataChannel() + } } // MARK: - Media Streams - public func createSignalingDataChannel() { + fileprivate func createSignalingDataChannel() { AssertIsOnMainThread() - PeerConnectionClient.signalingQueue.sync { - let dataChannel = peerConnection.dataChannel(forLabel: Identifiers.dataChannelSignaling.rawValue, - configuration: RTCDataChannelConfiguration()) - dataChannel.delegate = self + let dataChannel = peerConnection.dataChannel(forLabel: Identifiers.dataChannelSignaling.rawValue, + configuration: RTCDataChannelConfiguration()) + dataChannel.delegate = self - assert(self.dataChannel == nil) - self.dataChannel = dataChannel - } + assert(self.dataChannel == nil) + self.dataChannel = dataChannel } // MARK: Video diff --git a/Signal/test/call/PeerConnectionClientTest.swift b/Signal/test/call/PeerConnectionClientTest.swift index 85e615e1f..d9887915a 100644 --- a/Signal/test/call/PeerConnectionClientTest.swift +++ b/Signal/test/call/PeerConnectionClientTest.swift @@ -55,9 +55,8 @@ class PeerConnectionClientTest: XCTestCase { let iceServers = [RTCIceServer]() clientDelegate = FakePeerConnectionClientDelegate() - client = PeerConnectionClient(iceServers: iceServers, delegate: clientDelegate) + client = PeerConnectionClient(iceServers: iceServers, delegate: clientDelegate, callType: .Outgoing) peerConnection = client.peerConnectionForTests() - client.createSignalingDataChannel() dataChannel = client.dataChannelForTests() } From 4ae786d0a21682732c815d279659d4fb144e51bb Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Fri, 3 Feb 2017 20:17:42 -0500 Subject: [PATCH 2/4] Ignore CallService events related to obsolete calls. // FREEBIE --- Signal/src/call/CallService.swift | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/Signal/src/call/CallService.swift b/Signal/src/call/CallService.swift index 1ef4cb966..2694607d3 100644 --- a/Signal/src/call/CallService.swift +++ b/Signal/src/call/CallService.swift @@ -75,6 +75,7 @@ enum CallError: Error { case disconnected case externalError(underlyingError: Error) case timeout(description: String) + case unexpected(description: String) } // FIXME TODO do we need to timeout? @@ -123,7 +124,7 @@ protocol CallServiceObserver: class { didSet { AssertIsOnMainThread() - Logger.debug("\(self.TAG) .peerConnectionClient setter: \(oldValue != nil) -> \(peerConnectionClient != nil)") + Logger.debug("\(self.TAG) .peerConnectionClient setter: \(oldValue != nil) -> \(peerConnectionClient != nil) \(peerConnectionClient)") } } @@ -138,6 +139,8 @@ protocol CallServiceObserver: class { updateIsVideoEnabled() + Logger.debug("\(self.TAG) .call setter: \(oldValue != nil) -> \(call != nil) \(call)") + for observer in observers { observer.value?.didUpdateCall(call:call) } @@ -424,6 +427,9 @@ protocol CallServiceObserver: class { let backgroundTask = UIApplication.shared.beginBackgroundTask { let timeout = CallError.timeout(description: "background task time ran out before call connected.") DispatchQueue.main.async { + guard self.call == newCall else { + return + } self.handleFailedCall(error: timeout) } } @@ -433,6 +439,9 @@ protocol CallServiceObserver: class { }.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. + guard self.call == newCall else { + throw CallError.unexpected(description: "getIceServers() response for obsolete call") + } assert(self.peerConnectionClient == nil, "Unexpected PeerConnectionClient instance") Logger.debug("\(self.self.TAG) setting peerConnectionClient in \(#function)") self.peerConnectionClient = PeerConnectionClient(iceServers: iceServers, delegate: self, callType: .Incoming) @@ -443,6 +452,9 @@ 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 + guard self.call == newCall else { + throw CallError.unexpected(description: "negotiateSessionDescription() response for obsolete call") + } Logger.debug("\(self.TAG) set the remote description") let answerMessage = OWSCallAnswerMessage(callId: newCall.signalingId, sessionDescription: negotiatedSessionDescription.sdp) @@ -450,6 +462,9 @@ protocol CallServiceObserver: class { return self.messageSender.sendCallMessage(callAnswerMessage) }.then { + guard self.call == newCall else { + throw CallError.unexpected(description: "sendCallMessage() response for obsolete call") + } Logger.debug("\(self.TAG) successfully sent callAnswerMessage") let (promise, fulfill, _) = Promise.pending() @@ -464,6 +479,10 @@ protocol CallServiceObserver: class { return race(promise, timeout) }.catch { error in + guard self.call == newCall else { + Logger.debug("\(self.TAG) error for obsolete call: \(error)") + return + } if let callError = error as? CallError { self.handleFailedCall(error: callError) } else { From 6f3a45ff8c83005bfdec5e3f20b443e600c9c878 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Fri, 3 Feb 2017 20:43:35 -0500 Subject: [PATCH 3/4] Avoid crashes when deallocating remote video tracks. // FREEBIE --- Signal/src/call/CallService.swift | 2 -- Signal/src/call/PeerConnectionClient.swift | 23 +++++++++++----------- 2 files changed, 12 insertions(+), 13 deletions(-) diff --git a/Signal/src/call/CallService.swift b/Signal/src/call/CallService.swift index 2694607d3..bedf912b6 100644 --- a/Signal/src/call/CallService.swift +++ b/Signal/src/call/CallService.swift @@ -1011,7 +1011,6 @@ protocol CallServiceObserver: class { } self.localVideoTrack = videoTrack - self.fireDidUpdateVideoTracks() } internal func peerConnectionClient(_ peerConnectionClient: PeerConnectionClient, didUpdateRemote videoTrack: RTCVideoTrack?) { @@ -1023,7 +1022,6 @@ protocol CallServiceObserver: class { } self.remoteVideoTrack = videoTrack - self.fireDidUpdateVideoTracks() } // MARK: Helpers diff --git a/Signal/src/call/PeerConnectionClient.swift b/Signal/src/call/PeerConnectionClient.swift index 8dc63b664..a89d4d795 100644 --- a/Signal/src/call/PeerConnectionClient.swift +++ b/Signal/src/call/PeerConnectionClient.swift @@ -115,7 +115,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD private var videoSender: RTCRtpSender? private var localVideoTrack: RTCVideoTrack? - private var remoteVideoTrack: RTCVideoTrack? + private weak var remoteVideoTrack: RTCVideoTrack? private var cameraConstraints: RTCMediaConstraints init(iceServers: [RTCIceServer], delegate: PeerConnectionClientDelegate, callType: CallType) { @@ -545,22 +545,23 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD /** Called when media is received on a new stream from remote peer. */ internal func peerConnection(_ peerConnection: RTCPeerConnection, didAdd stream: RTCMediaStream) { + guard stream.videoTracks.count > 0 else { + return + } + weak var remoteVideoTrack = stream.videoTracks[0] + Logger.debug("\(self.TAG) didAdd stream:\(stream) video tracks: \(stream.videoTracks.count) audio tracks: \(stream.audioTracks.count)") + PeerConnectionClient.signalingQueue.async { guard self.peerConnection != nil else { Logger.debug("\(self.TAG) \(#function) Ignoring obsolete event in terminated client") return } - Logger.debug("\(self.TAG) didAdd stream:\(stream) video tracks: \(stream.videoTracks.count) audio tracks: \(stream.audioTracks.count)") - if stream.videoTracks.count > 0 { - self.remoteVideoTrack = stream.videoTracks[0] - if let delegate = self.delegate { - let remoteVideoTrack = self.remoteVideoTrack - DispatchQueue.main.async { [weak self, weak remoteVideoTrack] in - guard let strongSelf = self else { return } - guard let strongRemoteVideoTrack = remoteVideoTrack else { return } - delegate.peerConnectionClient(strongSelf, didUpdateRemote: strongRemoteVideoTrack) - } + self.remoteVideoTrack = remoteVideoTrack + if let delegate = self.delegate { + DispatchQueue.main.async { [weak self] in + guard let strongSelf = self else { return } + delegate.peerConnectionClient(strongSelf, didUpdateRemote: remoteVideoTrack) } } } From ca76ec6f36ddcf0a6343f92e488f35f4028f31c9 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Mon, 6 Feb 2017 09:39:14 -0500 Subject: [PATCH 4/4] Respond to CR. // FREEBIE --- Signal/src/call/CallService.swift | 11 +++++------ Signal/src/call/PeerConnectionClient.swift | 10 +++++----- Signal/test/call/PeerConnectionClientTest.swift | 2 +- 3 files changed, 11 insertions(+), 12 deletions(-) diff --git a/Signal/src/call/CallService.swift b/Signal/src/call/CallService.swift index bedf912b6..5f39c0d72 100644 --- a/Signal/src/call/CallService.swift +++ b/Signal/src/call/CallService.swift @@ -75,7 +75,6 @@ enum CallError: Error { case disconnected case externalError(underlyingError: Error) case timeout(description: String) - case unexpected(description: String) } // FIXME TODO do we need to timeout? @@ -286,7 +285,7 @@ protocol CallServiceObserver: class { return getIceServers().then { iceServers -> Promise in Logger.debug("\(self.TAG) got ice servers:\(iceServers)") - let peerConnectionClient = PeerConnectionClient(iceServers: iceServers, delegate: self, callType: .Outgoing) + let peerConnectionClient = PeerConnectionClient(iceServers: iceServers, delegate: self, callType: .outgoing) assert(self.peerConnectionClient == nil, "Unexpected PeerConnectionClient instance") Logger.debug("\(self.TAG) setting peerConnectionClient in \(#function)") @@ -440,11 +439,11 @@ 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. guard self.call == newCall else { - throw CallError.unexpected(description: "getIceServers() response for obsolete call") + throw CallError.assertionError(description: "getIceServers() response for obsolete call") } assert(self.peerConnectionClient == nil, "Unexpected PeerConnectionClient instance") Logger.debug("\(self.self.TAG) setting peerConnectionClient in \(#function)") - self.peerConnectionClient = PeerConnectionClient(iceServers: iceServers, delegate: self, callType: .Incoming) + self.peerConnectionClient = PeerConnectionClient(iceServers: iceServers, delegate: self, callType: .incoming) let offerSessionDescription = RTCSessionDescription(type: .offer, sdp: callerSessionDescription) let constraints = RTCMediaConstraints(mandatoryConstraints: nil, optionalConstraints: nil) @@ -453,7 +452,7 @@ protocol CallServiceObserver: class { return self.peerConnectionClient!.negotiateSessionDescription(remoteDescription: offerSessionDescription, constraints: constraints) }.then { (negotiatedSessionDescription: HardenedRTCSessionDescription) in guard self.call == newCall else { - throw CallError.unexpected(description: "negotiateSessionDescription() response for obsolete call") + throw CallError.assertionError(description: "negotiateSessionDescription() response for obsolete call") } Logger.debug("\(self.TAG) set the remote description") @@ -463,7 +462,7 @@ protocol CallServiceObserver: class { return self.messageSender.sendCallMessage(callAnswerMessage) }.then { guard self.call == newCall else { - throw CallError.unexpected(description: "sendCallMessage() response for obsolete call") + throw CallError.assertionError(description: "sendCallMessage() response for obsolete call") } Logger.debug("\(self.TAG) successfully sent callAnswerMessage") diff --git a/Signal/src/call/PeerConnectionClient.swift b/Signal/src/call/PeerConnectionClient.swift index a89d4d795..8d9cc4234 100644 --- a/Signal/src/call/PeerConnectionClient.swift +++ b/Signal/src/call/PeerConnectionClient.swift @@ -65,8 +65,8 @@ protocol PeerConnectionClientDelegate: class { class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelDelegate { enum CallType { - case Incoming - case Outgoing + case incoming + case outgoing } let TAG = "[PeerConnectionClient]" @@ -145,7 +145,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD createAudioSender() createVideoSender() - if callType == .Outgoing { + if callType == .outgoing { // When placing an outgoing call, it's our responsibility to create the DataChannel. // Recipient will not have to do this explicitly. createSignalingDataChannel() @@ -154,7 +154,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD // MARK: - Media Streams - fileprivate func createSignalingDataChannel() { + private func createSignalingDataChannel() { AssertIsOnMainThread() let dataChannel = peerConnection.dataChannel(forLabel: Identifiers.dataChannelSignaling.rawValue, @@ -550,7 +550,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD } weak var remoteVideoTrack = stream.videoTracks[0] Logger.debug("\(self.TAG) didAdd stream:\(stream) video tracks: \(stream.videoTracks.count) audio tracks: \(stream.audioTracks.count)") - + PeerConnectionClient.signalingQueue.async { guard self.peerConnection != nil else { Logger.debug("\(self.TAG) \(#function) Ignoring obsolete event in terminated client") diff --git a/Signal/test/call/PeerConnectionClientTest.swift b/Signal/test/call/PeerConnectionClientTest.swift index d9887915a..a977a9d2b 100644 --- a/Signal/test/call/PeerConnectionClientTest.swift +++ b/Signal/test/call/PeerConnectionClientTest.swift @@ -55,7 +55,7 @@ class PeerConnectionClientTest: XCTestCase { let iceServers = [RTCIceServer]() clientDelegate = FakePeerConnectionClientDelegate() - client = PeerConnectionClient(iceServers: iceServers, delegate: clientDelegate, callType: .Outgoing) + client = PeerConnectionClient(iceServers: iceServers, delegate: clientDelegate, callType: .outgoing) peerConnection = client.peerConnectionForTests() dataChannel = client.dataChannelForTests() }