From 1b350cf422103125c57d37b3d8c5d61e7b926744 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Mon, 24 Oct 2022 14:52:28 +1100 Subject: [PATCH] Fixed a number of crashes Consolidated the PagedDatabaseObserver updating logic into a static function (can be improved further in the future) Added defensive coding to prevent the LinkDeviceVC from crashing when the nav controller doesn't exist Fixed an issue where the 'Permissions' callbacks were doing UI logic on background threads Fixed an issue where the 'reloadCurrent' load type for the PagedDatabaseObserver was incorrectly increasing the 'currentCount' of the PageInfo Fixed an issue where loading all of the data for paged data could result in a crash when the 'loadMore' section was removed --- Podfile | 1 + Podfile.lock | 14 ++-- .../ConversationVC+Interaction.swift | 4 +- Session/Conversations/ConversationVC.swift | 1 + .../Conversations/ConversationViewModel.swift | 32 ++------ Session/Home/HomeViewModel.swift | 63 ++++------------ .../MessageRequestsViewModel.swift | 32 ++------ Session/Home/New Conversation/NewDMVC.swift | 6 +- .../MediaGalleryViewModel.swift | 32 ++------ .../SendMediaNavigationController.swift | 8 +- Session/Onboarding/LinkDeviceVC.swift | 18 +++-- Session/Open Groups/JoinOpenGroupVC.swift | 6 +- .../Settings/BlockedContactsViewModel.swift | 32 ++------ Session/Settings/QRCodeVC.swift | 6 +- Session/Settings/SettingsViewModel.swift | 14 ++-- .../Types/PagedDatabaseObserver.swift | 74 ++++++++++++++++++- 16 files changed, 167 insertions(+), 176 deletions(-) diff --git a/Podfile b/Podfile index 3f04ac5c2..133a7c87a 100644 --- a/Podfile +++ b/Podfile @@ -82,6 +82,7 @@ abstract_target 'GlobalDependencies' do target 'SessionUtilitiesKit' do pod 'SAMKeychain' pod 'YYImage/libwebp', git: 'https://github.com/signalapp/YYImage' + pod 'DifferenceKit' target 'SessionUtilitiesKitTests' do inherit! :complete diff --git a/Podfile.lock b/Podfile.lock index 51312bec3..c47cf02c1 100644 --- a/Podfile.lock +++ b/Podfile.lock @@ -21,11 +21,11 @@ PODS: - Curve25519Kit (2.1.0): - CocoaLumberjack - SignalCoreKit - - DifferenceKit (1.2.0): - - DifferenceKit/Core (= 1.2.0) - - DifferenceKit/UIKitExtension (= 1.2.0) - - DifferenceKit/Core (1.2.0) - - DifferenceKit/UIKitExtension (1.2.0): + - DifferenceKit (1.3.0): + - DifferenceKit/Core (= 1.3.0) + - DifferenceKit/UIKitExtension (= 1.3.0) + - DifferenceKit/Core (1.3.0) + - DifferenceKit/UIKitExtension (1.3.0): - DifferenceKit/Core - GRDB.swift/SQLCipher (6.1.0): - SQLCipher (>= 3.4.2) @@ -221,7 +221,7 @@ SPEC CHECKSUMS: CocoaLumberjack: 543c79c114dadc3b1aba95641d8738b06b05b646 CryptoSwift: a532e74ed010f8c95f611d00b8bbae42e9fe7c17 Curve25519Kit: e63f9859ede02438ae3defc5e1a87e09d1ec7ee6 - DifferenceKit: 5659c430bb7fe45876fa32ce5cba5d6167f0c805 + DifferenceKit: ab185c4d7f9cef8af3fcf593e5b387fb81e999ca GRDB.swift: 611778a5e113385373baeb3e2ce474887d1aadb7 libwebp: 98a37e597e40bfdb4c911fc98f2c53d0b12d05fc Nimble: 5316ef81a170ce87baf72dd961f22f89a602ff84 @@ -242,6 +242,6 @@ SPEC CHECKSUMS: YYImage: f1ddd15ac032a58b78bbed1e012b50302d318331 ZXingObjC: fdbb269f25dd2032da343e06f10224d62f537bdb -PODFILE CHECKSUM: 402850f74d70b3b57fc81eff82d0fc86d695b392 +PODFILE CHECKSUM: 7452ce88370eadd58d21fdf6a4c4945d6554ee95 COCOAPODS: 1.11.3 diff --git a/Session/Conversations/ConversationVC+Interaction.swift b/Session/Conversations/ConversationVC+Interaction.swift index a0fbbfcb8..bb70bea7f 100644 --- a/Session/Conversations/ConversationVC+Interaction.swift +++ b/Session/Conversations/ConversationVC+Interaction.swift @@ -2019,7 +2019,9 @@ extension ConversationVC: func startVoiceMessageRecording() { // Request permission if needed Permissions.requestMicrophonePermissionIfNeeded() { [weak self] in - self?.cancelVoiceMessageRecording() + DispatchQueue.main.async { + self?.cancelVoiceMessageRecording() + } } // Keep screen on diff --git a/Session/Conversations/ConversationVC.swift b/Session/Conversations/ConversationVC.swift index 620a4c4ea..8bb622e84 100644 --- a/Session/Conversations/ConversationVC.swift +++ b/Session/Conversations/ConversationVC.swift @@ -1,6 +1,7 @@ // Copyright © 2022 Rangeproof Pty Ltd. All rights reserved. import UIKit +import AVKit import GRDB import DifferenceKit import SessionUIKit diff --git a/Session/Conversations/ConversationViewModel.swift b/Session/Conversations/ConversationViewModel.swift index 5d7d7eccd..cdb434a40 100644 --- a/Session/Conversations/ConversationViewModel.swift +++ b/Session/Conversations/ConversationViewModel.swift @@ -247,32 +247,14 @@ public class ConversationViewModel: OWSAudioPlayerDelegate { ) ], onChangeUnsorted: { [weak self] updatedData, updatedPageInfo in - guard - let currentData: [SectionModel] = self?.interactionData, - let updatedInteractionData: [SectionModel] = self?.process(data: updatedData, for: updatedPageInfo) - else { return } - - let changeset: StagedChangeset<[SectionModel]> = StagedChangeset( - source: currentData, - target: updatedInteractionData - ) - - // No need to do anything if there were no changes - guard !changeset.isEmpty else { return } - - // Run any changes on the main thread (as they will generally trigger UI updates) - DispatchQueue.main.async { - // If we have the callback then trigger it, otherwise just store the changes to be sent - // to the callback if we ever start observing again (when we have the callback it needs - // to do the data updating as it's tied to UI updates and can cause crashes if not updated - // in the correct order) - guard let onInteractionChange: (([SectionModel], StagedChangeset<[SectionModel]>) -> ()) = self?.onInteractionChange else { - self?.unobservedInteractionDataChanges = (updatedInteractionData, changeset) - return + PagedData.processAndTriggerUpdates( + updatedData: self?.process(data: updatedData, for: updatedPageInfo), + currentDataRetriever: { self?.interactionData }, + onDataChange: self?.onInteractionChange, + onUnobservedDataChange: { updatedData, changeset in + self?.unobservedInteractionDataChanges = (updatedData, changeset) } - - onInteractionChange(updatedInteractionData, changeset) - } + ) } ) } diff --git a/Session/Home/HomeViewModel.swift b/Session/Home/HomeViewModel.swift index 8a14aa19a..30f44b6d5 100644 --- a/Session/Home/HomeViewModel.swift +++ b/Session/Home/HomeViewModel.swift @@ -150,42 +150,14 @@ public class HomeViewModel { orderSQL: SessionThreadViewModel.homeOrderSQL ), onChangeUnsorted: { [weak self] updatedData, updatedPageInfo in - guard - let currentData: [SectionModel] = self?.threadData, - let updatedThreadData: [SectionModel] = self?.process(data: updatedData, for: updatedPageInfo) - else { return } - - let changeset: StagedChangeset<[SectionModel]> = StagedChangeset( - source: currentData, - target: updatedThreadData - ) - - // No need to do anything if there were no changes - guard !changeset.isEmpty else { return } - - let performUpdates = { - // If we have the callback then trigger it, otherwise just store the changes to be sent - // to the callback if we ever start observing again (when we have the callback it needs - // to do the data updating as it's tied to UI updates and can cause crashes if not updated - // in the correct order) - guard let onThreadChange: (([SectionModel], StagedChangeset<[SectionModel]>) -> ()) = self?.onThreadChange else { - self?.unobservedThreadDataChanges = (updatedThreadData, changeset) - return + PagedData.processAndTriggerUpdates( + updatedData: self?.process(data: updatedData, for: updatedPageInfo), + currentDataRetriever: { self?.threadData }, + onDataChange: self?.onThreadChange, + onUnobservedDataChange: { updatedData, changeset in + self?.unobservedThreadDataChanges = (updatedData, changeset) } - - onThreadChange(updatedThreadData, changeset) - } - - // Note: On the initial launch the data will be fetched on the main thread and we want it - // to block so don't dispatch to the next run loop - guard !Thread.isMainThread else { - return performUpdates() - } - - // Run any changes on the main thread (as they will generally trigger UI updates) - DispatchQueue.main.async { - performUpdates() - } + ) } ) @@ -246,20 +218,15 @@ public class HomeViewModel { data: currentData.flatMap { $0.elements }, for: currentPageInfo ) - let changeset: StagedChangeset<[SectionModel]> = StagedChangeset( - source: currentData, - target: updatedThreadData - ) - - // No need to do anything if there were no changes - guard !changeset.isEmpty else { return } - guard let onThreadChange: (([SectionModel], StagedChangeset<[SectionModel]>) -> ()) = self.onThreadChange else { - self.unobservedThreadDataChanges = (updatedThreadData, changeset) - return - } - - onThreadChange(updatedThreadData, changeset) + PagedData.processAndTriggerUpdates( + updatedData: updatedThreadData, + currentDataRetriever: { [weak self] in (self?.unobservedThreadDataChanges?.0 ?? self?.threadData) }, + onDataChange: onThreadChange, + onUnobservedDataChange: { [weak self] updatedThreadData, changeset in + self?.unobservedThreadDataChanges = (updatedThreadData, changeset) + } + ) } // MARK: - Thread Data diff --git a/Session/Home/Message Requests/MessageRequestsViewModel.swift b/Session/Home/Message Requests/MessageRequestsViewModel.swift index 555b768e9..cd0dbf18d 100644 --- a/Session/Home/Message Requests/MessageRequestsViewModel.swift +++ b/Session/Home/Message Requests/MessageRequestsViewModel.swift @@ -98,32 +98,14 @@ public class MessageRequestsViewModel { orderSQL: SessionThreadViewModel.messageRequetsOrderSQL ), onChangeUnsorted: { [weak self] updatedData, updatedPageInfo in - guard - let currentData: [SectionModel] = self?.threadData, - let updatedThreadData: [SectionModel] = self?.process(data: updatedData, for: updatedPageInfo) - else { return } - - let changeset: StagedChangeset<[SectionModel]> = StagedChangeset( - source: currentData, - target: updatedThreadData - ) - - // No need to do anything if there were no changes - guard !changeset.isEmpty else { return } - - // Run any changes on the main thread (as they will generally trigger UI updates) - DispatchQueue.main.async { - // If we have the callback then trigger it, otherwise just store the changes to be sent - // to the callback if we ever start observing again (when we have the callback it needs - // to do the data updating as it's tied to UI updates and can cause crashes if not updated - // in the correct order) - guard let onThreadChange: (([SectionModel], StagedChangeset<[SectionModel]>) -> ()) = self?.onThreadChange else { - self?.unobservedThreadDataChanges = (updatedThreadData, changeset) - return + PagedData.processAndTriggerUpdates( + updatedData: self?.process(data: updatedData, for: updatedPageInfo), + currentDataRetriever: { self?.threadData }, + onDataChange: self?.onThreadChange, + onUnobservedDataChange: { updatedData, changeset in + self?.unobservedThreadDataChanges = (updatedData, changeset) } - - onThreadChange(updatedThreadData, changeset) - } + ) } ) diff --git a/Session/Home/New Conversation/NewDMVC.swift b/Session/Home/New Conversation/NewDMVC.swift index 6e9bd16ba..5779c076d 100644 --- a/Session/Home/New Conversation/NewDMVC.swift +++ b/Session/Home/New Conversation/NewDMVC.swift @@ -131,8 +131,10 @@ final class NewDMVC: BaseVC, UIPageViewControllerDataSource, UIPageViewControlle } fileprivate func handleCameraAccessGranted() { - pages[1] = scanQRCodeWrapperVC - pageVC.setViewControllers([ scanQRCodeWrapperVC ], direction: .forward, animated: false, completion: nil) + DispatchQueue.main.async { + self.pages[1] = self.scanQRCodeWrapperVC + self.pageVC.setViewControllers([ self.scanQRCodeWrapperVC ], direction: .forward, animated: false, completion: nil) + } } // MARK: - Updating diff --git a/Session/Media Viewing & Editing/MediaGalleryViewModel.swift b/Session/Media Viewing & Editing/MediaGalleryViewModel.swift index c9e25c925..7c7e39151 100644 --- a/Session/Media Viewing & Editing/MediaGalleryViewModel.swift +++ b/Session/Media Viewing & Editing/MediaGalleryViewModel.swift @@ -93,32 +93,14 @@ public class MediaGalleryViewModel { orderSQL: Item.galleryOrderSQL, dataQuery: Item.baseQuery(orderSQL: Item.galleryOrderSQL), onChangeUnsorted: { [weak self] updatedData, updatedPageInfo in - guard - let currentData: [SectionModel] = self?.galleryData, - let updatedGalleryData: [SectionModel] = self?.process(data: updatedData, for: updatedPageInfo) - else { return } - - let changeset: StagedChangeset<[SectionModel]> = StagedChangeset( - source: currentData, - target: updatedGalleryData - ) - - // No need to do anything if there were no changes - guard !changeset.isEmpty else { return } - - // Run any changes on the main thread (as they will generally trigger UI updates) - DispatchQueue.main.async { - // If we have the callback then trigger it, otherwise just store the changes to be sent - // to the callback if we ever start observing again (when we have the callback it needs - // to do the data updating as it's tied to UI updates and can cause crashes if not updated - // in the correct order) - guard let onGalleryChange: (([SectionModel], StagedChangeset<[SectionModel]>) -> ()) = self?.onGalleryChange else { - self?.unobservedGalleryDataChanges = (updatedGalleryData, changeset) - return + PagedData.processAndTriggerUpdates( + updatedData: self?.process(data: updatedData, for: updatedPageInfo), + currentDataRetriever: { self?.galleryData }, + onDataChange: self?.onGalleryChange, + onUnobservedDataChange: { updatedData, changeset in + self?.unobservedGalleryDataChanges = (updatedData, changeset) } - - onGalleryChange(updatedGalleryData, changeset) - } + ) } ) diff --git a/Session/Media Viewing & Editing/SendMediaNavigationController.swift b/Session/Media Viewing & Editing/SendMediaNavigationController.swift index b70281552..1b6f7e5ec 100644 --- a/Session/Media Viewing & Editing/SendMediaNavigationController.swift +++ b/Session/Media Viewing & Editing/SendMediaNavigationController.swift @@ -142,13 +142,17 @@ class SendMediaNavigationController: UINavigationController { private func didTapCameraModeButton() { Permissions.requestCameraPermissionIfNeeded { [weak self] in - self?.fadeTo(viewControllers: ((self?.captureViewController).map { [$0] } ?? [])) + DispatchQueue.main.async { + self?.fadeTo(viewControllers: ((self?.captureViewController).map { [$0] } ?? [])) + } } } private func didTapMediaLibraryModeButton() { Permissions.requestLibraryPermissionIfNeeded { [weak self] in - self?.fadeTo(viewControllers: ((self?.mediaLibraryViewController).map { [$0] } ?? [])) + DispatchQueue.main.async { + self?.fadeTo(viewControllers: ((self?.mediaLibraryViewController).map { [$0] } ?? [])) + } } } diff --git a/Session/Onboarding/LinkDeviceVC.swift b/Session/Onboarding/LinkDeviceVC.swift index 4a81ee495..d0b694423 100644 --- a/Session/Onboarding/LinkDeviceVC.swift +++ b/Session/Onboarding/LinkDeviceVC.swift @@ -109,8 +109,10 @@ final class LinkDeviceVC: BaseVC, UIPageViewControllerDataSource, UIPageViewCont } fileprivate func handleCameraAccessGranted() { - pages[1] = scanQRCodeWrapperVC - pageVC.setViewControllers([ scanQRCodeWrapperVC ], direction: .forward, animated: false, completion: nil) + DispatchQueue.main.async { + self.pages[1] = self.scanQRCodeWrapperVC + self.pageVC.setViewControllers([ self.scanQRCodeWrapperVC ], direction: .forward, animated: false, completion: nil) + } } // MARK: - Updating @@ -161,9 +163,15 @@ final class LinkDeviceVC: BaseVC, UIPageViewControllerDataSource, UIPageViewCont GetSnodePoolJob.run() NotificationCenter.default.addObserver(self, selector: #selector(handleInitialConfigurationMessageReceived), name: .initialConfigurationMessageReceived, object: nil) - ModalActivityIndicatorViewController.present(fromViewController: navigationController!) { [weak self] modal in - self?.activityIndicatorModal = modal - } + + ModalActivityIndicatorViewController + .present( + // There was some crashing here due to force-unwrapping so just falling back to + // using self if there is no nav controller + fromViewController: (self.navigationController ?? self) + ) { [weak self] modal in + self?.activityIndicatorModal = modal + } } @objc private func handleInitialConfigurationMessageReceived(_ notification: Notification) { diff --git a/Session/Open Groups/JoinOpenGroupVC.swift b/Session/Open Groups/JoinOpenGroupVC.swift index 14ed61363..d46e3f9bc 100644 --- a/Session/Open Groups/JoinOpenGroupVC.swift +++ b/Session/Open Groups/JoinOpenGroupVC.swift @@ -105,8 +105,10 @@ final class JoinOpenGroupVC: BaseVC, UIPageViewControllerDataSource, UIPageViewC } fileprivate func handleCameraAccessGranted() { - pages[1] = scanQRCodeWrapperVC - pageVC.setViewControllers([ scanQRCodeWrapperVC ], direction: .forward, animated: false, completion: nil) + DispatchQueue.main.async { + self.pages[1] = self.scanQRCodeWrapperVC + self.pageVC.setViewControllers([ self.scanQRCodeWrapperVC ], direction: .forward, animated: false, completion: nil) + } } // MARK: - Updating diff --git a/Session/Settings/BlockedContactsViewModel.swift b/Session/Settings/BlockedContactsViewModel.swift index 68ff46a86..1a414edf3 100644 --- a/Session/Settings/BlockedContactsViewModel.swift +++ b/Session/Settings/BlockedContactsViewModel.swift @@ -62,32 +62,14 @@ public class BlockedContactsViewModel { orderSQL: DataModel.orderSQL ), onChangeUnsorted: { [weak self] updatedData, updatedPageInfo in - guard - let currentData: [SectionModel] = self?.contactData, - let updatedContactData: [SectionModel] = self?.process(data: updatedData, for: updatedPageInfo) - else { return } - - let changeset: StagedChangeset<[SectionModel]> = StagedChangeset( - source: currentData, - target: updatedContactData - ) - - // No need to do anything if there were no changes - guard !changeset.isEmpty else { return } - - // Run any changes on the main thread (as they will generally trigger UI updates) - DispatchQueue.main.async { - // If we have the callback then trigger it, otherwise just store the changes to be sent - // to the callback if we ever start observing again (when we have the callback it needs - // to do the data updating as it's tied to UI updates and can cause crashes if not updated - // in the correct order) - guard let onContactChange: (([SectionModel], StagedChangeset<[SectionModel]>) -> ()) = self?.onContactChange else { - self?.unobservedContactDataChanges = (updatedContactData, changeset) - return + PagedData.processAndTriggerUpdates( + updatedData: self?.process(data: updatedData, for: updatedPageInfo), + currentDataRetriever: { self?.contactData }, + onDataChange: self?.onContactChange, + onUnobservedDataChange: { updatedData, changeset in + self?.unobservedContactDataChanges = (updatedData, changeset) } - - onContactChange(updatedContactData, changeset) - } + ) } ) diff --git a/Session/Settings/QRCodeVC.swift b/Session/Settings/QRCodeVC.swift index c1b34d08e..c0cb10efd 100644 --- a/Session/Settings/QRCodeVC.swift +++ b/Session/Settings/QRCodeVC.swift @@ -95,8 +95,10 @@ final class QRCodeVC : BaseVC, UIPageViewControllerDataSource, UIPageViewControl } fileprivate func handleCameraAccessGranted() { - pages[1] = scanQRCodeWrapperVC - pageVC.setViewControllers([ scanQRCodeWrapperVC ], direction: .forward, animated: false, completion: nil) + DispatchQueue.main.async { + self.pages[1] = self.scanQRCodeWrapperVC + self.pageVC.setViewControllers([ self.scanQRCodeWrapperVC ], direction: .forward, animated: false, completion: nil) + } } // MARK: - Updating diff --git a/Session/Settings/SettingsViewModel.swift b/Session/Settings/SettingsViewModel.swift index 0b073c273..8ee5ba0dd 100644 --- a/Session/Settings/SettingsViewModel.swift +++ b/Session/Settings/SettingsViewModel.swift @@ -407,12 +407,14 @@ class SettingsViewModel: SessionTableViewModel: TransactionObserver where // Update the cache, pageInfo and the change callback self?.dataCache.mutate { $0 = finalUpdatedDataCache } self?.pageInfo.mutate { $0 = updatedPageInfo } + + + // Make sure the updates run on the main thread + guard Thread.isMainThread else { + DispatchQueue.main.async { [weak self] in + self?.onChangeUnsorted(finalUpdatedDataCache.values, updatedPageInfo) + } + return + } + self?.onChangeUnsorted(finalUpdatedDataCache.values, updatedPageInfo) } @@ -673,7 +684,12 @@ public class PagedDatabaseObserver: TransactionObserver where let updatedLimitInfo: PagedData.PageInfo = PagedData.PageInfo( pageSize: currentPageInfo.pageSize, pageOffset: queryInfo.updatedCacheOffset, - currentCount: (currentPageInfo.currentCount + newData.count), + currentCount: { + switch target { + case .reloadCurrent: return currentPageInfo.currentCount + default: return (currentPageInfo.currentCount + newData.count) + } + }(), totalCount: totalCount ) @@ -726,6 +742,12 @@ public class PagedDatabaseObserver: TransactionObserver where self?.isLoadingMoreData.mutate { $0 = false } } + // Make sure the updates run on the main thread + guard Thread.isMainThread else { + DispatchQueue.main.async { triggerUpdates() } + return + } + triggerUpdates() } @@ -996,6 +1018,56 @@ public enum PagedData { let rowIndex: Int64 } + // MARK: - Convenience Functions + + // FIXME: Would be good to clean this up further in the future (should be able to do more processing on BG threads) + public static func processAndTriggerUpdates( + updatedData: [SectionModel]?, + currentDataRetriever: @escaping (() -> [SectionModel]?), + onDataChange: (([SectionModel], StagedChangeset<[SectionModel]>) -> ())?, + onUnobservedDataChange: @escaping (([SectionModel], StagedChangeset<[SectionModel]>) -> Void) + ) { + guard let updatedData: [SectionModel] = updatedData else { return } + + // Note: While it would be nice to generate the changeset on a background thread it introduces + // a multi-threading issue where a data change can come in while the table is processing multiple + // updates resulting in the data being in a partially updated state (which makes the subsequent + // table reload crash due to inconsistent state) + let performUpdates = { + guard let currentData: [SectionModel] = currentDataRetriever() else { return } + + let changeset: StagedChangeset<[SectionModel]> = StagedChangeset( + source: currentData, + target: updatedData + ) + + // No need to do anything if there were no changes + guard !changeset.isEmpty else { return } + + // If we have the callback then trigger it, otherwise just store the changes to be sent + // to the callback if we ever start observing again (when we have the callback it needs + // to do the data updating as it's tied to UI updates and can cause crashes if not updated + // in the correct order) + guard let onDataChange: (([SectionModel], StagedChangeset<[SectionModel]>) -> ()) = onDataChange else { + onUnobservedDataChange(updatedData, changeset) + return + } + + onDataChange(updatedData, changeset) + } + + // No need to dispatch to the next run loop if we are alread on the main thread + guard !Thread.isMainThread else { + performUpdates() + return + } + + // Run any changes on the main thread (as they will generally trigger UI updates) + DispatchQueue.main.async { + performUpdates() + } + } + // MARK: - Internal Functions fileprivate static func totalCount(