From b4464f91a288458c919c10ed3f5b46d50b5b5a11 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 18 Apr 2017 12:12:17 -0400 Subject: [PATCH] Improve handling of call service edge cases. // FREEBIE --- Signal/src/call/CallService.swift | 153 ++++++++++++++++-------------- 1 file changed, 83 insertions(+), 70 deletions(-) diff --git a/Signal/src/call/CallService.swift b/Signal/src/call/CallService.swift index 682355f9e..8878da875 100644 --- a/Signal/src/call/CallService.swift +++ b/Signal/src/call/CallService.swift @@ -331,8 +331,7 @@ protocol CallServiceObserver: class { return race(timeout, callConnectedPromise) }.then { guard self.call == call else { - Logger.debug("\(self.TAG) obsolete call in \(#function)") - throw CallError.assertionError(description:"obsolete call in \(#function)") + throw CallError.obsoleteCall(description: "obsolete outgoing call connected") } Logger.info("\(self.TAG) outgoing call connected.") @@ -345,10 +344,10 @@ protocol CallServiceObserver: class { Logger.error("\(self.TAG) placing call failed with error: \(error)") if let callError = error as? CallError { - self.handleFailedCall(failedCall:call, error: callError) + self.handleFailedCall(failedCall: call, error: callError) } else { let externalError = CallError.externalError(underlyingError: error) - self.handleFailedCall(failedCall:call, error: externalError) + self.handleFailedCall(failedCall: call, error: externalError) } } } @@ -381,7 +380,7 @@ protocol CallServiceObserver: class { } guard let peerConnectionClient = self.peerConnectionClient else { - handleFailedCall(failedCall:call, error: CallError.assertionError(description: "peerConnectionClient was unexpectedly nil in \(#function)")) + handleFailedCall(failedCall: call, error: CallError.assertionError(description: "peerConnectionClient was unexpectedly nil in \(#function)")) return } @@ -390,10 +389,10 @@ protocol CallServiceObserver: class { Logger.debug("\(self.TAG) successfully set remote description") }.catch { error in if let callError = error as? CallError { - self.handleFailedCall(failedCall:call, error: callError) + self.handleFailedCall(failedCall: call, error: callError) } else { let externalError = CallError.externalError(underlyingError: error) - self.handleFailedCall(failedCall:call, error: externalError) + self.handleFailedCall(failedCall: call, error: externalError) } } } @@ -443,9 +442,9 @@ protocol CallServiceObserver: class { Logger.debug("\(TAG) \(#function) for thread: \(thread)") AssertIsOnMainThread() - // Assume this event pertains to the current call. guard let call = self.call else { - handleFailedCall(failedCall: self.call, error: .assertionError(description: "call unexpectedly nil in \(#function)")) + // Assume this event pertains to the current call. + handleFailedCurrentCall(error: .assertionError(description: "call unexpectedly nil in \(#function)")) return } @@ -487,7 +486,7 @@ protocol CallServiceObserver: class { Logger.warn("\(self.TAG) ignoring obsolete call in \(#function)") return } - self.handleFailedCall(failedCall:newCall, error: timeout) + self.handleFailedCall(failedCall: newCall, error: timeout) } } @@ -544,7 +543,7 @@ protocol CallServiceObserver: class { return race(promise, timeout) }.then { guard self.call == newCall else { - throw CallError.obsoleteCall(description: "time out for obsolete incoming call") + throw CallError.obsoleteCall(description: "obsolete incoming call connected") } Logger.info("\(self.TAG) incoming call connected.") @@ -558,10 +557,10 @@ protocol CallServiceObserver: class { return } if let callError = error as? CallError { - self.handleFailedCall(failedCall : newCall, error: callError) + self.handleFailedCall(failedCall: newCall, error: callError) } else { let externalError = CallError.externalError(underlyingError: error) - self.handleFailedCall(failedCall : newCall, error: externalError) + self.handleFailedCall(failedCall: newCall, error: externalError) } }.always { Logger.debug("\(self.TAG) ending background task awaiting inbound call connection") @@ -577,27 +576,27 @@ protocol CallServiceObserver: class { Logger.debug("\(TAG) called \(#function)") guard self.thread != nil else { - handleFailedCall(failedCall : nil, error: .assertionError(description: "ignoring remote ice update for thread: \(thread.uniqueId) since there is no current thread. Call already ended?")) + Logger.warn("ignoring remote ice update for thread: \(thread.uniqueId) since there is no current thread. Call already ended?") return } guard thread.contactIdentifier() == self.thread!.contactIdentifier() else { - handleFailedCall(failedCall : nil, error: .assertionError(description: "ignoring remote ice update for thread: \(thread.uniqueId) since the current call is for thread: \(self.thread!.uniqueId)")) + Logger.warn("ignoring remote ice update for thread: \(thread.uniqueId) since there is no current thread. Call already ended?") return } guard let call = self.call else { - handleFailedCall(failedCall : nil, error: .assertionError(description: "ignoring remote ice update for callId: \(callId), since there is no current call.")) + Logger.warn("ignoring remote ice update for thread: \(thread.uniqueId) since there is no current thread. Call already ended?") return } guard call.signalingId == callId else { - handleFailedCall(failedCall : nil, error: .assertionError(description: "ignoring remote ice update for call: \(callId) since the current call is: \(call.signalingId)")) + Logger.warn("ignoring remote ice update for thread: \(thread.uniqueId) since there is no current thread. Call already ended?") return } guard let peerConnectionClient = self.peerConnectionClient else { - handleFailedCall(failedCall: call, error: .assertionError(description: "ignoring remote ice update for thread: \(thread) since the current call hasn't initialized it's peerConnectionClient")) + Logger.warn("ignoring remote ice update for thread: \(thread.uniqueId) since there is no current thread. Call already ended?") return } @@ -612,17 +611,23 @@ protocol CallServiceObserver: class { AssertIsOnMainThread() guard let call = self.call else { - handleFailedCall(failedCall:self.call, error: .assertionError(description: "ignoring local ice candidate, since there is no current call.")) + // This will only be called for the current peerConnectionClient, so + // fail the current call. + handleFailedCurrentCall(error: .assertionError(description: "ignoring local ice candidate, since there is no current call.")) return } guard call.state != .idle else { - handleFailedCall(failedCall:self.call, error: .assertionError(description: "ignoring local ice candidate, since call is now idle.")) + // This will only be called for the current peerConnectionClient, so + // fail the current call. + handleFailedCurrentCall(error: .assertionError(description: "ignoring local ice candidate, since call is now idle.")) return } guard let thread = self.thread else { - handleFailedCall(failedCall:self.call, error: .assertionError(description: "ignoring local ice candidate, because there was no current TSContactThread.")) + // This will only be called for the current peerConnectionClient, so + // fail the current call. + handleFailedCurrentCall(error: .assertionError(description: "ignoring local ice candidate, because there was no current TSContactThread.")) return } @@ -653,12 +658,16 @@ protocol CallServiceObserver: class { Logger.debug("\(TAG) in \(#function)") guard let call = self.call else { - handleFailedCall(failedCall:self.call, error: .assertionError(description:"\(TAG) ignoring \(#function) since there is no current call.")) + // This will only be called for the current peerConnectionClient, so + // fail the current call. + handleFailedCurrentCall(error: .assertionError(description:"\(TAG) ignoring \(#function) since there is no current call.")) return } guard let thread = self.thread else { - handleFailedCall(failedCall:self.call, error: .assertionError(description:"\(TAG) ignoring \(#function) since there is no current thread.")) + // This will only be called for the current peerConnectionClient, so + // fail the current call. + handleFailedCurrentCall(error: .assertionError(description:"\(TAG) ignoring \(#function) since there is no current thread.")) return } @@ -692,7 +701,8 @@ protocol CallServiceObserver: class { } guard let call = self.call else { - handleFailedCall(failedCall:nil, error: .assertionError(description:"\(TAG) call was unexpectedly nil in \(#function)")) + // This should never happen; return to a known good state. + handleFailedCurrentCall(error: .assertionError(description:"\(TAG) call was unexpectedly nil in \(#function)")) return } @@ -720,12 +730,14 @@ protocol CallServiceObserver: class { AssertIsOnMainThread() guard let call = self.call else { - handleFailedCall(failedCall:self.call, error: .assertionError(description:"\(TAG) call was unexpectedly nil in \(#function)")) + // This should never happen; return to a known good state. + handleFailedCurrentCall(error: .assertionError(description:"\(TAG) call was unexpectedly nil in \(#function)")) return } guard call.localId == localId else { - handleFailedCall(failedCall:self.call, error: .assertionError(description:"\(TAG) callLocalId:\(localId) doesn't match current calls: \(call.localId)")) + // This should never happen; return to a known good state. + handleFailedCurrentCall(error: .assertionError(description:"\(TAG) callLocalId:\(localId) doesn't match current calls: \(call.localId)")) return } @@ -741,7 +753,7 @@ protocol CallServiceObserver: class { Logger.debug("\(TAG) in \(#function)") guard self.call != nil else { - handleFailedCall(failedCall:call, error: .assertionError(description:"\(TAG) ignoring \(#function) since there is no current call")) + handleFailedCall(failedCall: call, error: .assertionError(description:"\(TAG) ignoring \(#function) since there is no current call")) return } @@ -753,12 +765,12 @@ protocol CallServiceObserver: class { } guard let thread = self.thread else { - handleFailedCall(failedCall:call, error: .assertionError(description:"\(TAG) ignoring \(#function) for call other than current call")) + handleFailedCall(failedCall: call, error: .assertionError(description:"\(TAG) ignoring \(#function) for call other than current call")) return } guard let peerConnectionClient = self.peerConnectionClient else { - handleFailedCall(failedCall:call, error: .assertionError(description:"\(TAG) missing peerconnection client in \(#function)")) + handleFailedCall(failedCall: call, error: .assertionError(description:"\(TAG) missing peerconnection client in \(#function)")) return } @@ -781,7 +793,7 @@ protocol CallServiceObserver: class { AssertIsOnMainThread() guard let peerConnectionClient = self.peerConnectionClient else { - handleFailedCall(failedCall:call, error: .assertionError(description:"\(TAG) peerConnectionClient unexpectedly nil in \(#function)")) + handleFailedCall(failedCall: call, error: .assertionError(description:"\(TAG) peerConnectionClient unexpectedly nil in \(#function)")) return } @@ -807,12 +819,14 @@ protocol CallServiceObserver: class { AssertIsOnMainThread() guard let call = self.call else { - handleFailedCall(failedCall:nil, error: .assertionError(description:"\(TAG) call was unexpectedly nil in \(#function)")) + // This should never happen; return to a known good state. + handleFailedCurrentCall(error: .assertionError(description:"\(TAG) call was unexpectedly nil in \(#function)")) return } guard call.localId == localId else { - handleFailedCall(failedCall:nil, error: .assertionError(description:"\(TAG) callLocalId:\(localId) doesn't match current calls: \(call.localId)")) + // This should never happen; return to a known good state. + handleFailedCurrentCall(error: .assertionError(description:"\(TAG) callLocalId:\(localId) doesn't match current calls: \(call.localId)")) return } @@ -842,22 +856,22 @@ protocol CallServiceObserver: class { AssertIsOnMainThread() guard self.call != nil else { - handleFailedCall(failedCall:call, error: .assertionError(description:"\(TAG) ignoring \(#function) since there is no current call")) + handleFailedCall(failedCall: call, error: .assertionError(description:"\(TAG) ignoring \(#function) since there is no current call")) return } guard call == self.call! else { - handleFailedCall(failedCall:call, error: .assertionError(description:"\(TAG) ignoring \(#function) for call other than current call")) + handleFailedCall(failedCall: call, error: .assertionError(description:"\(TAG) ignoring \(#function) for call other than current call")) return } guard let peerConnectionClient = self.peerConnectionClient else { - handleFailedCall(failedCall:call, error: .assertionError(description:"\(TAG) missing peerconnection client in \(#function)")) + handleFailedCall(failedCall: call, error: .assertionError(description:"\(TAG) missing peerconnection client in \(#function)")) return } guard let thread = self.thread else { - handleFailedCall(failedCall:call, error: .assertionError(description:"\(TAG) missing thread in \(#function)")) + handleFailedCall(failedCall: call, error: .assertionError(description:"\(TAG) missing thread in \(#function)")) return } @@ -892,12 +906,14 @@ protocol CallServiceObserver: class { AssertIsOnMainThread() guard let peerConnectionClient = self.peerConnectionClient else { - handleFailedCall(failedCall:self.call, error: .assertionError(description:"\(TAG) peerConnectionClient unexpectedly nil in \(#function)")) + // This should never happen; return to a known good state. + handleFailedCurrentCall(error: .assertionError(description:"\(TAG) peerConnectionClient unexpectedly nil in \(#function)")) return } guard let call = self.call else { - handleFailedCall(failedCall:self.call, error: .assertionError(description:"\(TAG) call unexpectedly nil in \(#function)")) + // This should never happen; return to a known good state. + handleFailedCurrentCall(error: .assertionError(description:"\(TAG) call unexpectedly nil in \(#function)")) return } @@ -944,12 +960,14 @@ protocol CallServiceObserver: class { } guard let peerConnectionClient = self.peerConnectionClient else { - handleFailedCall(failedCall:self.call, error: .assertionError(description:"\(TAG) peerConnectionClient unexpectedly nil in \(#function)")) + // This should never happen; return to a known good state. + handleFailedCurrentCall(error: .assertionError(description:"\(TAG) peerConnectionClient unexpectedly nil in \(#function)")) return } guard let call = self.call else { - handleFailedCall(failedCall:self.call, error: .assertionError(description:"\(TAG) call unexpectedly nil in \(#function)")) + // This should never happen; return to a known good state. + handleFailedCurrentCall(error: .assertionError(description:"\(TAG) call unexpectedly nil in \(#function)")) return } @@ -977,7 +995,8 @@ protocol CallServiceObserver: class { AssertIsOnMainThread() guard let call = self.call else { - handleFailedCall(failedCall:self.call, error: .assertionError(description:"\(TAG) received data message, but there is no current call. Ignoring.")) + // This should never happen; return to a known good state. + handleFailedCurrentCall(error: .assertionError(description:"\(TAG) received data message, but there is no current call. Ignoring.")) return } @@ -987,7 +1006,8 @@ protocol CallServiceObserver: class { let connected = message.connected! guard connected.id == call.signalingId else { - handleFailedCall(failedCall:self.call, error: .assertionError(description:"\(TAG) received connected message for call with id:\(connected.id) but current call has id:\(call.signalingId)")) + // This should never happen; return to a known good state. + handleFailedCurrentCall(error: .assertionError(description:"\(TAG) received connected message for call with id:\(connected.id) but current call has id:\(call.signalingId)")) return } @@ -1000,12 +1020,14 @@ protocol CallServiceObserver: class { let hangup = message.hangup! guard hangup.id == call.signalingId else { - handleFailedCall(failedCall:self.call, error: .assertionError(description:"\(TAG) received hangup message for call with id:\(hangup.id) but current call has id:\(call.signalingId)")) + // This should never happen; return to a known good state. + handleFailedCurrentCall(error: .assertionError(description:"\(TAG) received hangup message for call with id:\(hangup.id) but current call has id:\(call.signalingId)")) return } guard let thread = self.thread else { - handleFailedCall(failedCall:self.call, error: .assertionError(description:"\(TAG) current contact thread is unexpectedly nil when receiving hangup DataChannelMessage")) + // This should never happen; return to a known good state. + handleFailedCurrentCall(error: .assertionError(description:"\(TAG) current contact thread is unexpectedly nil when receiving hangup DataChannelMessage")) return } @@ -1046,7 +1068,8 @@ protocol CallServiceObserver: class { return } - self.handleFailedCall(failedCall:nil, error: CallError.disconnected) + // Return to a known good state. + self.handleFailedCurrentCall(error: CallError.disconnected) } /** @@ -1133,32 +1156,20 @@ protocol CallServiceObserver: class { } } - // TODO: -// private func handleFailedCall(error: CallError) { -// handleFailedCall(failedCall: nil, error: error) -// } -// -// private func handleFailedCall(callId: UInt64, error: CallError) { -// AssertIsOnMainThread() -// -// guard let call = self.call else { -// handleFailedCall(failedCall:nil, error: error) -// return -// } -// -// guard call.signalingId == callId else { -// handleFailedCall(failedCall:nil, error: error) -// return -// } -// -// handleFailedCall(failedCall: call, error: error) -// } - + // This method should be called when either: a) we know or assume that + // the error is related to the current call. b) the error is so serious + // that we want to terminate the current call (if any) in order to + // return to a known good state. public func handleFailedCurrentCall(error: CallError) { - handleFailedCall(failedCall: self.call, error: error) + handleFailedCall(failedCall: self.call, error: error, forceTerminate:true) } - public func handleFailedCall(failedCall: SignalCall?, error: CallError) { + // This method should be called when a fatal error occurred for a call. + // + // * If we know which call it was, we should update that call's state + // to reflect the error. + // * IFF that call is the current call, we want to terminate it. + public func handleFailedCall(failedCall: SignalCall?, error: CallError, forceTerminate: Bool = false) { AssertIsOnMainThread() Logger.error("\(TAG) call failed with error: \(error)") @@ -1172,8 +1183,10 @@ protocol CallServiceObserver: class { failedCall.state = .localFailure self.callUIAdapter.failCall(failedCall, error: error) - // Only terminate the current call if it is the current call. - guard failedCall == self.call else { + // Only terminate the current call if the error pertains to the current call, + // or if we're trying to return to a known good state. + let shouldTerminate = forceTerminate || failedCall == self.call + guard shouldTerminate else { Logger.debug("\(TAG) in \(#function) ignoring obsolete call.") return }