From 2ec893d315f5c457cafc49c91f92deaf812e8ce0 Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Thu, 4 May 2017 13:49:44 -0400 Subject: [PATCH] Ensure we're retaining all promises to completion // FREEBIE --- Signal.xcodeproj/project.pbxproj | 8 +++++++- Signal/src/Models/SyncPushTokensJob.swift | 11 ++++------ Signal/src/call/CallService.swift | 19 +++++++++++------- Signal/src/call/NonCallKitCallUIAdaptee.swift | 4 ++-- .../Speakerbox/CallKitCallUIAdaptee.swift | 4 +++- .../util/Promise+retainUntilComplete.swift | 20 +++++++++++++++++++ 6 files changed, 48 insertions(+), 18 deletions(-) create mode 100644 Signal/src/util/Promise+retainUntilComplete.swift diff --git a/Signal.xcodeproj/project.pbxproj b/Signal.xcodeproj/project.pbxproj index 514ec6b99..da5e33dc6 100644 --- a/Signal.xcodeproj/project.pbxproj +++ b/Signal.xcodeproj/project.pbxproj @@ -100,6 +100,8 @@ 453D28B71D32BA5F00D523F0 /* OWSDisplayedMessage.m in Sources */ = {isa = PBXBuildFile; fileRef = 453D28B61D32BA5F00D523F0 /* OWSDisplayedMessage.m */; }; 453D28BA1D332DB100D523F0 /* OWSMessagesBubblesSizeCalculator.m in Sources */ = {isa = PBXBuildFile; fileRef = 453D28B91D332DB100D523F0 /* OWSMessagesBubblesSizeCalculator.m */; }; 453D28BB1D332DB100D523F0 /* OWSMessagesBubblesSizeCalculator.m in Sources */ = {isa = PBXBuildFile; fileRef = 453D28B91D332DB100D523F0 /* OWSMessagesBubblesSizeCalculator.m */; }; + 4542F0961EBB9E9A00C7EE92 /* Promise+retainUntilComplete.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4542F0951EBB9E9A00C7EE92 /* Promise+retainUntilComplete.swift */; }; + 4542F0971EBB9E9A00C7EE92 /* Promise+retainUntilComplete.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4542F0951EBB9E9A00C7EE92 /* Promise+retainUntilComplete.swift */; }; 45464DBC1DFA041F001D3FD6 /* DataChannelMessage.swift in Sources */ = {isa = PBXBuildFile; fileRef = 45464DBB1DFA041F001D3FD6 /* DataChannelMessage.swift */; }; 45666EC61D99483D008FE134 /* OWSAvatarBuilder.m in Sources */ = {isa = PBXBuildFile; fileRef = 45666EC51D99483D008FE134 /* OWSAvatarBuilder.m */; }; 45666EC91D994C0D008FE134 /* OWSGroupAvatarBuilder.m in Sources */ = {isa = PBXBuildFile; fileRef = 45666EC81D994C0D008FE134 /* OWSGroupAvatarBuilder.m */; }; @@ -437,8 +439,8 @@ 34B3F86E1E8DF1700035BE1A /* SignalsNavigationController.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = SignalsNavigationController.m; sourceTree = ""; }; 34B3F86F1E8DF1700035BE1A /* SignalsViewController.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = SignalsViewController.h; sourceTree = ""; }; 34B3F8701E8DF1700035BE1A /* SignalsViewController.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = SignalsViewController.m; sourceTree = ""; }; - 34B3F8981E8DF1B90035BE1A /* TSMessageAdapterTest.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = TSMessageAdapterTest.m; sourceTree = ""; }; 34B3F89A1E8DF3270035BE1A /* BlockListViewController.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = BlockListViewController.h; sourceTree = ""; }; + 34B3F8981E8DF1B90035BE1A /* TSMessageAdapterTest.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = TSMessageAdapterTest.m; sourceTree = ""; }; 34B3F89B1E8DF3270035BE1A /* BlockListViewController.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = BlockListViewController.m; sourceTree = ""; }; 34B3F89D1E8DF5490035BE1A /* OWSTableViewController.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = OWSTableViewController.h; sourceTree = ""; }; 34B3F89E1E8DF5490035BE1A /* OWSTableViewController.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = OWSTableViewController.m; sourceTree = ""; }; @@ -489,6 +491,7 @@ 453D28B61D32BA5F00D523F0 /* OWSDisplayedMessage.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = OWSDisplayedMessage.m; sourceTree = ""; }; 453D28B81D332DB100D523F0 /* OWSMessagesBubblesSizeCalculator.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = OWSMessagesBubblesSizeCalculator.h; sourceTree = ""; }; 453D28B91D332DB100D523F0 /* OWSMessagesBubblesSizeCalculator.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = OWSMessagesBubblesSizeCalculator.m; sourceTree = ""; }; + 4542F0951EBB9E9A00C7EE92 /* Promise+retainUntilComplete.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = "Promise+retainUntilComplete.swift"; sourceTree = ""; }; 45464DBB1DFA041F001D3FD6 /* DataChannelMessage.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = DataChannelMessage.swift; sourceTree = ""; }; 454B35071D08EED80026D658 /* mk */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = mk; path = translations/mk.lproj/Localizable.strings; sourceTree = ""; }; 45666EC41D99483D008FE134 /* OWSAvatarBuilder.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = OWSAvatarBuilder.h; sourceTree = ""; }; @@ -1246,6 +1249,7 @@ 76EB04FB18170B33006006FC /* Util.h */, 45F170D51E315310003FC1F2 /* Weak.swift */, 45F170CB1E310E22003FC1F2 /* WeakTimer.swift */, + 4542F0951EBB9E9A00C7EE92 /* Promise+retainUntilComplete.swift */, ); path = util; sourceTree = ""; @@ -1990,6 +1994,7 @@ 34B3F8791E8DF1700035BE1A /* CountryCodeViewController.m in Sources */, 4CE0E3771B954546007210CF /* TSAnimatedAdapter.m in Sources */, 4531C9C41DD8E6D800F08304 /* JSQMessagesCollectionViewCell+OWS.m in Sources */, + 4542F0961EBB9E9A00C7EE92 /* Promise+retainUntilComplete.swift in Sources */, 4516E3FF1DD2193B00DC4206 /* OWS101ExistingUsersBlockOnIdentityChange.m in Sources */, 4505C2BF1E648EA300CEBF41 /* ExperienceUpgrade.swift in Sources */, 45387B041E36D650005D00B3 /* OWS102MoveLoggingPreferenceToUserDefaults.m in Sources */, @@ -2169,6 +2174,7 @@ 45FBC5D21DF8592E00E9B410 /* SignalCall.swift in Sources */, 451A13B21E13DED2000A50FD /* CallNotificationsAdapter.swift in Sources */, 45C681B81D305A580050903A /* OWSCall.m in Sources */, + 4542F0971EBB9E9A00C7EE92 /* Promise+retainUntilComplete.swift in Sources */, 45855F381D9498A40084F340 /* OWSContactAvatarBuilder.m in Sources */, 45DF5DF31DDB843F00C936C7 /* CompareSafetyNumbersActivity.swift in Sources */, 456AC8341E3A775E00A3C7FC /* Weak.swift in Sources */, diff --git a/Signal/src/Models/SyncPushTokensJob.swift b/Signal/src/Models/SyncPushTokensJob.swift index b76049538..f5b11815b 100644 --- a/Signal/src/Models/SyncPushTokensJob.swift +++ b/Signal/src/Models/SyncPushTokensJob.swift @@ -12,8 +12,6 @@ class SyncPushTokensJob: NSObject { let accountManager: AccountManager let preferences: PropertyListPreferences var uploadOnlyIfStale = true - // useful to ensure promise runs to completion - var retainCycle: SyncPushTokensJob? required init(pushManager: PushManager, accountManager: AccountManager, preferences: PropertyListPreferences) { self.pushManager = pushManager @@ -32,14 +30,12 @@ class SyncPushTokensJob: NSObject { func run() -> Promise { Logger.debug("\(TAG) Starting.") - // Make sure we don't GC until completion. - self.retainCycle = self // Required to potentially prompt user for notifications settings // before `requestPushTokens` will return. self.pushManager.validateUserNotificationSettings() - return self.requestPushTokens().then { (pushToken: String, voipToken: String) in + let runPromise: Promise = self.requestPushTokens().then { (pushToken: String, voipToken: String) in var shouldUploadTokens = !self.uploadOnlyIfStale if self.preferences.getPushToken() != pushToken || self.preferences.getVoipToken() != voipToken { Logger.debug("\(self.TAG) push tokens changed.") @@ -56,9 +52,10 @@ class SyncPushTokensJob: NSObject { Logger.info("\(self.TAG) Recording tokens locally.") return self.recordNewPushTokens(pushToken:pushToken, voipToken:voipToken) } - }.always { - self.retainCycle = nil } + runPromise.retainUntilComplete() + + return runPromise } private func requestPushTokens() -> Promise<(pushToken: String, voipToken: String)> { diff --git a/Signal/src/call/CallService.swift b/Signal/src/call/CallService.swift index 783ed87ab..e78ad3904 100644 --- a/Signal/src/call/CallService.swift +++ b/Signal/src/call/CallService.swift @@ -125,7 +125,7 @@ protocol CallServiceObserver: class { didSet { AssertIsOnMainThread() - Logger.debug("\(self.TAG) .peerConnectionClient setter: \(oldValue != nil) -> \(peerConnectionClient != nil) \(peerConnectionClient)") + Logger.debug("\(self.TAG) .peerConnectionClient setter: \(oldValue != nil) -> \(peerConnectionClient != nil) \(String(describing: peerConnectionClient))") } } @@ -141,7 +141,7 @@ protocol CallServiceObserver: class { updateIsVideoEnabled() updateLockTimerEnabling() - Logger.debug("\(self.TAG) .call setter: \(oldValue != nil) -> \(call != nil) \(call)") + Logger.debug("\(self.TAG) .call setter: \(oldValue != nil) -> \(call != nil) \(String(describing: call))") for observer in observers { observer.value?.didUpdateCall(call:call) @@ -368,9 +368,10 @@ protocol CallServiceObserver: class { if pendingIceUpdateMessages.count > 0 { let callMessage = OWSOutgoingCallMessage(thread: thread, iceUpdateMessages: pendingIceUpdateMessages) - _ = messageSender.sendCallMessage(callMessage).catch { error in + let sendPromise = messageSender.sendCallMessage(callMessage).catch { error in Logger.error("\(self.TAG) failed to send ice updates in \(#function) with error: \(error)") } + sendPromise.retainUntilComplete() } guard let peerConnectionClient = self.peerConnectionClient else { @@ -379,7 +380,7 @@ protocol CallServiceObserver: class { } let sessionDescription = RTCSessionDescription(type: .answer, sdp: sessionDescription) - _ = peerConnectionClient.setRemoteSessionDescription(sessionDescription).then { + let setDescriptionPromise = peerConnectionClient.setRemoteSessionDescription(sessionDescription).then { Logger.debug("\(self.TAG) successfully set remote description") }.catch { error in if let callError = error as? CallError { @@ -389,6 +390,7 @@ protocol CallServiceObserver: class { self.handleFailedCall(failedCall: call, error: externalError) } } + setDescriptionPromise.retainUntilComplete() } /** @@ -424,7 +426,8 @@ protocol CallServiceObserver: class { let busyMessage = OWSCallBusyMessage(callId: call.signalingId) let callMessage = OWSOutgoingCallMessage(thread: thread, busyMessage: busyMessage) - _ = messageSender.sendCallMessage(callMessage) + let sendPromise = messageSender.sendCallMessage(callMessage) + sendPromise.retainUntilComplete() handleMissedCall(call, thread: thread) } @@ -622,7 +625,8 @@ protocol CallServiceObserver: class { if self.sendIceUpdatesImmediately { let callMessage = OWSOutgoingCallMessage(thread: thread, iceUpdateMessage: iceUpdateMessage) - _ = self.messageSender.sendCallMessage(callMessage) + let sendPromise = self.messageSender.sendCallMessage(callMessage) + sendPromise.retainUntilComplete() } else { // For outgoing messages, we wait to send ice updates until we're sure client received our call message. // e.g. if the client has blocked our message due to an identity change, we'd otherwise @@ -880,11 +884,12 @@ protocol CallServiceObserver: class { // If the call hasn't started yet, we don't have a data channel to communicate the hang up. Use Signal Service Message. let hangupMessage = OWSCallHangupMessage(callId: call.signalingId) let callMessage = OWSOutgoingCallMessage(thread: thread, hangupMessage: hangupMessage) - _ = self.messageSender.sendCallMessage(callMessage).then { + let sendPromise = self.messageSender.sendCallMessage(callMessage).then { Logger.debug("\(self.TAG) successfully sent hangup call message to \(thread)") }.catch { error in Logger.error("\(self.TAG) failed to send hangup call message to \(thread) with error: \(error)") } + sendPromise.retainUntilComplete() terminateCall() } diff --git a/Signal/src/call/NonCallKitCallUIAdaptee.swift b/Signal/src/call/NonCallKitCallUIAdaptee.swift index a987ffd5a..b2e1c501e 100644 --- a/Signal/src/call/NonCallKitCallUIAdaptee.swift +++ b/Signal/src/call/NonCallKitCallUIAdaptee.swift @@ -29,8 +29,8 @@ class NonCallKitCallUIAdaptee: CallUIAdaptee { self.callService.handleOutgoingCall(call).then { Logger.debug("\(self.TAG) handleOutgoingCall succeeded") - }.catch { error in - Logger.error("\(self.TAG) handleOutgoingCall failed with error: \(error)") + }.catch { error in + Logger.error("\(self.TAG) handleOutgoingCall failed with error: \(error)") } return call diff --git a/Signal/src/call/Speakerbox/CallKitCallUIAdaptee.swift b/Signal/src/call/Speakerbox/CallKitCallUIAdaptee.swift index 521787178..159ebbdbf 100644 --- a/Signal/src/call/Speakerbox/CallKitCallUIAdaptee.swift +++ b/Signal/src/call/Speakerbox/CallKitCallUIAdaptee.swift @@ -239,7 +239,9 @@ final class CallKitCallUIAdaptee: NSObject, CallUIAdaptee, CXProviderDelegate { // We can't wait for long before fulfilling the CXAction, else CallKit will show a "Failed Call". We don't // actually need to wait for the outcome of the handleOutgoingCall promise, because it handles any errors by // manually failing the call. - _ = self.callService.handleOutgoingCall(call) + let callPromise = self.callService.handleOutgoingCall(call) + callPromise.retainUntilComplete() + action.fulfill() self.provider.reportOutgoingCall(with: call.localId, startedConnectingAt: nil) diff --git a/Signal/src/util/Promise+retainUntilComplete.swift b/Signal/src/util/Promise+retainUntilComplete.swift new file mode 100644 index 000000000..d3a74a61a --- /dev/null +++ b/Signal/src/util/Promise+retainUntilComplete.swift @@ -0,0 +1,20 @@ +// +// Copyright (c) 2017 Open Whisper Systems. All rights reserved. +// + +import PromiseKit + +public extension Promise { + /** + * Sometimes there isn't a straight forward candidate to retain a promise, in that case we tell the + * promise to self retain, until it completes to avoid the risk it's GC'd before completion. + */ + func retainUntilComplete() { + // Unfortunately, there is (currently) no way to surpress the + // compiler warning: "Variable 'retainCycle' was written to, but never read" + var retainCycle: Promise? = self + self.always { + retainCycle = nil + } + } +}