From ec81236615f84a7314f0e187fc93a4733c99e9a9 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Fri, 30 Jun 2023 17:59:37 +1000 Subject: [PATCH] Fixed a few more threading-related bugs Updated the libSession build script to reset and checkout submodules based on a flag (simplify the process) Updated the share and notification extensions to generate the DSYM on debug builds to allow for better debugging Added additional startup logging for debugging purposes Tweaked the SNLog function to indicate when the log happens on the main thread (slightly less efficient but should help track down logic incorrectly running on the main thread) Fixed a bug where we weren't recording the last PN server registration (so would re-register every time) Fixed a bug where if the user sent the app to the background too quickly after launching the app wouldn't successfully startup when re-opening Fixed a bug where the Notification and Share extensions would assert because they were dispatching the post-migration logic to the main thread (due to the threading changes in a previous release) Fixed a bug where the Notification extension would incorrectly poll open groups on the main thread --- Scripts/build_libSession_util.sh | 79 +++++++++++++++-- Session.xcodeproj/project.pbxproj | 16 ++-- .../xcshareddata/xcschemes/Session.xcscheme | 2 +- .../xcschemes/SessionMessagingKit.xcscheme | 2 +- ...ssionNotificationServiceExtension.xcscheme | 2 +- .../xcschemes/SessionShareExtension.xcscheme | 2 +- .../xcschemes/SessionUtilitiesKit.xcscheme | 2 +- .../xcschemes/SignalUtilitiesKit.xcscheme | 2 +- Session/Meta/AppDelegate.swift | 84 +++++++++++++++++-- Session/Meta/MainAppContext.h | 2 + Session/Notifications/SyncPushTokensJob.swift | 31 +++---- .../NotificationServiceExtension.swift | 24 +++--- .../ShareNavController.swift | 27 +++--- SessionUtilitiesKit/Database/Storage.swift | 10 ++- SessionUtilitiesKit/General/Logging.swift | 6 +- 15 files changed, 225 insertions(+), 66 deletions(-) diff --git a/Scripts/build_libSession_util.sh b/Scripts/build_libSession_util.sh index bb3cb671b..562c9871a 100755 --- a/Scripts/build_libSession_util.sh +++ b/Scripts/build_libSession_util.sh @@ -21,9 +21,13 @@ # path = "{FRAMEWORK NAME GOES HERE}"; # sourceTree = BUILD_DIR; # }; +# +# Note: We might one day be able to replace this with a local podspec if this GitHub feature +# request ever gets implemented: https://github.com/CocoaPods/CocoaPods/issues/8464 # Need to set the path or we won't find cmake PATH=${PATH}:/usr/local/bin:/opt/homebrew/bin:/sbin/md5 +SHOULD_AUTO_INIT_SUBMODULES=${1:-false} # Ensure the build directory exists (in case we need it before XCode creates it) mkdir -p "${TARGET_BUILD_DIR}" @@ -41,11 +45,76 @@ if ! which cmake > /dev/null; then exit 0 fi -if [ ! -d "${SRCROOT}/LibSession-Util" ] || [ ! -d "${SRCROOT}/LibSession-Util/src" ]; then - touch "${TARGET_BUILD_DIR}/libsession_util_error.log" - echo "error: Need to fetch LibSession-Util submodule." - echo "error: Need to fetch LibSession-Util submodule." > "${TARGET_BUILD_DIR}/libsession_util_error.log" - exit 1 +# Check if we have the `LibSession-Util` submodule checked out and if not (depending on the 'SHOULD_AUTO_INIT_SUBMODULES' argument) perform the checkout +if [ ! -d "${SRCROOT}/LibSession-Util" ] || [ ! -d "${SRCROOT}/LibSession-Util/src" ] || [ ! "$(ls -A "${SRCROOT}/LibSession-Util")" ]; then + if [ "${SHOULD_AUTO_INIT_SUBMODULES}" != "false" ] & command -v git >/dev/null 2>&1; then + echo "info: LibSession-Util submodule doesn't exist, resetting and checking out recusively now" + git submodule foreach --recursive git reset --hard + git submodule update --init --recursive + echo "info: Checkout complete" + else + touch "${TARGET_BUILD_DIR}/libsession_util_error.log" + echo "error: Need to fetch LibSession-Util submodule (git submodule update --init --recursive)." + echo "error: Need to fetch LibSession-Util submodule (git submodule update --init --recursive)." > "${TARGET_BUILD_DIR}/libsession_util_error.log" + exit 1 + fi +else + are_submodules_valid() { + local PARENT_PATH=$1 + + # Change into the path to check for it's submodules + cd "${PARENT_PATH}" + local SUB_MODULE_PATHS=($(git config --file .gitmodules --get-regexp path | awk '{ print $2 }')) + + # If there are no submodules then return success based on whether the folder has any content + if [ ${#SUB_MODULE_PATHS[@]} -eq 0 ]; then + if [ "$(ls -A "${SRCROOT}/LibSession-Util")" ]; then + return 1 + else + return 0 + fi + fi + + # Loop through the child submodules and check if they are valid + for i in "${!SUB_MODULE_PATHS[@]}"; do + local CHILD_PATH="${SUB_MODULE_PATHS[$i]}" + + # If the child path doesn't exist then it's invalid + if [ ! -d "${CHILD_PATH}" ]; then + return 1 + fi + + are_submodules_valid "${PARENT_PATH}/${CHILD_PATH}" + local RESULT=$? + + if [ "${RESULT}" -eq 1 ]; then + echo "info: Submodule ${CHILD_PATH} is in an invalid state." + return 1 + fi + done + + return 0 + } + + # Validate the state of the submodules + are_submodules_valid "${SRCROOT}/LibSession-Util" + + HAS_INVALID_SUBMODULE=$? + + if [ "${HAS_INVALID_SUBMODULE}" -eq 1 ]; then + if [ "${SHOULD_AUTO_INIT_SUBMODULES}" != "false" ] && command -v git >/dev/null 2>&1; then + echo "info: Submodules are in an invalid state, resetting and checking out recusively now" + cd "${SRCROOT}/LibSession-Util" + git submodule foreach --recursive git reset --hard + git submodule update --init --recursive + echo "info: Checkout complete" + else + touch "${TARGET_BUILD_DIR}/libsession_util_error.log" + echo "error: Submodules are in an invalid state, please delete 'LibSession-Util' and run 'git submodule update --init --recursive'." + echo "error: Submodules are in an invalid state, please delete 'LibSession-Util' and run 'git submodule update --init --recursive'." > "${TARGET_BUILD_DIR}/libsession_util_error.log" + exit 1 + fi + fi fi # Generate a hash of the libSession-util source files and check if they differ from the last hash diff --git a/Session.xcodeproj/project.pbxproj b/Session.xcodeproj/project.pbxproj index 458f0c863..771ff32af 100644 --- a/Session.xcodeproj/project.pbxproj +++ b/Session.xcodeproj/project.pbxproj @@ -6377,8 +6377,8 @@ "CODE_SIGN_IDENTITY[sdk=iphoneos*]" = "iPhone Developer"; CODE_SIGN_STYLE = Automatic; COPY_PHASE_STRIP = NO; - CURRENT_PROJECT_VERSION = 412; - DEBUG_INFORMATION_FORMAT = dwarf; + CURRENT_PROJECT_VERSION = 413; + DEBUG_INFORMATION_FORMAT = "dwarf-with-dsym"; DEVELOPMENT_TEAM = SUQ8J2PCT7; FRAMEWORK_SEARCH_PATHS = "$(inherited)"; GCC_C_LANGUAGE_STANDARD = gnu11; @@ -6449,7 +6449,7 @@ "CODE_SIGN_IDENTITY[sdk=iphoneos*]" = "iPhone Developer"; CODE_SIGN_STYLE = Automatic; COPY_PHASE_STRIP = NO; - CURRENT_PROJECT_VERSION = 412; + CURRENT_PROJECT_VERSION = 413; DEBUG_INFORMATION_FORMAT = "dwarf-with-dsym"; DEVELOPMENT_TEAM = SUQ8J2PCT7; ENABLE_NS_ASSERTIONS = NO; @@ -6514,8 +6514,8 @@ "CODE_SIGN_IDENTITY[sdk=iphoneos*]" = "iPhone Developer"; CODE_SIGN_STYLE = Automatic; COPY_PHASE_STRIP = NO; - CURRENT_PROJECT_VERSION = 412; - DEBUG_INFORMATION_FORMAT = dwarf; + CURRENT_PROJECT_VERSION = 413; + DEBUG_INFORMATION_FORMAT = "dwarf-with-dsym"; DEVELOPMENT_TEAM = SUQ8J2PCT7; FRAMEWORK_SEARCH_PATHS = "$(inherited)"; GCC_C_LANGUAGE_STANDARD = gnu11; @@ -6588,7 +6588,7 @@ "CODE_SIGN_IDENTITY[sdk=iphoneos*]" = "iPhone Developer"; CODE_SIGN_STYLE = Automatic; COPY_PHASE_STRIP = NO; - CURRENT_PROJECT_VERSION = 412; + CURRENT_PROJECT_VERSION = 413; DEBUG_INFORMATION_FORMAT = "dwarf-with-dsym"; DEVELOPMENT_TEAM = SUQ8J2PCT7; ENABLE_NS_ASSERTIONS = NO; @@ -7496,7 +7496,7 @@ CODE_SIGN_ENTITLEMENTS = Session/Meta/Signal.entitlements; CODE_SIGN_IDENTITY = "iPhone Developer"; "CODE_SIGN_IDENTITY[sdk=iphoneos*]" = "iPhone Developer"; - CURRENT_PROJECT_VERSION = 412; + CURRENT_PROJECT_VERSION = 413; DEVELOPMENT_TEAM = SUQ8J2PCT7; FRAMEWORK_SEARCH_PATHS = ( "$(inherited)", @@ -7567,7 +7567,7 @@ CODE_SIGN_ENTITLEMENTS = Session/Meta/Signal.entitlements; CODE_SIGN_IDENTITY = "iPhone Developer"; "CODE_SIGN_IDENTITY[sdk=iphoneos*]" = "iPhone Developer"; - CURRENT_PROJECT_VERSION = 412; + CURRENT_PROJECT_VERSION = 413; DEVELOPMENT_TEAM = SUQ8J2PCT7; FRAMEWORK_SEARCH_PATHS = ( "$(inherited)", diff --git a/Session.xcodeproj/xcshareddata/xcschemes/Session.xcscheme b/Session.xcodeproj/xcshareddata/xcschemes/Session.xcscheme index f2e5c8744..58585f0bb 100644 --- a/Session.xcodeproj/xcshareddata/xcschemes/Session.xcscheme +++ b/Session.xcodeproj/xcshareddata/xcschemes/Session.xcscheme @@ -10,7 +10,7 @@ ActionType = "Xcode.IDEStandardExecutionActionsCore.ExecutionActionType.ShellScriptAction"> + scriptText = ""${SRCROOT}/Scripts/build_libSession_util.sh" true "> + scriptText = ""${SRCROOT}/Scripts/build_libSession_util.sh" true "> + scriptText = ""${SRCROOT}/Scripts/build_libSession_util.sh" true "> + scriptText = ""${SRCROOT}/Scripts/build_libSession_util.sh" true "> + scriptText = ""${SRCROOT}/Scripts/build_libSession_util.sh" true "> + scriptText = ""${SRCROOT}/Scripts/build_libSession_util.sh" true "> Bool { + switch (lhs, rhs) { + case (.finishLaunching, .finishLaunching): return true + case (.enterForeground(let lhsFailed), .enterForeground(let rhsFailed)): return (lhsFailed == rhsFailed) + case (.didBecomeActive, .didBecomeActive): return true + default: return false + } + } } // MARK: - StartupError @@ -867,6 +928,15 @@ private enum StartupError: Error { case failedToRestore case startupTimeout + var name: String { + switch self { + case .databaseError(StorageError.startupFailed): return "Database startup failed" + case .failedToRestore: return "Failed to restore" + case .databaseError: return "Database error" + case .startupTimeout: return "Startup timeout" + } + } + var message: String { switch self { case .databaseError(StorageError.startupFailed): return "DATABASE_STARTUP_FAILED".localized() diff --git a/Session/Meta/MainAppContext.h b/Session/Meta/MainAppContext.h index a5c0ac97e..6fab6a1ad 100644 --- a/Session/Meta/MainAppContext.h +++ b/Session/Meta/MainAppContext.h @@ -10,6 +10,8 @@ extern NSString *const ReportedApplicationStateDidChangeNotification; @interface MainAppContext : NSObject +- (instancetype)init; + @end NS_ASSUME_NONNULL_END diff --git a/Session/Notifications/SyncPushTokensJob.swift b/Session/Notifications/SyncPushTokensJob.swift index 36b32e56a..2a93618fb 100644 --- a/Session/Notifications/SyncPushTokensJob.swift +++ b/Session/Notifications/SyncPushTokensJob.swift @@ -22,23 +22,25 @@ public enum SyncPushTokensJob: JobExecutor { deferred: @escaping (Job) -> () ) { // Don't run when inactive or not in main app or if the user doesn't exist yet - guard - (UserDefaults.sharedLokiProject?[.isMainAppActive]).defaulting(to: false), - Identity.userCompletedRequiredOnboarding() - else { + guard (UserDefaults.sharedLokiProject?[.isMainAppActive]).defaulting(to: false) else { + return deferred(job) // Don't need to do anything if it's not the main app + } + guard Identity.userCompletedRequiredOnboarding() else { SNLog("[SyncPushTokensJob] Deferred due to incomplete registration") - deferred(job) // Don't need to do anything if it's not the main app - return + return deferred(job) } - // We need to check a UIApplication setting which needs to run on the main thread so if we aren't on - // the main thread then swap to it - guard Thread.isMainThread else { - DispatchQueue.main.async { - run(job, queue: queue, success: success, failure: failure, deferred: deferred) + // We need to check a UIApplication setting which needs to run on the main thread so synchronously + // retrieve the value so we can continue + let isRegisteredForRemoteNotifications: Bool = { + guard !Thread.isMainThread else { + return UIApplication.shared.isRegisteredForRemoteNotifications } - return - } + + return DispatchQueue.main.sync { + return UIApplication.shared.isRegisteredForRemoteNotifications + } + }() // Push tokens don't normally change while the app is launched, so you would assume checking once // during launch is sufficient, but e.g. on iOS11, users who have disabled "Allow Notifications" @@ -60,7 +62,7 @@ public enum SyncPushTokensJob: JobExecutor { guard job.behaviour == .runOnce || - !UIApplication.shared.isRegisteredForRemoteNotifications || + !isRegisteredForRemoteNotifications || Date().timeIntervalSince(lastPushNotificationSync) >= SyncPushTokensJob.maxFrequency else { SNLog("[SyncPushTokensJob] Deferred due to Fast Mode disabled or recent-enough registration") @@ -94,6 +96,7 @@ public enum SyncPushTokensJob: JobExecutor { case .finished: Logger.warn("Recording push tokens locally. pushToken: \(redact(pushToken)), voipToken: \(redact(voipToken))") SNLog("[SyncPushTokensJob] Completed") + UserDefaults.standard[.lastPushNotificationSync] = Date() Storage.shared.write { db in db[.lastRecordedPushToken] = pushToken diff --git a/SessionNotificationServiceExtension/NotificationServiceExtension.swift b/SessionNotificationServiceExtension/NotificationServiceExtension.swift index 59b31366b..0526360a4 100644 --- a/SessionNotificationServiceExtension/NotificationServiceExtension.swift +++ b/SessionNotificationServiceExtension/NotificationServiceExtension.swift @@ -12,7 +12,6 @@ import SignalCoreKit public final class NotificationServiceExtension: UNNotificationServiceExtension { private var didPerformSetup = false - private var areVersionMigrationsComplete = false private var contentHandler: ((UNNotificationContent) -> Void)? private var request: UNNotificationRequest? @@ -50,6 +49,8 @@ public final class NotificationServiceExtension: UNNotificationServiceExtension defer { Publishers .MergeMany(openGroupPollingPublishers) + .subscribe(on: DispatchQueue.global(qos: .background)) + .subscribe(on: DispatchQueue.main) .sinkUntilComplete( receiveCompletion: { _ in self.completeSilenty() @@ -234,19 +235,23 @@ public final class NotificationServiceExtension: UNNotificationServiceExtension $0 = NSENotificationPresenter() } }, - migrationsCompletion: { [weak self] _, needsConfigSync in - self?.versionMigrationsDidComplete(needsConfigSync: needsConfigSync) + migrationsCompletion: { [weak self] result, needsConfigSync in + switch result { + case .failure: SNLog("[NotificationServiceExtension] Failed to complete migrations") + case .success: + DispatchQueue.main.async { + self?.versionMigrationsDidComplete(needsConfigSync: needsConfigSync) + } + } + completion() } ) } - @objc private func versionMigrationsDidComplete(needsConfigSync: Bool) { AssertIsOnMainThread() - areVersionMigrationsComplete = true - // If we need a config sync then trigger it now if needsConfigSync { Storage.shared.write { db in @@ -254,18 +259,17 @@ public final class NotificationServiceExtension: UNNotificationServiceExtension } } - checkIsAppReady() + checkIsAppReady(migrationsCompleted: true) } - @objc - private func checkIsAppReady() { + private func checkIsAppReady(migrationsCompleted: Bool) { AssertIsOnMainThread() // Only mark the app as ready once. guard !AppReadiness.isAppReady() else { return } // App isn't ready until storage is ready AND all version migrations are complete. - guard Storage.shared.isValid && areVersionMigrationsComplete else { return } + guard Storage.shared.isValid && migrationsCompleted else { return } SignalUtilitiesKit.Configuration.performMainSetup() diff --git a/SessionShareExtension/ShareNavController.swift b/SessionShareExtension/ShareNavController.swift index d8a1cfde4..496674eae 100644 --- a/SessionShareExtension/ShareNavController.swift +++ b/SessionShareExtension/ShareNavController.swift @@ -8,7 +8,6 @@ import SessionUIKit import SignalCoreKit final class ShareNavController: UINavigationController, ShareViewDelegate { - private var areVersionMigrationsComplete = false public static var attachmentPrepPublisher: AnyPublisher<[SignalAttachment], Error>? // MARK: - Error @@ -58,10 +57,16 @@ final class ShareNavController: UINavigationController, ShareViewDelegate { $0 = NoopNotificationsManager() } }, - migrationsCompletion: { [weak self] _, needsConfigSync in - // performUpdateCheck must be invoked after Environment has been initialized because - // upgrade process may depend on Environment. - self?.versionMigrationsDidComplete(needsConfigSync: needsConfigSync) + migrationsCompletion: { [weak self] result, needsConfigSync in + switch result { + case .failure: SNLog("[SessionShareExtension] Failed to complete migrations") + case .success: + DispatchQueue.main.async { + // performUpdateCheck must be invoked after Environment has been initialized because + // upgrade process may depend on Environment. + self?.versionMigrationsDidComplete(needsConfigSync: needsConfigSync) + } + } } ) @@ -82,14 +87,11 @@ final class ShareNavController: UINavigationController, ShareViewDelegate { ThemeManager.traitCollectionDidChange(previousTraitCollection) } - @objc func versionMigrationsDidComplete(needsConfigSync: Bool) { AssertIsOnMainThread() Logger.debug("") - areVersionMigrationsComplete = true - // If we need a config sync then trigger it now if needsConfigSync { Storage.shared.write { db in @@ -97,15 +99,14 @@ final class ShareNavController: UINavigationController, ShareViewDelegate { } } - checkIsAppReady() + checkIsAppReady(migrationsCompleted: true) } - @objc - func checkIsAppReady() { + func checkIsAppReady(migrationsCompleted: Bool) { AssertIsOnMainThread() // App isn't ready until storage is ready AND all version migrations are complete. - guard areVersionMigrationsComplete else { return } + guard migrationsCompleted else { return } guard Storage.shared.isValid else { return } guard !AppReadiness.isAppReady() else { // Only mark the app as ready once. @@ -195,6 +196,8 @@ final class ShareNavController: UINavigationController, ShareViewDelegate { message: "vc_share_loading_message".localized() ) { activityIndicator in publisher + .subscribe(on: DispatchQueue.global(qos: .userInitiated)) + .receive(on: DispatchQueue.main) .sinkUntilComplete( receiveCompletion: { _ in activityIndicator.dismiss { } } ) diff --git a/SessionUtilitiesKit/Database/Storage.swift b/SessionUtilitiesKit/Database/Storage.swift index 1feb26663..4d8f9fe39 100644 --- a/SessionUtilitiesKit/Database/Storage.swift +++ b/SessionUtilitiesKit/Database/Storage.swift @@ -214,8 +214,14 @@ open class Storage { self?.migrationProgressUpdater = nil SUKLegacy.clearLegacyDatabaseInstance() - if case .failure(let error) = result { - SNLog("[Migration Error] Migration failed with error: \(error)") + // Don't log anything in the case of a 'success' or if the database is suspended (the + // latter will happen if the user happens to return to the background too quickly on + // launch so is unnecessarily alarming, it also gets caught and logged separately by + // the 'write' functions anyway) + switch result { + case .success: break + case .failure(DatabaseError.SQLITE_ABORT): break + case .failure(let error): SNLog("[Migration Error] Migration failed with error: \(error)") } onComplete(result, needsConfigSync) diff --git a/SessionUtilitiesKit/General/Logging.swift b/SessionUtilitiesKit/General/Logging.swift index 7bd18d8f7..b3f64899e 100644 --- a/SessionUtilitiesKit/General/Logging.swift +++ b/SessionUtilitiesKit/General/Logging.swift @@ -4,10 +4,12 @@ import Foundation import SignalCoreKit public func SNLog(_ message: String) { + let threadString: String = (Thread.isMainThread ? " Main" : "") + #if DEBUG - print("[Session] \(message)") + print("[Session\(threadString)] \(message)") #endif - OWSLogger.info("[Session] \(message)") + OWSLogger.info("[Session\(threadString)] \(message)") } public func SNLogNotTests(_ message: String) {