From 4dec1e2dedda81cd0bb2889d6c95ca553a38f92c Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Wed, 1 Feb 2017 17:38:17 -0500 Subject: [PATCH] Avoid crashes when closing peer connection client. // FREEBIE --- Signal/src/call/CallService.swift | 3 +- Signal/src/call/PeerConnectionClient.swift | 120 +++++++++++++++++---- 2 files changed, 100 insertions(+), 23 deletions(-) diff --git a/Signal/src/call/CallService.swift b/Signal/src/call/CallService.swift index caee436d1..d92909551 100644 --- a/Signal/src/call/CallService.swift +++ b/Signal/src/call/CallService.swift @@ -250,6 +250,7 @@ protocol CallServiceObserver: class { // to do this explicitly. peerConnectionClient.createSignalingDataChannel() + assert(self.peerConnectionClient == nil, "Unexpected PeerConnectionClient instance") self.peerConnectionClient = peerConnectionClient return self.peerConnectionClient!.createOffer() @@ -395,6 +396,7 @@ protocol CallServiceObserver: class { }.then() { (iceServers: [RTCIceServer]) -> Promise in // FIXME for first time call recipients I think we'll see mic/camera permission requests here, // even though, from the users perspective, no incoming call is yet visible. + assert(self.peerConnectionClient == nil, "Unexpected PeerConnectionClient instance") self.peerConnectionClient = PeerConnectionClient(iceServers: iceServers, delegate: self) let offerSessionDescription = RTCSessionDescription(type: .offer, sdp: callerSessionDescription) @@ -994,7 +996,6 @@ protocol CallServiceObserver: class { Logger.debug("\(TAG) in \(#function)") PeerConnectionClient.stopAudioSession() - peerConnectionClient?.setDelegate(delegate:nil) peerConnectionClient?.terminate() peerConnectionClient = nil diff --git a/Signal/src/call/PeerConnectionClient.swift b/Signal/src/call/PeerConnectionClient.swift index 46aab4679..acfd9ed71 100644 --- a/Signal/src/call/PeerConnectionClient.swift +++ b/Signal/src/call/PeerConnectionClient.swift @@ -194,6 +194,10 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD AssertIsOnMainThread() PeerConnectionClient.signalingQueue.async { + guard self.peerConnection != nil else { + Logger.debug("\(self.TAG) \(#function) Ignoring obsolete event in terminated client") + return + } guard let localVideoTrack = self.localVideoTrack else { let action = enabled ? "enable" : "disable" Logger.error("\(self.TAG)) trying to \(action) videoTrack which doesn't exist") @@ -203,8 +207,10 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD localVideoTrack.isEnabled = enabled if let delegate = self.delegate { - DispatchQueue.main.async { - delegate.peerConnectionClient(self, didUpdateLocal: enabled ? localVideoTrack : nil) + DispatchQueue.main.async { [weak self, weak localVideoTrack] in + guard let strongSelf = self else { return } + guard let strongLocalVideoTrack = localVideoTrack else { return } + delegate.peerConnectionClient(strongSelf, didUpdateLocal: enabled ? strongLocalVideoTrack : nil) } } } @@ -238,6 +244,10 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD AssertIsOnMainThread() PeerConnectionClient.signalingQueue.async { + guard self.peerConnection != nil else { + Logger.debug("\(self.TAG) \(#function) Ignoring obsolete event in terminated client") + return + } guard let audioTrack = self.audioTrack else { let action = enabled ? "enable" : "disable" Logger.error("\(self.TAG) trying to \(action) audioTrack which doesn't exist.") @@ -265,10 +275,17 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD AssertIsOnMainThread() PeerConnectionClient.signalingQueue.async { - self.assertOnSignalingQueue() + guard self.peerConnection != nil else { + Logger.debug("\(self.TAG) \(#function) Ignoring obsolete event in terminated client") + 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") + return + } guard error == nil else { reject(error!) return @@ -301,7 +318,10 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD return PromiseKit.wrap { resolve in PeerConnectionClient.signalingQueue.async { - self.assertOnSignalingQueue() + guard self.peerConnection != nil else { + Logger.debug("\(self.TAG) \(#function) Ignoring obsolete event in terminated client") + return + } Logger.verbose("\(self.TAG) setting local session description: \(sessionDescription)") self.peerConnection.setLocalDescription(sessionDescription.rtcSessionDescription, completionHandler:resolve) } @@ -313,6 +333,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD return setRemoteSessionDescription(remoteDescription) .then(on: PeerConnectionClient.signalingQueue) { + assert(self.peerConnection != nil) return self.negotiateAnswerSessionDescription(constraints: constraints) } } @@ -322,7 +343,10 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD return PromiseKit.wrap { resolve in PeerConnectionClient.signalingQueue.async { - self.assertOnSignalingQueue() + guard self.peerConnection != nil else { + Logger.debug("\(self.TAG) \(#function) Ignoring obsolete event in terminated client") + return + } Logger.verbose("\(self.TAG) setting remote description: \(sessionDescription)") self.peerConnection.setRemoteDescription(sessionDescription, completionHandler: resolve) } @@ -339,6 +363,10 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD 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") + return + } guard error == nil else { reject(error!) return @@ -366,13 +394,24 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD public func addIceCandidate(_ candidate: RTCIceCandidate) { PeerConnectionClient.signalingQueue.async { + guard self.peerConnection != nil else { + Logger.debug("\(self.TAG) \(#function) Ignoring obsolete event in terminated client") + return + } Logger.debug("\(self.TAG) adding candidate") self.peerConnection.add(candidate) } } public func terminate() { + AssertIsOnMainThread() + Logger.debug("\(TAG) in \(#function)") + + delegate.peerConnectionClient(self, didUpdateLocal: nil) + delegate.peerConnectionClient(self, didUpdateRemote: nil) + PeerConnectionClient.signalingQueue.async { + assert(self.peerConnection != nil) self.terminateInternal() } } @@ -380,6 +419,8 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD private func terminateInternal() { assertOnSignalingQueue() + Logger.debug("\(TAG) in \(#function)") + // Some notes on preventing crashes while disposing of peerConnection for video calls // from: https://groups.google.com/forum/#!searchin/discuss-webrtc/objc$20crash$20dealloc%7Csort:relevance/discuss-webrtc/7D-vk5yLjn8/rBW2D6EW4GYJ // The sequence to make it work appears to be @@ -392,16 +433,22 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD // audioTrack is a strong property because we need access to it to mute/unmute, but I was seeing it // 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 - Logger.debug("\(TAG) in \(#function)") - audioTrack = nil - localVideoTrack = nil - remoteVideoTrack = nil + + localVideoTrack?.isEnabled = false + remoteVideoTrack?.isEnabled = false + + peerConnection.delegate = nil + peerConnection.close() + peerConnection = nil + dataChannel = nil audioSender = nil + audioTrack = nil videoSender = nil + localVideoTrack = nil + remoteVideoTrack = nil - peerConnection.delegate = nil - peerConnection.close() + delegate = nil } // MARK: - Data Channel @@ -410,7 +457,10 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD AssertIsOnMainThread() PeerConnectionClient.signalingQueue.async { - self.assertOnSignalingQueue() + guard self.peerConnection != nil else { + Logger.debug("\(self.TAG) \(#function) Ignoring obsolete event in terminated client") + return + } guard let dataChannel = self.dataChannel else { Logger.error("\(self.TAG) in \(#function) ignoring sending \(data) for nil dataChannel") @@ -438,6 +488,10 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD /** 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") + return + } Logger.debug("\(self.TAG) dataChannel didReceiveMessageWith buffer:\(buffer)") guard let dataChannelMessage = OWSWebRTCProtosData.parse(from:buffer.data) else { @@ -447,8 +501,9 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD } if let delegate = self.delegate { - DispatchQueue.main.async { - delegate.peerConnectionClient(self, received: dataChannelMessage) + DispatchQueue.main.async { [weak self] in + guard let strongSelf = self else { return } + delegate.peerConnectionClient(strongSelf, received: dataChannelMessage) } } } @@ -469,14 +524,20 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD /** Called when media is received on a new stream from remote peer. */ internal func peerConnection(_ peerConnection: RTCPeerConnection, didAdd stream: RTCMediaStream) { PeerConnectionClient.signalingQueue.async { + guard self.peerConnection != nil else { + Logger.debug("\(self.TAG) \(#function) Ignoring obsolete event in terminated client") + return + } Logger.debug("\(self.TAG) didAdd stream:\(stream) video tracks: \(stream.videoTracks.count) audio tracks: \(stream.audioTracks.count)") if stream.videoTracks.count > 0 { self.remoteVideoTrack = stream.videoTracks[0] if let delegate = self.delegate { let remoteVideoTrack = self.remoteVideoTrack - DispatchQueue.main.async { - delegate.peerConnectionClient(self, didUpdateRemote: remoteVideoTrack) + DispatchQueue.main.async { [weak self, weak remoteVideoTrack] in + guard let strongSelf = self else { return } + guard let strongRemoteVideoTrack = remoteVideoTrack else { return } + delegate.peerConnectionClient(strongSelf, didUpdateRemote: strongRemoteVideoTrack) } } } @@ -496,19 +557,25 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD /** 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") + return + } Logger.debug("\(self.TAG) didChange IceConnectionState:\(newState.debugDescription)") switch newState { case .connected, .completed: if let delegate = self.delegate { - DispatchQueue.main.async { - delegate.peerConnectionClientIceConnected(self) + DispatchQueue.main.async { [weak self] in + guard let strongSelf = self else { return } + delegate.peerConnectionClientIceConnected(strongSelf) } } case .failed: Logger.warn("\(self.TAG) RTCIceConnection failed.") if let delegate = self.delegate { - DispatchQueue.main.async { - delegate.peerConnectionClientIceFailed(self) + DispatchQueue.main.async { [weak self] in + guard let strongSelf = self else { return } + delegate.peerConnectionClientIceFailed(strongSelf) } } case .disconnected: @@ -527,10 +594,15 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD /** 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") + return + } Logger.debug("\(self.TAG) didGenerate IceCandidate:\(candidate.sdp)") if let delegate = self.delegate { - DispatchQueue.main.async { - delegate.peerConnectionClient(self, addedLocalIceCandidate: candidate) + DispatchQueue.main.async { [weak self] in + guard let strongSelf = self else { return } + delegate.peerConnectionClient(strongSelf, addedLocalIceCandidate: candidate) } } } @@ -544,6 +616,10 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD /** 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") + return + } Logger.debug("\(self.TAG) didOpen dataChannel:\(dataChannel)") assert(self.dataChannel == nil) self.dataChannel = dataChannel