From 98acf9687a6bcd7fc4116ee9fe282cd556ad52ac Mon Sep 17 00:00:00 2001 From: george-signal <98181642+george-signal@users.noreply.github.com> Date: Mon, 8 Aug 2022 10:34:39 -0700 Subject: [PATCH] Fix a crash in stories tableview. (#4692) Commit 04737ff044cedddaf9715300e18d5ccf414b207a attempted to fix an assertion in StoriesViewController due to UITableView complaining about inconsistent state. The issue continues to happen. This diff fixes it once and for all (I hope!). The scenario that caused a crash prior to 04737ff044cedddaf9715300e18d5ccf414b207a was: Initially: models = [a,b,c] Database changes to [a,b] reloadStories (loading queue) newModels = [a,b] schedule block to run on main queue Database changes to [x, b] updateStories (loading queue) captures self.models, value is [a,b,c] // cue ominous music load from database newModels = [x,b] schedule block to run on main queue; will delete c and change a to x reloadStories.block runs on main queue self.models=newModels, value is [a,b] tableView.reloadData() // UITableView thinks c is deleted updateStories.block runs on main queue self.models=newModels, value is [x,b] tableView.beginUpdates() tableView.delete(index: 2) // crashes because c was already deleted Post-04737ff the following scenario causes a crash: Initially: models = [a,b,c] Database changes to [a,b] updateStories (loading queue) load from database models.set([a,b]) schedule block to run on main queue which will delete c iOS gets a wild hair and decides to call some UITableViewDataSource methods. tableView(_, numberOfRowsInSection:) return [a,b].count // Now UIKit thinks c is gone updateStories.block runs on main queue tableView.beginUpdates() tableView.delete(index: 2) // crashes because 2 is out of bounds vs number of rows This commit fixes the problem by ensuring the read-modify-write that happens in updateStories never reads older data than what UITableView has been told about and also that updates to the model are properly synchronized with beginUpdates()...endUpdates(). Here's how to think about it: the database goes through a series of changes. Call them v1, v2, ..., vN. The loading queue will see each in turn. For each, it will enqueue a corresponding block on the main queue to update `model` and tell UITableView about it. From the POV of the main queue, each update occurs in the scope of a UITableView update. From the POV of the loading queue, each update reads from exactly the preceding version (from `trueModels`) and writes to exactly the next version of `trueModels`. Therefore, there are no data races. --- .../Stories/StoriesViewController.swift | 233 +++++++++++------- 1 file changed, 147 insertions(+), 86 deletions(-) diff --git a/Signal/src/ViewControllers/HomeView/Stories/StoriesViewController.swift b/Signal/src/ViewControllers/HomeView/Stories/StoriesViewController.swift index 6122aa520c..5b258278c0 100644 --- a/Signal/src/ViewControllers/HomeView/Stories/StoriesViewController.swift +++ b/Signal/src/ViewControllers/HomeView/Stories/StoriesViewController.swift @@ -7,9 +7,11 @@ import SignalServiceKit import UIKit import SignalUI +private let loadingQueue = DispatchQueue(label: "StoriesViewController.loadingQueue", qos: .userInitiated) + class StoriesViewController: OWSViewController { let tableView = UITableView() - private var models = AtomicArray() + private let syncingModels = SyncingStoryViewModelArray() private var myStoryModel = AtomicOptional(nil) private lazy var emptyStateLabel: UILabel = { @@ -213,25 +215,23 @@ class StoriesViewController: OWSViewController { presentFormSheet(AppSettingsViewController.inModalNavigationController(), animated: true) } - private static let loadingQueue = DispatchQueue(label: "StoriesViewController.loadingQueue", qos: .userInitiated) private func reloadStories() { - Self.loadingQueue.async { - let (listStories, outgoingStories) = Self.databaseStorage.read { - ( - StoryFinder.storiesForListView(transaction: $0), - StoryFinder.outgoingStories(limit: 2, transaction: $0) - ) - } - let groupedMessages = Dictionary(grouping: listStories) { $0.context } - let newModels = Self.databaseStorage.read { transaction in - groupedMessages.compactMap { try? StoryViewModel(messages: $1, transaction: transaction) } - }.sorted(by: self.sortStoryModels) - let myStoryModel = Self.databaseStorage.read { MyStoryViewModel(messages: outgoingStories, transaction: $0) } - - self.models.set(newModels) - self.myStoryModel.set(myStoryModel) - - DispatchQueue.main.async { + loadingQueue.async { + self.syncingModels.mutate { _ -> ([StoryViewModel], Void)? in + let (listStories, outgoingStories) = Self.databaseStorage.read { + ( + StoryFinder.storiesForListView(transaction: $0), + StoryFinder.outgoingStories(limit: 2, transaction: $0) + ) + } + let myStoryModel = Self.databaseStorage.read { MyStoryViewModel(messages: outgoingStories, transaction: $0) } + let groupedMessages = Dictionary(grouping: listStories) { $0.context } + self.myStoryModel.set(myStoryModel) + let newValues = Self.databaseStorage.read { transaction in + groupedMessages.compactMap { try? StoryViewModel(messages: $1, transaction: transaction) } + }.sorted(by: self.sortStoryModels) + return (newValues, ()) + } sync: { (_, _) in self.tableView.reloadData() } } @@ -242,77 +242,73 @@ class StoriesViewController: OWSViewController { guard !rowIds.isEmpty else { return } - Self.loadingQueue.async { - let models = self.models.get() - let myStoryModel = self.myStoryModel.get() + loadingQueue.async { + let ok = + self.syncingModels.mutate { models -> ([StoryViewModel], (Bool, [BatchUpdate.Item]))? in + let myStoryModel = self.myStoryModel.get() - let (updatedListMessages, outgoingStories) = Self.databaseStorage.read { - ( - StoryFinder.listStoriesWithRowIds(Array(rowIds), transaction: $0), - StoryFinder.outgoingStories(limit: 2, transaction: $0) - ) - } - var deletedRowIds = rowIds.subtracting(updatedListMessages.lazy.map { $0.id! }) - var groupedMessages = Dictionary(grouping: updatedListMessages) { $0.context } + let (updatedListMessages, outgoingStories) = Self.databaseStorage.read { + ( + StoryFinder.listStoriesWithRowIds(Array(rowIds), transaction: $0), + StoryFinder.outgoingStories(limit: 2, transaction: $0) + ) + } + var deletedRowIds = rowIds.subtracting(updatedListMessages.lazy.map { $0.id! }) + var groupedMessages = Dictionary(grouping: updatedListMessages) { $0.context } - let oldContexts = models.map { $0.context } - var changedContexts = [StoryContext]() + let oldContexts = models.map { $0.context } + var changedContexts = [StoryContext]() - let newModels: [StoryViewModel] - do { - newModels = try Self.databaseStorage.read { transaction in - try models.compactMap { model in - guard let latestMessage = model.messages.first else { return model } + let newModels: [StoryViewModel] + do { + newModels = try Self.databaseStorage.read { transaction in + try models.compactMap { model in + guard let latestMessage = model.messages.first else { return model } - let modelDeletedRowIds: [Int64] = model.messages.lazy.compactMap { $0.id }.filter { deletedRowIds.contains($0) } - deletedRowIds.subtract(modelDeletedRowIds) + let modelDeletedRowIds: [Int64] = model.messages.lazy.compactMap { $0.id }.filter { deletedRowIds.contains($0) } + deletedRowIds.subtract(modelDeletedRowIds) - let modelUpdatedMessages = groupedMessages.removeValue(forKey: latestMessage.context) ?? [] + let modelUpdatedMessages = groupedMessages.removeValue(forKey: latestMessage.context) ?? [] - guard !modelUpdatedMessages.isEmpty || !modelDeletedRowIds.isEmpty else { return model } + guard !modelUpdatedMessages.isEmpty || !modelDeletedRowIds.isEmpty else { return model } - changedContexts.append(model.context) + changedContexts.append(model.context) - return try model.copy( - updatedMessages: modelUpdatedMessages, - deletedMessageRowIds: modelDeletedRowIds, - transaction: transaction - ) - } + groupedMessages.map { try StoryViewModel(messages: $1, transaction: transaction) } - }.sorted(by: self.sortStoryModels) - } catch { - owsFailDebug("Failed to build new models, hard reloading \(error)") - DispatchQueue.main.async { self.reloadStories() } - return - } + return try model.copy( + updatedMessages: modelUpdatedMessages, + deletedMessageRowIds: modelDeletedRowIds, + transaction: transaction + ) + } + groupedMessages.map { try StoryViewModel(messages: $1, transaction: transaction) } + }.sorted(by: self.sortStoryModels) + } catch { + owsFailDebug("Failed to build new models, hard reloading \(error)") + return nil + } + let myStoryChanged = rowIds.intersection(outgoingStories.map { $0.id! }).count > 0 + || Set(myStoryModel?.messages.map { $0.uniqueId } ?? []) != Set(outgoingStories.map { $0.uniqueId }) + let newMyStoryModel = myStoryChanged + ? Self.databaseStorage.read { MyStoryViewModel(messages: outgoingStories, transaction: $0) } + : myStoryModel - let myStoryChanged = rowIds.intersection(outgoingStories.map { $0.id! }).count > 0 - || Set(myStoryModel?.messages.map { $0.uniqueId } ?? []) != Set(outgoingStories.map { $0.uniqueId }) - let newMyStoryModel = myStoryChanged - ? Self.databaseStorage.read { MyStoryViewModel(messages: outgoingStories, transaction: $0) } - : myStoryModel - - let batchUpdateItems: [BatchUpdate.Item] - do { - batchUpdateItems = try BatchUpdate.build( - viewType: .uiTableView, - oldValues: oldContexts, - newValues: newModels.map { $0.context }, - changedValues: changedContexts - ) - } catch { - owsFailDebug("Failed to calculate batch updates, hard reloading \(error)") - DispatchQueue.main.async { self.reloadStories() } - return - } - - self.models.set(newModels) - self.myStoryModel.set(newMyStoryModel) - - DispatchQueue.main.async { + let batchUpdateItems: [BatchUpdate.Item] + do { + batchUpdateItems = try BatchUpdate.build( + viewType: .uiTableView, + oldValues: oldContexts, + newValues: newModels.map { $0.context }, + changedValues: changedContexts + ) + } catch { + owsFailDebug("Failed to calculate batch updates, hard reloading \(error)") + return nil + } + self.myStoryModel.set(newMyStoryModel) + return (newModels, (myStoryChanged, batchUpdateItems)) + } sync: { (newModels, userInfo: (Bool, [BatchUpdate.Item])) in + let (myStoryChanged, batchUpdateItems) = userInfo guard self.isViewLoaded else { return } self.tableView.beginUpdates() - if myStoryChanged { self.tableView.reloadRows(at: [IndexPath(row: 0, section: Section.myStory.rawValue)], with: .automatic) } @@ -331,7 +327,7 @@ class StoriesViewController: OWSViewController { let path = IndexPath(row: newIndex, section: Section.visibleStories.rawValue) if (self.tableView.indexPathsForVisibleRows ?? []).contains(path), let visibleCell = self.tableView.cellForRow(at: path) as? StoryCell { - guard let model = self.models.get()[safe: newIndex] else { + guard let model = newModels[safe: newIndex] else { return owsFailDebug("Missing model for story") } visibleCell.configure(with: model) @@ -342,6 +338,11 @@ class StoriesViewController: OWSViewController { } self.tableView.endUpdates() } + + if !ok { + DispatchQueue.main.async { self.reloadStories() } + return + } } } @@ -420,15 +421,15 @@ extension StoriesViewController: UITableViewDataSource { func model(for indexPath: IndexPath) -> StoryViewModel? { // TODO: Hidden stories guard indexPath.section == Section.visibleStories.rawValue else { return nil } - return models.get()[safe: indexPath.row] + return syncingModels.get()[safe: indexPath.row] } func model(for context: StoryContext) -> StoryViewModel? { - models.get().first { $0.context == context } + syncingModels.get().first { $0.context == context } } func cell(for context: StoryContext) -> StoryCell? { - guard let row = models.get().firstIndex(where: { $0.context == context }) else { return nil } + guard let row = syncingModels.get().firstIndex(where: { $0.context == context }) else { return nil } let indexPath = IndexPath(row: row, section: Section.visibleStories.rawValue) guard tableView.indexPathsForVisibleRows?.contains(indexPath) == true else { return nil } return tableView.cellForRow(at: indexPath) as? StoryCell @@ -465,13 +466,13 @@ extension StoriesViewController: UITableViewDataSource { } func tableView(_ tableView: UITableView, numberOfRowsInSection section: Int) -> Int { - emptyStateLabel.isHidden = models.get().count > 0 || myStoryModel.get()?.messages.isEmpty == false + emptyStateLabel.isHidden = syncingModels.get().count > 0 || myStoryModel.get()?.messages.isEmpty == false switch Section(rawValue: section) { case .myStory: return 1 case .visibleStories: - return models.count + return syncingModels.get().count case .hiddenStories: return 0 // TODO: Hidden stories default: @@ -483,7 +484,7 @@ extension StoriesViewController: UITableViewDataSource { extension StoriesViewController: StoryPageViewControllerDataSource { func storyPageViewControllerAvailableContexts(_ storyPageViewController: StoryPageViewController) -> [StoryContext] { - models.get().map { $0.context } + syncingModels.get().map { $0.context } } } @@ -647,3 +648,63 @@ extension StoriesViewController: ForwardMessageDelegate { self.dismiss(animated: true) } } + +@propertyWrapper +struct ThreadBoundValue { + private var value: T + private var queue: DispatchQueue + + var wrappedValue: T { + get { + dispatchPrecondition(condition: .onQueue(queue)) + return value + } + set { + dispatchPrecondition(condition: .onQueue(queue)) + value = newValue + } + } + + init(wrappedValue: T, queue: DispatchQueue) { + self.value = wrappedValue + self.queue = queue + } +} + +private class SyncingStoryViewModelArray { + // This is always the most up-to-date collection of models. + @ThreadBoundValue(wrappedValue: [], queue: loadingQueue) private var trueModels: [StoryViewModel] + + // This may lag behind `trueModels` and is eventually consistent. It is exposed to UITableView. + @ThreadBoundValue(wrappedValue: [], queue: .main) private var exposedModels: [StoryViewModel] + + /// Safely modify the list of models. This method must be called on the loading queue. + /// + /// - Parameters + /// - closure: Called synchronously. Returns nil to abort mutation without side-effects. Otherwise, it returns the new values for the models array and user data to pass to `sync`. + /// - models: The existing array of models. + /// - sync: Runs asynchronously on the main queue after `closure` returns. + /// - newModels: The array of models returned by `closure`. + /// - userData: The second value returned by `closure`. + /// + /// - Returns whether the closure returned a nonnil list of models. + @discardableResult + func mutate(_ closure: (_ models: [StoryViewModel]) -> ([StoryViewModel], T)?, + sync: @escaping (_ newModels: [StoryViewModel], _ userData: T) -> Void) -> Bool { + dispatchPrecondition(condition: .onQueue(loadingQueue)) + + guard let (newModels, userInfo) = closure(trueModels) else { + return false + } + trueModels = newModels + DispatchQueue.main.async { + self.exposedModels = newModels + sync(newModels, userInfo) + } + return true + } + + func get() -> [StoryViewModel] { + return exposedModels + } +}