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..4d352ea8 --- /dev/null +++ b/ListableUI/Tests/ListView/ListView.ReorderTeardownTests.swift @@ -0,0 +1,128 @@ +// +// ListView.ReorderTeardownTests.swift +// ListableUI-Unit-Tests +// + +@testable import ListableUI +import XCTest + + +class ListView_ReorderTeardownTests: XCTestCase { + + /// A reorder gesture that is still in flight when the list leaves the window — for example, + /// navigating away while a drag is held — must be cancelled during teardown. Otherwise the + /// native interactive-movement session outlives the content it was started against, and a + /// later content update or layout pass reads a now-stale index and crashes. + func test_reorder_is_cancelled_when_list_leaves_window() { + + let viewController = ReorderTestViewController() + + show(vc: viewController) { viewController in + let listView = viewController.list + + let indexPath = IndexPath(item: 0, section: 0) + let item = listView.storage.presentationState.item(at: indexPath) + + // Simulate an in-progress reorder without depending on the flakier native + // `beginInteractiveMovementForItem` gesture plumbing. + item.beginReorder(from: indexPath, with: listView.environment) + + XCTAssertTrue(listView.hasInProgressReorders) + + // 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." + ) + } + } + + /// A list deallocated with a reorder still in progress must not crash: teardown cancels the + /// interactive movement while the data source and layout are still valid. + 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) + } + + // Reaching here without a crash — and with the list deallocated — is the assertion. + 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) + } + } +}