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
This commit is contained in:
Morgan Pretty 2022-10-24 14:52:28 +11:00
parent 89df1261e3
commit 1b350cf422
16 changed files with 167 additions and 176 deletions

View File

@ -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

View File

@ -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

View File

@ -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

View File

@ -1,6 +1,7 @@
// Copyright © 2022 Rangeproof Pty Ltd. All rights reserved.
import UIKit
import AVKit
import GRDB
import DifferenceKit
import SessionUIKit

View File

@ -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)
}
)
}
)
}

View File

@ -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
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)
}
)
// 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)
}
// MARK: - Thread Data

View File

@ -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)
}
)
}
)

View File

@ -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

View File

@ -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)
}
)
}
)

View File

@ -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] } ?? []))
}
}
}

View File

@ -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) {

View File

@ -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

View File

@ -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)
}
)
}
)

View File

@ -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

View File

@ -407,12 +407,14 @@ class SettingsViewModel: SessionTableViewModel<SettingsViewModel.NavButton, Sett
private func showPhotoLibraryForAvatar() {
Permissions.requestLibraryPermissionIfNeeded { [weak self] in
let picker: UIImagePickerController = UIImagePickerController()
picker.sourceType = .photoLibrary
picker.mediaTypes = [ "public.image" ]
picker.delegate = self?.imagePickerHandler
self?.transitionToScreen(picker, transitionType: .present)
DispatchQueue.main.async {
let picker: UIImagePickerController = UIImagePickerController()
picker.sourceType = .photoLibrary
picker.mediaTypes = [ "public.image" ]
picker.delegate = self?.imagePickerHandler
self?.transitionToScreen(picker, transitionType: .present)
}
}
}

View File

@ -2,6 +2,7 @@
import Foundation
import GRDB
import DifferenceKit
// MARK: - PagedDatabaseObserver
@ -198,6 +199,16 @@ public class PagedDatabaseObserver<ObservedTable, T>: 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<ObservedTable, T>: 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<ObservedTable, T>: 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<SectionModel: DifferentiableSection>(
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(