From ae4999c3a78bfe7e90ac003c1fdd279a3b9f00dc Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Thu, 28 Jul 2022 15:36:56 +1000 Subject: [PATCH 1/2] Fixed a couple of crashes and a couple of other bugs Fixed a crash due to database re-entrancy Fixed an issue where interacting with a push notification wouldn't open the conversation in some cases Added code to prevent a user from being able to start a DM with a blinded id Updated some open group polling logs to be clearer --- Session.xcodeproj/project.pbxproj | 20 +------ Session/DMs/NewDMVC.swift | 54 +++++++++++-------- Session/Meta/AppDelegate.swift | 12 ++++- .../Jobs/Types/UpdateProfilePictureJob.swift | 9 +++- .../Pollers/OpenGroupPoller.swift | 10 +++- 5 files changed, 62 insertions(+), 43 deletions(-) diff --git a/Session.xcodeproj/project.pbxproj b/Session.xcodeproj/project.pbxproj index e6a50a7d4..985a383ac 100644 --- a/Session.xcodeproj/project.pbxproj +++ b/Session.xcodeproj/project.pbxproj @@ -3070,8 +3070,6 @@ children = ( C3C2A5B0255385C700C340D1 /* Meta */, FD17D79D27F40CAA00122BE0 /* Database */, - FD17D7DF27F67BC400122BE0 /* Models */, - FD17D7D027F5795300122BE0 /* Types */, FDC438AF27BB158500C60D73 /* Models */, C3C2A5CD255385F300C340D1 /* Utilities */, C3C2A5B9255385ED00C340D1 /* Configuration.swift */, @@ -3558,20 +3556,6 @@ path = Models; sourceTree = ""; }; - FD17D7D027F5795300122BE0 /* Types */ = { - isa = PBXGroup; - children = ( - ); - path = Types; - sourceTree = ""; - }; - FD17D7DF27F67BC400122BE0 /* Models */ = { - isa = PBXGroup; - children = ( - ); - path = Models; - sourceTree = ""; - }; FD17D7E827F6A1B800122BE0 /* LegacyDatabase */ = { isa = PBXGroup; children = ( @@ -6830,7 +6814,7 @@ CODE_SIGN_ENTITLEMENTS = Session/Meta/Signal.entitlements; CODE_SIGN_IDENTITY = "iPhone Developer"; "CODE_SIGN_IDENTITY[sdk=iphoneos*]" = "iPhone Developer"; - CURRENT_PROJECT_VERSION = 357; + CURRENT_PROJECT_VERSION = 360; DEVELOPMENT_TEAM = SUQ8J2PCT7; FRAMEWORK_SEARCH_PATHS = ( "$(inherited)", @@ -6902,7 +6886,7 @@ CODE_SIGN_ENTITLEMENTS = Session/Meta/Signal.entitlements; CODE_SIGN_IDENTITY = "iPhone Developer"; "CODE_SIGN_IDENTITY[sdk=iphoneos*]" = "iPhone Developer"; - CURRENT_PROJECT_VERSION = 357; + CURRENT_PROJECT_VERSION = 360; DEVELOPMENT_TEAM = SUQ8J2PCT7; FRAMEWORK_SEARCH_PATHS = ( "$(inherited)", diff --git a/Session/DMs/NewDMVC.swift b/Session/DMs/NewDMVC.swift index 8767c8263..b55fbb3df 100644 --- a/Session/DMs/NewDMVC.swift +++ b/Session/DMs/NewDMVC.swift @@ -134,30 +134,42 @@ final class NewDMVC : BaseVC, UIPageViewControllerDataSource, UIPageViewControll } fileprivate func startNewDMIfPossible(with onsNameOrPublicKey: String) { - if ECKeyPair.isValidHexEncodedPublicKey(candidate: onsNameOrPublicKey) { + let maybeSessionId: SessionId? = SessionId(from: onsNameOrPublicKey) + + if ECKeyPair.isValidHexEncodedPublicKey(candidate: onsNameOrPublicKey) && maybeSessionId?.prefix == .standard { startNewDM(with: onsNameOrPublicKey) - } else { - // This could be an ONS name - ModalActivityIndicatorViewController.present(fromViewController: navigationController!, canCancel: false) { [weak self] modalActivityIndicator in - SnodeAPI.getSessionID(for: onsNameOrPublicKey).done { sessionID in - modalActivityIndicator.dismiss { - self?.startNewDM(with: sessionID) - } - }.catch { error in - modalActivityIndicator.dismiss { - var messageOrNil: String? - if let error = error as? SnodeAPIError { - switch error { - case .decryptionFailed, .hashingFailed, .validationFailed: - messageOrNil = error.errorDescription - default: break - } + return + } + + // This could be an ONS name + ModalActivityIndicatorViewController.present(fromViewController: navigationController!, canCancel: false) { [weak self] modalActivityIndicator in + SnodeAPI.getSessionID(for: onsNameOrPublicKey).done { sessionID in + modalActivityIndicator.dismiss { + self?.startNewDM(with: sessionID) + } + }.catch { error in + modalActivityIndicator.dismiss { + var messageOrNil: String? + if let error = error as? SnodeAPIError { + switch error { + case .decryptionFailed, .hashingFailed, .validationFailed: + messageOrNil = error.errorDescription + default: break } - let message = messageOrNil ?? "Please check the Session ID or ONS name and try again" - let alert = UIAlertController(title: "Error", message: message, preferredStyle: .alert) - alert.addAction(UIAlertAction(title: NSLocalizedString("BUTTON_OK", comment: ""), style: .default, handler: nil)) - self?.presentAlert(alert) } + let message: String = { + if let messageOrNil: String = messageOrNil { + return messageOrNil + } + + return (maybeSessionId?.prefix == .blinded ? + "You can only send messages to Blinded IDs from within an Open Group" : + "Please check the Session ID or ONS name and try again" + ) + }() + let alert = UIAlertController(title: "Error", message: message, preferredStyle: .alert) + alert.addAction(UIAlertAction(title: "BUTTON_OK".localized(), style: .default, handler: nil)) + self?.presentAlert(alert) } } } diff --git a/Session/Meta/AppDelegate.swift b/Session/Meta/AppDelegate.swift index 84286342e..7067ea3db 100644 --- a/Session/Meta/AppDelegate.swift +++ b/Session/Meta/AppDelegate.swift @@ -114,6 +114,16 @@ class AppDelegate: UIResponder, UIApplicationDelegate, UNUserNotificationCenterD return true } + func applicationWillEnterForeground(_ application: UIApplication) { + /// **Note:** We _shouldn't_ need to call this here but for some reason the OS doesn't seems to + /// be calling the `userNotificationCenter(_:,didReceive:withCompletionHandler:)` + /// method when the device is locked while the app is in the foreground (or if the user returns to the + /// springboard without swapping to another app) - adding this here in addition to the one in + /// `appDidFinishLaunching` seems to fix this odd behaviour (even though it doesn't match + /// Apple's documentation on the matter) + UNUserNotificationCenter.current().delegate = self + } + func applicationDidEnterBackground(_ application: UIApplication) { DDLog.flushLog() @@ -149,7 +159,7 @@ class AppDelegate: UIResponder, UIApplicationDelegate, UNUserNotificationCenterD /// no longer always called before `applicationDidBecomeActive` we need to trigger the "clear notifications" logic /// within the `runNowOrWhenAppDidBecomeReady` callback and dispatch to the next run loop to ensure it runs after /// the notification has actually been handled - DispatchQueue.main.asyncAfter(deadline: .now() + .milliseconds(100)) { [weak self] in + DispatchQueue.main.async { [weak self] in self?.clearAllNotificationsAndRestoreBadgeCount() } } diff --git a/SessionMessagingKit/Jobs/Types/UpdateProfilePictureJob.swift b/SessionMessagingKit/Jobs/Types/UpdateProfilePictureJob.swift index 803ea34b9..260f150be 100644 --- a/SessionMessagingKit/Jobs/Types/UpdateProfilePictureJob.swift +++ b/SessionMessagingKit/Jobs/Types/UpdateProfilePictureJob.swift @@ -52,7 +52,14 @@ public enum UpdateProfilePictureJob: JobExecutor { image: nil, imageFilePath: profileFilePath, requiredSync: true, - success: { _, _ in success(job, false) }, + success: { _, _ in + // Need to call the 'success' closure asynchronously on the queue to prevent a reentrancy + // issue as it will write to the database and this closure is already called within + // another database write + queue.async { + success(job, false) + } + }, failure: { error in failure(job, error, false) } ) } diff --git a/SessionMessagingKit/Sending & Receiving/Pollers/OpenGroupPoller.swift b/SessionMessagingKit/Sending & Receiving/Pollers/OpenGroupPoller.swift index 72f5126b7..c46938844 100644 --- a/SessionMessagingKit/Sending & Receiving/Pollers/OpenGroupPoller.swift +++ b/SessionMessagingKit/Sending & Receiving/Pollers/OpenGroupPoller.swift @@ -194,7 +194,10 @@ extension OpenGroupAPI { case .roomPollInfo(let roomToken, _): guard let responseData: BatchSubResponse = endpointResponse.data as? BatchSubResponse, let responseBody: RoomPollInfo = responseData.body else { - SNLog("Open group polling failed due to invalid room info data.") + switch (endpointResponse.data as? BatchSubResponse)?.code { + case 404: SNLog("Open group polling failed to retrieve info for unknown room '\(roomToken)'.") + default: SNLog("Open group polling failed due to invalid room info data.") + } return } @@ -209,7 +212,10 @@ extension OpenGroupAPI { case .roomMessagesRecent(let roomToken), .roomMessagesBefore(let roomToken, _), .roomMessagesSince(let roomToken, _): guard let responseData: BatchSubResponse<[Failable]> = endpointResponse.data as? BatchSubResponse<[Failable]>, let responseBody: [Failable] = responseData.body else { - SNLog("Open group polling failed due to invalid messages data.") + switch (endpointResponse.data as? BatchSubResponse<[Failable]>)?.code { + case 404: SNLog("Open group polling failed to retrieve messages for unknown room '\(roomToken)'.") + default: SNLog("Open group polling failed due to invalid messages data.") + } return } let successfulMessages: [Message] = responseBody.compactMap { $0.value } From c022f7cda23afbf8d1fe480a05f56c5de55834cd Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Thu, 28 Jul 2022 18:26:22 +1000 Subject: [PATCH 2/2] Added an exponential back-off to polling open groups when they fail to poll --- .../_001_InitialSetupMigration.swift | 3 + .../Database/Models/OpenGroup.swift | 14 +++- .../Pollers/OpenGroupPoller.swift | 66 +++++++++++++++++-- SessionUtilitiesKit/Database/Storage.swift | 8 +++ 4 files changed, 82 insertions(+), 9 deletions(-) diff --git a/SessionMessagingKit/Database/Migrations/_001_InitialSetupMigration.swift b/SessionMessagingKit/Database/Migrations/_001_InitialSetupMigration.swift index 0e18775b6..61747d7ea 100644 --- a/SessionMessagingKit/Database/Migrations/_001_InitialSetupMigration.swift +++ b/SessionMessagingKit/Database/Migrations/_001_InitialSetupMigration.swift @@ -140,6 +140,9 @@ enum _001_InitialSetupMigration: Migration { t.column(.sequenceNumber, .integer).notNull() t.column(.inboxLatestMessageId, .integer).notNull() t.column(.outboxLatestMessageId, .integer).notNull() + t.column(.pollFailureCount, .integer) + .notNull() + .defaults(to: 0) } /// Create a full-text search table synchronized with the OpenGroup table diff --git a/SessionMessagingKit/Database/Models/OpenGroup.swift b/SessionMessagingKit/Database/Models/OpenGroup.swift index fec7569ba..71912d32d 100644 --- a/SessionMessagingKit/Database/Models/OpenGroup.swift +++ b/SessionMessagingKit/Database/Models/OpenGroup.swift @@ -26,6 +26,7 @@ public struct OpenGroup: Codable, Identifiable, FetchableRecord, PersistableReco case sequenceNumber case inboxLatestMessageId case outboxLatestMessageId + case pollFailureCount } public var id: String { threadId } // Identifiable @@ -86,6 +87,9 @@ public struct OpenGroup: Codable, Identifiable, FetchableRecord, PersistableReco /// updated whenever this value changes) public let outboxLatestMessageId: Int64 + /// The number of times this room has failed to poll since the last successful poll + public let pollFailureCount: Int64 + // MARK: - Relationships public var thread: QueryInterfaceRequest { @@ -117,7 +121,8 @@ public struct OpenGroup: Codable, Identifiable, FetchableRecord, PersistableReco infoUpdates: Int64, sequenceNumber: Int64 = 0, inboxLatestMessageId: Int64 = 0, - outboxLatestMessageId: Int64 = 0 + outboxLatestMessageId: Int64 = 0, + pollFailureCount: Int64 = 0 ) { self.threadId = OpenGroup.idFor(roomToken: roomToken, server: server) self.server = server.lowercased() @@ -133,6 +138,7 @@ public struct OpenGroup: Codable, Identifiable, FetchableRecord, PersistableReco self.sequenceNumber = sequenceNumber self.inboxLatestMessageId = inboxLatestMessageId self.outboxLatestMessageId = outboxLatestMessageId + self.pollFailureCount = pollFailureCount } } @@ -159,7 +165,8 @@ public extension OpenGroup { infoUpdates: 0, sequenceNumber: 0, inboxLatestMessageId: 0, - outboxLatestMessageId: 0 + outboxLatestMessageId: 0, + pollFailureCount: 0 ) } @@ -192,7 +199,8 @@ extension OpenGroup: CustomStringConvertible, CustomDebugStringConvertible { "infoUpdates: \(infoUpdates)", "sequenceNumber: \(sequenceNumber)", "inboxLatestMessageId: \(inboxLatestMessageId)", - "outboxLatestMessageId: \(outboxLatestMessageId))" + "outboxLatestMessageId: \(outboxLatestMessageId)", + "pollFailureCount: \(pollFailureCount))" ].joined(separator: ", ") } } diff --git a/SessionMessagingKit/Sending & Receiving/Pollers/OpenGroupPoller.swift b/SessionMessagingKit/Sending & Receiving/Pollers/OpenGroupPoller.swift index c46938844..52c2714fd 100644 --- a/SessionMessagingKit/Sending & Receiving/Pollers/OpenGroupPoller.swift +++ b/SessionMessagingKit/Sending & Receiving/Pollers/OpenGroupPoller.swift @@ -15,7 +15,8 @@ extension OpenGroupAPI { // MARK: - Settings - private static let pollInterval: TimeInterval = 4 + private static let minPollInterval: TimeInterval = 3 + private static let maxPollInterval: Double = (60 * 60) internal static let maxInactivityPeriod: Double = (14 * 24 * 60 * 60) // MARK: - Lifecycle @@ -28,10 +29,7 @@ extension OpenGroupAPI { guard !hasStarted else { return } hasStarted = true - timer = Timer.scheduledTimerOnMainThread(withTimeInterval: Poller.pollInterval, repeats: true) { _ in - self.poll(using: dependencies).retainUntilComplete() - } - poll(using: dependencies).retainUntilComplete() + pollRecursively(using: dependencies) } @objc public func stop() { @@ -41,6 +39,30 @@ extension OpenGroupAPI { // MARK: - Polling + private func pollRecursively(using dependencies: OpenGroupManager.OGMDependencies = OpenGroupManager.OGMDependencies()) { + guard hasStarted else { return } + + let minPollFailureCount: TimeInterval = Storage.shared + .read { db in + try OpenGroup + .filter(OpenGroup.Columns.server == server) + .select(min(OpenGroup.Columns.pollFailureCount)) + .asRequest(of: TimeInterval.self) + .fetchOne(db) + } + .defaulting(to: 0) + let nextPollInterval: TimeInterval = getInterval(for: minPollFailureCount, minInterval: Poller.minPollInterval, maxInterval: Poller.maxPollInterval) + + poll(using: dependencies).retainUntilComplete() + timer = Timer.scheduledTimerOnMainThread(withTimeInterval: nextPollInterval, repeats: false) { [weak self] timer in + timer.invalidate() + + Threading.pollerQueue.async { + self?.pollRecursively(using: dependencies) + } + } + } + @discardableResult public func poll(using dependencies: OpenGroupManager.OGMDependencies = OpenGroupManager.OGMDependencies()) -> Promise { return poll(isBackgroundPoll: false, isPostCapabilitiesRetry: false, using: dependencies) @@ -83,6 +105,14 @@ extension OpenGroupAPI { cache.timeSinceLastPoll[server] = Date().timeIntervalSince1970 UserDefaults.standard[.lastOpen] = Date() } + + // Reset the failure count + Storage.shared.writeAsync { db in + try OpenGroup + .filter(OpenGroup.Columns.server == server) + .updateAll(db, OpenGroup.Columns.pollFailureCount.set(to: 0)) + } + SNLog("Open group polling finished for \(server).") seal.fulfill(()) } @@ -97,7 +127,24 @@ extension OpenGroupAPI { ) .done(on: OpenGroupAPI.workQueue) { [weak self] didHandleError in if !didHandleError { - SNLog("Open group polling failed due to error: \(error).") + // Increase the failure count + let pollFailureCount: Int64 = Storage.shared + .read { db in + try OpenGroup + .filter(OpenGroup.Columns.server == server) + .select(max(OpenGroup.Columns.pollFailureCount)) + .asRequest(of: Int64.self) + .fetchOne(db) + } + .defaulting(to: 0) + + Storage.shared.writeAsync { db in + try OpenGroup + .filter(OpenGroup.Columns.server == server) + .updateAll(db, OpenGroup.Columns.pollFailureCount.set(to: (pollFailureCount + 1))) + } + + SNLog("Open group polling failed due to error: \(error). Setting failure count to \(pollFailureCount).") } self?.isPolling = false @@ -265,4 +312,11 @@ extension OpenGroupAPI { } } } + + // MARK: - Convenience + + fileprivate static func getInterval(for failureCount: TimeInterval, minInterval: TimeInterval, maxInterval: TimeInterval) -> TimeInterval { + // Arbitrary backoff factor... + return min(maxInterval, minInterval + pow(2, failureCount)) + } } diff --git a/SessionUtilitiesKit/Database/Storage.swift b/SessionUtilitiesKit/Database/Storage.swift index 3c1d3871f..92ba77ec0 100644 --- a/SessionUtilitiesKit/Database/Storage.swift +++ b/SessionUtilitiesKit/Database/Storage.swift @@ -200,6 +200,14 @@ public final class Storage { WHERE openGroup.infoUpdates = -1 """) // TODO: Remove this once everyone has updated + let openGroupTableInfo: [Row] = (try? Row.fetchAll(db, sql: "PRAGMA table_info(openGroup)")) + .defaulting(to: []) + if !openGroupTableInfo.contains(where: { $0["name"] == "pollFailureCount" }) { + try? db.execute(literal: """ + ALTER TABLE openGroup + ADD pollFailureCount INTEGER NOT NULL DEFAULT 0 + """) + } onComplete(finalError, needsConfigSync) }