From a4a5e995342818f8b81d1d7f5e5ccd09edb156d2 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Mon, 21 May 2018 10:55:48 -0400 Subject: [PATCH 01/14] PeerConnectionClient thread safety. --- Signal/src/call/PeerConnectionClient.swift | 619 ++++++++++++--------- 1 file changed, 365 insertions(+), 254 deletions(-) diff --git a/Signal/src/call/PeerConnectionClient.swift b/Signal/src/call/PeerConnectionClient.swift index 2a03883b4..26cd26163 100644 --- a/Signal/src/call/PeerConnectionClient.swift +++ b/Signal/src/call/PeerConnectionClient.swift @@ -74,7 +74,6 @@ protocol PeerConnectionClientDelegate: class { */ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelDelegate { - let TAG = "[PeerConnectionClient]" enum Identifiers: String { case mediaStream = "ARDAMS", videoTrack = "ARDAMSv0", @@ -95,7 +94,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,10 +120,6 @@ 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 @@ -139,10 +134,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"] @@ -175,6 +170,11 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD private func createSignalingDataChannel() { SwiftAssertIsOnMainThread(#function) + guard let peerConnection = peerConnection else { + owsFail("\(logTag) in \(#function) peerConnection was unexpectedly nil") + return + } + let configuration = RTCDataChannelConfiguration() // Insist upon an "ordered" TCP data channel for delivery reliability. configuration.isOrdered = true @@ -191,11 +191,15 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD fileprivate func createVideoSender() { SwiftAssertIsOnMainThread(#function) - Logger.debug("\(TAG) in \(#function)") + Logger.debug("\(logTag) 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 { + owsFail("\(logTag) in \(#function) peerConnection was unexpectedly nil") return } @@ -227,15 +231,16 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD public func setCameraSource(useBackCamera: Bool) { SwiftAssertIsOnMainThread(#function) - PeerConnectionClient.signalingQueue.async { - guard let localVideoSource = self.localVideoSource else { - owsFail("\(self.logTag) in \(#function) localVideoSource was unexpectedly nil") + PeerConnectionClient.signalingQueue.async { [weak self] in + guard let strongSelf = self else { return } + guard let localVideoSource = strongSelf.localVideoSource else { + owsFail("\(strongSelf.logTag) in \(#function) localVideoSource was unexpectedly nil") 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 } @@ -246,37 +251,40 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD public func setLocalVideoEnabled(enabled: Bool) { SwiftAssertIsOnMainThread(#function) - PeerConnectionClient.signalingQueue.async { - guard self.peerConnection != nil else { - Logger.debug("\(self.TAG) \(#function) Ignoring obsolete event in terminated client") + let completion = { [weak self] in + guard let strongSelf = self 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 { [weak self] in + guard let strongSelf = self else { return } + 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.error("\(self.TAG)) trying to \(action) videoTrack which doesn't exist") + Logger.error("\(strongSelf.logTag)) trying to \(action) videoTrack which doesn't exist") return } - guard let videoCaptureSession = self.videoCaptureSession else { - Logger.debug("\(self.TAG) videoCaptureSession was unexpectedly nil") + guard let videoCaptureSession = strongSelf.videoCaptureSession else { + Logger.debug("\(strongSelf.logTag) videoCaptureSession was unexpectedly nil") 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 - 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) - } + DispatchQueue.main.async(execute: completion) } } @@ -285,9 +293,14 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD fileprivate func createAudioSender() { SwiftAssertIsOnMainThread(#function) - Logger.debug("\(TAG) in \(#function)") + Logger.debug("\(logTag) in \(#function)") assert(self.audioSender == nil, "\(#function) should only be called once.") + guard let peerConnection = peerConnection else { + owsFail("\(logTag) in \(#function) peerConnection was unexpectedly nil") + return + } + let audioSource = factory.audioSource(with: self.audioConstraints) let audioTrack = factory.audioTrack(with: audioSource, trackId: Identifiers.audioTrack.rawValue) @@ -307,14 +320,15 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD public func setAudioEnabled(enabled: Bool) { SwiftAssertIsOnMainThread(#function) - PeerConnectionClient.signalingQueue.async { - guard self.peerConnection != nil else { - Logger.debug("\(self.TAG) \(#function) Ignoring obsolete event in terminated client") + PeerConnectionClient.signalingQueue.async { [weak self] in + guard let strongSelf = self else { return } + 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.error("\(self.TAG) trying to \(action) audioTrack which doesn't exist.") + Logger.error("\(strongSelf.logTag) trying to \(action) audioTrack which doesn't exist.") return } @@ -332,72 +346,103 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD return RTCMediaConstraints(mandatoryConstraints: mandatoryConstraints, optionalConstraints: nil) } + // TODO: Review all self.peerConnection + // TODO: Review all .async + // TODO: Review all error == nil public func createOffer() -> Promise { SwiftAssertIsOnMainThread(#function) + let completion: ((((HardenedRTCSessionDescription) -> Void), ((Error) -> Void), RTCSessionDescription?, Error?) -> Void) = { [weak self] (fulfill, reject, sdp, error) in + guard let strongSelf = self else { return } + strongSelf.assertOnSignalingQueue() + guard strongSelf.peerConnection != nil else { + Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client") + reject(NSError(domain: "Obsolete client", code: 0, userInfo: nil)) + return + } + if let error = error { + reject(error) + return + } + + guard let sessionDescription = sdp else { + Logger.error("\(strongSelf.logTag) No session description was obtained, even though there was no error reported.") + let error = OWSErrorMakeUnableToProcessServerResponseError() + reject(error) + return + } + + fulfill(HardenedRTCSessionDescription(rtcSessionDescription: sessionDescription)) + } + + let workBlock : ((@escaping ((HardenedRTCSessionDescription) -> Void), @escaping ((Error) -> Void)) -> Void) = { [weak self] (fulfill, reject) in + guard let strongSelf = self else { 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(fulfill, reject, sdp, error) + } + }) + } + return Promise { fulfill, reject in SwiftAssertIsOnMainThread(#function) 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 - } - - self.peerConnection.offer(for: self.defaultOfferConstraints, 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 { - reject(error!) - return - } - - guard let sessionDescription = sdp else { - Logger.error("\(self.TAG) No session description was obtained, even though there was no error reported.") - let error = OWSErrorMakeUnableToProcessServerResponseError() - reject(error) - return - } - - fulfill(HardenedRTCSessionDescription(rtcSessionDescription: sessionDescription)) - } - }) + workBlock(fulfill, reject) } } } + // TODO: Review all promises public func setLocalSessionDescriptionInternal(_ sessionDescription: HardenedRTCSessionDescription) -> Promise { - return PromiseKit.wrap { resolve in - self.assertOnSignalingQueue() - Logger.verbose("\(self.TAG) setting local session description: \(sessionDescription)") - self.peerConnection.setLocalDescription(sessionDescription.rtcSessionDescription, completionHandler: resolve) + return PromiseKit.wrap { [weak self] resolve in + guard let strongSelf = self else { return } + strongSelf.assertOnSignalingQueue() + + guard let peerConnection = peerConnection else { + owsFail("\(strongSelf.logTag) in \(#function) peerConnection was unexpectedly nil") + return + } + + Logger.verbose("\(strongSelf.logTag) setting local session description: \(sessionDescription)") + peerConnection.setLocalDescription(sessionDescription.rtcSessionDescription, completionHandler: resolve) } } + // TODO: Review all self. public func setLocalSessionDescription(_ sessionDescription: HardenedRTCSessionDescription) -> Promise { SwiftAssertIsOnMainThread(#function) + let workBlock : ((@escaping (() -> Void), @escaping ((Error) -> Void)) -> Void) = { [weak self] (fulfill, reject) in + guard let strongSelf = self else { 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 + guard error == nil else { + reject(error!) + return + } + fulfill() + }) + } + return Promise { fulfill, reject 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 - } - Logger.verbose("\(self.TAG) setting local session description: \(sessionDescription)") - self.peerConnection.setLocalDescription(sessionDescription.rtcSessionDescription, - completionHandler: { error in - guard error == nil else { - reject(error!) - return - } - fulfill() - }) + workBlock(fulfill, reject) } } } @@ -406,30 +451,41 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD SwiftAssertIsOnMainThread(#function) return setRemoteSessionDescription(remoteDescription) - .then(on: PeerConnectionClient.signalingQueue) { - return self.negotiateAnswerSessionDescription(constraints: constraints) + .then(on: PeerConnectionClient.signalingQueue) { [weak self] in + guard let strongSelf = self else { + return Promise { _, reject in + reject(NSError(domain: "Obsolete client", code: 0, userInfo: nil)) + } + } + return strongSelf.negotiateAnswerSessionDescription(constraints: constraints) } } public func setRemoteSessionDescription(_ sessionDescription: RTCSessionDescription) -> Promise { SwiftAssertIsOnMainThread(#function) + let workBlock : ((@escaping (() -> Void), @escaping ((Error) -> Void)) -> Void) = { [weak self] (fulfill, reject) in + guard let strongSelf = self else { 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 remote description: \(sessionDescription)") + peerConnection.setRemoteDescription(sessionDescription, + completionHandler: { error in + guard error == nil else { + reject(error!) + return + } + fulfill() + }) + } + return Promise { fulfill, reject 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 - } - Logger.verbose("\(self.TAG) setting remote description: \(sessionDescription)") - self.peerConnection.setRemoteDescription(sessionDescription, - completionHandler: { error in - guard error == nil else { - reject(error!) - return - } - fulfill() - }) + workBlock(fulfill, reject) } } } @@ -437,78 +493,87 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD private func negotiateAnswerSessionDescription(constraints: RTCMediaConstraints) -> Promise { assertOnSignalingQueue() - return Promise { fulfill, reject in - assertOnSignalingQueue() - - guard self.peerConnection != nil else { - Logger.debug("\(self.TAG) \(#function) Ignoring obsolete event in terminated client") + let completion : ((@escaping ((HardenedRTCSessionDescription) -> Void), @escaping ((Error) -> Void), RTCSessionDescription?, Error?) -> Void) = { [weak self] (fulfill, reject, sdp, error) in + guard let strongSelf = self else { return } + strongSelf.assertOnSignalingQueue() + guard strongSelf.peerConnection != nil else { + Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client") reject(NSError(domain: "Obsolete client", code: 0, userInfo: nil)) return } + if let error = error { + reject(error) + return + } - Logger.debug("\(self.TAG) negotiating answer session.") + guard let sessionDescription = sdp else { + Logger.error("\(strongSelf.logTag) unexpected empty session description, even though no error was reported.") + let error = OWSErrorMakeUnableToProcessServerResponseError() + reject(error) + return + } - 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 { - reject(error!) - return - } + let hardenedSessionDescription = HardenedRTCSessionDescription(rtcSessionDescription: sessionDescription) - guard let sessionDescription = sdp else { - Logger.error("\(self.TAG) unexpected empty session description, even though no error was reported.") - let error = OWSErrorMakeUnableToProcessServerResponseError() - reject(error) - return - } + strongSelf.setLocalSessionDescriptionInternal(hardenedSessionDescription) + .then(on: PeerConnectionClient.signalingQueue) { _ in + fulfill(hardenedSessionDescription) + }.catch { error in + reject(error) + } + } - let hardenedSessionDescription = HardenedRTCSessionDescription(rtcSessionDescription: sessionDescription) + return Promise { [weak self] fulfill, reject in + guard let strongSelf = self else { return } + strongSelf.assertOnSignalingQueue() - self.setLocalSessionDescriptionInternal(hardenedSessionDescription) - .then(on: PeerConnectionClient.signalingQueue) { - fulfill(hardenedSessionDescription) - }.catch { error in - reject(error) - } + guard let peerConnection = strongSelf.peerConnection else { + Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client") + return + } + + Logger.debug("\(strongSelf.logTag) negotiating answer session.") + + peerConnection.answer(for: constraints, completionHandler: { (sdp: RTCSessionDescription?, error: Error?) in + PeerConnectionClient.signalingQueue.async { + completion(fulfill, reject, sdp, error) } }) } } public func addRemoteIceCandidate(_ candidate: RTCIceCandidate) { - PeerConnectionClient.signalingQueue.async { - guard self.peerConnection != nil else { - Logger.debug("\(self.TAG) \(#function) Ignoring obsolete event in terminated client") + PeerConnectionClient.signalingQueue.async { [weak self] in + guard let strongSelf = self 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)") - self.peerConnection.add(candidate) + Logger.info("\(strongSelf.logTag) adding remote ICE candidate: \(candidate.sdp)") + 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 - PeerConnectionClient.signalingQueue.async { - assert(self.peerConnection != nil) - self.terminateInternal() + PeerConnectionClient.signalingQueue.async { [weak self] in + guard let strongSelf = self else { return } + + strongSelf.terminateInternal() } } private func terminateInternal() { assertOnSignalingQueue() - Logger.debug("\(TAG) in \(#function)") + Logger.debug("\(logTag) 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,9 +588,6 @@ 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 @@ -537,11 +599,11 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD localVideoTrack = nil remoteVideoTrack = nil - peerConnection.delegate = nil - peerConnection.close() + if let peerConnection = peerConnection { + peerConnection.delegate = nil + peerConnection.close() + } peerConnection = nil - - objc_sync_exit(self) } // MARK: - Data Channel @@ -557,31 +619,33 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD public func sendDataChannelMessage(data: Data, description: String, isCritical: Bool) { SwiftAssertIsOnMainThread(#function) - PeerConnectionClient.signalingQueue.async { - guard self.peerConnection != nil else { - Logger.debug("\(self.TAG) \(#function) Ignoring obsolete event in terminated client: \(description)") + PeerConnectionClient.signalingQueue.async { [weak self] in + guard let strongSelf = self else { return } + + guard strongSelf.peerConnection != nil else { + Logger.debug("\(strongSelf.logTag) \(#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)") - self.pendingDataChannelMessages.append(PendingDataChannelMessage(data: data, description: description, isCritical: isCritical)) + Logger.info("\(strongSelf.logTag) in \(#function) enqueuing critical data channel message for after we have a dataChannel: \(description)") + 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,177 +657,224 @@ 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 { - guard self.peerConnection != nil else { - Logger.debug("\(self.TAG) \(#function) Ignoring obsolete event in terminated client") + + let completion: (OWSWebRTCProtosData) -> Void = { [weak self] (dataChannelMessage) in + SwiftAssertIsOnMainThread(#function) + guard let strongSelf = self else { return } + guard let strongDelegate = strongSelf.delegate else { return } + strongDelegate.peerConnectionClient(strongSelf, received: dataChannelMessage) + } + + PeerConnectionClient.signalingQueue.async { [weak self] in + guard let strongSelf = self 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 - guard let strongSelf = self else { return } - guard let strongDelegate = strongSelf.delegate else { return } - strongDelegate.peerConnectionClient(strongSelf, received: dataChannelMessage) + DispatchQueue.main.async { + completion(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) { - Logger.debug("\(TAG) didChange signalingState:\(stateChanged.debugDescription)") + internal func peerConnection(_ peerConnectionParam: RTCPeerConnection, didChange stateChanged: RTCSignalingState) { + Logger.debug("\(logTag) didChange signalingState:\(stateChanged.debugDescription)") } + // TODO: Review all peerConnection params + // TODO: There's work being done here on the wrong queue. /** 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 - } - let remoteVideoTrack = stream.videoTracks[0] - Logger.debug("\(self.TAG) didAdd stream:\(stream) video tracks: \(stream.videoTracks.count) audio tracks: \(stream.audioTracks.count)") + internal func peerConnection(_ peerConnectionParam: RTCPeerConnection, didAdd stream: RTCMediaStream) { - // See the comments on the remoteVideoTrack property. - // - // We only set the remoteVideoTrack property if peerConnection is non-nil. - objc_sync_enter(self) - if self.peerConnection != nil { - self.remoteVideoTrack = remoteVideoTrack + let completion: (RTCVideoTrack) -> Void = { [weak self] (remoteVideoTrack) in + SwiftAssertIsOnMainThread(#function) + guard let strongSelf = self else { return } + guard let strongDelegate = strongSelf.delegate else { return } + + // TODO: Consider checking for termination here. + + strongDelegate.peerConnectionClient(strongSelf, didUpdateRemote: remoteVideoTrack) } - objc_sync_exit(self) - PeerConnectionClient.signalingQueue.async { - guard self.peerConnection != nil else { - Logger.debug("\(self.TAG) \(#function) Ignoring obsolete event in terminated client") + PeerConnectionClient.signalingQueue.async { [weak self] in + guard let strongSelf = self else { return } + + guard let peerConnection = strongSelf.peerConnection else { + owsFail("\(strongSelf.logTag) in \(#function) peerConnection was unexpectedly nil") return } + guard peerConnection == peerConnectionParam else { + owsFail("\(strongSelf.logTag) in \(#function) mismatched peerConnection callback.") + return + } + guard stream.videoTracks.count > 0 else { + owsFail("\(strongSelf.logTag) in \(#function) didAdd stream missing stream.") + return + } + let remoteVideoTrack = stream.videoTracks[0] + Logger.debug("\(strongSelf.logTag) didAdd stream:\(stream) video tracks: \(stream.videoTracks.count) audio tracks: \(stream.audioTracks.count)") - DispatchQueue.main.async { [weak self] in - guard let strongSelf = self else { return } - guard let strongDelegate = strongSelf.delegate else { return } - - // See the comments on the remoteVideoTrack property. - // - // 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) + strongSelf.remoteVideoTrack = remoteVideoTrack - strongDelegate.peerConnectionClient(strongSelf, didUpdateRemote: remoteVideoTrack) + DispatchQueue.main.async { + completion(remoteVideoTrack) } } } /** Called when a remote peer closes a stream. */ - internal func peerConnection(_ peerConnection: RTCPeerConnection, didRemove stream: RTCMediaStream) { - Logger.debug("\(TAG) didRemove Stream:\(stream)") + internal func peerConnection(_ peerConnectionParam: RTCPeerConnection, didRemove stream: RTCMediaStream) { + Logger.debug("\(logTag) didRemove Stream:\(stream)") } /** Called when negotiation is needed, for example ICE has restarted. */ - internal func peerConnectionShouldNegotiate(_ peerConnection: RTCPeerConnection) { - Logger.debug("\(TAG) shouldNegotiate") + internal func peerConnectionShouldNegotiate(_ peerConnectionParam: RTCPeerConnection) { + Logger.debug("\(logTag) shouldNegotiate") } /** Called any time the IceConnectionState changes. */ - internal func peerConnection(_ peerConnection: RTCPeerConnection, didChange newState: RTCIceConnectionState) { - PeerConnectionClient.signalingQueue.async { - guard self.peerConnection != nil else { - Logger.debug("\(self.TAG) \(#function) Ignoring obsolete event in terminated client") + internal func peerConnection(_ peerConnectionParam: RTCPeerConnection, didChange newState: RTCIceConnectionState) { + + let connectedCompletion : () -> Void = { [weak self] in + SwiftAssertIsOnMainThread(#function) + guard let strongSelf = self else { return } + guard let strongDelegate = strongSelf.delegate else { return } + strongDelegate.peerConnectionClientIceConnected(strongSelf) + } + let failedCompletion : () -> Void = { [weak self] in + SwiftAssertIsOnMainThread(#function) + guard let strongSelf = self else { return } + guard let strongDelegate = strongSelf.delegate else { return } + strongDelegate.peerConnectionClientIceFailed(strongSelf) + } + let disconnectedCompletion : () -> Void = { [weak self] in + SwiftAssertIsOnMainThread(#function) + 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 = self else { return } + + guard let peerConnection = strongSelf.peerConnection else { + owsFail("\(strongSelf.logTag) in \(#function) peerConnection was unexpectedly nil") return } - Logger.info("\(self.TAG) didChange IceConnectionState:\(newState.debugDescription)") + 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 { [weak self] in - guard let strongSelf = self else { return } - guard let strongDelegate = strongSelf.delegate else { return } - strongDelegate.peerConnectionClientIceConnected(strongSelf) - } + DispatchQueue.main.async(execute: connectedCompletion) case .failed: - Logger.warn("\(self.TAG) RTCIceConnection failed.") - DispatchQueue.main.async { [weak self] in - guard let strongSelf = self else { return } - guard let strongDelegate = strongSelf.delegate else { return } - strongDelegate.peerConnectionClientIceFailed(strongSelf) - } + Logger.warn("\(strongSelf.logTag) RTCIceConnection failed.") + DispatchQueue.main.async(execute: failedCompletion) case .disconnected: - Logger.warn("\(self.TAG) RTCIceConnection disconnected.") - DispatchQueue.main.async { [weak self] in - guard let strongSelf = self else { return } - guard let strongDelegate = strongSelf.delegate else { return } - strongDelegate.peerConnectionClientIceDisconnected(strongSelf) - } + 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) { - Logger.info("\(TAG) didChange IceGatheringState:\(newState.debugDescription)") + internal func peerConnection(_ peerConnectionParam: RTCPeerConnection, didChange newState: RTCIceGatheringState) { + Logger.info("\(logTag) didChange IceGatheringState:\(newState.debugDescription)") } /** New ice candidate has been found. */ - internal func peerConnection(_ peerConnection: RTCPeerConnection, didGenerate candidate: RTCIceCandidate) { - PeerConnectionClient.signalingQueue.async { - guard self.peerConnection != nil else { - Logger.debug("\(self.TAG) \(#function) Ignoring obsolete event in terminated client") + internal func peerConnection(_ peerConnectionParam: RTCPeerConnection, didGenerate candidate: RTCIceCandidate) { + + let completion: (RTCIceCandidate) -> Void = { [weak self] (candidate) in + SwiftAssertIsOnMainThread(#function) + 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 = self else { return } + + guard let peerConnection = strongSelf.peerConnection else { + owsFail("\(strongSelf.logTag) in \(#function) peerConnection was unexpectedly nil") + return + } + guard peerConnection == peerConnectionParam else { + owsFail("\(strongSelf.logTag) in \(#function) mismatched peerConnection callback.") 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) + 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]) { - Logger.debug("\(TAG) didRemove IceCandidates:\(candidates)") + internal func peerConnection(_ peerConnectionParam: RTCPeerConnection, didRemove candidates: [RTCIceCandidate]) { + Logger.debug("\(logTag) didRemove IceCandidates:\(candidates)") } /** New data channel has been opened. */ - internal func peerConnection(_ peerConnection: RTCPeerConnection, didOpen dataChannel: RTCDataChannel) { - PeerConnectionClient.signalingQueue.async { - guard self.peerConnection != nil else { - Logger.debug("\(self.TAG) \(#function) Ignoring obsolete event in terminated client") + internal func peerConnection(_ peerConnectionParam: RTCPeerConnection, didOpen dataChannel: RTCDataChannel) { + + let completion: ([PendingDataChannelMessage]) -> Void = { [weak self] (pendingMessages) in + SwiftAssertIsOnMainThread(#function) + 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 = self else { return } + + guard let peerConnection = strongSelf.peerConnection else { + owsFail("\(strongSelf.logTag) in \(#function) peerConnection was unexpectedly nil") + return + } + guard peerConnection == peerConnectionParam else { + owsFail("\(strongSelf.logTag) in \(#function) mismatched peerConnection callback.") return } - Logger.info("\(self.TAG) didOpen dataChannel:\(dataChannel)") - assert(self.dataChannel == nil) - self.dataChannel = dataChannel + 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 = 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) - } + let pendingMessages = strongSelf.pendingDataChannelMessages + strongSelf.pendingDataChannelMessages = [] + DispatchQueue.main.async { + completion(pendingMessages) } } } @@ -785,7 +896,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 +907,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! } From 729769afa7bc2fb1611320c6ebed42e40ba003ac Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Mon, 21 May 2018 11:01:25 -0400 Subject: [PATCH 02/14] PeerConnectionClient thread safety. --- Signal/src/call/PeerConnectionClient.swift | 4 ---- 1 file changed, 4 deletions(-) diff --git a/Signal/src/call/PeerConnectionClient.swift b/Signal/src/call/PeerConnectionClient.swift index 26cd26163..d424eb51a 100644 --- a/Signal/src/call/PeerConnectionClient.swift +++ b/Signal/src/call/PeerConnectionClient.swift @@ -346,7 +346,6 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD return RTCMediaConstraints(mandatoryConstraints: mandatoryConstraints, optionalConstraints: nil) } - // TODO: Review all self.peerConnection // TODO: Review all .async // TODO: Review all error == nil public func createOffer() -> Promise { @@ -416,7 +415,6 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD } } - // TODO: Review all self. public func setLocalSessionDescription(_ sessionDescription: HardenedRTCSessionDescription) -> Promise { SwiftAssertIsOnMainThread(#function) @@ -703,8 +701,6 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD Logger.debug("\(logTag) didChange signalingState:\(stateChanged.debugDescription)") } - // TODO: Review all peerConnection params - // TODO: There's work being done here on the wrong queue. /** Called when media is received on a new stream from remote peer. */ internal func peerConnection(_ peerConnectionParam: RTCPeerConnection, didAdd stream: RTCMediaStream) { From 1a0347b78207471f121b164a88bd8850a2c27729 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Mon, 21 May 2018 11:04:07 -0400 Subject: [PATCH 03/14] PeerConnectionClient thread safety. --- Signal/src/call/PeerConnectionClient.swift | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/Signal/src/call/PeerConnectionClient.swift b/Signal/src/call/PeerConnectionClient.swift index d424eb51a..af3ba8384 100644 --- a/Signal/src/call/PeerConnectionClient.swift +++ b/Signal/src/call/PeerConnectionClient.swift @@ -346,8 +346,6 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD return RTCMediaConstraints(mandatoryConstraints: mandatoryConstraints, optionalConstraints: nil) } - // TODO: Review all .async - // TODO: Review all error == nil public func createOffer() -> Promise { SwiftAssertIsOnMainThread(#function) @@ -399,7 +397,6 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD } } - // TODO: Review all promises public func setLocalSessionDescriptionInternal(_ sessionDescription: HardenedRTCSessionDescription) -> Promise { return PromiseKit.wrap { [weak self] resolve in guard let strongSelf = self else { return } @@ -430,8 +427,8 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD Logger.verbose("\(strongSelf.logTag) setting local session description: \(sessionDescription)") peerConnection.setLocalDescription(sessionDescription.rtcSessionDescription, completionHandler: { error in - guard error == nil else { - reject(error!) + if let error = error { + reject(error) return } fulfill() @@ -473,8 +470,8 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD Logger.verbose("\(strongSelf.logTag) setting remote description: \(sessionDescription)") peerConnection.setRemoteDescription(sessionDescription, completionHandler: { error in - guard error == nil else { - reject(error!) + if let error = error { + reject(error) return } fulfill() From c3e8fde24cfb6fa8f7e315ccc787fef5e6fc5e0e Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Mon, 21 May 2018 12:00:17 -0400 Subject: [PATCH 04/14] PeerConnectionClient thread safety. --- Signal/src/call/PeerConnectionClient.swift | 138 ++++++++++++++++++++- 1 file changed, 137 insertions(+), 1 deletion(-) diff --git a/Signal/src/call/PeerConnectionClient.swift b/Signal/src/call/PeerConnectionClient.swift index af3ba8384..7ecb51b04 100644 --- a/Signal/src/call/PeerConnectionClient.swift +++ b/Signal/src/call/PeerConnectionClient.swift @@ -74,6 +74,8 @@ protocol PeerConnectionClientDelegate: class { */ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelDelegate { +// private class AtomicFlag + enum Identifiers: String { case mediaStream = "ARDAMS", videoTrack = "ARDAMSv0", @@ -162,7 +164,10 @@ 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") + Logger.flush() } // MARK: - Media Streams @@ -233,6 +238,12 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD PeerConnectionClient.signalingQueue.async { [weak self] in guard let strongSelf = self else { return } + Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() + defer { + DispatchQueue.main.async { + Logger.info("\(strongSelf.logTag) \(#function) complete."); Logger.flush() + } + } guard let localVideoSource = strongSelf.localVideoSource else { owsFail("\(strongSelf.logTag) in \(#function) localVideoSource was unexpectedly nil") return @@ -260,6 +271,12 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD PeerConnectionClient.signalingQueue.async { [weak self] in guard let strongSelf = self else { return } + Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() + defer { + DispatchQueue.main.async { + Logger.info("\(strongSelf.logTag) \(#function) complete."); Logger.flush() + } + } guard strongSelf.peerConnection != nil else { Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client") return @@ -320,8 +337,21 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD public func setAudioEnabled(enabled: Bool) { SwiftAssertIsOnMainThread(#function) + Logger.info("\(self.logTag) setAudioEnabled 1.") + Logger.flush() + PeerConnectionClient.signalingQueue.async { [weak self] in guard let strongSelf = self else { return } + Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() + defer { + DispatchQueue.main.async { + Logger.info("\(strongSelf.logTag) \(#function) complete."); Logger.flush() + } + } + + Logger.info("\(strongSelf.logTag) setAudioEnabled 2.") + Logger.flush() + guard strongSelf.peerConnection != nil else { Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client") return @@ -333,6 +363,9 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD } audioTrack.isEnabled = enabled + + Logger.info("\(strongSelf.logTag) setAudioEnabled 3.") + Logger.flush() } } @@ -351,6 +384,12 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD let completion: ((((HardenedRTCSessionDescription) -> Void), ((Error) -> Void), RTCSessionDescription?, Error?) -> Void) = { [weak self] (fulfill, reject, sdp, error) in guard let strongSelf = self else { return } + Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() + defer { + DispatchQueue.main.async { + Logger.info("\(strongSelf.logTag) \(#function) complete."); Logger.flush() + } + } strongSelf.assertOnSignalingQueue() guard strongSelf.peerConnection != nil else { Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client") @@ -374,6 +413,12 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD let workBlock : ((@escaping ((HardenedRTCSessionDescription) -> Void), @escaping ((Error) -> Void)) -> Void) = { [weak self] (fulfill, reject) in guard let strongSelf = self else { return } + Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() + defer { + DispatchQueue.main.async { + Logger.info("\(strongSelf.logTag) \(#function) complete."); Logger.flush() + } + } strongSelf.assertOnSignalingQueue() guard let peerConnection = strongSelf.peerConnection else { Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client") @@ -400,6 +445,12 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD public func setLocalSessionDescriptionInternal(_ sessionDescription: HardenedRTCSessionDescription) -> Promise { return PromiseKit.wrap { [weak self] resolve in guard let strongSelf = self else { return } + Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() + defer { + DispatchQueue.main.async { + Logger.info("\(strongSelf.logTag) \(#function) complete."); Logger.flush() + } + } strongSelf.assertOnSignalingQueue() guard let peerConnection = peerConnection else { @@ -417,6 +468,12 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD let workBlock : ((@escaping (() -> Void), @escaping ((Error) -> Void)) -> Void) = { [weak self] (fulfill, reject) in guard let strongSelf = self else { return } + Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() + defer { + DispatchQueue.main.async { + Logger.info("\(strongSelf.logTag) \(#function) complete."); Logger.flush() + } + } strongSelf.assertOnSignalingQueue() guard let peerConnection = strongSelf.peerConnection else { Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client") @@ -452,6 +509,12 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD reject(NSError(domain: "Obsolete client", code: 0, userInfo: nil)) } } + Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() + defer { + DispatchQueue.main.async { + Logger.info("\(strongSelf.logTag) \(#function) complete."); Logger.flush() + } + } return strongSelf.negotiateAnswerSessionDescription(constraints: constraints) } } @@ -461,6 +524,12 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD let workBlock : ((@escaping (() -> Void), @escaping ((Error) -> Void)) -> Void) = { [weak self] (fulfill, reject) in guard let strongSelf = self else { return } + Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() + defer { + DispatchQueue.main.async { + Logger.info("\(strongSelf.logTag) \(#function) complete."); Logger.flush() + } + } strongSelf.assertOnSignalingQueue() guard let peerConnection = strongSelf.peerConnection else { Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client") @@ -490,6 +559,12 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD let completion : ((@escaping ((HardenedRTCSessionDescription) -> Void), @escaping ((Error) -> Void), RTCSessionDescription?, Error?) -> Void) = { [weak self] (fulfill, reject, sdp, error) in guard let strongSelf = self else { return } + Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() + defer { + DispatchQueue.main.async { + Logger.info("\(strongSelf.logTag) \(#function) complete."); Logger.flush() + } + } strongSelf.assertOnSignalingQueue() guard strongSelf.peerConnection != nil else { Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client") @@ -520,6 +595,12 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD return Promise { [weak self] fulfill, reject in guard let strongSelf = self else { return } + Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() + defer { + DispatchQueue.main.async { + Logger.info("\(strongSelf.logTag) \(#function) complete."); Logger.flush() + } + } strongSelf.assertOnSignalingQueue() guard let peerConnection = strongSelf.peerConnection else { @@ -540,6 +621,12 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD public func addRemoteIceCandidate(_ candidate: RTCIceCandidate) { PeerConnectionClient.signalingQueue.async { [weak self] in guard let strongSelf = self else { return } + Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() + defer { + DispatchQueue.main.async { + Logger.info("\(strongSelf.logTag) \(#function) complete."); Logger.flush() + } + } guard let peerConnection = strongSelf.peerConnection else { Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client") @@ -553,6 +640,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD public func terminate() { SwiftAssertIsOnMainThread(#function) Logger.debug("\(logTag) in \(#function)") + Logger.flush() // Clear the delegate immediately so that we can guarantee that // no delegate methods are called after terminate() returns. @@ -560,6 +648,12 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD PeerConnectionClient.signalingQueue.async { [weak self] in guard let strongSelf = self else { return } + Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() + defer { + DispatchQueue.main.async { + Logger.info("\(strongSelf.logTag) \(#function) complete."); Logger.flush() + } + } strongSelf.terminateInternal() } @@ -569,6 +663,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD assertOnSignalingQueue() Logger.debug("\(logTag) in \(#function)") + Logger.flush() // 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 @@ -599,6 +694,9 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD peerConnection.close() } peerConnection = nil + + Logger.debug("\(logTag) in \(#function) complete") + Logger.flush() } // MARK: - Data Channel @@ -616,6 +714,12 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD PeerConnectionClient.signalingQueue.async { [weak self] in guard let strongSelf = self else { return } + Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() + defer { + DispatchQueue.main.async { + Logger.info("\(strongSelf.logTag) \(#function) complete."); Logger.flush() + } + } guard strongSelf.peerConnection != nil else { Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client: \(description)") @@ -667,6 +771,12 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD PeerConnectionClient.signalingQueue.async { [weak self] in guard let strongSelf = self else { return } + Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() + defer { + DispatchQueue.main.async { + Logger.info("\(strongSelf.logTag) \(#function) complete."); Logger.flush() + } + } guard strongSelf.peerConnection != nil else { Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client") @@ -713,6 +823,12 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD PeerConnectionClient.signalingQueue.async { [weak self] in guard let strongSelf = self else { return } + Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() + defer { + DispatchQueue.main.async { + Logger.info("\(strongSelf.logTag) \(#function) complete."); Logger.flush() + } + } guard let peerConnection = strongSelf.peerConnection else { owsFail("\(strongSelf.logTag) in \(#function) peerConnection was unexpectedly nil") @@ -771,6 +887,12 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD PeerConnectionClient.signalingQueue.async { [weak self] in guard let strongSelf = self else { return } + Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() + defer { + DispatchQueue.main.async { + Logger.info("\(strongSelf.logTag) \(#function) complete."); Logger.flush() + } + } guard let peerConnection = strongSelf.peerConnection else { owsFail("\(strongSelf.logTag) in \(#function) peerConnection was unexpectedly nil") @@ -814,6 +936,12 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD PeerConnectionClient.signalingQueue.async { [weak self] in guard let strongSelf = self else { return } + Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() + defer { + DispatchQueue.main.async { + Logger.info("\(strongSelf.logTag) \(#function) complete."); Logger.flush() + } + } guard let peerConnection = strongSelf.peerConnection else { owsFail("\(strongSelf.logTag) in \(#function) peerConnection was unexpectedly nil") @@ -848,6 +976,12 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD PeerConnectionClient.signalingQueue.async { [weak self] in guard let strongSelf = self else { return } + Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() + defer { + DispatchQueue.main.async { + Logger.info("\(strongSelf.logTag) \(#function) complete."); Logger.flush() + } + } guard let peerConnection = strongSelf.peerConnection else { owsFail("\(strongSelf.logTag) in \(#function) peerConnection was unexpectedly nil") @@ -912,6 +1046,8 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD // Noop. } } + + private func } /** From e63a7f8fb0a723332c284270779c77a366b0eaf6 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Mon, 21 May 2018 12:39:49 -0400 Subject: [PATCH 05/14] PeerConnectionClient thread safety. --- Signal/src/call/PeerConnectionClient.swift | 201 ++++++++++++++------- 1 file changed, 133 insertions(+), 68 deletions(-) diff --git a/Signal/src/call/PeerConnectionClient.swift b/Signal/src/call/PeerConnectionClient.swift index 7ecb51b04..18bd90cf7 100644 --- a/Signal/src/call/PeerConnectionClient.swift +++ b/Signal/src/call/PeerConnectionClient.swift @@ -174,7 +174,9 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD private func createSignalingDataChannel() { SwiftAssertIsOnMainThread(#function) - + defer { + ensureTeardown(#function) + } guard let peerConnection = peerConnection else { owsFail("\(logTag) in \(#function) peerConnection was unexpectedly nil") return @@ -195,6 +197,9 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD fileprivate func createVideoSender() { SwiftAssertIsOnMainThread(#function) + defer { + ensureTeardown(#function) + } Logger.debug("\(logTag) in \(#function)") assert(self.videoSender == nil, "\(#function) should only be called once.") @@ -240,9 +245,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD guard let strongSelf = self else { return } Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() defer { - DispatchQueue.main.async { - Logger.info("\(strongSelf.logTag) \(#function) complete."); Logger.flush() - } + strongSelf.ensureTeardown(#function) } guard let localVideoSource = strongSelf.localVideoSource else { owsFail("\(strongSelf.logTag) in \(#function) localVideoSource was unexpectedly nil") @@ -261,9 +264,15 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD public func setLocalVideoEnabled(enabled: Bool) { SwiftAssertIsOnMainThread(#function) + defer { + ensureTeardown(#function) + } let completion = { [weak self] in guard let strongSelf = self else { return } + defer { + strongSelf.ensureTeardown(#function) + } guard let localVideoTrack = strongSelf.localVideoTrack else { return } guard let strongDelegate = strongSelf.delegate else { return } strongDelegate.peerConnectionClient(strongSelf, didUpdateLocal: enabled ? localVideoTrack : nil) @@ -273,9 +282,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD guard let strongSelf = self else { return } Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() defer { - DispatchQueue.main.async { - Logger.info("\(strongSelf.logTag) \(#function) complete."); Logger.flush() - } + strongSelf.ensureTeardown(#function) } guard strongSelf.peerConnection != nil else { Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client") @@ -309,6 +316,9 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD fileprivate func createAudioSender() { SwiftAssertIsOnMainThread(#function) + defer { + ensureTeardown(#function) + } Logger.debug("\(logTag) in \(#function)") assert(self.audioSender == nil, "\(#function) should only be called once.") @@ -336,22 +346,17 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD public func setAudioEnabled(enabled: Bool) { SwiftAssertIsOnMainThread(#function) - - Logger.info("\(self.logTag) setAudioEnabled 1.") - Logger.flush() + defer { + ensureTeardown(#function) + } PeerConnectionClient.signalingQueue.async { [weak self] in guard let strongSelf = self else { return } Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() defer { - DispatchQueue.main.async { - Logger.info("\(strongSelf.logTag) \(#function) complete."); Logger.flush() - } + strongSelf.ensureTeardown(#function) } - Logger.info("\(strongSelf.logTag) setAudioEnabled 2.") - Logger.flush() - guard strongSelf.peerConnection != nil else { Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client") return @@ -363,9 +368,6 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD } audioTrack.isEnabled = enabled - - Logger.info("\(strongSelf.logTag) setAudioEnabled 3.") - Logger.flush() } } @@ -381,14 +383,15 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD public func createOffer() -> Promise { SwiftAssertIsOnMainThread(#function) + defer { + ensureTeardown(#function) + } let completion: ((((HardenedRTCSessionDescription) -> Void), ((Error) -> Void), RTCSessionDescription?, Error?) -> Void) = { [weak self] (fulfill, reject, sdp, error) in guard let strongSelf = self else { return } Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() defer { - DispatchQueue.main.async { - Logger.info("\(strongSelf.logTag) \(#function) complete."); Logger.flush() - } + strongSelf.ensureTeardown(#function) } strongSelf.assertOnSignalingQueue() guard strongSelf.peerConnection != nil else { @@ -415,9 +418,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD guard let strongSelf = self else { return } Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() defer { - DispatchQueue.main.async { - Logger.info("\(strongSelf.logTag) \(#function) complete."); Logger.flush() - } + strongSelf.ensureTeardown(#function) } strongSelf.assertOnSignalingQueue() guard let peerConnection = strongSelf.peerConnection else { @@ -443,13 +444,15 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD } public func setLocalSessionDescriptionInternal(_ sessionDescription: HardenedRTCSessionDescription) -> Promise { + defer { + ensureTeardown(#function) + } + return PromiseKit.wrap { [weak self] resolve in guard let strongSelf = self else { return } Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() defer { - DispatchQueue.main.async { - Logger.info("\(strongSelf.logTag) \(#function) complete."); Logger.flush() - } + strongSelf.ensureTeardown(#function) } strongSelf.assertOnSignalingQueue() @@ -465,14 +468,15 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD public func setLocalSessionDescription(_ sessionDescription: HardenedRTCSessionDescription) -> Promise { SwiftAssertIsOnMainThread(#function) + defer { + ensureTeardown(#function) + } let workBlock : ((@escaping (() -> Void), @escaping ((Error) -> Void)) -> Void) = { [weak self] (fulfill, reject) in guard let strongSelf = self else { return } Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() defer { - DispatchQueue.main.async { - Logger.info("\(strongSelf.logTag) \(#function) complete."); Logger.flush() - } + strongSelf.ensureTeardown(#function) } strongSelf.assertOnSignalingQueue() guard let peerConnection = strongSelf.peerConnection else { @@ -501,6 +505,9 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD public func negotiateSessionDescription(remoteDescription: RTCSessionDescription, constraints: RTCMediaConstraints) -> Promise { SwiftAssertIsOnMainThread(#function) + defer { + ensureTeardown(#function) + } return setRemoteSessionDescription(remoteDescription) .then(on: PeerConnectionClient.signalingQueue) { [weak self] in @@ -511,9 +518,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD } Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() defer { - DispatchQueue.main.async { - Logger.info("\(strongSelf.logTag) \(#function) complete."); Logger.flush() - } + strongSelf.ensureTeardown(#function) } return strongSelf.negotiateAnswerSessionDescription(constraints: constraints) } @@ -521,14 +526,15 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD public func setRemoteSessionDescription(_ sessionDescription: RTCSessionDescription) -> Promise { SwiftAssertIsOnMainThread(#function) + defer { + ensureTeardown(#function) + } let workBlock : ((@escaping (() -> Void), @escaping ((Error) -> Void)) -> Void) = { [weak self] (fulfill, reject) in guard let strongSelf = self else { return } Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() defer { - DispatchQueue.main.async { - Logger.info("\(strongSelf.logTag) \(#function) complete."); Logger.flush() - } + strongSelf.ensureTeardown(#function) } strongSelf.assertOnSignalingQueue() guard let peerConnection = strongSelf.peerConnection else { @@ -556,14 +562,15 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD private func negotiateAnswerSessionDescription(constraints: RTCMediaConstraints) -> Promise { assertOnSignalingQueue() + defer { + ensureTeardown(#function) + } let completion : ((@escaping ((HardenedRTCSessionDescription) -> Void), @escaping ((Error) -> Void), RTCSessionDescription?, Error?) -> Void) = { [weak self] (fulfill, reject, sdp, error) in guard let strongSelf = self else { return } Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() defer { - DispatchQueue.main.async { - Logger.info("\(strongSelf.logTag) \(#function) complete."); Logger.flush() - } + strongSelf.ensureTeardown(#function) } strongSelf.assertOnSignalingQueue() guard strongSelf.peerConnection != nil else { @@ -597,9 +604,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD guard let strongSelf = self else { return } Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() defer { - DispatchQueue.main.async { - Logger.info("\(strongSelf.logTag) \(#function) complete."); Logger.flush() - } + strongSelf.ensureTeardown(#function) } strongSelf.assertOnSignalingQueue() @@ -619,13 +624,14 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD } public func addRemoteIceCandidate(_ candidate: RTCIceCandidate) { + defer { + ensureTeardown(#function) + } PeerConnectionClient.signalingQueue.async { [weak self] in guard let strongSelf = self else { return } Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() defer { - DispatchQueue.main.async { - Logger.info("\(strongSelf.logTag) \(#function) complete."); Logger.flush() - } + strongSelf.ensureTeardown(#function) } guard let peerConnection = strongSelf.peerConnection else { @@ -639,6 +645,10 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD public func terminate() { SwiftAssertIsOnMainThread(#function) + defer { + ensureTeardown(#function) + } + Logger.debug("\(logTag) in \(#function)") Logger.flush() @@ -650,9 +660,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD guard let strongSelf = self else { return } Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() defer { - DispatchQueue.main.async { - Logger.info("\(strongSelf.logTag) \(#function) complete."); Logger.flush() - } + strongSelf.ensureTeardown(#function) } strongSelf.terminateInternal() @@ -661,6 +669,9 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD private func terminateInternal() { assertOnSignalingQueue() + defer { + ensureTeardown(#function) + } Logger.debug("\(logTag) in \(#function)") Logger.flush() @@ -711,14 +722,15 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD public func sendDataChannelMessage(data: Data, description: String, isCritical: Bool) { SwiftAssertIsOnMainThread(#function) + defer { + ensureTeardown(#function) + } PeerConnectionClient.signalingQueue.async { [weak self] in guard let strongSelf = self else { return } Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() defer { - DispatchQueue.main.async { - Logger.info("\(strongSelf.logTag) \(#function) complete."); Logger.flush() - } + strongSelf.ensureTeardown(#function) } guard strongSelf.peerConnection != nil else { @@ -756,15 +768,24 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD /** The data channel state changed. */ internal func dataChannelDidChangeState(_ dataChannel: RTCDataChannel) { + defer { + ensureTeardown(#function) + } Logger.debug("\(logTag) dataChannelDidChangeState: \(dataChannel)") } /** The data channel successfully received a data buffer. */ internal func dataChannel(_ dataChannel: RTCDataChannel, didReceiveMessageWith buffer: RTCDataBuffer) { + defer { + ensureTeardown(#function) + } let completion: (OWSWebRTCProtosData) -> Void = { [weak self] (dataChannelMessage) in SwiftAssertIsOnMainThread(#function) guard let strongSelf = self else { return } + defer { + strongSelf.ensureTeardown(#function) + } guard let strongDelegate = strongSelf.delegate else { return } strongDelegate.peerConnectionClient(strongSelf, received: dataChannelMessage) } @@ -773,9 +794,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD guard let strongSelf = self else { return } Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() defer { - DispatchQueue.main.async { - Logger.info("\(strongSelf.logTag) \(#function) complete."); Logger.flush() - } + strongSelf.ensureTeardown(#function) } guard strongSelf.peerConnection != nil else { @@ -798,6 +817,9 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD /** The data channel's |bufferedAmount| changed. */ internal func dataChannel(_ dataChannel: RTCDataChannel, didChangeBufferedAmount amount: UInt64) { + defer { + ensureTeardown(#function) + } Logger.debug("\(logTag) didChangeBufferedAmount: \(amount)") } @@ -805,15 +827,24 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD /** Called when the SignalingState changed. */ internal func peerConnection(_ peerConnectionParam: RTCPeerConnection, didChange stateChanged: RTCSignalingState) { + defer { + ensureTeardown(#function) + } Logger.debug("\(logTag) didChange signalingState:\(stateChanged.debugDescription)") } /** Called when media is received on a new stream from remote peer. */ internal func peerConnection(_ peerConnectionParam: RTCPeerConnection, didAdd stream: RTCMediaStream) { + defer { + ensureTeardown(#function) + } let completion: (RTCVideoTrack) -> Void = { [weak self] (remoteVideoTrack) in SwiftAssertIsOnMainThread(#function) guard let strongSelf = self else { return } + defer { + strongSelf.ensureTeardown(#function) + } guard let strongDelegate = strongSelf.delegate else { return } // TODO: Consider checking for termination here. @@ -825,9 +856,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD guard let strongSelf = self else { return } Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() defer { - DispatchQueue.main.async { - Logger.info("\(strongSelf.logTag) \(#function) complete."); Logger.flush() - } + strongSelf.ensureTeardown(#function) } guard let peerConnection = strongSelf.peerConnection else { @@ -855,32 +884,50 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD /** Called when a remote peer closes a stream. */ internal func peerConnection(_ peerConnectionParam: RTCPeerConnection, didRemove stream: RTCMediaStream) { + defer { + ensureTeardown(#function) + } Logger.debug("\(logTag) didRemove Stream:\(stream)") } /** Called when negotiation is needed, for example ICE has restarted. */ internal func peerConnectionShouldNegotiate(_ peerConnectionParam: RTCPeerConnection) { + defer { + ensureTeardown(#function) + } Logger.debug("\(logTag) shouldNegotiate") } /** Called any time the IceConnectionState changes. */ internal func peerConnection(_ peerConnectionParam: RTCPeerConnection, didChange newState: RTCIceConnectionState) { + defer { + ensureTeardown(#function) + } let connectedCompletion : () -> Void = { [weak self] in SwiftAssertIsOnMainThread(#function) guard let strongSelf = self else { return } + defer { + strongSelf.ensureTeardown(#function) + } guard let strongDelegate = strongSelf.delegate else { return } strongDelegate.peerConnectionClientIceConnected(strongSelf) } let failedCompletion : () -> Void = { [weak self] in SwiftAssertIsOnMainThread(#function) guard let strongSelf = self else { return } + defer { + strongSelf.ensureTeardown(#function) + } guard let strongDelegate = strongSelf.delegate else { return } strongDelegate.peerConnectionClientIceFailed(strongSelf) } let disconnectedCompletion : () -> Void = { [weak self] in SwiftAssertIsOnMainThread(#function) guard let strongSelf = self else { return } + defer { + strongSelf.ensureTeardown(#function) + } guard let strongDelegate = strongSelf.delegate else { return } strongDelegate.peerConnectionClientIceDisconnected(strongSelf) } @@ -889,9 +936,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD guard let strongSelf = self else { return } Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() defer { - DispatchQueue.main.async { - Logger.info("\(strongSelf.logTag) \(#function) complete."); Logger.flush() - } + strongSelf.ensureTeardown(#function) } guard let peerConnection = strongSelf.peerConnection else { @@ -921,15 +966,24 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD /** Called any time the IceGatheringState changes. */ internal func peerConnection(_ peerConnectionParam: RTCPeerConnection, didChange newState: RTCIceGatheringState) { + defer { + ensureTeardown(#function) + } Logger.info("\(logTag) didChange IceGatheringState:\(newState.debugDescription)") } /** New ice candidate has been found. */ internal func peerConnection(_ peerConnectionParam: RTCPeerConnection, didGenerate candidate: RTCIceCandidate) { + defer { + ensureTeardown(#function) + } let completion: (RTCIceCandidate) -> Void = { [weak self] (candidate) in SwiftAssertIsOnMainThread(#function) guard let strongSelf = self else { return } + defer { + strongSelf.ensureTeardown(#function) + } guard let strongDelegate = strongSelf.delegate else { return } strongDelegate.peerConnectionClient(strongSelf, addedLocalIceCandidate: candidate) } @@ -938,9 +992,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD guard let strongSelf = self else { return } Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() defer { - DispatchQueue.main.async { - Logger.info("\(strongSelf.logTag) \(#function) complete."); Logger.flush() - } + strongSelf.ensureTeardown(#function) } guard let peerConnection = strongSelf.peerConnection else { @@ -960,15 +1012,24 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD /** Called when a group of local Ice candidates have been removed. */ internal func peerConnection(_ peerConnectionParam: RTCPeerConnection, didRemove candidates: [RTCIceCandidate]) { + defer { + ensureTeardown(#function) + } Logger.debug("\(logTag) didRemove IceCandidates:\(candidates)") } /** New data channel has been opened. */ internal func peerConnection(_ peerConnectionParam: RTCPeerConnection, didOpen dataChannel: RTCDataChannel) { + defer { + ensureTeardown(#function) + } let completion: ([PendingDataChannelMessage]) -> Void = { [weak self] (pendingMessages) in SwiftAssertIsOnMainThread(#function) guard let strongSelf = self else { return } + defer { + strongSelf.ensureTeardown(#function) + } pendingMessages.forEach { message in strongSelf.sendDataChannelMessage(data: message.data, description: message.description, isCritical: message.isCritical) } @@ -978,9 +1039,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD guard let strongSelf = self else { return } Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() defer { - DispatchQueue.main.async { - Logger.info("\(strongSelf.logTag) \(#function) complete."); Logger.flush() - } + strongSelf.ensureTeardown(#function) } guard let peerConnection = strongSelf.peerConnection else { @@ -1047,7 +1106,13 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD } } - private func + private func ensureTeardown(_ functionName: String) { + PeerConnectionClient.signalingQueue.async { +// DispatchQueue.main.async { + Logger.info("\(self.logTag) \(functionName) complete.") + Logger.flush() + } + } } /** From 88c2ff26e792afbcbbe0a60e4f2296bfe940605d Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Mon, 21 May 2018 12:45:46 -0400 Subject: [PATCH 06/14] PeerConnectionClient thread safety. --- Signal/src/call/PeerConnectionClient.swift | 26 ++++++++++------------ 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/Signal/src/call/PeerConnectionClient.swift b/Signal/src/call/PeerConnectionClient.swift index 18bd90cf7..aefa41cd6 100644 --- a/Signal/src/call/PeerConnectionClient.swift +++ b/Signal/src/call/PeerConnectionClient.swift @@ -178,7 +178,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD ensureTeardown(#function) } guard let peerConnection = peerConnection else { - owsFail("\(logTag) in \(#function) peerConnection was unexpectedly nil") + Logger.debug("\(logTag) \(#function) Ignoring obsolete event in terminated client") return } @@ -209,7 +209,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD return } guard let peerConnection = peerConnection else { - owsFail("\(logTag) in \(#function) peerConnection was unexpectedly nil") + Logger.debug("\(logTag) \(#function) Ignoring obsolete event in terminated client") return } @@ -248,7 +248,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD strongSelf.ensureTeardown(#function) } guard let localVideoSource = strongSelf.localVideoSource else { - owsFail("\(strongSelf.logTag) in \(#function) localVideoSource was unexpectedly nil") + Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client") return } @@ -289,12 +289,11 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD return } guard let localVideoTrack = strongSelf.localVideoTrack else { - let action = enabled ? "enable" : "disable" - Logger.error("\(strongSelf.logTag)) trying to \(action) videoTrack which doesn't exist") + Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client") return } guard let videoCaptureSession = strongSelf.videoCaptureSession else { - Logger.debug("\(strongSelf.logTag) videoCaptureSession was unexpectedly nil") + Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client") return } @@ -324,7 +323,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD assert(self.audioSender == nil, "\(#function) should only be called once.") guard let peerConnection = peerConnection else { - owsFail("\(logTag) in \(#function) peerConnection was unexpectedly nil") + Logger.debug("\(logTag) \(#function) Ignoring obsolete event in terminated client") return } @@ -362,8 +361,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD return } guard let audioTrack = strongSelf.audioTrack else { - let action = enabled ? "enable" : "disable" - Logger.error("\(strongSelf.logTag) trying to \(action) audioTrack which doesn't exist.") + Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client") return } @@ -457,7 +455,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD strongSelf.assertOnSignalingQueue() guard let peerConnection = peerConnection else { - owsFail("\(strongSelf.logTag) in \(#function) peerConnection was unexpectedly nil") + Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client") return } @@ -860,7 +858,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD } guard let peerConnection = strongSelf.peerConnection else { - owsFail("\(strongSelf.logTag) in \(#function) peerConnection was unexpectedly nil") + Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client") return } guard peerConnection == peerConnectionParam else { @@ -940,7 +938,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD } guard let peerConnection = strongSelf.peerConnection else { - owsFail("\(strongSelf.logTag) in \(#function) peerConnection was unexpectedly nil") + Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client") return } guard peerConnection == peerConnectionParam else { @@ -996,7 +994,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD } guard let peerConnection = strongSelf.peerConnection else { - owsFail("\(strongSelf.logTag) in \(#function) peerConnection was unexpectedly nil") + Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client") return } guard peerConnection == peerConnectionParam else { @@ -1043,7 +1041,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD } guard let peerConnection = strongSelf.peerConnection else { - owsFail("\(strongSelf.logTag) in \(#function) peerConnection was unexpectedly nil") + Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client") return } guard peerConnection == peerConnectionParam else { From c2f1a12d9f1a78e4536c6afa133514774febcfb6 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Mon, 21 May 2018 13:37:14 -0400 Subject: [PATCH 07/14] PeerConnectionClient thread safety. --- Signal/src/call/PeerConnectionClient.swift | 42 +++++++++------------- 1 file changed, 16 insertions(+), 26 deletions(-) diff --git a/Signal/src/call/PeerConnectionClient.swift b/Signal/src/call/PeerConnectionClient.swift index aefa41cd6..b82a96e37 100644 --- a/Signal/src/call/PeerConnectionClient.swift +++ b/Signal/src/call/PeerConnectionClient.swift @@ -385,7 +385,8 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD ensureTeardown(#function) } - let completion: ((((HardenedRTCSessionDescription) -> Void), ((Error) -> Void), RTCSessionDescription?, Error?) -> Void) = { [weak self] (fulfill, reject, sdp, error) in + let (promise, fulfill, reject) = Promise.pending() + let completion: ((RTCSessionDescription?, Error?) -> Void) = { [weak self] (sdp, error) in guard let strongSelf = self else { return } Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() defer { @@ -412,7 +413,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD fulfill(HardenedRTCSessionDescription(rtcSessionDescription: sessionDescription)) } - let workBlock : ((@escaping ((HardenedRTCSessionDescription) -> Void), @escaping ((Error) -> Void)) -> Void) = { [weak self] (fulfill, reject) in + PeerConnectionClient.signalingQueue.async { [weak self] in guard let strongSelf = self else { return } Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() defer { @@ -427,18 +428,12 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD peerConnection.offer(for: strongSelf.defaultOfferConstraints, completionHandler: { (sdp: RTCSessionDescription?, error: Error?) in PeerConnectionClient.signalingQueue.async { - completion(fulfill, reject, sdp, error) + completion(sdp, error) } }) } - return Promise { fulfill, reject in - SwiftAssertIsOnMainThread(#function) - - PeerConnectionClient.signalingQueue.async { - workBlock(fulfill, reject) - } - } + return promise } public func setLocalSessionDescriptionInternal(_ sessionDescription: HardenedRTCSessionDescription) -> Promise { @@ -470,7 +465,8 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD ensureTeardown(#function) } - let workBlock : ((@escaping (() -> Void), @escaping ((Error) -> Void)) -> Void) = { [weak self] (fulfill, reject) in + let (promise, fulfill, reject) = Promise.pending() + PeerConnectionClient.signalingQueue.async { [weak self] in guard let strongSelf = self else { return } Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() defer { @@ -494,11 +490,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD }) } - return Promise { fulfill, reject in - PeerConnectionClient.signalingQueue.async { - workBlock(fulfill, reject) - } - } + return promise } public func negotiateSessionDescription(remoteDescription: RTCSessionDescription, constraints: RTCMediaConstraints) -> Promise { @@ -528,7 +520,8 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD ensureTeardown(#function) } - let workBlock : ((@escaping (() -> Void), @escaping ((Error) -> Void)) -> Void) = { [weak self] (fulfill, reject) in + let (promise, fulfill, reject) = Promise.pending() + PeerConnectionClient.signalingQueue.async { [weak self] in guard let strongSelf = self else { return } Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() defer { @@ -550,12 +543,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD fulfill() }) } - - return Promise { fulfill, reject in - PeerConnectionClient.signalingQueue.async { - workBlock(fulfill, reject) - } - } + return promise } private func negotiateAnswerSessionDescription(constraints: RTCMediaConstraints) -> Promise { @@ -564,7 +552,8 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD ensureTeardown(#function) } - let completion : ((@escaping ((HardenedRTCSessionDescription) -> Void), @escaping ((Error) -> Void), RTCSessionDescription?, Error?) -> Void) = { [weak self] (fulfill, reject, sdp, error) in + let (promise, fulfill, reject) = Promise.pending() + let completion: ((RTCSessionDescription?, Error?) -> Void) = { [weak self] (sdp, error) in guard let strongSelf = self else { return } Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() defer { @@ -598,7 +587,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD } } - return Promise { [weak self] fulfill, reject in + PeerConnectionClient.signalingQueue.async { [weak self] in guard let strongSelf = self else { return } Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() defer { @@ -615,10 +604,11 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD peerConnection.answer(for: constraints, completionHandler: { (sdp: RTCSessionDescription?, error: Error?) in PeerConnectionClient.signalingQueue.async { - completion(fulfill, reject, sdp, error) + completion(sdp, error) } }) } + return promise } public func addRemoteIceCandidate(_ candidate: RTCIceCandidate) { From 9075a12ac6a3f24ece5c2bc97ff327fc822c88a5 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Mon, 21 May 2018 14:15:33 -0400 Subject: [PATCH 08/14] PeerConnectionClient thread safety. --- Signal/src/call/PeerConnectionClient.swift | 330 +++++---------------- 1 file changed, 79 insertions(+), 251 deletions(-) diff --git a/Signal/src/call/PeerConnectionClient.swift b/Signal/src/call/PeerConnectionClient.swift index b82a96e37..54ab4ebcf 100644 --- a/Signal/src/call/PeerConnectionClient.swift +++ b/Signal/src/call/PeerConnectionClient.swift @@ -74,7 +74,30 @@ protocol PeerConnectionClientDelegate: class { */ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelDelegate { -// private class AtomicFlag + private class AtomicHandle : 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", @@ -125,6 +148,8 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD private var remoteVideoTrack: RTCVideoTrack? private var cameraConstraints: RTCMediaConstraints + private let handle: AtomicHandle + init(iceServers: [RTCIceServer], delegate: PeerConnectionClientDelegate, callDirection: CallDirection, useTurnOnly: Bool) { SwiftAssertIsOnMainThread(#function) @@ -148,8 +173,12 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD audioConstraints = RTCMediaConstraints(mandatoryConstraints: nil, optionalConstraints: nil) cameraConstraints = RTCMediaConstraints(mandatoryConstraints: nil, optionalConstraints: nil) + handle = AtomicHandle() + super.init() + handle.set(value: self) + peerConnection = factory.peerConnection(with: configuration, constraints: connectionConstraints, delegate: self) @@ -167,16 +196,12 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD // TODO: We can demote this log level to debug once we're confident that // this class is always deallocated. Logger.info("[PeerConnectionClient] deinit") - Logger.flush() } // MARK: - Media Streams private func createSignalingDataChannel() { SwiftAssertIsOnMainThread(#function) - defer { - ensureTeardown(#function) - } guard let peerConnection = peerConnection else { Logger.debug("\(logTag) \(#function) Ignoring obsolete event in terminated client") return @@ -197,10 +222,6 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD fileprivate func createVideoSender() { SwiftAssertIsOnMainThread(#function) - defer { - ensureTeardown(#function) - } - Logger.debug("\(logTag) in \(#function)") assert(self.videoSender == nil, "\(#function) should only be called once.") @@ -241,12 +262,9 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD public func setCameraSource(useBackCamera: Bool) { SwiftAssertIsOnMainThread(#function) + let handle = self.handle PeerConnectionClient.signalingQueue.async { [weak self] in - guard let strongSelf = self else { return } - Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() - defer { - strongSelf.ensureTeardown(#function) - } + 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 @@ -264,26 +282,16 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD public func setLocalVideoEnabled(enabled: Bool) { SwiftAssertIsOnMainThread(#function) - defer { - ensureTeardown(#function) - } - + let handle = self.handle let completion = { [weak self] in - guard let strongSelf = self else { return } - defer { - strongSelf.ensureTeardown(#function) - } + 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 { [weak self] in - guard let strongSelf = self else { return } - Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() - defer { - strongSelf.ensureTeardown(#function) - } + guard let strongSelf = handle.get() else { return } guard strongSelf.peerConnection != nil else { Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client") return @@ -315,10 +323,6 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD fileprivate func createAudioSender() { SwiftAssertIsOnMainThread(#function) - defer { - ensureTeardown(#function) - } - Logger.debug("\(logTag) in \(#function)") assert(self.audioSender == nil, "\(#function) should only be called once.") @@ -345,17 +349,9 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD public func setAudioEnabled(enabled: Bool) { SwiftAssertIsOnMainThread(#function) - defer { - ensureTeardown(#function) - } - + let handle = self.handle PeerConnectionClient.signalingQueue.async { [weak self] in - guard let strongSelf = self else { return } - Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() - defer { - strongSelf.ensureTeardown(#function) - } - + guard let strongSelf = handle.get() else { return } guard strongSelf.peerConnection != nil else { Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client") return @@ -381,17 +377,10 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD public func createOffer() -> Promise { SwiftAssertIsOnMainThread(#function) - defer { - ensureTeardown(#function) - } - + let handle = self.handle let (promise, fulfill, reject) = Promise.pending() let completion: ((RTCSessionDescription?, Error?) -> Void) = { [weak self] (sdp, error) in - guard let strongSelf = self else { return } - Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() - defer { - strongSelf.ensureTeardown(#function) - } + guard let strongSelf = handle.get() else { return } strongSelf.assertOnSignalingQueue() guard strongSelf.peerConnection != nil else { Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client") @@ -414,11 +403,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD } PeerConnectionClient.signalingQueue.async { [weak self] in - guard let strongSelf = self else { return } - Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() - defer { - strongSelf.ensureTeardown(#function) - } + guard let strongSelf = handle.get() else { return } strongSelf.assertOnSignalingQueue() guard let peerConnection = strongSelf.peerConnection else { Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client") @@ -437,16 +422,9 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD } public func setLocalSessionDescriptionInternal(_ sessionDescription: HardenedRTCSessionDescription) -> Promise { - defer { - ensureTeardown(#function) - } - + let handle = self.handle return PromiseKit.wrap { [weak self] resolve in - guard let strongSelf = self else { return } - Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() - defer { - strongSelf.ensureTeardown(#function) - } + guard let strongSelf = handle.get() else { return } strongSelf.assertOnSignalingQueue() guard let peerConnection = peerConnection else { @@ -461,17 +439,10 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD public func setLocalSessionDescription(_ sessionDescription: HardenedRTCSessionDescription) -> Promise { SwiftAssertIsOnMainThread(#function) - defer { - ensureTeardown(#function) - } - + let handle = self.handle let (promise, fulfill, reject) = Promise.pending() PeerConnectionClient.signalingQueue.async { [weak self] in - guard let strongSelf = self else { return } - Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() - defer { - strongSelf.ensureTeardown(#function) - } + guard let strongSelf = handle.get() else { return } strongSelf.assertOnSignalingQueue() guard let peerConnection = strongSelf.peerConnection else { Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client") @@ -495,38 +466,24 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD public func negotiateSessionDescription(remoteDescription: RTCSessionDescription, constraints: RTCMediaConstraints) -> Promise { SwiftAssertIsOnMainThread(#function) - defer { - ensureTeardown(#function) - } - + let handle = self.handle return setRemoteSessionDescription(remoteDescription) .then(on: PeerConnectionClient.signalingQueue) { [weak self] in - guard let strongSelf = self else { + guard let strongSelf = handle.get() else { return Promise { _, reject in reject(NSError(domain: "Obsolete client", code: 0, userInfo: nil)) } } - Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() - defer { - strongSelf.ensureTeardown(#function) - } - return strongSelf.negotiateAnswerSessionDescription(constraints: constraints) + return strongSelf.negotiateAnswerSessionDescription(constraints: constraints) } } public func setRemoteSessionDescription(_ sessionDescription: RTCSessionDescription) -> Promise { SwiftAssertIsOnMainThread(#function) - defer { - ensureTeardown(#function) - } - + let handle = self.handle let (promise, fulfill, reject) = Promise.pending() PeerConnectionClient.signalingQueue.async { [weak self] in - guard let strongSelf = self else { return } - Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() - defer { - strongSelf.ensureTeardown(#function) - } + guard let strongSelf = handle.get() else { return } strongSelf.assertOnSignalingQueue() guard let peerConnection = strongSelf.peerConnection else { Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client") @@ -548,17 +505,10 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD private func negotiateAnswerSessionDescription(constraints: RTCMediaConstraints) -> Promise { assertOnSignalingQueue() - defer { - ensureTeardown(#function) - } - + let handle = self.handle let (promise, fulfill, reject) = Promise.pending() let completion: ((RTCSessionDescription?, Error?) -> Void) = { [weak self] (sdp, error) in - guard let strongSelf = self else { return } - Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() - defer { - strongSelf.ensureTeardown(#function) - } + guard let strongSelf = handle.get() else { return } strongSelf.assertOnSignalingQueue() guard strongSelf.peerConnection != nil else { Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client") @@ -588,11 +538,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD } PeerConnectionClient.signalingQueue.async { [weak self] in - guard let strongSelf = self else { return } - Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() - defer { - strongSelf.ensureTeardown(#function) - } + guard let strongSelf = handle.get() else { return } strongSelf.assertOnSignalingQueue() guard let peerConnection = strongSelf.peerConnection else { @@ -612,16 +558,9 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD } public func addRemoteIceCandidate(_ candidate: RTCIceCandidate) { - defer { - ensureTeardown(#function) - } + let handle = self.handle PeerConnectionClient.signalingQueue.async { [weak self] in - guard let strongSelf = self else { return } - Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() - defer { - strongSelf.ensureTeardown(#function) - } - + 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 @@ -633,36 +572,25 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD public func terminate() { SwiftAssertIsOnMainThread(#function) - defer { - ensureTeardown(#function) - } - Logger.debug("\(logTag) in \(#function)") - Logger.flush() // Clear the delegate immediately so that we can guarantee that // no delegate methods are called after terminate() returns. delegate = nil - PeerConnectionClient.signalingQueue.async { [weak self] in - guard let strongSelf = self else { return } - Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() - defer { - strongSelf.ensureTeardown(#function) - } + // Clear the handle immediately so that enqueued work is aborted + // going forward. + handle.clear() - strongSelf.terminateInternal() + // Don't use [weak self]; we always want to perform terminateInternal(). + PeerConnectionClient.signalingQueue.async { + self.terminateInternal() } } private func terminateInternal() { assertOnSignalingQueue() - defer { - ensureTeardown(#function) - } - Logger.debug("\(logTag) in \(#function)") - Logger.flush() // 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 @@ -693,9 +621,6 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD peerConnection.close() } peerConnection = nil - - Logger.debug("\(logTag) in \(#function) complete") - Logger.flush() } // MARK: - Data Channel @@ -710,16 +635,9 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD public func sendDataChannelMessage(data: Data, description: String, isCritical: Bool) { SwiftAssertIsOnMainThread(#function) - defer { - ensureTeardown(#function) - } - + let handle = self.handle PeerConnectionClient.signalingQueue.async { [weak self] in - guard let strongSelf = self else { return } - Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() - defer { - strongSelf.ensureTeardown(#function) - } + guard let strongSelf = handle.get() else { return } guard strongSelf.peerConnection != nil else { Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client: \(description)") @@ -756,35 +674,21 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD /** The data channel state changed. */ internal func dataChannelDidChangeState(_ dataChannel: RTCDataChannel) { - defer { - ensureTeardown(#function) - } Logger.debug("\(logTag) dataChannelDidChangeState: \(dataChannel)") } /** The data channel successfully received a data buffer. */ internal func dataChannel(_ dataChannel: RTCDataChannel, didReceiveMessageWith buffer: RTCDataBuffer) { - defer { - ensureTeardown(#function) - } - + let handle = self.handle let completion: (OWSWebRTCProtosData) -> Void = { [weak self] (dataChannelMessage) in SwiftAssertIsOnMainThread(#function) - guard let strongSelf = self else { return } - defer { - strongSelf.ensureTeardown(#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 = self else { return } - Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() - defer { - strongSelf.ensureTeardown(#function) - } - + guard let strongSelf = handle.get() else { return } guard strongSelf.peerConnection != nil else { Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client") return @@ -805,9 +709,6 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD /** The data channel's |bufferedAmount| changed. */ internal func dataChannel(_ dataChannel: RTCDataChannel, didChangeBufferedAmount amount: UInt64) { - defer { - ensureTeardown(#function) - } Logger.debug("\(logTag) didChangeBufferedAmount: \(amount)") } @@ -815,24 +716,15 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD /** Called when the SignalingState changed. */ internal func peerConnection(_ peerConnectionParam: RTCPeerConnection, didChange stateChanged: RTCSignalingState) { - defer { - ensureTeardown(#function) - } Logger.debug("\(logTag) didChange signalingState:\(stateChanged.debugDescription)") } /** Called when media is received on a new stream from remote peer. */ internal func peerConnection(_ peerConnectionParam: RTCPeerConnection, didAdd stream: RTCMediaStream) { - defer { - ensureTeardown(#function) - } - + let handle = self.handle let completion: (RTCVideoTrack) -> Void = { [weak self] (remoteVideoTrack) in SwiftAssertIsOnMainThread(#function) - guard let strongSelf = self else { return } - defer { - strongSelf.ensureTeardown(#function) - } + guard let strongSelf = handle.get() else { return } guard let strongDelegate = strongSelf.delegate else { return } // TODO: Consider checking for termination here. @@ -841,12 +733,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD } PeerConnectionClient.signalingQueue.async { [weak self] in - guard let strongSelf = self else { return } - Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() - defer { - strongSelf.ensureTeardown(#function) - } - + 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 @@ -872,61 +759,38 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD /** Called when a remote peer closes a stream. */ internal func peerConnection(_ peerConnectionParam: RTCPeerConnection, didRemove stream: RTCMediaStream) { - defer { - ensureTeardown(#function) - } Logger.debug("\(logTag) didRemove Stream:\(stream)") } /** Called when negotiation is needed, for example ICE has restarted. */ internal func peerConnectionShouldNegotiate(_ peerConnectionParam: RTCPeerConnection) { - defer { - ensureTeardown(#function) - } Logger.debug("\(logTag) shouldNegotiate") } /** Called any time the IceConnectionState changes. */ internal func peerConnection(_ peerConnectionParam: RTCPeerConnection, didChange newState: RTCIceConnectionState) { - defer { - ensureTeardown(#function) - } - + let handle = self.handle let connectedCompletion : () -> Void = { [weak self] in SwiftAssertIsOnMainThread(#function) - guard let strongSelf = self else { return } - defer { - strongSelf.ensureTeardown(#function) - } + guard let strongSelf = handle.get() else { return } guard let strongDelegate = strongSelf.delegate else { return } strongDelegate.peerConnectionClientIceConnected(strongSelf) } let failedCompletion : () -> Void = { [weak self] in SwiftAssertIsOnMainThread(#function) - guard let strongSelf = self else { return } - defer { - strongSelf.ensureTeardown(#function) - } + guard let strongSelf = handle.get() else { return } guard let strongDelegate = strongSelf.delegate else { return } strongDelegate.peerConnectionClientIceFailed(strongSelf) } let disconnectedCompletion : () -> Void = { [weak self] in SwiftAssertIsOnMainThread(#function) - guard let strongSelf = self else { return } - defer { - strongSelf.ensureTeardown(#function) - } + guard let strongSelf = handle.get() else { return } guard let strongDelegate = strongSelf.delegate else { return } strongDelegate.peerConnectionClientIceDisconnected(strongSelf) } PeerConnectionClient.signalingQueue.async { [weak self] in - guard let strongSelf = self else { return } - Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() - defer { - strongSelf.ensureTeardown(#function) - } - + 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 @@ -954,35 +818,21 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD /** Called any time the IceGatheringState changes. */ internal func peerConnection(_ peerConnectionParam: RTCPeerConnection, didChange newState: RTCIceGatheringState) { - defer { - ensureTeardown(#function) - } Logger.info("\(logTag) didChange IceGatheringState:\(newState.debugDescription)") } /** New ice candidate has been found. */ internal func peerConnection(_ peerConnectionParam: RTCPeerConnection, didGenerate candidate: RTCIceCandidate) { - defer { - ensureTeardown(#function) - } - + let handle = self.handle let completion: (RTCIceCandidate) -> Void = { [weak self] (candidate) in SwiftAssertIsOnMainThread(#function) - guard let strongSelf = self else { return } - defer { - strongSelf.ensureTeardown(#function) - } + guard let strongSelf = handle.get() else { return } guard let strongDelegate = strongSelf.delegate else { return } strongDelegate.peerConnectionClient(strongSelf, addedLocalIceCandidate: candidate) } PeerConnectionClient.signalingQueue.async { [weak self] in - guard let strongSelf = self else { return } - Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() - defer { - strongSelf.ensureTeardown(#function) - } - + 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 @@ -1000,36 +850,22 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD /** Called when a group of local Ice candidates have been removed. */ internal func peerConnection(_ peerConnectionParam: RTCPeerConnection, didRemove candidates: [RTCIceCandidate]) { - defer { - ensureTeardown(#function) - } Logger.debug("\(logTag) didRemove IceCandidates:\(candidates)") } /** New data channel has been opened. */ internal func peerConnection(_ peerConnectionParam: RTCPeerConnection, didOpen dataChannel: RTCDataChannel) { - defer { - ensureTeardown(#function) - } - + let handle = self.handle let completion: ([PendingDataChannelMessage]) -> Void = { [weak self] (pendingMessages) in SwiftAssertIsOnMainThread(#function) - guard let strongSelf = self else { return } - defer { - strongSelf.ensureTeardown(#function) - } + guard let strongSelf = handle.get() 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 = self else { return } - Logger.info("\(strongSelf.logTag) \(#function) starting."); Logger.flush() - defer { - strongSelf.ensureTeardown(#function) - } - + 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 @@ -1093,14 +929,6 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD // Noop. } } - - private func ensureTeardown(_ functionName: String) { - PeerConnectionClient.signalingQueue.async { -// DispatchQueue.main.async { - Logger.info("\(self.logTag) \(functionName) complete.") - Logger.flush() - } - } } /** From 078da69ee023a7bb4f4644a5fe139be2b3f42fb2 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Mon, 21 May 2018 14:25:56 -0400 Subject: [PATCH 09/14] PeerConnectionClient thread safety. --- Signal/src/call/CallService.swift | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Signal/src/call/CallService.swift b/Signal/src/call/CallService.swift index 651fa288b..166e98da4 100644 --- a/Signal/src/call/CallService.swift +++ b/Signal/src/call/CallService.swift @@ -749,8 +749,7 @@ private class SignalCallData: NSObject { Logger.verbose("\(logTag) \(#function) callId: \(callId)") guard let callData = self.callData else { - OWSProdError(OWSAnalyticsEvents.callServiceCallMissing(), file: #file, function: #function, line: #line) - self.handleFailedCurrentCall(error: .obsoleteCall(description: "ignoring remote ice update, since there is no current call.")) + Logger.info("\(logTag) ignoring remote ice update, since there is no current call.") return } From 7eab0569b6c6e1013cf9043a3663553f7af085a8 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Mon, 21 May 2018 14:59:57 -0400 Subject: [PATCH 10/14] PeerConnectionClient thread safety. --- Signal/src/call/CallService.swift | 6 +++--- Signal/src/call/PeerConnectionClient.swift | 16 ++++++++++------ 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/Signal/src/call/CallService.swift b/Signal/src/call/CallService.swift index 166e98da4..51ff18276 100644 --- a/Signal/src/call/CallService.swift +++ b/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,6 +745,7 @@ 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 { @@ -1001,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 diff --git a/Signal/src/call/PeerConnectionClient.swift b/Signal/src/call/PeerConnectionClient.swift index 54ab4ebcf..f8880821c 100644 --- a/Signal/src/call/PeerConnectionClient.swift +++ b/Signal/src/call/PeerConnectionClient.swift @@ -412,7 +412,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD } peerConnection.offer(for: strongSelf.defaultOfferConstraints, completionHandler: { (sdp: RTCSessionDescription?, error: Error?) in - PeerConnectionClient.signalingQueue.async { + PeerConnectionClient.signalingQueue.async { [weak self] in completion(sdp, error) } }) @@ -549,7 +549,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD Logger.debug("\(strongSelf.logTag) negotiating answer session.") peerConnection.answer(for: constraints, completionHandler: { (sdp: RTCSessionDescription?, error: Error?) in - PeerConnectionClient.signalingQueue.async { + PeerConnectionClient.signalingQueue.async { [weak self] in completion(sdp, error) } }) @@ -608,6 +608,10 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD localVideoTrack?.isEnabled = false remoteVideoTrack?.isEnabled = false + if let dataChannel = self.dataChannel { + dataChannel.delegate = nil + } + dataChannel = nil audioSender = nil audioTrack = nil @@ -701,7 +705,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD return } - DispatchQueue.main.async { + DispatchQueue.main.async { [weak self] in completion(dataChannelMessage) } } @@ -751,7 +755,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD strongSelf.remoteVideoTrack = remoteVideoTrack - DispatchQueue.main.async { + DispatchQueue.main.async { [weak self] in completion(remoteVideoTrack) } } @@ -842,7 +846,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD return } Logger.info("\(strongSelf.logTag) adding local ICE candidate:\(candidate.sdp)") - DispatchQueue.main.async { + DispatchQueue.main.async { [weak self] in completion(candidate) } } @@ -883,7 +887,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD let pendingMessages = strongSelf.pendingDataChannelMessages strongSelf.pendingDataChannelMessages = [] - DispatchQueue.main.async { + DispatchQueue.main.async { [weak self] in completion(pendingMessages) } } From 9c0c87a8c3aba090fa2ca6e143ac5e05239336ef Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 22 May 2018 11:18:35 -0400 Subject: [PATCH 11/14] Remove capture of self. --- Signal/src/call/PeerConnectionClient.swift | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Signal/src/call/PeerConnectionClient.swift b/Signal/src/call/PeerConnectionClient.swift index f8880821c..a66f0c13c 100644 --- a/Signal/src/call/PeerConnectionClient.swift +++ b/Signal/src/call/PeerConnectionClient.swift @@ -705,7 +705,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD return } - DispatchQueue.main.async { [weak self] in + DispatchQueue.main.async { completion(dataChannelMessage) } } @@ -755,7 +755,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD strongSelf.remoteVideoTrack = remoteVideoTrack - DispatchQueue.main.async { [weak self] in + DispatchQueue.main.async { completion(remoteVideoTrack) } } @@ -846,7 +846,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD return } Logger.info("\(strongSelf.logTag) adding local ICE candidate:\(candidate.sdp)") - DispatchQueue.main.async { [weak self] in + DispatchQueue.main.async { completion(candidate) } } @@ -883,11 +883,11 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD owsFail("\(strongSelf.logTag) in \(#function) dataChannel unexpectedly set twice.") } strongSelf.dataChannel = dataChannel - dataChannel.delegate = self + dataChannel.delegate = strongSelf let pendingMessages = strongSelf.pendingDataChannelMessages strongSelf.pendingDataChannelMessages = [] - DispatchQueue.main.async { [weak self] in + DispatchQueue.main.async { completion(pendingMessages) } } From 918abb02a1615d966898d178cfd9f3fbb22258a0 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 22 May 2018 11:19:45 -0400 Subject: [PATCH 12/14] Remove capture of self. --- Signal/src/call/PeerConnectionClient.swift | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Signal/src/call/PeerConnectionClient.swift b/Signal/src/call/PeerConnectionClient.swift index a66f0c13c..44454a44a 100644 --- a/Signal/src/call/PeerConnectionClient.swift +++ b/Signal/src/call/PeerConnectionClient.swift @@ -412,7 +412,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD } peerConnection.offer(for: strongSelf.defaultOfferConstraints, completionHandler: { (sdp: RTCSessionDescription?, error: Error?) in - PeerConnectionClient.signalingQueue.async { [weak self] in + PeerConnectionClient.signalingQueue.async { completion(sdp, error) } }) @@ -549,7 +549,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD Logger.debug("\(strongSelf.logTag) negotiating answer session.") peerConnection.answer(for: constraints, completionHandler: { (sdp: RTCSessionDescription?, error: Error?) in - PeerConnectionClient.signalingQueue.async { [weak self] in + PeerConnectionClient.signalingQueue.async { completion(sdp, error) } }) From 735b4e07b1be74e37d34d3daa2e20ed155c6ae84 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 22 May 2018 15:33:15 -0400 Subject: [PATCH 13/14] Respond to CR. --- Signal/src/call/PeerConnectionClient.swift | 55 +++++++++++++++++----- 1 file changed, 42 insertions(+), 13 deletions(-) diff --git a/Signal/src/call/PeerConnectionClient.swift b/Signal/src/call/PeerConnectionClient.swift index 44454a44a..561aff4f6 100644 --- a/Signal/src/call/PeerConnectionClient.swift +++ b/Signal/src/call/PeerConnectionClient.swift @@ -380,7 +380,10 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD let handle = self.handle let (promise, fulfill, reject) = Promise.pending() let completion: ((RTCSessionDescription?, Error?) -> Void) = { [weak self] (sdp, error) in - guard let strongSelf = handle.get() else { return } + guard let strongSelf = handle.get() else { + reject(NSError(domain: "Obsolete client", code: 0, userInfo: nil)) + return + } strongSelf.assertOnSignalingQueue() guard strongSelf.peerConnection != nil else { Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client") @@ -403,7 +406,10 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD } PeerConnectionClient.signalingQueue.async { [weak self] in - guard let strongSelf = handle.get() else { return } + 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") @@ -423,18 +429,30 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD public func setLocalSessionDescriptionInternal(_ sessionDescription: HardenedRTCSessionDescription) -> Promise { let handle = self.handle - return PromiseKit.wrap { [weak self] resolve in - guard let strongSelf = handle.get() else { return } + let (promise, fulfill, reject) = Promise.pending() + 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 = peerConnection else { + 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: resolve) + peerConnection.setLocalDescription(sessionDescription.rtcSessionDescription, completionHandler: { (error) in + if let error = error { + reject(error) + } else { + fulfill() + } + }) } + return promise } public func setLocalSessionDescription(_ sessionDescription: HardenedRTCSessionDescription) -> Promise { @@ -442,7 +460,10 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD let handle = self.handle let (promise, fulfill, reject) = Promise.pending() PeerConnectionClient.signalingQueue.async { [weak self] in - guard let strongSelf = handle.get() else { return } + 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") @@ -470,9 +491,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD return setRemoteSessionDescription(remoteDescription) .then(on: PeerConnectionClient.signalingQueue) { [weak self] in guard let strongSelf = handle.get() else { - return Promise { _, reject in - reject(NSError(domain: "Obsolete client", code: 0, userInfo: nil)) - } + return Promise(error: NSError(domain: "Obsolete client", code: 0, userInfo: nil)) } return strongSelf.negotiateAnswerSessionDescription(constraints: constraints) } @@ -483,7 +502,10 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD let handle = self.handle let (promise, fulfill, reject) = Promise.pending() PeerConnectionClient.signalingQueue.async { [weak self] in - guard let strongSelf = handle.get() else { return } + 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") @@ -508,7 +530,10 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD let handle = self.handle let (promise, fulfill, reject) = Promise.pending() let completion: ((RTCSessionDescription?, Error?) -> Void) = { [weak self] (sdp, error) in - guard let strongSelf = handle.get() else { return } + guard let strongSelf = handle.get() else { + reject(NSError(domain: "Obsolete client", code: 0, userInfo: nil)) + return + } strongSelf.assertOnSignalingQueue() guard strongSelf.peerConnection != nil else { Logger.debug("\(strongSelf.logTag) \(#function) Ignoring obsolete event in terminated client") @@ -538,11 +563,15 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD } PeerConnectionClient.signalingQueue.async { [weak self] in - guard let strongSelf = handle.get() else { return } + 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 } From bfb87582feda15c6cfddae15511adcb7706be63b Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 22 May 2018 15:37:57 -0400 Subject: [PATCH 14/14] Respond to CR. --- Signal/src/call/PeerConnectionClient.swift | 23 ++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/Signal/src/call/PeerConnectionClient.swift b/Signal/src/call/PeerConnectionClient.swift index 561aff4f6..61da0048c 100644 --- a/Signal/src/call/PeerConnectionClient.swift +++ b/Signal/src/call/PeerConnectionClient.swift @@ -74,6 +74,29 @@ protocol PeerConnectionClientDelegate: class { */ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelDelegate { + // 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 : NSObject { var value: ValueType?