Merge branch 'charlesmchen/callThreadSafety4_'

Signal/src/call/CallService.swift

@@ -123,7 +123,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 +138,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)
             }
@@ -283,11 +285,7 @@ protocol CallServiceObserver: class {
         return getIceServers().then { iceServers -> Promise<HardenedRTCSessionDescription> in
             Logger.debug("\(self.TAG) got ice servers:\(iceServers)")
 
-            let peerConnectionClient = PeerConnectionClient(iceServers: iceServers, delegate: self)
+            let peerConnectionClient = PeerConnectionClient(iceServers: iceServers, delegate: self, callType: .outgoing)
-
-            // When placing an outgoing call, it's our responsibility to create the DataChannel. Recipient will not have
-            // to do this explicitly.
-            peerConnectionClient.createSignalingDataChannel()
 
             assert(self.peerConnectionClient == nil, "Unexpected PeerConnectionClient instance")
             Logger.debug("\(self.TAG) setting peerConnectionClient in \(#function)")
@@ -428,6 +426,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)
             }
         }
@@ -437,9 +438,12 @@ protocol CallServiceObserver: class {
         }.then { (iceServers: [RTCIceServer]) -> Promise<HardenedRTCSessionDescription> 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.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)
+            self.peerConnectionClient = PeerConnectionClient(iceServers: iceServers, delegate: self, callType: .incoming)
 
             let offerSessionDescription = RTCSessionDescription(type: .offer, sdp: callerSessionDescription)
             let constraints = RTCMediaConstraints(mandatoryConstraints: nil, optionalConstraints: nil)
@@ -447,6 +451,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.assertionError(description: "negotiateSessionDescription() response for obsolete call")
+            }
             Logger.debug("\(self.TAG) set the remote description")
 
             let answerMessage = OWSCallAnswerMessage(callId: newCall.signalingId, sessionDescription: negotiatedSessionDescription.sdp)
@@ -454,6 +461,9 @@ protocol CallServiceObserver: class {
 
             return self.messageSender.sendCallMessage(callAnswerMessage)
         }.then {
+            guard self.call == newCall else {
+                throw CallError.assertionError(description: "sendCallMessage() response for obsolete call")
+            }
             Logger.debug("\(self.TAG) successfully sent callAnswerMessage")
 
             let (promise, fulfill, _) = Promise<Void>.pending()
@@ -468,6 +478,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 {
@@ -996,7 +1010,6 @@ protocol CallServiceObserver: class {
         }
 
         self.localVideoTrack = videoTrack
-        self.fireDidUpdateVideoTracks()
     }
 
     internal func peerConnectionClient(_ peerConnectionClient: PeerConnectionClient, didUpdateRemote videoTrack: RTCVideoTrack?) {
@@ -1008,7 +1021,6 @@ protocol CallServiceObserver: class {
         }
 
         self.remoteVideoTrack = videoTrack
-        self.fireDidUpdateVideoTracks()
     }
 
     // MARK: Helpers

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",
@@ -110,10 +115,10 @@ 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) {
+    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() {
+    private func createSignalingDataChannel() {
         AssertIsOnMainThread()
 
-        PeerConnectionClient.signalingQueue.sync {
+        let dataChannel = peerConnection.dataChannel(forLabel: Identifiers.dataChannelSignaling.rawValue,
-            let dataChannel = peerConnection.dataChannel(forLabel: Identifiers.dataChannelSignaling.rawValue,
+                                                     configuration: RTCDataChannelConfiguration())
-                                                         configuration: RTCDataChannelConfiguration())
+        dataChannel.delegate = self
-            dataChannel.delegate = self
 
-            assert(self.dataChannel == nil)
+        assert(self.dataChannel == nil)
-            self.dataChannel = dataChannel
+        self.dataChannel = dataChannel
-        }
     }
 
     // MARK: Video
@@ -536,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 = remoteVideoTrack
-                self.remoteVideoTrack = stream.videoTracks[0]
+            if let delegate = self.delegate {
-                if let delegate = self.delegate {
+                DispatchQueue.main.async { [weak self] in
-                    let remoteVideoTrack = self.remoteVideoTrack
+                    guard let strongSelf = self else { return }
-                    DispatchQueue.main.async { [weak self, weak remoteVideoTrack] in
+                    delegate.peerConnectionClient(strongSelf, didUpdateRemote: remoteVideoTrack)
-                        guard let strongSelf = self else { return }
-                        guard let strongRemoteVideoTrack = remoteVideoTrack else { return }
-                        delegate.peerConnectionClient(strongSelf, didUpdateRemote: strongRemoteVideoTrack)
-                    }
                 }
             }
         }

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()
     }