From 4feb0011d703995f392cebc320690a7eb9a1af09 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Wed, 30 Jan 2019 10:36:11 -0500 Subject: [PATCH 1/8] Reduce logging. --- .../ConversationView/ConversationViewController.m | 4 ---- Signal/src/ViewControllers/HomeView/HomeViewController.m | 2 -- .../src/Messages/Interactions/TSOutgoingMessage.m | 2 +- 3 files changed, 1 insertion(+), 7 deletions(-) diff --git a/Signal/src/ViewControllers/ConversationView/ConversationViewController.m b/Signal/src/ViewControllers/ConversationView/ConversationViewController.m index 2c3d14173..a14ddbd68 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationViewController.m +++ b/Signal/src/ViewControllers/ConversationView/ConversationViewController.m @@ -4580,8 +4580,6 @@ typedef enum : NSUInteger { OWSAssertIsOnMainThread(); OWSAssertDebug(self.conversationViewModel); - OWSLogVerbose(@""); - // HACK to work around radar #28167779 // "UICollectionView performBatchUpdates can trigger a crash if the collection view is flagged for layout" // more: https://github.com/PSPDFKit-labs/radar.apple.com/tree/master/28167779%20-%20CollectionViewBatchingIssue @@ -4604,8 +4602,6 @@ typedef enum : NSUInteger { return; } - OWSLogVerbose(@""); - [self updateBackButtonUnreadCount]; [self updateNavigationBarSubtitleLabel]; diff --git a/Signal/src/ViewControllers/HomeView/HomeViewController.m b/Signal/src/ViewControllers/HomeView/HomeViewController.m index 70f330944..318fabf84 100644 --- a/Signal/src/ViewControllers/HomeView/HomeViewController.m +++ b/Signal/src/ViewControllers/HomeView/HomeViewController.m @@ -1321,8 +1321,6 @@ NSString *const kArchivedConversationsReuseIdentifier = @"kArchivedConversations return; } - OWSLogVerbose(@""); - NSArray *notifications = [self.uiDatabaseConnection beginLongLivedReadTransaction]; if (![[self.uiDatabaseConnection ext:TSThreadDatabaseViewExtensionName] hasChangesForGroup:self.currentGrouping diff --git a/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.m b/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.m index 82505106f..208f6035a 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.m +++ b/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.m @@ -462,7 +462,7 @@ NSString *NSStringForOutgoingMessageRecipientState(OWSOutgoingMessageRecipientSt // There's no need to save this message, since it's not displayed to the user. // // Should we find a need to save this in the future, we need to exclude any non-serializable properties. - OWSLogDebug(@"Skipping save for group meta message."); + OWSLogDebug(@"Skipping save for transient outgoing message."); return; } From 6b5952abda92b8024de31cb5bf844f02e219ab4d Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Wed, 30 Jan 2019 10:36:23 -0500 Subject: [PATCH 2/8] Move work off main thread. --- SignalMessaging/profiles/ProfileFetcherJob.swift | 9 +++------ SignalServiceKit/src/Messages/UD/OWSRequestMaker.swift | 10 +++++----- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/SignalMessaging/profiles/ProfileFetcherJob.swift b/SignalMessaging/profiles/ProfileFetcherJob.swift index e3ad5b0f6..176729b02 100644 --- a/SignalMessaging/profiles/ProfileFetcherJob.swift +++ b/SignalMessaging/profiles/ProfileFetcherJob.swift @@ -1,5 +1,5 @@ // -// Copyright (c) 2018 Open Whisper Systems. All rights reserved. +// Copyright (c) 2019 Open Whisper Systems. All rights reserved. // import Foundation @@ -98,7 +98,7 @@ public class ProfileFetcherJob: NSObject { Logger.error("background task time ran out before profile fetch completed.") }) - DispatchQueue.main.async { + DispatchQueue.global().async { for recipientId in recipientIds { self.getAndUpdateProfile(recipientId: recipientId) } @@ -112,7 +112,7 @@ public class ProfileFetcherJob: NSObject { public func getAndUpdateProfile(recipientId: String, remainingRetries: Int = 3) { self.getProfile(recipientId: recipientId).map(on: DispatchQueue.global()) { profile in self.updateProfile(signalServiceProfile: profile) - }.catch { error in + }.catch(on: DispatchQueue.global()) { error in switch error { case ProfileFetcherJobError.throttled(let lastTimeInterval): Logger.info("skipping updateProfile: \(recipientId), lastTimeInterval: \(lastTimeInterval)") @@ -129,7 +129,6 @@ public class ProfileFetcherJob: NSObject { } public func getProfile(recipientId: String) -> Promise { - AssertIsOnMainThread() if !ignoreThrottling { if let lastDate = ProfileFetcherJob.fetchDateMap[recipientId] { let lastTimeInterval = fabs(lastDate.timeIntervalSinceNow) @@ -162,8 +161,6 @@ public class ProfileFetcherJob: NSObject { private func requestProfile(recipientId: String, udAccess: OWSUDAccess?, canFailoverUDAuth: Bool) -> Promise { - AssertIsOnMainThread() - let requestMaker = RequestMaker(label: "Profile Fetch", requestFactoryBlock: { (udAccessKeyForRequest) -> TSRequest in return OWSRequestFactory.getProfileRequest(recipientId: recipientId, udAccessKey: udAccessKeyForRequest) diff --git a/SignalServiceKit/src/Messages/UD/OWSRequestMaker.swift b/SignalServiceKit/src/Messages/UD/OWSRequestMaker.swift index 2f28fbd0e..7646ce858 100644 --- a/SignalServiceKit/src/Messages/UD/OWSRequestMaker.swift +++ b/SignalServiceKit/src/Messages/UD/OWSRequestMaker.swift @@ -1,5 +1,5 @@ // -// Copyright (c) 2018 Open Whisper Systems. All rights reserved. +// Copyright (c) 2019 Open Whisper Systems. All rights reserved. // import Foundation @@ -95,7 +95,7 @@ public class RequestMaker: NSObject { @objc public func makeRequestObjc() -> AnyPromise { let promise = makeRequest() - .recover { (error: Error) -> Promise in + .recover(on: DispatchQueue.global()) { (error: Error) -> Promise in switch error { case NetworkManagerError.taskError(_, let underlyingError): throw underlyingError @@ -140,7 +140,7 @@ public class RequestMaker: NSObject { }) { (statusCode: Int, responseData: Data?, error: Error) in resolver.reject(RequestMakerError.websocketRequestError(statusCode: statusCode, responseData: responseData, underlyingError: error)) } - }.recover { (error: Error) -> Promise in + }.recover(on: DispatchQueue.global()) { (error: Error) -> Promise in switch error { case RequestMakerError.websocketRequestError(let statusCode, _, _): if isUDRequest && (statusCode == 401 || statusCode == 403) { @@ -173,7 +173,7 @@ public class RequestMaker: NSObject { } } else { return self.networkManager.makePromise(request: request) - .map { (networkManagerResult: TSNetworkManager.NetworkManagerResult) -> RequestMakerResult in + .map(on: DispatchQueue.global()) { (networkManagerResult: TSNetworkManager.NetworkManagerResult) -> RequestMakerResult in if self.udManager.isUDVerboseLoggingEnabled() { if isUDRequest { Logger.debug("UD REST request '\(self.label)' succeeded.") @@ -188,7 +188,7 @@ public class RequestMaker: NSObject { return RequestMakerResult(responseObject: networkManagerResult.responseObject, wasSentByUD: isUDRequest, wasSentByWebsocket: false) - }.recover { (error: Error) -> Promise in + }.recover(on: DispatchQueue.global()) { (error: Error) -> Promise in switch error { case NetworkManagerError.taskError(let task, _): let statusCode = task.statusCode() From 5bb78cba25643c89082f5be9606e9e5328e1d526 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Wed, 30 Jan 2019 11:52:00 -0500 Subject: [PATCH 3/8] Tune concurrency in call service. --- Signal/src/call/CallService.swift | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/Signal/src/call/CallService.swift b/Signal/src/call/CallService.swift index 76fc275ac..b3ea7c409 100644 --- a/Signal/src/call/CallService.swift +++ b/Signal/src/call/CallService.swift @@ -249,6 +249,9 @@ private class SignalCallData: NSObject { return callData?.call } + didSet { + AssertIsOnMainThread() + } } var peerConnectionClient: PeerConnectionClient? { get { @@ -256,6 +259,9 @@ private class SignalCallData: NSObject { return callData?.peerConnectionClient } + didSet { + AssertIsOnMainThread() + } } weak var localCaptureSession: AVCaptureSession? { @@ -264,6 +270,9 @@ private class SignalCallData: NSObject { return callData?.localCaptureSession } + didSet { + AssertIsOnMainThread() + } } var remoteVideoTrack: RTCVideoTrack? { @@ -272,6 +281,9 @@ private class SignalCallData: NSObject { return callData?.remoteVideoTrack } + didSet { + AssertIsOnMainThread() + } } var isRemoteVideoEnabled: Bool { get { @@ -282,6 +294,9 @@ private class SignalCallData: NSObject { } return callData.isRemoteVideoEnabled } + didSet { + AssertIsOnMainThread() + } } @objc public override init() { @@ -371,7 +386,8 @@ private class SignalCallData: NSObject { callRecord.save() call.callRecord = callRecord - let promise = getIceServers().then { iceServers -> Promise in + let promise = getIceServers() + .then { iceServers -> Promise in Logger.debug("got ice servers:\(iceServers) for call: \(call.identifiersForLogs)") guard self.call == call else { @@ -686,9 +702,8 @@ private class SignalCallData: NSObject { strongSelf.handleFailedCall(failedCall: newCall, error: timeout) }) - firstly { - getIceServers() - }.then { (iceServers: [RTCIceServer]) -> Promise in + getIceServers() + .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. guard self.call == newCall else { @@ -1520,11 +1535,9 @@ private class SignalCallData: NSObject { * a list of servers, plus we have fallback servers hardcoded in the app. */ private func getIceServers() -> Promise<[RTCIceServer]> { - AssertIsOnMainThread() - return firstly { - accountManager.getTurnServerInfo() - }.map { turnServerInfo -> [RTCIceServer] in + self.accountManager.getTurnServerInfo() + .map(on: DispatchQueue.global()) { turnServerInfo -> [RTCIceServer] in Logger.debug("got turn server urls: \(turnServerInfo.urls)") return turnServerInfo.urls.map { url in @@ -1537,7 +1550,7 @@ private class SignalCallData: NSObject { return RTCIceServer(urlStrings: [url]) } } + [CallService.fallbackIceServer] - }.recover { (error: Error) -> Guarantee<[RTCIceServer]> in + }.recover(on: DispatchQueue.global()) { (error: Error) -> Guarantee<[RTCIceServer]> in Logger.error("fetching ICE servers failed with error: \(error)") Logger.warn("using fallback ICE Servers") From 70185dd872862744f9a318e30a3e675b46662799 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Wed, 30 Jan 2019 13:17:40 -0500 Subject: [PATCH 4/8] Batch outgoing ICE updates. --- Signal/src/call/CallService.swift | 123 +++++++++++++----- .../src/Messages/OWSOutgoingCallMessage.h | 1 - .../src/Messages/OWSOutgoingCallMessage.m | 14 +- 3 files changed, 88 insertions(+), 50 deletions(-) diff --git a/Signal/src/call/CallService.swift b/Signal/src/call/CallService.swift index b3ea7c409..c1799f812 100644 --- a/Signal/src/call/CallService.swift +++ b/Signal/src/call/CallService.swift @@ -192,6 +192,70 @@ private class SignalCallData: NSObject { peerConnectionClient?.terminate() Logger.debug("setting peerConnectionClient") + + outgoingIceUpdateQueue.removeAll() + } + + // MARK: - Dependencies + + private var messageSender: MessageSender { + return SSKEnvironment.shared.messageSender + } + + // MARK: - Outgoing ICE updates. + + // Setting up a call involves sending many (currently 20+) ICE updates. + // We send messages serially in order to preserve outgoing message order. + // There are so many ICE updates per call that the cost of sending all of + // those messages becomes significant. So we batch outgoing ICE updates, + // making sure that we only have one outgoing ICE update message at a time. + // + // This variable should only be accessed on the main thread. + private var outgoingIceUpdateQueue = [SSKProtoCallMessageIceUpdate]() + private var outgoingIceUpdatesInFlight = false + + func sendOrEnqueue(outgoingIceUpdate iceUpdateProto: SSKProtoCallMessageIceUpdate) { + AssertIsOnMainThread() + + outgoingIceUpdateQueue.append(iceUpdateProto) + + tryToSendIceUpdates() + } + + private func tryToSendIceUpdates() { + + guard !outgoingIceUpdatesInFlight else { + Logger.verbose("Enqueued outgoing ice update") + return + } + + let iceUpdateProtos = outgoingIceUpdateQueue + guard iceUpdateProtos.count > 0 else { + // Nothing in the queue. + return + } + + outgoingIceUpdateQueue.removeAll() + outgoingIceUpdatesInFlight = true + + /** + * Sent by both parties out of band of the RTC calling channels, as part of setting up those channels. The messages + * include network accessibility information from the perspective of each client. Once compatible ICEUpdates have been + * exchanged, the clients can connect. + */ + let callMessage = OWSOutgoingCallMessage(thread: call.thread, iceUpdateMessages: iceUpdateProtos) + let sendPromise = self.messageSender.sendPromise(message: callMessage) + .ensure { [weak self] in + AssertIsOnMainThread() + + guard let strongSelf = self else { + return + } + + strongSelf.outgoingIceUpdatesInFlight = false + strongSelf.tryToSendIceUpdates() + } + sendPromise.retainUntilComplete() } } @@ -249,9 +313,6 @@ private class SignalCallData: NSObject { return callData?.call } - didSet { - AssertIsOnMainThread() - } } var peerConnectionClient: PeerConnectionClient? { get { @@ -259,9 +320,6 @@ private class SignalCallData: NSObject { return callData?.peerConnectionClient } - didSet { - AssertIsOnMainThread() - } } weak var localCaptureSession: AVCaptureSession? { @@ -270,9 +328,6 @@ private class SignalCallData: NSObject { return callData?.localCaptureSession } - didSet { - AssertIsOnMainThread() - } } var remoteVideoTrack: RTCVideoTrack? { @@ -281,9 +336,6 @@ private class SignalCallData: NSObject { return callData?.remoteVideoTrack } - didSet { - AssertIsOnMainThread() - } } var isRemoteVideoEnabled: Bool { get { @@ -294,9 +346,6 @@ private class SignalCallData: NSObject { } return callData.isRemoteVideoEnabled } - didSet { - AssertIsOnMainThread() - } } @objc public override init() { @@ -420,9 +469,9 @@ private class SignalCallData: NSObject { Logger.info("session description for outgoing call: \(call.identifiersForLogs), sdp: \(sessionDescription.logSafeDescription).") - return firstly { + return peerConnectionClient.setLocalSessionDescription(sessionDescription) - }.then { _ -> Promise in + .then { _ -> Promise in do { let offerBuilder = SSKProtoCallMessageOffer.builder(id: call.signalingId, sessionDescription: sessionDescription.sdp) @@ -516,9 +565,8 @@ private class SignalCallData: NSObject { let sessionDescription = RTCSessionDescription(type: .answer, sdp: sessionDescription) - firstly { - peerConnectionClient.setRemoteSessionDescription(sessionDescription) - }.done { + peerConnectionClient.setRemoteSessionDescription(sessionDescription) + .done { Logger.debug("successfully set remote description") }.catch { error in if let callError = error as? CallError { @@ -872,23 +920,24 @@ private class SignalCallData: NSObject { Logger.info("sending ICE Candidate \(call.identifiersForLogs).") + let iceUpdateProto: SSKProtoCallMessageIceUpdate do { - /** - * Sent by both parties out of band of the RTC calling channels, as part of setting up those channels. The messages - * include network accessibility information from the perspective of each client. Once compatible ICEUpdates have been - * exchanged, the clients can connect. - */ let iceUpdateBuilder = SSKProtoCallMessageIceUpdate.builder(id: call.signalingId, - sdpMid: sdpMid, - sdpMlineIndex: UInt32(iceCandidate.sdpMLineIndex), - sdp: iceCandidate.sdp) - let callMessage = OWSOutgoingCallMessage(thread: call.thread, iceUpdateMessage: try iceUpdateBuilder.build()) - let sendPromise = self.messageSender.sendPromise(message: callMessage) - sendPromise.retainUntilComplete() + sdpMid: sdpMid, + sdpMlineIndex: UInt32(iceCandidate.sdpMLineIndex), + sdp: iceCandidate.sdp) + iceUpdateProto = try iceUpdateBuilder.build() } catch { owsFailDebug("Couldn't build proto") throw CallError.fatalError(description: "Couldn't build proto") } + + /** + * Sent by both parties out of band of the RTC calling channels, as part of setting up those channels. The messages + * include network accessibility information from the perspective of each client. Once compatible ICEUpdates have been + * exchanged, the clients can connect. + */ + callData.sendOrEnqueue(outgoingIceUpdate: iceUpdateProto) }.catch { error in OWSProdInfo(OWSAnalyticsEvents.callServiceErrorHandleLocalAddedIceCandidate(), file: #file, function: #function, line: #line) Logger.error("waitUntilReadyToSendIceUpdates failed with error: \(error)") @@ -898,12 +947,14 @@ private class SignalCallData: NSObject { /** * The clients can now communicate via WebRTC. * - * Called by both caller and callee. Compatible ICE messages have been exchanged between the local and remote + * Called by both caller and callee. Compatible ICE messages have been exchanged between the local and remote * client. */ private func handleIceConnected() { AssertIsOnMainThread() + Logger.verbose("-----.") + guard let call = self.call else { // This will only be called for the current peerConnectionClient, so // fail the current call. @@ -1216,9 +1267,9 @@ private class SignalCallData: NSObject { do { let hangupBuilder = SSKProtoCallMessageHangup.builder(id: call.signalingId) let callMessage = OWSOutgoingCallMessage(thread: call.thread, hangupMessage: try hangupBuilder.build()) - firstly { - self.messageSender.sendPromise(message: callMessage) - }.done { + + self.messageSender.sendPromise(message: callMessage) + .done { Logger.debug("successfully sent hangup call message to \(call.thread.contactIdentifier())") }.catch { error in OWSProdInfo(OWSAnalyticsEvents.callServiceErrorHandleLocalHungupCall(), file: #file, function: #function, line: #line) @@ -1536,7 +1587,7 @@ private class SignalCallData: NSObject { */ private func getIceServers() -> Promise<[RTCIceServer]> { - self.accountManager.getTurnServerInfo() + return self.accountManager.getTurnServerInfo() .map(on: DispatchQueue.global()) { turnServerInfo -> [RTCIceServer] in Logger.debug("got turn server urls: \(turnServerInfo.urls)") diff --git a/SignalServiceKit/src/Messages/OWSOutgoingCallMessage.h b/SignalServiceKit/src/Messages/OWSOutgoingCallMessage.h index e9a47474f..490f5632e 100644 --- a/SignalServiceKit/src/Messages/OWSOutgoingCallMessage.h +++ b/SignalServiceKit/src/Messages/OWSOutgoingCallMessage.h @@ -32,7 +32,6 @@ NS_ASSUME_NONNULL_BEGIN - (instancetype)initWithThread:(TSThread *)thread offerMessage:(SSKProtoCallMessageOffer *)offerMessage; - (instancetype)initWithThread:(TSThread *)thread answerMessage:(SSKProtoCallMessageAnswer *)answerMessage; -- (instancetype)initWithThread:(TSThread *)thread iceUpdateMessage:(SSKProtoCallMessageIceUpdate *)iceUpdateMessage; - (instancetype)initWithThread:(TSThread *)thread iceUpdateMessages:(NSArray *)iceUpdateMessage; - (instancetype)initWithThread:(TSThread *)thread hangupMessage:(SSKProtoCallMessageHangup *)hangupMessage; diff --git a/SignalServiceKit/src/Messages/OWSOutgoingCallMessage.m b/SignalServiceKit/src/Messages/OWSOutgoingCallMessage.m index 0903bec88..4ad3b1ec7 100644 --- a/SignalServiceKit/src/Messages/OWSOutgoingCallMessage.m +++ b/SignalServiceKit/src/Messages/OWSOutgoingCallMessage.m @@ -58,18 +58,6 @@ NS_ASSUME_NONNULL_BEGIN return self; } -- (instancetype)initWithThread:(TSThread *)thread iceUpdateMessage:(SSKProtoCallMessageIceUpdate *)iceUpdateMessage -{ - self = [self initWithThread:thread]; - if (!self) { - return self; - } - - _iceUpdateMessages = @[ iceUpdateMessage ]; - - return self; -} - - (instancetype)initWithThread:(TSThread *)thread iceUpdateMessages:(NSArray *)iceUpdateMessages { @@ -189,7 +177,7 @@ NS_ASSUME_NONNULL_BEGIN } else if (self.answerMessage) { payload = @"answerMessage"; } else if (self.iceUpdateMessages.count > 0) { - payload = @"iceUpdateMessage"; + payload = [NSString stringWithFormat:@"iceUpdateMessages: %lu", (unsigned long)self.iceUpdateMessages.count]; } else if (self.hangupMessage) { payload = @"hangupMessage"; } else if (self.busyMessage) { From bb175929715eccacc1a334c150ef73bc21bb6338 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Wed, 30 Jan 2019 15:12:30 -0500 Subject: [PATCH 5/8] Fail call if ICE updates fail to send. --- Signal/src/call/CallService.swift | 38 +++++++++++++++++++++++++++---- 1 file changed, 33 insertions(+), 5 deletions(-) diff --git a/Signal/src/call/CallService.swift b/Signal/src/call/CallService.swift index c1799f812..53decbba0 100644 --- a/Signal/src/call/CallService.swift +++ b/Signal/src/call/CallService.swift @@ -99,8 +99,14 @@ protocol CallServiceObserver: class { remoteVideoTrack: RTCVideoTrack?) } +protocol SignalCallDataDelegate: class { + func outgoingIceUpdateDidFail(call: SignalCall) +} + // Gather all per-call state in one place. private class SignalCallData: NSObject { + fileprivate weak var delegate: SignalCallDataDelegate? + public let call: SignalCall // Used to coordinate promises across delegate methods @@ -147,8 +153,9 @@ private class SignalCallData: NSObject { } } - required init(call: SignalCall) { + required init(call: SignalCall, delegate: SignalCallDataDelegate) { self.call = call + self.delegate = delegate let (callConnectedPromise, callConnectedResolver) = Promise.pending() self.callConnectedPromise = callConnectedPromise @@ -245,7 +252,7 @@ private class SignalCallData: NSObject { */ let callMessage = OWSOutgoingCallMessage(thread: call.thread, iceUpdateMessages: iceUpdateProtos) let sendPromise = self.messageSender.sendPromise(message: callMessage) - .ensure { [weak self] in + .done { [weak self] in AssertIsOnMainThread() guard let strongSelf = self else { @@ -254,13 +261,22 @@ private class SignalCallData: NSObject { strongSelf.outgoingIceUpdatesInFlight = false strongSelf.tryToSendIceUpdates() + }.catch { [weak self] (_) in + AssertIsOnMainThread() + + guard let strongSelf = self else { + return + } + + strongSelf.outgoingIceUpdatesInFlight = false + strongSelf.delegate?.outgoingIceUpdateDidFail(call: strongSelf.call) } sendPromise.retainUntilComplete() } } // This class' state should only be accessed on the main queue. -@objc public class CallService: NSObject, CallObserver, PeerConnectionClientDelegate { +@objc public class CallService: NSObject, CallObserver, PeerConnectionClientDelegate, SignalCallDataDelegate { // MARK: - Properties @@ -280,6 +296,7 @@ private class SignalCallData: NSObject { didSet { AssertIsOnMainThread() + oldValue?.delegate = nil oldValue?.call.removeObserver(self) callData?.call.addObserverAndSyncState(observer: self) @@ -427,7 +444,7 @@ private class SignalCallData: NSObject { return Promise(error: CallError.assertionError(description: errorDescription)) } - let callData = SignalCallData(call: call) + let callData = SignalCallData(call: call, delegate: self) self.callData = callData // MJK TODO remove this timestamp param @@ -728,7 +745,7 @@ private class SignalCallData: NSObject { Logger.info("starting new call: \(newCall.identifiersForLogs)") - let callData = SignalCallData(call: newCall) + let callData = SignalCallData(call: newCall, delegate: self) self.callData = callData var backgroundTask: OWSBackgroundTask? = OWSBackgroundTask(label: "\(#function)", completionBlock: { [weak self] status in @@ -1876,4 +1893,15 @@ private class SignalCallData: NSObject { self.activeCallTimer?.invalidate() self.activeCallTimer = nil } + + // MARK: - SignalCallDataDelegate + + func outgoingIceUpdateDidFail(call: SignalCall) { + guard self.call == call else { + Logger.warn("obsolete call") + return + } + + terminateCall() + } } From 6b3fe045378579d2a4f93a5bfc0c39d41b7aa09c Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Wed, 30 Jan 2019 16:47:35 -0500 Subject: [PATCH 6/8] Use connection property for errors in message receiver. --- SignalServiceKit/src/Messages/OWSMessageReceiver.m | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/SignalServiceKit/src/Messages/OWSMessageReceiver.m b/SignalServiceKit/src/Messages/OWSMessageReceiver.m index f35c6ad1a..3948e4bae 100644 --- a/SignalServiceKit/src/Messages/OWSMessageReceiver.m +++ b/SignalServiceKit/src/Messages/OWSMessageReceiver.m @@ -380,12 +380,11 @@ NSString *const OWSMessageDecryptJobFinderExtensionGroup = @"OWSMessageProcessin OWSFailDebug(@"Could not parse proto."); // TODO: Add analytics. - [[OWSPrimaryStorage.sharedManager newDatabaseConnection] - readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { - TSErrorMessage *errorMessage = [TSErrorMessage corruptedMessageInUnknownThread]; - [SSKEnvironment.shared.notificationsManager notifyUserForThreadlessErrorMessage:errorMessage - transaction:transaction]; - }]; + [self.dbConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { + TSErrorMessage *errorMessage = [TSErrorMessage corruptedMessageInUnknownThread]; + [SSKEnvironment.shared.notificationsManager notifyUserForThreadlessErrorMessage:errorMessage + transaction:transaction]; + }]; dispatch_async(self.serialQueue, ^{ completion(NO); From 41b7205b024dfcfb1a932b7cbfe408d614ecb2a4 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Thu, 31 Jan 2019 13:18:20 -0500 Subject: [PATCH 7/8] Clean up ahead of PR. --- Signal/src/call/CallService.swift | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/Signal/src/call/CallService.swift b/Signal/src/call/CallService.swift index 53decbba0..9b66d0cdd 100644 --- a/Signal/src/call/CallService.swift +++ b/Signal/src/call/CallService.swift @@ -79,6 +79,7 @@ public enum CallError: Error { case timeout(description: String) case obsoleteCall(description: String) case fatalError(description: String) + case messageSendFailure(underlyingError: Error) } // Should be roughly synced with Android client for consistency @@ -100,7 +101,7 @@ protocol CallServiceObserver: class { } protocol SignalCallDataDelegate: class { - func outgoingIceUpdateDidFail(call: SignalCall) + func outgoingIceUpdateDidFail(call: SignalCall, error: Error) } // Gather all per-call state in one place. @@ -261,7 +262,7 @@ private class SignalCallData: NSObject { strongSelf.outgoingIceUpdatesInFlight = false strongSelf.tryToSendIceUpdates() - }.catch { [weak self] (_) in + }.catch { [weak self] (error) in AssertIsOnMainThread() guard let strongSelf = self else { @@ -269,7 +270,7 @@ private class SignalCallData: NSObject { } strongSelf.outgoingIceUpdatesInFlight = false - strongSelf.delegate?.outgoingIceUpdateDidFail(call: strongSelf.call) + strongSelf.delegate?.outgoingIceUpdateDidFail(call: strongSelf.call, error: error) } sendPromise.retainUntilComplete() } @@ -970,8 +971,6 @@ private class SignalCallData: NSObject { private func handleIceConnected() { AssertIsOnMainThread() - Logger.verbose("-----.") - guard let call = self.call else { // This will only be called for the current peerConnectionClient, so // fail the current call. @@ -1896,12 +1895,14 @@ private class SignalCallData: NSObject { // MARK: - SignalCallDataDelegate - func outgoingIceUpdateDidFail(call: SignalCall) { + func outgoingIceUpdateDidFail(call: SignalCall, error: Error) { + AssertIsOnMainThread() + guard self.call == call else { Logger.warn("obsolete call") return } - terminateCall() + handleFailedCall(failedCall: call, error: CallError.messageSendFailure(underlyingError: error)) } } From 867efb62ffc992cb6227e62685c9ee42a5a8288d Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Thu, 31 Jan 2019 15:48:01 -0500 Subject: [PATCH 8/8] Respond to CR. --- Signal/src/call/CallService.swift | 1 + SignalServiceKit/src/Messages/UD/OWSRequestMaker.swift | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/Signal/src/call/CallService.swift b/Signal/src/call/CallService.swift index 9b66d0cdd..9e1d699f5 100644 --- a/Signal/src/call/CallService.swift +++ b/Signal/src/call/CallService.swift @@ -231,6 +231,7 @@ private class SignalCallData: NSObject { } private func tryToSendIceUpdates() { + AssertIsOnMainThread() guard !outgoingIceUpdatesInFlight else { Logger.verbose("Enqueued outgoing ice update") diff --git a/SignalServiceKit/src/Messages/UD/OWSRequestMaker.swift b/SignalServiceKit/src/Messages/UD/OWSRequestMaker.swift index 7646ce858..b8f7ad63b 100644 --- a/SignalServiceKit/src/Messages/UD/OWSRequestMaker.swift +++ b/SignalServiceKit/src/Messages/UD/OWSRequestMaker.swift @@ -140,7 +140,7 @@ public class RequestMaker: NSObject { }) { (statusCode: Int, responseData: Data?, error: Error) in resolver.reject(RequestMakerError.websocketRequestError(statusCode: statusCode, responseData: responseData, underlyingError: error)) } - }.recover(on: DispatchQueue.global()) { (error: Error) -> Promise in + }.recover { (error: Error) -> Promise in switch error { case RequestMakerError.websocketRequestError(let statusCode, _, _): if isUDRequest && (statusCode == 401 || statusCode == 403) { @@ -188,7 +188,7 @@ public class RequestMaker: NSObject { return RequestMakerResult(responseObject: networkManagerResult.responseObject, wasSentByUD: isUDRequest, wasSentByWebsocket: false) - }.recover(on: DispatchQueue.global()) { (error: Error) -> Promise in + }.recover { (error: Error) -> Promise in switch error { case NetworkManagerError.taskError(let task, _): let statusCode = task.statusCode()