Merge branch 'charlesmchen/weakPeerConnectionClient'

Signal/src/call/CallService.swift

@@ -449,9 +449,8 @@ private class SignalCallData: NSObject {
             self.handleFailedCall(failedCall: call, error: .obsoleteCall(description:"obsolete call in \(#function)"))
             return
         }
-
         guard callData.call == call else {
-            self.handleFailedCall(failedCall: call, error: .obsoleteCall(description:"obsolete call in \(#function)"))
+            Logger.warn("\(self.logTag) ignoring \(#function) for call other than current call")
             return
         }
 
@@ -746,11 +745,11 @@ private class SignalCallData: NSObject {
      * 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) {
+        SwiftAssertIsOnMainThread(#function)
         Logger.verbose("\(logTag) \(#function) callId: \(callId)")
 
         guard let callData = self.callData else {
-            OWSProdError(OWSAnalyticsEvents.callServiceCallMissing(), file: #file, function: #function, line: #line)
+            Logger.info("\(logTag) ignoring remote ice update, since there is no current call.")
-            self.handleFailedCurrentCall(error: .obsoleteCall(description: "ignoring remote ice update, since there is no current call."))
             return
         }
 
@@ -1002,7 +1001,7 @@ private class SignalCallData: NSObject {
         Logger.info("\(self.logTag) in \(#function)")
         SwiftAssertIsOnMainThread(#function)
 
-        guard let peerConnectionClient = self.peerConnectionClient else {
+        guard let peerConnectionClient = callData.peerConnectionClient else {
             OWSProdError(OWSAnalyticsEvents.callServicePeerConnectionMissing(), file: #file, function: #function, line: #line)
             handleFailedCall(failedCall: call, error: CallError.assertionError(description: "\(self.logTag) peerConnectionClient unexpectedly nil in \(#function)"))
             return

Signal/src/call/PeerConnectionClient.swift

@@ -74,7 +74,54 @@ protocol PeerConnectionClientDelegate: class {
  */
 class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelDelegate {
 
-    let TAG = "[PeerConnectionClient]"
+    // In Swift (at least in Swift v3.3), weak variables aren't thread safe. It
+    // isn't safe to resolve/acquire/lock a weak reference into a strong reference
+    // while the instance might be being deallocated on another thread.
+    //
+    // AtomicHandle provides thread-safe access to a strong reference.
+    // PeerConnectionClient has an AtomicHandle to itself that its many async blocks
+    // (which run on more than one thread) can use to safely try to acquire a strong
+    // reference to the PeerConnectionClient. In ARC we'd normally, we'd avoid
+    // having an instance retain a strong reference to itself to avoid retain
+    // cycles, but it's safe in this case: PeerConnectionClient is owned (and only
+    // used by) a single entity CallService and CallService always calls
+    // [PeerConnectionClient terminate] when it is done with a PeerConnectionClient
+    // instance, so terminate is a reliable place where we can break the retain cycle.
+    //
+    // TODO: This should be fixed in Swift 4.  To verify, remove AtomicHandle and
+    // use [weak self] as usual.  Test using the following scenarios:
+    //
+    // * Alice and Bob place simultaneous calls to each other. Both should get busy.
+    //   Repeat 10-20x.  Then verify that they can connect a call by having just one
+    //   call the other.
+    // * Alice or Bob (randomly alternating) calls the other. Recipient (randomly)
+    //   accepts call or hangs up.  If accepted, Alice or Bob (randomly) hangs up.
+    //   Repeat immediately, as fast as you can, 10-20x.
+    private class AtomicHandle<ValueType> : NSObject {
+        var value: ValueType?
+
+        func set(value: ValueType) {
+            objc_sync_enter(self)
+            self.value = value
+            objc_sync_exit(self)
+        }
+
+        func get() -> ValueType? {
+            objc_sync_enter(self)
+            let result = value
+            objc_sync_exit(self)
+            return result
+        }
+
+        func clear() {
+            Logger.info("\(logTag) \(#function)")
+
+            objc_sync_enter(self)
+            value = nil
+            objc_sync_exit(self)
+        }
+    }
+
     enum Identifiers: String {
         case mediaStream = "ARDAMS",
              videoTrack = "ARDAMSv0",
@@ -95,7 +142,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD
 
     // Connection
 
-    private var peerConnection: RTCPeerConnection!
+    private var peerConnection: RTCPeerConnection?
     private let iceServers: [RTCIceServer]
     private let connectionConstraints: RTCMediaConstraints
     private let configuration: RTCConfiguration
@@ -121,13 +168,11 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD
     // RTCVideoTrack is fragile and prone to throwing exceptions and/or
     // causing deadlock in its destructor.  Therefore we take great care
     // with this property.
-    //
-    // We synchronize access to this property and ensure that we never
-    // set or use a strong reference to the remote video track if 
-    // peerConnection is nil.
     private var remoteVideoTrack: RTCVideoTrack?
     private var cameraConstraints: RTCMediaConstraints
 
+    private let handle: AtomicHandle<PeerConnectionClient>
+
     init(iceServers: [RTCIceServer], delegate: PeerConnectionClientDelegate, callDirection: CallDirection, useTurnOnly: Bool) {
         SwiftAssertIsOnMainThread(#function)
 
@@ -139,10 +184,10 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD
         configuration.bundlePolicy = .maxBundle
         configuration.rtcpMuxPolicy = .require
         if useTurnOnly {
-            Logger.debug("\(TAG) using iceTransportPolicy: relay")
+            Logger.debug("\(PeerConnectionClient.logTag) using iceTransportPolicy: relay")
             configuration.iceTransportPolicy = .relay
         } else {
-            Logger.debug("\(TAG) using iceTransportPolicy: default")
+            Logger.debug("\(PeerConnectionClient.logTag) using iceTransportPolicy: default")
         }
 
         let connectionConstraintsDict = ["DtlsSrtpKeyAgreement": "true"]
@@ -151,8 +196,12 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD
         audioConstraints = RTCMediaConstraints(mandatoryConstraints: nil, optionalConstraints: nil)
         cameraConstraints = RTCMediaConstraints(mandatoryConstraints: nil, optionalConstraints: nil)
 
+        handle = AtomicHandle<PeerConnectionClient>()
+
         super.init()
 
+        handle.set(value: self)
+
         peerConnection = factory.peerConnection(with: configuration,
                                                 constraints: connectionConstraints,
                                                 delegate: self)
@@ -167,13 +216,19 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD
     }
 
     deinit {
-        Logger.debug("[PeerConnectionClient] deinit")
+        // TODO: We can demote this log level to debug once we're confident that
+        // this class is always deallocated.
+        Logger.info("[PeerConnectionClient] deinit")
     }
 
     // MARK: - Media Streams
 
     private func createSignalingDataChannel() {
         SwiftAssertIsOnMainThread(#function)
+        guard let peerConnection = peerConnection else {
+            Logger.debug("\(logTag) \(#function) Ignoring obsolete event in terminated client")
+            return
+        }
 
         let configuration = RTCDataChannelConfiguration()
         // Insist upon an "ordered" TCP data channel for delivery reliability.
@@ -190,12 +245,15 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD
 
     fileprivate func createVideoSender() {
         SwiftAssertIsOnMainThread(#function)
-
+        Logger.debug("\(logTag) in \(#function)")
-        Logger.debug("\(TAG) in \(#function)")
         assert(self.videoSender == nil, "\(#function) should only be called once.")
 
         guard !Platform.isSimulator else {
-            Logger.warn("\(TAG) Refusing to create local video track on simulator which has no capture device.")
+            Logger.warn("\(logTag) Refusing to create local video track on simulator which has no capture device.")
+            return
+        }
+        guard let peerConnection = peerConnection else {
+            Logger.debug("\(logTag) \(#function) Ignoring obsolete event in terminated client")
             return
         }
 
@@ -227,15 +285,17 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD
     public func setCameraSource(useBackCamera: Bool) {
         SwiftAssertIsOnMainThread(#function)
 
-        PeerConnectionClient.signalingQueue.async {
+        let handle = self.handle
-            guard let localVideoSource = self.localVideoSource else {
+        PeerConnectionClient.signalingQueue.async { [weak self] in
-                owsFail("\(self.logTag) in \(#function) localVideoSource was unexpectedly nil")
+            guard let strongSelf = handle.get() else { return }
+            guard let localVideoSource = strongSelf.localVideoSource else {
+                Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client")
                 return
             }
 
             // certain devices, e.g. 16GB iPod touch don't have a back camera
             guard localVideoSource.canUseBackCamera else {
-                owsFail("\(self.logTag) in \(#function) canUseBackCamera was unexpectedly false")
+                owsFail("\(strongSelf.logTag) in \(#function) canUseBackCamera was unexpectedly false")
                 return
             }
 
@@ -245,38 +305,40 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD
 
     public func setLocalVideoEnabled(enabled: Bool) {
         SwiftAssertIsOnMainThread(#function)
+        let handle = self.handle
+        let completion = { [weak self] in
+            guard let strongSelf = handle.get() else { return }
+            guard let localVideoTrack = strongSelf.localVideoTrack else { return }
+            guard let strongDelegate = strongSelf.delegate else { return }
+            strongDelegate.peerConnectionClient(strongSelf, didUpdateLocal: enabled ? localVideoTrack : nil)
+        }
 
-        PeerConnectionClient.signalingQueue.async {
+        PeerConnectionClient.signalingQueue.async { [weak self] in
-            guard self.peerConnection != nil else {
+            guard let strongSelf = handle.get() else { return }
-                Logger.debug("\(self.TAG) \(#function) Ignoring obsolete event in terminated client")
+            guard strongSelf.peerConnection != nil else {
+                Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client")
                 return
             }
-            guard let localVideoTrack = self.localVideoTrack else {
+            guard let localVideoTrack = strongSelf.localVideoTrack else {
-                let action = enabled ? "enable" : "disable"
+                Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client")
-                Logger.error("\(self.TAG)) trying to \(action) videoTrack which doesn't exist")
                 return
             }
-            guard let videoCaptureSession = self.videoCaptureSession else {
+            guard let videoCaptureSession = strongSelf.videoCaptureSession else {
-                Logger.debug("\(self.TAG) videoCaptureSession was unexpectedly nil")
+                Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client")
                 return
             }
 
             localVideoTrack.isEnabled = enabled
 
             if enabled {
-                Logger.debug("\(self.TAG) in \(#function) starting videoCaptureSession")
+                Logger.debug("\(strongSelf.logTag) in \(#function) starting videoCaptureSession")
                 videoCaptureSession.startRunning()
             } else {
-                Logger.debug("\(self.TAG) in \(#function) stopping videoCaptureSession")
+                Logger.debug("\(strongSelf.logTag) in \(#function) stopping videoCaptureSession")
                 videoCaptureSession.stopRunning()
             }
 
-            DispatchQueue.main.async { [weak self, weak localVideoTrack] in
+            DispatchQueue.main.async(execute: completion)
-                guard let strongSelf = self else { return }
-                guard let strongLocalVideoTrack = localVideoTrack else { return }
-                guard let strongDelegate = strongSelf.delegate else { return }
-                strongDelegate.peerConnectionClient(strongSelf, didUpdateLocal: enabled ? strongLocalVideoTrack : nil)
-            }
         }
     }
 
@@ -284,10 +346,14 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD
 
     fileprivate func createAudioSender() {
         SwiftAssertIsOnMainThread(#function)
-
+        Logger.debug("\(logTag) in \(#function)")
-        Logger.debug("\(TAG) in \(#function)")
         assert(self.audioSender == nil, "\(#function) should only be called once.")
 
+        guard let peerConnection = peerConnection else {
+            Logger.debug("\(logTag) \(#function) Ignoring obsolete event in terminated client")
+            return
+        }
+
         let audioSource = factory.audioSource(with: self.audioConstraints)
 
         let audioTrack = factory.audioTrack(with: audioSource, trackId: Identifiers.audioTrack.rawValue)
@@ -306,15 +372,15 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD
 
     public func setAudioEnabled(enabled: Bool) {
         SwiftAssertIsOnMainThread(#function)
-
+        let handle = self.handle
-        PeerConnectionClient.signalingQueue.async {
+        PeerConnectionClient.signalingQueue.async { [weak self] in
-            guard self.peerConnection != nil else {
+            guard let strongSelf = handle.get() else { return }
-                Logger.debug("\(self.TAG) \(#function) Ignoring obsolete event in terminated client")
+            guard strongSelf.peerConnection != nil else {
+                Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client")
                 return
             }
-            guard let audioTrack = self.audioTrack else {
+            guard let audioTrack = strongSelf.audioTrack else {
-                let action = enabled ? "enable" : "disable"
+                Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client")
-                Logger.error("\(self.TAG) trying to \(action) audioTrack which doesn't exist.")
                 return
             }
 
@@ -334,31 +400,26 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD
 
     public func createOffer() -> Promise<HardenedRTCSessionDescription> {
         SwiftAssertIsOnMainThread(#function)
-
+        let handle = self.handle
-        return Promise { fulfill, reject in
+        let (promise, fulfill, reject) = Promise<HardenedRTCSessionDescription>.pending()
-            SwiftAssertIsOnMainThread(#function)
+        let completion: ((RTCSessionDescription?, Error?) -> Void) = { [weak self] (sdp, error) in
-
+            guard let strongSelf = handle.get() else {
-            PeerConnectionClient.signalingQueue.async {
-                guard self.peerConnection != nil else {
-                    Logger.debug("\(self.TAG) \(#function) Ignoring obsolete event in terminated client")
                 reject(NSError(domain: "Obsolete client", code: 0, userInfo: nil))
                 return
             }
-
+            strongSelf.assertOnSignalingQueue()
-                self.peerConnection.offer(for: self.defaultOfferConstraints, completionHandler: { (sdp: RTCSessionDescription?, error: Error?) in
+            guard strongSelf.peerConnection != nil else {
-                    PeerConnectionClient.signalingQueue.async {
+                Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client")
-                        guard self.peerConnection != nil else {
-                            Logger.debug("\(self.TAG) \(#function) Ignoring obsolete event in terminated client")
                 reject(NSError(domain: "Obsolete client", code: 0, userInfo: nil))
                 return
             }
-                        guard error == nil else {
+            if let error = error {
-                            reject(error!)
+                reject(error)
                 return
             }
 
             guard let sessionDescription = sdp else {
-                            Logger.error("\(self.TAG) No session description was obtained, even though there was no error reported.")
+                Logger.error("\(strongSelf.logTag) No session description was obtained, even though there was no error reported.")
                 let error = OWSErrorMakeUnableToProcessServerResponseError()
                 reject(error)
                 return
@@ -366,102 +427,149 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD
 
             fulfill(HardenedRTCSessionDescription(rtcSessionDescription: sessionDescription))
         }
-                })
+
+        PeerConnectionClient.signalingQueue.async { [weak self] in
+            guard let strongSelf = handle.get() else {
+                reject(NSError(domain: "Obsolete client", code: 0, userInfo: nil))
+                return
             }
+            strongSelf.assertOnSignalingQueue()
+            guard let peerConnection = strongSelf.peerConnection else {
+                Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client")
+                reject(NSError(domain: "Obsolete client", code: 0, userInfo: nil))
+                return
+            }
+
+            peerConnection.offer(for: strongSelf.defaultOfferConstraints, completionHandler: { (sdp: RTCSessionDescription?, error: Error?) in
+                PeerConnectionClient.signalingQueue.async {
+                    completion(sdp, error)
                 }
+            })
+        }
+
+        return promise
     }
 
     public func setLocalSessionDescriptionInternal(_ sessionDescription: HardenedRTCSessionDescription) -> Promise<Void> {
-        return PromiseKit.wrap { resolve in
+        let handle = self.handle
-            self.assertOnSignalingQueue()
+        let (promise, fulfill, reject) = Promise<Void>.pending()
-            Logger.verbose("\(self.TAG) setting local session description: \(sessionDescription)")
+        PeerConnectionClient.signalingQueue.async { [weak self] in
-            self.peerConnection.setLocalDescription(sessionDescription.rtcSessionDescription, completionHandler: resolve)
+            guard let strongSelf = handle.get() else {
+                reject(NSError(domain: "Obsolete client", code: 0, userInfo: nil))
+                return
             }
+            strongSelf.assertOnSignalingQueue()
+
+            guard let peerConnection = strongSelf.peerConnection else {
+                Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client")
+                reject(NSError(domain: "Obsolete client", code: 0, userInfo: nil))
+                return
+            }
+
+            Logger.verbose("\(strongSelf.logTag) setting local session description: \(sessionDescription)")
+            peerConnection.setLocalDescription(sessionDescription.rtcSessionDescription, completionHandler: { (error) in
+                if let error = error {
+                    reject(error)
+                } else {
+                    fulfill()
+                }
+            })
+        }
+        return promise
     }
 
     public func setLocalSessionDescription(_ sessionDescription: HardenedRTCSessionDescription) -> Promise<Void> {
         SwiftAssertIsOnMainThread(#function)
-
+        let handle = self.handle
-        return Promise { fulfill, reject in
+        let (promise, fulfill, reject) = Promise<Void>.pending()
-            PeerConnectionClient.signalingQueue.async {
+        PeerConnectionClient.signalingQueue.async { [weak self] in
-                guard self.peerConnection != nil else {
+            guard let strongSelf = handle.get() else {
-                    Logger.debug("\(self.TAG) \(#function) Ignoring obsolete event in terminated client")
+                reject(NSError(domain: "Obsolete client", code: 0, userInfo: nil))
+                return
+            }
+            strongSelf.assertOnSignalingQueue()
+            guard let peerConnection = strongSelf.peerConnection else {
+                Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client")
                 reject(NSError(domain: "Obsolete client", code: 0, userInfo: nil))
                 return
             }
-                Logger.verbose("\(self.TAG) setting local session description: \(sessionDescription)")
+
-                self.peerConnection.setLocalDescription(sessionDescription.rtcSessionDescription,
+            Logger.verbose("\(strongSelf.logTag) setting local session description: \(sessionDescription)")
+            peerConnection.setLocalDescription(sessionDescription.rtcSessionDescription,
                                                completionHandler: { error in
-                                                            guard error == nil else {
+                                                if let error = error {
-                                                                reject(error!)
+                                                    reject(error)
                                                     return
                                                 }
                                                 fulfill()
             })
         }
-        }
+
+        return promise
     }
 
     public func negotiateSessionDescription(remoteDescription: RTCSessionDescription, constraints: RTCMediaConstraints) -> Promise<HardenedRTCSessionDescription> {
         SwiftAssertIsOnMainThread(#function)
-
+        let handle = self.handle
         return setRemoteSessionDescription(remoteDescription)
-            .then(on: PeerConnectionClient.signalingQueue) {
+            .then(on: PeerConnectionClient.signalingQueue) { [weak self] in
-                return self.negotiateAnswerSessionDescription(constraints: constraints)
+                guard let strongSelf = handle.get() else {
+                    return Promise(error: NSError(domain: "Obsolete client", code: 0, userInfo: nil))
+                }
+                    return strongSelf.negotiateAnswerSessionDescription(constraints: constraints)
         }
     }
 
     public func setRemoteSessionDescription(_ sessionDescription: RTCSessionDescription) -> Promise<Void> {
         SwiftAssertIsOnMainThread(#function)
-
+        let handle = self.handle
-        return Promise { fulfill, reject in
+        let (promise, fulfill, reject) = Promise<Void>.pending()
-            PeerConnectionClient.signalingQueue.async {
+        PeerConnectionClient.signalingQueue.async { [weak self] in
-                guard self.peerConnection != nil else {
+            guard let strongSelf = handle.get() else {
-                    Logger.debug("\(self.TAG) \(#function) Ignoring obsolete event in terminated client")
+                reject(NSError(domain: "Obsolete client", code: 0, userInfo: nil))
+                return
+            }
+            strongSelf.assertOnSignalingQueue()
+            guard let peerConnection = strongSelf.peerConnection else {
+                Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client")
                 reject(NSError(domain: "Obsolete client", code: 0, userInfo: nil))
                 return
             }
-                Logger.verbose("\(self.TAG) setting remote description: \(sessionDescription)")
+            Logger.verbose("\(strongSelf.logTag) setting remote description: \(sessionDescription)")
-                self.peerConnection.setRemoteDescription(sessionDescription,
+            peerConnection.setRemoteDescription(sessionDescription,
                                                 completionHandler: { error in
-                                                            guard error == nil else {
+                                                    if let error = error {
-                                                                reject(error!)
+                                                        reject(error)
                                                         return
                                                     }
                                                     fulfill()
             })
         }
-        }
+        return promise
     }
 
     private func negotiateAnswerSessionDescription(constraints: RTCMediaConstraints) -> Promise<HardenedRTCSessionDescription> {
         assertOnSignalingQueue()
-
+        let handle = self.handle
-        return Promise { fulfill, reject in
+        let (promise, fulfill, reject) = Promise<HardenedRTCSessionDescription>.pending()
-            assertOnSignalingQueue()
+        let completion: ((RTCSessionDescription?, Error?) -> Void) = { [weak self] (sdp, error) in
-
+            guard let strongSelf = handle.get() else {
-            guard self.peerConnection != nil else {
-                Logger.debug("\(self.TAG) \(#function) Ignoring obsolete event in terminated client")
                 reject(NSError(domain: "Obsolete client", code: 0, userInfo: nil))
                 return
             }
-
+            strongSelf.assertOnSignalingQueue()
-            Logger.debug("\(self.TAG) negotiating answer session.")
+            guard strongSelf.peerConnection != nil else {
-
+                Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client")
-            peerConnection.answer(for: constraints, completionHandler: { (sdp: RTCSessionDescription?, error: Error?) in
-                PeerConnectionClient.signalingQueue.async {
-                    guard self.peerConnection != nil else {
-                        Logger.debug("\(self.TAG) \(#function) Ignoring obsolete event in terminated client")
                 reject(NSError(domain: "Obsolete client", code: 0, userInfo: nil))
                 return
             }
-                    guard error == nil else {
+            if let error = error {
-                        reject(error!)
+                reject(error)
                 return
             }
 
             guard let sessionDescription = sdp else {
-                        Logger.error("\(self.TAG) unexpected empty session description, even though no error was reported.")
+                Logger.error("\(strongSelf.logTag) unexpected empty session description, even though no error was reported.")
                 let error = OWSErrorMakeUnableToProcessServerResponseError()
                 reject(error)
                 return
@@ -469,46 +577,72 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD
 
             let hardenedSessionDescription = HardenedRTCSessionDescription(rtcSessionDescription: sessionDescription)
 
-                    self.setLocalSessionDescriptionInternal(hardenedSessionDescription)
+            strongSelf.setLocalSessionDescriptionInternal(hardenedSessionDescription)
-                        .then(on: PeerConnectionClient.signalingQueue) {
+                .then(on: PeerConnectionClient.signalingQueue) { _ in
                     fulfill(hardenedSessionDescription)
                 }.catch { error in
                     reject(error)
             }
         }
+
+        PeerConnectionClient.signalingQueue.async { [weak self] in
+            guard let strongSelf = handle.get() else {
+                reject(NSError(domain: "Obsolete client", code: 0, userInfo: nil))
+                return
+            }
+            strongSelf.assertOnSignalingQueue()
+
+            guard let peerConnection = strongSelf.peerConnection else {
+                Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client")
+                reject(NSError(domain: "Obsolete client", code: 0, userInfo: nil))
+                return
+            }
+
+            Logger.debug("\(strongSelf.logTag) negotiating answer session.")
+
+            peerConnection.answer(for: constraints, completionHandler: { (sdp: RTCSessionDescription?, error: Error?) in
+                PeerConnectionClient.signalingQueue.async {
+                    completion(sdp, error)
+                }
             })
         }
+        return promise
     }
 
     public func addRemoteIceCandidate(_ candidate: RTCIceCandidate) {
-        PeerConnectionClient.signalingQueue.async {
+        let handle = self.handle
-            guard self.peerConnection != nil else {
+        PeerConnectionClient.signalingQueue.async { [weak self] in
-                Logger.debug("\(self.TAG) \(#function) Ignoring obsolete event in terminated client")
+            guard let strongSelf = handle.get() else { return }
+            guard let peerConnection = strongSelf.peerConnection else {
+                Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client")
                 return
             }
-            Logger.info("\(self.TAG) adding remote ICE candidate: \(candidate.sdp)")
+            Logger.info("\(strongSelf.logTag) adding remote ICE candidate: \(candidate.sdp)")
-            self.peerConnection.add(candidate)
+            peerConnection.add(candidate)
         }
     }
 
     public func terminate() {
         SwiftAssertIsOnMainThread(#function)
-        Logger.debug("\(TAG) in \(#function)")
+        Logger.debug("\(logTag) in \(#function)")
 
         // Clear the delegate immediately so that we can guarantee that
         // no delegate methods are called after terminate() returns.
         delegate = nil
 
+        // Clear the handle immediately so that enqueued work is aborted
+        // going forward.
+        handle.clear()
+
+        // Don't use [weak self]; we always want to perform terminateInternal().
         PeerConnectionClient.signalingQueue.async {
-            assert(self.peerConnection != nil)
             self.terminateInternal()
         }
     }
 
     private func terminateInternal() {
         assertOnSignalingQueue()
-
+        Logger.debug("\(logTag) in \(#function)")
-        Logger.debug("\(TAG) in \(#function)")
 
         //        Some notes on preventing crashes while disposing of peerConnection for video calls
         //        from: https://groups.google.com/forum/#!searchin/discuss-webrtc/objc$20crash$20dealloc%7Csort:relevance/discuss-webrtc/7D-vk5yLjn8/rBW2D6EW4GYJ
@@ -523,12 +657,13 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD
         // become nil when it was only a weak property. So we retain it and manually nil the reference here, because
         // we are likely to crash if we retain any peer connection properties when the peerconnection is released
 
-        // See the comments on the remoteVideoTrack property.
-        objc_sync_enter(self)
-
         localVideoTrack?.isEnabled = false
         remoteVideoTrack?.isEnabled = false
 
+        if let dataChannel = self.dataChannel {
+            dataChannel.delegate = nil
+        }
+
         dataChannel = nil
         audioSender = nil
         audioTrack = nil
@@ -537,11 +672,11 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD
         localVideoTrack = nil
         remoteVideoTrack = nil
 
+        if let peerConnection = peerConnection {
             peerConnection.delegate = nil
             peerConnection.close()
+        }
         peerConnection = nil
-
-        objc_sync_exit(self)
     }
 
     // MARK: - Data Channel
@@ -556,32 +691,34 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD
 
     public func sendDataChannelMessage(data: Data, description: String, isCritical: Bool) {
         SwiftAssertIsOnMainThread(#function)
+        let handle = self.handle
+        PeerConnectionClient.signalingQueue.async { [weak self] in
+            guard let strongSelf = handle.get() else { return }
 
-        PeerConnectionClient.signalingQueue.async {
+            guard strongSelf.peerConnection != nil else {
-            guard self.peerConnection != nil else {
+                Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client: \(description)")
-                Logger.debug("\(self.TAG) \(#function) Ignoring obsolete event in terminated client: \(description)")
                 return
             }
 
-            guard let dataChannel = self.dataChannel else {
+            guard let dataChannel = strongSelf.dataChannel else {
                 if isCritical {
-                    Logger.info("\(self.TAG) in \(#function) enqueuing critical data channel message for after we have a dataChannel: \(description)")
+                    Logger.info("\(strongSelf.logTag) in \(#function) enqueuing critical data channel message for after we have a dataChannel: \(description)")
-                    self.pendingDataChannelMessages.append(PendingDataChannelMessage(data: data, description: description, isCritical: isCritical))
+                    strongSelf.pendingDataChannelMessages.append(PendingDataChannelMessage(data: data, description: description, isCritical: isCritical))
                 } else {
-                    Logger.error("\(self.TAG) in \(#function) ignoring sending \(data) for nil dataChannel: \(description)")
+                    Logger.error("\(strongSelf.logTag) in \(#function) ignoring sending \(data) for nil dataChannel: \(description)")
                 }
                 return
             }
 
-            Logger.debug("\(self.TAG) sendDataChannelMessage trying: \(description)")
+            Logger.debug("\(strongSelf.logTag) sendDataChannelMessage trying: \(description)")
 
             let buffer = RTCDataBuffer(data: data, isBinary: false)
             let result = dataChannel.sendData(buffer)
 
             if result {
-                Logger.debug("\(self.TAG) sendDataChannelMessage succeeded: \(description)")
+                Logger.debug("\(strongSelf.logTag) sendDataChannelMessage succeeded: \(description)")
             } else {
-                Logger.warn("\(self.TAG) sendDataChannelMessage failed: \(description)")
+                Logger.warn("\(strongSelf.logTag) sendDataChannelMessage failed: \(description)")
                 if isCritical {
                     OWSProdError(OWSAnalyticsEvents.peerConnectionClientErrorSendDataChannelMessageFailed(), file: #file, function: #function, line: #line)
                 }
@@ -593,178 +730,218 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD
 
     /** The data channel state changed. */
     internal func dataChannelDidChangeState(_ dataChannel: RTCDataChannel) {
-        Logger.debug("\(TAG) dataChannelDidChangeState: \(dataChannel)")
+        Logger.debug("\(logTag) dataChannelDidChangeState: \(dataChannel)")
     }
 
     /** The data channel successfully received a data buffer. */
     internal func dataChannel(_ dataChannel: RTCDataChannel, didReceiveMessageWith buffer: RTCDataBuffer) {
-        PeerConnectionClient.signalingQueue.async {
+        let handle = self.handle
-            guard self.peerConnection != nil else {
+        let completion: (OWSWebRTCProtosData) -> Void = { [weak self] (dataChannelMessage) in
-                Logger.debug("\(self.TAG) \(#function) Ignoring obsolete event in terminated client")
+            SwiftAssertIsOnMainThread(#function)
+            guard let strongSelf = handle.get() else { return }
+            guard let strongDelegate = strongSelf.delegate else { return }
+            strongDelegate.peerConnectionClient(strongSelf, received: dataChannelMessage)
+        }
+
+        PeerConnectionClient.signalingQueue.async { [weak self] in
+            guard let strongSelf = handle.get() else { return }
+            guard strongSelf.peerConnection != nil else {
+                Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client")
                 return
             }
-            Logger.debug("\(self.TAG) dataChannel didReceiveMessageWith buffer:\(buffer)")
+            Logger.debug("\(strongSelf.logTag) dataChannel didReceiveMessageWith buffer:\(buffer)")
 
             guard let dataChannelMessage = OWSWebRTCProtosData.parse(from: buffer.data) else {
                 // TODO can't proto parsings throw an exception? Is it just being lost in the Objc->Swift?
-                Logger.error("\(self.TAG) failed to parse dataProto")
+                Logger.error("\(strongSelf.logTag) failed to parse dataProto")
                 return
             }
 
-            DispatchQueue.main.async { [weak self] in
+            DispatchQueue.main.async {
-                guard let strongSelf = self else { return }
+                completion(dataChannelMessage)
-                guard let strongDelegate = strongSelf.delegate else { return }
-                strongDelegate.peerConnectionClient(strongSelf, received: dataChannelMessage)
             }
         }
     }
 
     /** The data channel's |bufferedAmount| changed. */
     internal func dataChannel(_ dataChannel: RTCDataChannel, didChangeBufferedAmount amount: UInt64) {
-        Logger.debug("\(TAG) didChangeBufferedAmount: \(amount)")
+        Logger.debug("\(logTag) didChangeBufferedAmount: \(amount)")
     }
 
     // MARK: - RTCPeerConnectionDelegate
 
     /** Called when the SignalingState changed. */
-    internal func peerConnection(_ peerConnection: RTCPeerConnection, didChange stateChanged: RTCSignalingState) {
+    internal func peerConnection(_ peerConnectionParam: RTCPeerConnection, didChange stateChanged: RTCSignalingState) {
-        Logger.debug("\(TAG) didChange signalingState:\(stateChanged.debugDescription)")
+        Logger.debug("\(logTag) didChange signalingState:\(stateChanged.debugDescription)")
     }
 
     /** Called when media is received on a new stream from remote peer. */
-    internal func peerConnection(_ peerConnection: RTCPeerConnection, didAdd stream: RTCMediaStream) {
+    internal func peerConnection(_ peerConnectionParam: RTCPeerConnection, didAdd stream: RTCMediaStream) {
-        guard stream.videoTracks.count > 0 else {
+        let handle = self.handle
-            return
+        let completion: (RTCVideoTrack) -> Void = { [weak self] (remoteVideoTrack) in
-        }
+            SwiftAssertIsOnMainThread(#function)
-        let remoteVideoTrack = stream.videoTracks[0]
+            guard let strongSelf = handle.get() else { return }
-        Logger.debug("\(self.TAG) didAdd stream:\(stream) video tracks: \(stream.videoTracks.count) audio tracks: \(stream.audioTracks.count)")
+            guard let strongDelegate = strongSelf.delegate else { return }
 
-        // See the comments on the remoteVideoTrack property.
+            // TODO: Consider checking for termination here.
-        //
+
-        // We only set the remoteVideoTrack property if peerConnection is non-nil.
+            strongDelegate.peerConnectionClient(strongSelf, didUpdateRemote: remoteVideoTrack)
-        objc_sync_enter(self)
-        if self.peerConnection != nil {
-            self.remoteVideoTrack = remoteVideoTrack
         }
-        objc_sync_exit(self)
 
-        PeerConnectionClient.signalingQueue.async {
+        PeerConnectionClient.signalingQueue.async { [weak self] in
-            guard self.peerConnection != nil else {
+            guard let strongSelf = handle.get() else { return }
-                Logger.debug("\(self.TAG) \(#function) Ignoring obsolete event in terminated client")
+            guard let peerConnection = strongSelf.peerConnection else {
+                Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client")
                 return
             }
-
+            guard peerConnection == peerConnectionParam else {
-            DispatchQueue.main.async { [weak self] in
+                owsFail("\(strongSelf.logTag) in \(#function) mismatched peerConnection callback.")
-                guard let strongSelf = self else { return }
+                return
-                guard let strongDelegate = strongSelf.delegate else { return }
+            }
-
+            guard stream.videoTracks.count > 0 else {
-                // See the comments on the remoteVideoTrack property.
+                owsFail("\(strongSelf.logTag) in \(#function) didAdd stream missing stream.")
-                //
+                return
-                // We only access the remoteVideoTrack property if peerConnection is non-nil.
-                var remoteVideoTrack: RTCVideoTrack?
-                objc_sync_enter(strongSelf)
-                if strongSelf.peerConnection != nil {
-                    remoteVideoTrack = strongSelf.remoteVideoTrack
             }
-                objc_sync_exit(strongSelf)
+            let remoteVideoTrack = stream.videoTracks[0]
+            Logger.debug("\(strongSelf.logTag) didAdd stream:\(stream) video tracks: \(stream.videoTracks.count) audio tracks: \(stream.audioTracks.count)")
 
-                strongDelegate.peerConnectionClient(strongSelf, didUpdateRemote: remoteVideoTrack)
+            strongSelf.remoteVideoTrack = remoteVideoTrack
+
+            DispatchQueue.main.async {
+                completion(remoteVideoTrack)
             }
         }
     }
 
     /** Called when a remote peer closes a stream. */
-    internal func peerConnection(_ peerConnection: RTCPeerConnection, didRemove stream: RTCMediaStream) {
+    internal func peerConnection(_ peerConnectionParam: RTCPeerConnection, didRemove stream: RTCMediaStream) {
-        Logger.debug("\(TAG) didRemove Stream:\(stream)")
+        Logger.debug("\(logTag) didRemove Stream:\(stream)")
     }
 
     /** Called when negotiation is needed, for example ICE has restarted. */
-    internal func peerConnectionShouldNegotiate(_ peerConnection: RTCPeerConnection) {
+    internal func peerConnectionShouldNegotiate(_ peerConnectionParam: RTCPeerConnection) {
-        Logger.debug("\(TAG) shouldNegotiate")
+        Logger.debug("\(logTag) shouldNegotiate")
     }
 
     /** Called any time the IceConnectionState changes. */
-    internal func peerConnection(_ peerConnection: RTCPeerConnection, didChange newState: RTCIceConnectionState) {
+    internal func peerConnection(_ peerConnectionParam: RTCPeerConnection, didChange newState: RTCIceConnectionState) {
-        PeerConnectionClient.signalingQueue.async {
+        let handle = self.handle
-            guard self.peerConnection != nil else {
+        let connectedCompletion : () -> Void = { [weak self] in
-                Logger.debug("\(self.TAG) \(#function) Ignoring obsolete event in terminated client")
+            SwiftAssertIsOnMainThread(#function)
-                return
+            guard let strongSelf = handle.get() else { return }
-            }
-            Logger.info("\(self.TAG) didChange IceConnectionState:\(newState.debugDescription)")
-            switch newState {
-            case .connected, .completed:
-                DispatchQueue.main.async { [weak self] in
-                    guard let strongSelf = self else { return }
             guard let strongDelegate = strongSelf.delegate else { return }
             strongDelegate.peerConnectionClientIceConnected(strongSelf)
         }
-            case .failed:
+        let failedCompletion : () -> Void = { [weak self] in
-                Logger.warn("\(self.TAG) RTCIceConnection failed.")
+            SwiftAssertIsOnMainThread(#function)
-                DispatchQueue.main.async { [weak self] in
+            guard let strongSelf = handle.get() else { return }
-                    guard let strongSelf = self else { return }
             guard let strongDelegate = strongSelf.delegate else { return }
             strongDelegate.peerConnectionClientIceFailed(strongSelf)
         }
-            case .disconnected:
+        let disconnectedCompletion : () -> Void = { [weak self] in
-                Logger.warn("\(self.TAG) RTCIceConnection disconnected.")
+            SwiftAssertIsOnMainThread(#function)
-                DispatchQueue.main.async { [weak self] in
+            guard let strongSelf = handle.get() else { return }
-                    guard let strongSelf = self else { return }
             guard let strongDelegate = strongSelf.delegate else { return }
             strongDelegate.peerConnectionClientIceDisconnected(strongSelf)
         }
+
+        PeerConnectionClient.signalingQueue.async { [weak self] in
+            guard let strongSelf = handle.get() else { return }
+            guard let peerConnection = strongSelf.peerConnection else {
+                Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client")
+                return
+            }
+            guard peerConnection == peerConnectionParam else {
+                owsFail("\(strongSelf.logTag) in \(#function) mismatched peerConnection callback.")
+                return
+            }
+
+            Logger.info("\(strongSelf.logTag) didChange IceConnectionState:\(newState.debugDescription)")
+            switch newState {
+            case .connected, .completed:
+                DispatchQueue.main.async(execute: connectedCompletion)
+            case .failed:
+                Logger.warn("\(strongSelf.logTag) RTCIceConnection failed.")
+                DispatchQueue.main.async(execute: failedCompletion)
+            case .disconnected:
+                Logger.warn("\(strongSelf.logTag) RTCIceConnection disconnected.")
+                DispatchQueue.main.async(execute: disconnectedCompletion)
             default:
-                Logger.debug("\(self.TAG) ignoring change IceConnectionState:\(newState.debugDescription)")
+                Logger.debug("\(strongSelf.logTag) ignoring change IceConnectionState:\(newState.debugDescription)")
             }
         }
     }
 
     /** Called any time the IceGatheringState changes. */
-    internal func peerConnection(_ peerConnection: RTCPeerConnection, didChange newState: RTCIceGatheringState) {
+    internal func peerConnection(_ peerConnectionParam: RTCPeerConnection, didChange newState: RTCIceGatheringState) {
-        Logger.info("\(TAG) didChange IceGatheringState:\(newState.debugDescription)")
+        Logger.info("\(logTag) didChange IceGatheringState:\(newState.debugDescription)")
     }
 
     /** New ice candidate has been found. */
-    internal func peerConnection(_ peerConnection: RTCPeerConnection, didGenerate candidate: RTCIceCandidate) {
+    internal func peerConnection(_ peerConnectionParam: RTCPeerConnection, didGenerate candidate: RTCIceCandidate) {
-        PeerConnectionClient.signalingQueue.async {
+        let handle = self.handle
-            guard self.peerConnection != nil else {
+        let completion: (RTCIceCandidate) -> Void = { [weak self] (candidate) in
-                Logger.debug("\(self.TAG) \(#function) Ignoring obsolete event in terminated client")
+            SwiftAssertIsOnMainThread(#function)
-                return
+            guard let strongSelf = handle.get() else { return }
-            }
-            Logger.info("\(self.TAG) adding local ICE candidate:\(candidate.sdp)")
-            DispatchQueue.main.async { [weak self] in
-                guard let strongSelf = self else { return }
             guard let strongDelegate = strongSelf.delegate else { return }
             strongDelegate.peerConnectionClient(strongSelf, addedLocalIceCandidate: candidate)
         }
+
+        PeerConnectionClient.signalingQueue.async { [weak self] in
+            guard let strongSelf = handle.get() else { return }
+            guard let peerConnection = strongSelf.peerConnection else {
+                Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client")
+                return
+            }
+            guard peerConnection == peerConnectionParam else {
+                owsFail("\(strongSelf.logTag) in \(#function) mismatched peerConnection callback.")
+                return
+            }
+            Logger.info("\(strongSelf.logTag) adding local ICE candidate:\(candidate.sdp)")
+            DispatchQueue.main.async {
+                completion(candidate)
+            }
         }
     }
 
     /** Called when a group of local Ice candidates have been removed. */
-    internal func peerConnection(_ peerConnection: RTCPeerConnection, didRemove candidates: [RTCIceCandidate]) {
+    internal func peerConnection(_ peerConnectionParam: RTCPeerConnection, didRemove candidates: [RTCIceCandidate]) {
-        Logger.debug("\(TAG) didRemove IceCandidates:\(candidates)")
+        Logger.debug("\(logTag) didRemove IceCandidates:\(candidates)")
     }
 
     /** New data channel has been opened. */
-    internal func peerConnection(_ peerConnection: RTCPeerConnection, didOpen dataChannel: RTCDataChannel) {
+    internal func peerConnection(_ peerConnectionParam: RTCPeerConnection, didOpen dataChannel: RTCDataChannel) {
-        PeerConnectionClient.signalingQueue.async {
+        let handle = self.handle
-            guard self.peerConnection != nil else {
+        let completion: ([PendingDataChannelMessage]) -> Void = { [weak self] (pendingMessages) in
-                Logger.debug("\(self.TAG) \(#function) Ignoring obsolete event in terminated client")
+            SwiftAssertIsOnMainThread(#function)
-                return
+            guard let strongSelf = handle.get() else { return }
-            }
-            Logger.info("\(self.TAG) didOpen dataChannel:\(dataChannel)")
-            assert(self.dataChannel == nil)
-            self.dataChannel = dataChannel
-            dataChannel.delegate = self
-
-            let pendingMessages = self.pendingDataChannelMessages
-            self.pendingDataChannelMessages = []
-            DispatchQueue.main.async { [weak self] in
-                guard let strongSelf = self else { return }
-
             pendingMessages.forEach { message in
                 strongSelf.sendDataChannelMessage(data: message.data, description: message.description, isCritical: message.isCritical)
             }
         }
+
+        PeerConnectionClient.signalingQueue.async { [weak self] in
+            guard let strongSelf = handle.get() else { return }
+            guard let peerConnection = strongSelf.peerConnection else {
+                Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client")
+                return
+            }
+            guard peerConnection == peerConnectionParam else {
+                owsFail("\(strongSelf.logTag) in \(#function) mismatched peerConnection callback.")
+                return
+            }
+            Logger.info("\(strongSelf.logTag) didOpen dataChannel:\(dataChannel)")
+            if strongSelf.dataChannel != nil {
+                owsFail("\(strongSelf.logTag) in \(#function) dataChannel unexpectedly set twice.")
+            }
+            strongSelf.dataChannel = dataChannel
+            dataChannel.delegate = strongSelf
+
+            let pendingMessages = strongSelf.pendingDataChannelMessages
+            strongSelf.pendingDataChannelMessages = []
+            DispatchQueue.main.async {
+                completion(pendingMessages)
+            }
         }
     }
 
@@ -785,7 +962,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD
         var result: RTCPeerConnection? = nil
         PeerConnectionClient.signalingQueue.sync {
             result = peerConnection
-            Logger.info("\(self.TAG) called \(#function)")
+            Logger.info("\(self.logTag) called \(#function)")
         }
         return result!
     }
@@ -796,7 +973,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD
         var result: RTCDataChannel? = nil
         PeerConnectionClient.signalingQueue.sync {
             result = dataChannel
-            Logger.info("\(self.TAG) called \(#function)")
+            Logger.info("\(self.logTag) called \(#function)")
         }
         return result!
     }