Fixed a number of crashes currently affecting production

• Fixed a crash due to our ThreadSafe code using a struct instead of a class (rendering it non thread safe)
• Fixed a crash which could occur on the home screen if the data loaded before the UI finished loading
• (Hopefully) Fixed a crash which could occur when the OS optimised async execution to run immediately within an existing database transaction (potentially resulting in re-entrant database access)
• Fixed an issue where the database read/write publishers weren't checking for a valid database state before actual query execution (only during the creation of the stream)

Session.xcodeproj/project.pbxproj

@@ -7683,7 +7683,7 @@
 				CLANG_WARN__ARC_BRIDGE_CAST_NONARC = YES;
 				CLANG_WARN__DUPLICATE_METHOD_MATCH = YES;
 				CODE_SIGN_IDENTITY = "iPhone Developer";
-				CURRENT_PROJECT_VERSION = 519;
+				CURRENT_PROJECT_VERSION = 526;
 				ENABLE_BITCODE = NO;
 				ENABLE_STRICT_OBJC_MSGSEND = YES;
 				ENABLE_TESTABILITY = YES;
@@ -7720,7 +7720,7 @@
 				GCC_WARN_UNUSED_VARIABLE = YES;
 				HEADER_SEARCH_PATHS = "";
 				IPHONEOS_DEPLOYMENT_TARGET = 13.0;
-				MARKETING_VERSION = 2.8.5;
+				MARKETING_VERSION = 2.8.6;
 				ONLY_ACTIVE_ARCH = YES;
 				OTHER_CFLAGS = (
 					"-fobjc-arc-exceptions",
@@ -7762,7 +7762,7 @@
 				CLANG_WARN__ARC_BRIDGE_CAST_NONARC = YES;
 				CLANG_WARN__DUPLICATE_METHOD_MATCH = YES;
 				CODE_SIGN_IDENTITY = "iPhone Distribution";
-				CURRENT_PROJECT_VERSION = 519;
+				CURRENT_PROJECT_VERSION = 526;
 				ENABLE_BITCODE = NO;
 				ENABLE_STRICT_OBJC_MSGSEND = YES;
 				GCC_NO_COMMON_BLOCKS = YES;
@@ -7794,7 +7794,7 @@
 				GCC_WARN_UNUSED_VARIABLE = YES;
 				HEADER_SEARCH_PATHS = "";
 				IPHONEOS_DEPLOYMENT_TARGET = 13.0;
-				MARKETING_VERSION = 2.8.5;
+				MARKETING_VERSION = 2.8.6;
 				ONLY_ACTIVE_ARCH = NO;
 				OTHER_CFLAGS = (
 					"-DNS_BLOCK_ASSERTIONS=1",

Session/Conversations/ConversationVC.swift

@@ -656,7 +656,7 @@ final class ConversationVC: BaseVC, LibSessionRespondingViewController, Conversa
                     // PagedDatabaseObserver won't have them so we need to force a re-fetch of the current
                     // data to ensure everything is up to date
                     if didReturnFromBackground {
-                        DispatchQueue.global(qos: .background).async {
+                        DispatchQueue.global(qos: .background).asyncAfter(deadline: .now() + 0.01) {
                             self?.viewModel.pagedDataObserver?.reload()
                         }
                     }

Session/Conversations/ConversationViewModel.swift

@@ -198,7 +198,7 @@ public class ConversationViewModel: OWSAudioPlayerDelegate, NavigatableStateHold
         )
         
         // Run the initial query on a background thread so we don't block the push transition
-        DispatchQueue.global(qos: .userInitiated).async { [weak self] in
+        DispatchQueue.global(qos: .userInitiated).asyncAfter(deadline: .now() + 0.01) { [weak self] in
             // If we don't have a `initialFocusedInfo` then default to `.pageBefore` (it'll query
             // from a `0` offset)
             switch (focusedInteractionInfo ?? initialData?.initialUnreadInteractionInfo) {

Session/Home/HomeVC.swift

@@ -46,7 +46,7 @@ final class HomeVC: BaseVC, LibSessionRespondingViewController, UITableViewDataS
     
     // MARK: - UI
     
-    private var tableViewTopConstraint: NSLayoutConstraint!
+    private var tableViewTopConstraint: NSLayoutConstraint?
     
     private lazy var seedReminderView: SeedReminderView = {
         let result = SeedReminderView()
@@ -451,7 +451,7 @@ final class HomeVC: BaseVC, LibSessionRespondingViewController, UITableViewDataS
         
         // Update the 'view seed' UI
         if updatedState.showViewedSeedBanner != self.viewModel.state.showViewedSeedBanner {
-            tableViewTopConstraint.isActive = false
+            tableViewTopConstraint?.isActive = false
             seedReminderView.isHidden = !updatedState.showViewedSeedBanner
 
             if updatedState.showViewedSeedBanner {

Session/Meta/AppDelegate.swift

@@ -564,7 +564,7 @@ class AppDelegate: UIResponder, UIApplicationDelegate, UNUserNotificationCenterD
         /// There is a warning which can happen on launch because the Database read can be blocked by another database operation
         /// which could result in this blocking the main thread, as a result we want to check the identity exists on a background thread
         /// and then return to the main thread only when required
-        DispatchQueue.global(qos: .default).async { [weak self] in
+        DispatchQueue.global(qos: .default).asyncAfter(deadline: .now() + 0.01) { [weak self] in
             guard Identity.userExists() else { return }
             
             self?.enableBackgroundRefreshIfNecessary()
@@ -679,7 +679,7 @@ class AppDelegate: UIResponder, UIApplicationDelegate, UNUserNotificationCenterD
                     
                     /// We want to start observing the changes for the 'HomeVC' and want to wait until we actually get data back before we
                     /// continue as we don't want to show a blank home screen
-                    DispatchQueue.global(qos: .userInitiated).async {
+                    DispatchQueue.global(qos: .userInitiated).asyncAfter(deadline: .now() + 0.01) {
                         viewController.startObservingChanges() {
                             populateHomeScreenTimer.invalidate()
                             
@@ -719,7 +719,7 @@ class AppDelegate: UIResponder, UIApplicationDelegate, UNUserNotificationCenterD
             /// On application startup the `Storage.read` can be slightly slow while GRDB spins up it's database
             /// read pools (up to a few seconds), since this read is blocking we want to dispatch it to run async to ensure
             /// we don't block user interaction while it's running
-            DispatchQueue.global(qos: .default).async {
+            DispatchQueue.global(qos: .default).asyncAfter(deadline: .now() + 0.01) {
                 let unreadCount: Int = Storage.shared
                     .read { db in try Interaction.fetchUnreadCount(db) }
                     .defaulting(to: 0)
@@ -814,7 +814,10 @@ class AppDelegate: UIResponder, UIApplicationDelegate, UNUserNotificationCenterD
         
         /// Start the pollers on a background thread so that any database queries they need to run don't
         /// block the main thread
-        DispatchQueue.global(qos: .background).async { [weak self] in
+        ///
+        /// **Note:** We add a delay of `0.01` to prevent potential database re-entrancy if this is triggered
+        /// within the completion block of a database transaction, this gives it the time to complete the transaction
+        DispatchQueue.global(qos: .background).asyncAfter(deadline: .now() + 0.01) { [weak self] in
             self?.poller.start()
             
             guard shouldStartGroupPollers else { return }

Session/Meta/SessionApp.swift

@@ -80,8 +80,9 @@ public struct SessionApp {
             )
         }
         
-        /// The thread should generally exist at the time of calling this method, but on the off chance it doesn't then we need to `fetchOrCreate` it and
-        /// should do it on a background thread just in case something is keeping the DBWrite thread busy as in the past this could cause the app to hang
+        /// The thread should generally exist at the time of calling this method, but on the off chance it doesn't then we need to
+        /// `fetchOrCreate` it and should do it on a background thread just in case something is keeping the DBWrite thread
+        /// busy as in the past this could cause the app to hang
         guard threadInfo?.threadExists == true else {
             DispatchQueue.global(qos: .userInitiated).async {
                 Storage.shared.write { db in

SessionMessagingKit/Jobs/Types/GarbageCollectionJob.swift

@@ -344,8 +344,11 @@ public enum GarbageCollectionJob: JobExecutor {
                 }
             },
             completion: { _, _ in
-                // Dispatch async so we can swap from the write queue to a read one (we are done writing)
-                queue.async {
+                // Dispatch async so we can swap from the write queue to a read one (we are done
+                // writing), this has to be done after a slight delay to ensure the transaction
+                // provided by the completion block completes first (ie. so we don't hit
+                // re-entrancy issues)
+                queue.asyncAfter(deadline: .now() + 0.01) {
                     // Retrieve a list of all valid attachmnet and avatar file paths
                     struct FileInfo {
                         let attachmentLocalRelativePaths: Set<String>

SessionMessagingKit/Jobs/Types/UpdateProfilePictureJob.swift

@@ -50,10 +50,10 @@ public enum UpdateProfilePictureJob: JobExecutor {
             queue: queue,
             displayPictureUpdate: (profilePictureData.map { .currentUserUploadImageData($0) } ?? .none),
             success: { db 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 {
+                // Need to call the 'success' closure asynchronously on the queue after a slight
+                // delay to prevent a reentrancy issue as it will write to the database and this
+                // closure is already called within another database write
+                queue.asyncAfter(deadline: .now() + 0.01) {
                     SNLog("[UpdateProfilePictureJob] Profile successfully updated")
                     success(job, false, dependencies)
                 }

SessionMessagingKit/Sending & Receiving/MessageSender.swift

@@ -1054,9 +1054,10 @@ public final class MessageSender {
         
         guard !rowIds.isEmpty else { return error }
         
-        // Need to dispatch to a different thread to prevent a potential db re-entrancy
-        // issue from occuring in some cases
-        DispatchQueue.global(qos: .background).async {
+        // Note: We need to dispatch this after a small 0.01 delay to prevent any potential
+        // re-entrancy issues since the 'asyncMigrate' returns a result containing a DB instance
+        // within a transaction
+        DispatchQueue.global(qos: .background).asyncAfter(deadline: .now() + 0.01, using: dependencies) {
             dependencies.storage.write { db in
                 switch destination {
                     case .syncMessage:

SessionSnodeKit/LibSession/LibSession+Networking.swift

@@ -574,9 +574,12 @@ private extension LibSession {
                 return Log.error("[LibSession] CallbackWrapper called with null context.")
             }
             
-            // Dispatch async so we don't block libSession's internals with Swift logic (which can block other requests)
+            /// Dispatch async so we don't block libSession's internals with Swift logic (which can block other requests), we
+            /// add the `0.01` delay to ensure the closure isn't executed immediately
             let wrapper: CallbackWrapper<Output> = Unmanaged<CallbackWrapper<Output>>.fromOpaque(ctx).takeRetainedValue()
-            DispatchQueue.global(qos: .default).async { [wrapper] in wrapper.resultPublisher.send(output) }
+            DispatchQueue.global(qos: .default).asyncAfter(deadline: .now() + 0.01) { [wrapper] in
+                wrapper.resultPublisher.send(output)
+            }
         }
         
         public func unsafePointer() -> UnsafeMutableRawPointer { Unmanaged.passRetained(self).toOpaque() }

SessionUtilitiesKit/Database/Storage.swift

@@ -329,9 +329,10 @@ open class Storage {
                 }
             }()
             
-            // Note: We need to dispatch this to the next run toop to prevent any potential re-entrancy
-            // issues since the 'asyncMigrate' returns a result containing a DB instance
-            DispatchQueue.global(qos: .userInitiated).async {
+            // Note: We need to dispatch this after a small 0.01 delay to prevent any potential
+            // re-entrancy issues since the 'asyncMigrate' returns a result containing a DB instance
+            // within a transaction
+            DispatchQueue.global(qos: .userInitiated).asyncAfter(deadline: .now() + 0.01, using: dependencies) {
                 migrationCompleted(finalResult)
             }
         }
@@ -524,8 +525,8 @@ open class Storage {
         case valid(DatabaseWriter)
         case invalid(Error)
         
-        init(_ storage: Storage) {
-            switch (storage.isSuspended, storage.isValid, storage.dbWriter) {
+        init(_ storage: Storage?) {
+            switch (storage?.isSuspended, storage?.isValid, storage?.dbWriter) {
                 case (true, _, _): self = .invalid(StorageError.databaseSuspended)
                 case (false, true, .some(let dbWriter)): self = .valid(dbWriter)
                 default: self = .invalid(StorageError.databaseInvalid)
@@ -632,7 +633,7 @@ open class Storage {
     ) -> AnyPublisher<T, Error> {
         switch StorageState(self) {
             case .invalid(let error): return StorageState.logIfNeeded(error, isWrite: true)
-            case .valid(let dbWriter):
+            case .valid:
                 /// **Note:** GRDB does have a `writePublisher` method but it appears to asynchronously trigger
                 /// both the `output` and `complete` closures at the same time which causes a lot of unexpected
                 /// behaviours (this behaviour is apparently expected but still causes a number of odd behaviours in our code
@@ -642,11 +643,19 @@ open class Storage {
                 /// which behaves in a much more expected way than the GRDB `writePublisher` does
                 let info: CallInfo = CallInfo(fileName, functionName, lineNumber, true, self)
                 return Deferred {
-                    Future { resolver in
-                        do { resolver(Result.success(try dbWriter.write(Storage.perform(info: info, updates: updates)))) }
-                        catch {
-                            StorageState.logIfNeeded(error, isWrite: true)
-                            resolver(Result.failure(error))
+                    Future { [weak self] resolver in
+                        /// The `StorageState` may have changed between the creation of the publisher and it actually
+                        /// being executed so we need to check again
+                        switch StorageState(self) {
+                            case .invalid(let error): return StorageState.logIfNeeded(error, isWrite: true)
+                            case .valid(let dbWriter):
+                                do {
+                                    resolver(Result.success(try dbWriter.write(Storage.perform(info: info, updates: updates))))
+                                }
+                                catch {
+                                    StorageState.logIfNeeded(error, isWrite: true)
+                                    resolver(Result.failure(error))
+                                }
                         }
                     }
                 }.eraseToAnyPublisher()
@@ -678,7 +687,7 @@ open class Storage {
     ) -> AnyPublisher<T, Error> {
         switch StorageState(self) {
             case .invalid(let error): return StorageState.logIfNeeded(error, isWrite: false)
-            case .valid(let dbWriter):
+            case .valid:
                 /// **Note:** GRDB does have a `readPublisher` method but it appears to asynchronously trigger
                 /// both the `output` and `complete` closures at the same time which causes a lot of unexpected
                 /// behaviours (this behaviour is apparently expected but still causes a number of odd behaviours in our code
@@ -688,11 +697,19 @@ open class Storage {
                 /// which behaves in a much more expected way than the GRDB `readPublisher` does
                 let info: CallInfo = CallInfo(fileName, functionName, lineNumber, false, self)
                 return Deferred {
-                    Future { resolver in
-                        do { resolver(Result.success(try dbWriter.read(Storage.perform(info: info, updates: value)))) }
-                        catch {
-                            StorageState.logIfNeeded(error, isWrite: false)
-                            resolver(Result.failure(error))
+                    Future { [weak self] resolver in
+                        /// The `StorageState` may have changed between the creation of the publisher and it actually
+                        /// being executed so we need to check again
+                        switch StorageState(self) {
+                            case .invalid(let error): return StorageState.logIfNeeded(error, isWrite: false)
+                            case .valid(let dbWriter):
+                                do {
+                                    resolver(Result.success(try dbWriter.read(Storage.perform(info: info, updates: value))))
+                                }
+                                catch {
+                                    StorageState.logIfNeeded(error, isWrite: false)
+                                    resolver(Result.failure(error))
+                                }
                         }
                     }
                 }.eraseToAnyPublisher()

SessionUtilitiesKit/Database/Types/PagedDatabaseObserver.swift

@@ -158,19 +158,21 @@ public class PagedDatabaseObserver<ObservedTable, T>: TransactionObserver where
         _changesInCommit.performUpdate { $0.inserting(trackedChange) }
     }
     
-    /// We will process all updates which come through this method even if 'onChange' is null because if the UI stops observing and then starts
-    /// again later we don't want to have missed any changes which happened while the UI wasn't subscribed (and doing a full re-query seems painful...)
+    /// We will process all updates which come through this method even if 'onChange' is null because if the UI stops observing and
+    /// then starts again later we don't want to have missed any changes which happened while the UI wasn't subscribed (and doing
+    /// a full re-query seems painful...)
     ///
-    /// **Note:** This function is generally called within the DBWrite thread but we don't actually need write access to process the commit, in order
-    /// to avoid blocking the DBWrite thread we dispatch to a serial `commitProcessingQueue` to process the incoming changes (in the past not doing
-    /// so was resulting in hanging when there was a lot of activity happening)
+    /// **Note:** This function is generally called within the DBWrite thread but we don't actually need write access to process the
+    /// commit, in order to avoid blocking the DBWrite thread we dispatch to a serial `commitProcessingQueue` to process the
+    /// incoming changes (in the past not doing so was resulting in hanging when there was a lot of activity happening)
     public func databaseDidCommit(_ db: Database) {
         // If there were no pending changes in the commit then do nothing
         guard !self.changesInCommit.isEmpty else { return }
         
-        // Since we can't be sure the behaviours of 'databaseDidChange' and 'databaseDidCommit' won't change in
-        // the future we extract and clear the values in 'changesInCommit' since it's '@ThreadSafe' so will different
-        // threads modifying the data resulting in us missing a change
+        // Since we can't be sure the behaviours of 'databaseDidChange' and 'databaseDidCommit'
+        // won't change in the future we extract and clear the values in 'changesInCommit' since
+        // it's '@ThreadSafe' so will different threads modifying the data resulting in us
+        // missing a change
         var committedChanges: Set<PagedData.TrackedChange> = []
         
         self._changesInCommit.performUpdate { cachedChanges in
@@ -178,7 +180,12 @@ public class PagedDatabaseObserver<ObservedTable, T>: TransactionObserver where
             return []
         }
         
-        commitProcessingQueue.async { [weak self] in
+        // This looks odd but if we just use `commitProcessingQueue.async` then the code can
+        // get executed immediately wihch can result in a new transaction being started whilst
+        // we are still within the transaction wrapping `databaseDidCommit` (which we don't
+        // want), by adding this tiny 0.01 delay we should be giving it enough time to finish
+        // processing the current transaction
+        commitProcessingQueue.asyncAfter(deadline: .now() + 0.01) { [weak self] in
             self?.processDatabaseCommit(committedChanges: committedChanges)
         }
     }

SessionUtilitiesKit/JobRunner/JobRunner.swift

@@ -691,7 +691,9 @@ public final class JobRunner: JobRunnerType {
         
         // Don't add to the queue if the JobRunner isn't ready (it's been saved to the db so it'll be loaded
         // once the queue actually get started later)
-        guard canAddToQueue(updatedJob) || jobQueue?.isRunningInBackgroundTask == true else { return updatedJob }
+        guard canAddToQueue(updatedJob) || jobQueue?.isRunningInBackgroundTask == true else {
+            return updatedJob
+        }
         
         // The queue is ready or running in a background task so we can add the job
         jobQueue?.add(db, job: updatedJob, canStartJob: canStartJob, using: dependencies)
@@ -1292,10 +1294,14 @@ public final class JobQueue: Hashable {
         guard canStart?(self) == true || isRunningInBackgroundTask else { return }
         guard forceWhenAlreadyRunning || !isRunning || isRunningInBackgroundTask else { return }
         
-        // The JobRunner runs synchronously we need to ensure this doesn't start
-        // on the main thread (if it is on the main thread then swap to a different thread)
+        // The JobRunner runs synchronously so we need to ensure this doesn't start on the main
+        // thread and do so by creating a number of background queues to run the jobs on, if this
+        // function was called on the wrong queue then we need to dispatch to the correct one
         guard DispatchQueue.with(key: queueKey, matches: queueContext, using: dependencies) else {
-            internalQueue.async(using: dependencies) { [weak self] in
+            // Note: We need to dispatch this after a small 0.01 delay to prevent any potential
+            // re-entrancy issues since the `start` function can be called within an existing
+            // database transaction (eg. via `db.afterNextTransactionNestedOnce`)
+            internalQueue.asyncAfter(deadline: .now() + 0.01, using: dependencies) { [weak self] in
                 self?.start(forceWhenAlreadyRunning: forceWhenAlreadyRunning, using: dependencies)
             }
             return

SessionUtilitiesKit/Utilities/ThreadSafe.swift

@@ -18,7 +18,7 @@ import Foundation
 /// as `ThreadSafeType` (reference types or structs which have `mutating` functions **should not** use this mechanism
 /// as it cannot ensure thread safety for those types)
 @propertyWrapper
-public struct ThreadSafe<Value: ThreadSafeType> {
+public class ThreadSafe<Value: ThreadSafeType> {
     private var value: Value
     private let lock: ReadWriteLock = ReadWriteLock()
 
@@ -49,17 +49,17 @@ public struct ThreadSafe<Value: ThreadSafeType> {
     
     // MARK: - Functions
 
-    public mutating func performUpdateAndMap<T>(_ closure: (Value) -> (Value, T)) -> T {
+    public func performUpdateAndMap<T>(_ closure: (Value) -> (Value, T)) -> T {
         return try! performInternal { closure($0) }
     }
     
-    public mutating func performUpdateAndMap<T>(_ closure: (Value) throws -> (Value, T)) throws -> T {
+    public func performUpdateAndMap<T>(_ closure: (Value) throws -> (Value, T)) throws -> T {
         return try performInternal { try closure($0) }
     }
     
     // MARK: - Internal Functions
     
-    @discardableResult private mutating func performInternal<T>(_ mutation: (Value) throws -> (Value, T)) throws -> T {
+    @discardableResult private func performInternal<T>(_ mutation: (Value) throws -> (Value, T)) throws -> T {
         lock.writeLock()
         defer { lock.unlock() }