diff --git a/CHANGELOG.md b/CHANGELOG.md index 27a5ac3e..292e209e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ### Fixed +- Fixed a crash when a list left the window (or was deallocated) while an interactive reorder was still in progress — for example, navigating away while a drag was held. The native interactive-movement session outlived the content it was started against, so a later content update or layout pass read a now-stale index and trapped in the layout. `ListView` now cancels any in-progress reorder on `didMoveToWindow` (when leaving the window) and in `deinit`, while the data source and layout are still in sync. + ### Added ### Removed diff --git a/ListableUI/Sources/ListView/ListView.swift b/ListableUI/Sources/ListView/ListView.swift index 27601e62..332c7655 100644 --- a/ListableUI/Sources/ListView/ListView.swift +++ b/ListableUI/Sources/ListView/ListView.swift @@ -144,6 +144,15 @@ public final class ListView : UIView // because the display link driving it is retained by the main runloop. self.cancelScrollAnimation() + // If the list is deallocated while a reorder gesture is still in flight, the native + // interactive-movement session would otherwise outlive the data source and layout. + // UIKit would then try to resolve the move against content that no longer matches the + // drag's index paths, reading a stale index and crashing in the layout. Cancel it here, + // while everything is still in sync. + if self.hasInProgressReorders { + self.cancelAllInProgressReorders() + } + /** Even though these are zeroing weak references in UIKIt as of iOS 9.0, @@ -1567,9 +1576,15 @@ public final class ListView : UIView public override func didMoveToWindow() { super.didMoveToWindow() - + if self.window != nil { self.updateScrollViewInsets() + } else if self.hasInProgressReorders { + // Leaving the window — for example, navigating away while a drag is still held — ends + // any chance of the reorder gesture completing normally. Cancel it now, while the + // content and the drag's index paths still agree, so a later content update or layout + // pass cannot read a stale index and crash. + self.cancelAllInProgressReorders() } } @@ -2467,7 +2482,7 @@ extension ListView : ReorderingActionsDelegate self.collectionView.cancelInteractiveMovement() } - private var hasInProgressReorders : Bool { + var hasInProgressReorders : Bool { for section in self.storage.presentationState.sections { for item in section.items { diff --git a/ListableUI/Tests/ListView/ListView.ReorderTeardownTests.swift b/ListableUI/Tests/ListView/ListView.ReorderTeardownTests.swift new file mode 100644 index 00000000..57d2e62a --- /dev/null +++ b/ListableUI/Tests/ListView/ListView.ReorderTeardownTests.swift @@ -0,0 +1,174 @@ +// +// ListView.ReorderTeardownTests.swift +// ListableUI-Unit-Tests +// + +@testable import ListableUI +import XCTest + + +class ListView_ReorderTeardownTests: XCTestCase { + + /// A reorder that is still in flight when the list leaves the window — for example, the user + /// navigates away while a drag is held — must be cancelled during teardown. This asserts the + /// observable state: after leaving the window, the list reports no in-progress reorders. + func test_reorder_is_cancelled_when_list_leaves_window() { + + let viewController = ReorderTestViewController() + + show(vc: viewController) { viewController in + let listView = viewController.list + + // Force the collection view to build presentation state for the content. + listView.collectionView.layoutIfNeeded() + + let indexPath = IndexPath(item: 0, section: 0) + let item = listView.storage.presentationState.item(at: indexPath) + + item.beginReorder(from: indexPath, with: listView.environment) + + XCTAssertTrue(listView.hasInProgressReorders) + XCTAssertTrue(item.isReordering) + + // Leaving the window is what navigating away mid-drag does to the list. + listView.removeFromSuperview() + + XCTAssertFalse( + listView.hasInProgressReorders, + "Leaving the window should cancel any in-progress reorder." + ) + XCTAssertFalse(item.isReordering) + } + } + + /// Reproduces the actual crash: a real `UICollectionView` interactive-movement session is + /// started (the way a drag does), the list then leaves the window mid-drag, and its content + /// is updated with index-affecting changes. + /// + /// Before the fix, the still-open interactive-movement session outlived the content it was + /// started against, and applying the update resolved the move against now-stale index paths — + /// an out-of-range access deep in `ListLayoutContent`. With the fix, leaving the window + /// cancels the session first, so the update applies cleanly. Reaching the end of the test + /// without crashing is the assertion (mirrors `test_changing_to_empty_frame_does_not_crash`). + func test_reorder_interrupted_by_navigation_does_not_crash() { + + let viewController = ReorderTestViewController() + + show(vc: viewController) { viewController in + let listView = viewController.list + + listView.collectionView.layoutIfNeeded() + + let indexPath = IndexPath(item: 0, section: 0) + let item = listView.storage.presentationState.item(at: indexPath) + + // Start a real interactive-movement session through the same entry point a drag uses. + _ = listView.beginReorder(for: item) + + // Navigate away mid-drag: the list leaves the window... + listView.removeFromSuperview() + + // ...and its content is replaced with fewer items, an index-affecting change. + listView.configure { list in + list.animatesChanges = false + list("section") { section in + for number in 1...3 { + section += Item( + ReorderTestContent(title: "Item \(number)"), + reordering: ItemReordering(sections: .all) + ) + } + } + } + + listView.collectionView.layoutIfNeeded() + } + } + + /// A list deallocated with a reorder still in progress must not crash: teardown cancels the + /// reorder while the data source and layout are still valid, and doing so introduces no + /// retain cycle. Reaching the end with the list deallocated is the assertion. + func test_reorder_in_progress_does_not_crash_on_deinit() { + + weak var weakList: ListView? + + autoreleasepool { + let listView = ListView(frame: CGRect(x: 0, y: 0, width: 400, height: 600)) + weakList = listView + + listView.configure { list in + list.animatesChanges = false + list("section") { section in + for number in 1...10 { + section += Item( + ReorderTestContent(title: "Item \(number)"), + reordering: ItemReordering(sections: .all) + ) + } + } + } + + listView.collectionView.layoutIfNeeded() + + let indexPath = IndexPath(item: 0, section: 0) + let item = listView.storage.presentationState.item(at: indexPath) + item.beginReorder(from: indexPath, with: listView.environment) + + XCTAssertTrue(listView.hasInProgressReorders) + } + + XCTAssertNil(weakList) + } +} + + +fileprivate final class ReorderTestViewController: UIViewController { + + let list = ListView() + + override func loadView() { + view = UIView() + view.addSubview(list) + list.frame = CGRect(x: 0, y: 0, width: 400, height: 600) + + list.configure { list in + list.animatesChanges = false + list("section") { section in + for number in 1...10 { + section += Item( + ReorderTestContent(title: "Item \(number)"), + reordering: ItemReordering(sections: .all) + ) + } + } + } + } +} + + +fileprivate struct ReorderTestContent: ItemContent, Equatable { + + var title: String + + var identifierValue: String { title } + + func apply( + to views: ItemContentViews, + for reason: ApplyReason, + with info: ApplyItemContentInfo + ) { + views.content.backgroundColor = .red + } + + typealias ContentView = UIView + + static func createReusableContentView(frame: CGRect) -> UIView { + UIView(frame: frame) + } + + var defaultItemProperties: DefaultProperties { + .defaults { defaults in + defaults.sizing = .fixed(height: 50) + } + } +}