diff --git a/CHANGELOG.md b/CHANGELOG.md index 5e39e8f1e..768b7566f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ### Fixed +- Fixed a crash in `CollectionViewLayout` when the delegate was deallocated before the layout. The delegate is now held weakly rather than unowned. + ### Added ### Removed diff --git a/ListableUI/Sources/Layout/CollectionViewLayout.swift b/ListableUI/Sources/Layout/CollectionViewLayout.swift index 1efd4fe5d..c6c1b334a 100644 --- a/ListableUI/Sources/Layout/CollectionViewLayout.swift +++ b/ListableUI/Sources/Layout/CollectionViewLayout.swift @@ -14,7 +14,7 @@ final class CollectionViewLayout : UICollectionViewLayout // MARK: Properties // - unowned let delegate : CollectionViewLayoutDelegate + private(set) weak var delegate : CollectionViewLayoutDelegate? var layoutDescription : LayoutDescription @@ -197,7 +197,7 @@ final class CollectionViewLayout : UICollectionViewLayout self.neededLayoutType.merge(with: context) } - private func sendEndQueuingEditsAfterDelay() { + func sendEndQueuingEditsAfterDelay() { /// /// Hello! Welcome to the source code. You're probably wondering why this perform after runloop hack is here. @@ -221,8 +221,10 @@ final class CollectionViewLayout : UICollectionViewLayout /// processing any further updates 🥴. /// - OperationQueue.main.addOperation { - self.delegate.listViewShouldEndQueueingEditsForReorder() + let delegate = self.delegate + + OperationQueue.main.addOperation { [weak delegate] in + delegate?.listViewShouldEndQueueingEditsForReorder() } } @@ -390,6 +392,10 @@ final class CollectionViewLayout : UICollectionViewLayout self.changesDuringCurrentUpdate = UpdateItems(with: []) + guard let delegate = self.delegate else { + return + } + let size = self.collectionView?.bounds.size ?? .zero self.neededLayoutType.update(with: { @@ -402,18 +408,18 @@ final class CollectionViewLayout : UICollectionViewLayout case .none: return true case .relayout: - self.performLayout() + self.performLayout(delegate: delegate) case .rebuild: - self.performRebuild(andLayout: shouldLayout) + self.performRebuild(andLayout: shouldLayout, delegate: delegate) } return true }()) - self.performLayoutUpdate() + self.performLayoutUpdate(delegate: delegate) if self.isReordering == false { - self.delegate.listViewLayoutDidLayoutContents() + delegate.listViewLayoutDidLayoutContents() } } @@ -439,7 +445,7 @@ final class CollectionViewLayout : UICollectionViewLayout // MARK: Performing Layouts // - private func performRebuild(andLayout layout : Bool) + private func performRebuild(andLayout layout : Bool, delegate : CollectionViewLayoutDelegate) { self.previousLayout = self.layout @@ -447,7 +453,7 @@ final class CollectionViewLayout : UICollectionViewLayout appearance: self.appearance, behavior: self.behavior, content: { - self.delegate.listLayoutContent(defaults: $0) + delegate.listLayoutContent(defaults: $0) } ) @@ -459,34 +465,34 @@ final class CollectionViewLayout : UICollectionViewLayout ) if layout { - self.performLayout() + self.performLayout(delegate: delegate) } } - private func performLayout() + private func performLayout(delegate : CollectionViewLayoutDelegate) { let view = self.collectionView! let context = ListLayoutLayoutContext( collectionView: view, - environment: self.delegate.listViewLayoutCurrentEnvironment() + environment: delegate.listViewLayoutCurrentEnvironment() ) self.layout.performLayout( - with: self.delegate, + with: delegate, in: context ) self.viewProperties = CollectionViewLayoutProperties(collectionView: view) } - private func performLayoutUpdate() + private func performLayoutUpdate(delegate : CollectionViewLayoutDelegate) { let view = self.collectionView! let context = ListLayoutLayoutContext( collectionView: view, - environment: self.delegate.listViewLayoutCurrentEnvironment() + environment: delegate.listViewLayoutCurrentEnvironment() ) self.layout.positionStickyListHeaderIfNeeded(in: context) diff --git a/ListableUI/Sources/ListView/ListView.LayoutManager.swift b/ListableUI/Sources/ListView/ListView.LayoutManager.swift index b0d27b9a1..ab26477b0 100644 --- a/ListableUI/Sources/ListView/ListView.LayoutManager.swift +++ b/ListableUI/Sources/ListView/ListView.LayoutManager.swift @@ -45,8 +45,12 @@ extension ListView self.collectionViewLayout.setNeedsRebuild(animated: animated) } } else { + guard let delegate = self.collectionViewLayout.delegate else { + listableInternalFatal("Cannot swap layout when the previous layout's delegate has been deallocated.") + } + self.collectionViewLayout = CollectionViewLayout( - delegate: self.collectionViewLayout.delegate, + delegate: delegate, layoutDescription: layout, appearance: self.collectionViewLayout.appearance, behavior: self.collectionViewLayout.behavior diff --git a/ListableUI/Sources/ListView/ListView.swift b/ListableUI/Sources/ListView/ListView.swift index 651b6b30c..6d44541e6 100644 --- a/ListableUI/Sources/ListView/ListView.swift +++ b/ListableUI/Sources/ListView/ListView.swift @@ -151,6 +151,8 @@ public final class ListView : UIView self.collectionView.delegate = nil self.collectionView.dataSource = nil + + self.collectionView.removeFromSuperview() } @available(*, unavailable) @@ -1111,9 +1113,13 @@ public final class ListView : UIView public override func didMoveToWindow() { super.didMoveToWindow() - + if self.window != nil { self.updateScrollViewInsets() + } else { + self.collectionView.delegate = nil + self.collectionView.dataSource = nil + self.collectionView.removeFromSuperview() } } diff --git a/ListableUI/Tests/Layout/CollectionViewLayoutTests.swift b/ListableUI/Tests/Layout/CollectionViewLayoutTests.swift new file mode 100644 index 000000000..a9956d49b --- /dev/null +++ b/ListableUI/Tests/Layout/CollectionViewLayoutTests.swift @@ -0,0 +1,85 @@ +// +// CollectionViewLayoutTests.swift +// ListableUI-Unit-Tests +// +// Created by Alex Odawa on 5/2/26. +// + +import XCTest +@testable import ListableUI + + +final class CollectionViewLayoutTests : XCTestCase +{ + func test_prepare_whenDelegateHasBeenDeallocated() + { + weak var weakDelegate : CollectionViewLayoutDelegateMock? + + let layout : CollectionViewLayout = { + let delegate = CollectionViewLayoutDelegateMock() + weakDelegate = delegate + + return CollectionViewLayout( + delegate: delegate, + layoutDescription: .table(), + appearance: Appearance(), + behavior: Behavior() + ) + }() + + XCTAssertNil(weakDelegate) + XCTAssertNil(layout.delegate) + + layout.prepare() + } + + /// Exercises the real production path that motivates the weak-delegate fix: + /// `sendEndQueuingEditsAfterDelay` schedules a closure on `OperationQueue.main` + /// that fires on a later runloop turn. If the owning `ListView` (and therefore + /// the delegate) deallocates between scheduling and dispatch, the closure + /// previously trapped on the `unowned` delegate access. + func test_sendEndQueuingEditsAfterDelay_whenDelegateDeallocatesBeforeDispatch() + { + weak var weakDelegate : CollectionViewLayoutDelegateMock? + + autoreleasepool { + let delegate = CollectionViewLayoutDelegateMock() + weakDelegate = delegate + + let layout = CollectionViewLayout( + delegate: delegate, + layoutDescription: .table(), + appearance: Appearance(), + behavior: Behavior() + ) + + layout.sendEndQueuingEditsAfterDelay() + } + + XCTAssertNil(weakDelegate) + + let drained = expectation(description: "main queue drained") + OperationQueue.main.addOperation { drained.fulfill() } + wait(for: [drained], timeout: 1.0) + } +} + + +private final class CollectionViewLayoutDelegateMock : CollectionViewLayoutDelegate +{ + func listViewLayoutUpdatedItemPositions() {} + + func listLayoutContent( + defaults: ListLayoutDefaults + ) -> ListLayoutContent { + ListLayoutContent() + } + + func listViewLayoutCurrentEnvironment() -> ListEnvironment { + .empty + } + + func listViewLayoutDidLayoutContents() {} + + func listViewShouldEndQueueingEditsForReorder() {} +}