From 99bfb40e1794a6dd960cf6014ca9247b16e7b478 Mon Sep 17 00:00:00 2001 From: Alex Odawa Date: Sat, 2 May 2026 17:05:26 +0200 Subject: [PATCH 1/5] Fix CollectionViewLayout delegate lifetime Hold the layout delegate weakly instead of unowned so the layout no longer crashes if the delegate is deallocated before the layout. prepare() and the deferred reorder flush now snapshot the delegate locally; LayoutManager's layout-swap path uses listableInternalFatal if the previous layout's delegate is missing. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 2 + .../Sources/Layout/CollectionViewLayout.swift | 36 +++++++----- .../ListView/ListView.LayoutManager.swift | 6 +- .../Layout/CollectionViewLayoutTests.swift | 55 +++++++++++++++++++ 4 files changed, 83 insertions(+), 16 deletions(-) create mode 100644 ListableUI/Tests/Layout/CollectionViewLayoutTests.swift 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..c26e27649 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 @@ -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/Tests/Layout/CollectionViewLayoutTests.swift b/ListableUI/Tests/Layout/CollectionViewLayoutTests.swift new file mode 100644 index 000000000..a9956657e --- /dev/null +++ b/ListableUI/Tests/Layout/CollectionViewLayoutTests.swift @@ -0,0 +1,55 @@ +// +// 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() + } +} + + +private final class CollectionViewLayoutDelegateMock : CollectionViewLayoutDelegate +{ + func listViewLayoutUpdatedItemPositions() {} + + func listLayoutContent( + defaults: ListLayoutDefaults + ) -> ListLayoutContent { + ListLayoutContent() + } + + func listViewLayoutCurrentEnvironment() -> ListEnvironment { + .empty + } + + func listViewLayoutDidLayoutContents() {} + + func listViewShouldEndQueueingEditsForReorder() {} +} From 17556b3cc61938055d0fff509563058eebea7b5a Mon Sep 17 00:00:00 2001 From: Alex Odawa Date: Sat, 2 May 2026 22:09:46 +0200 Subject: [PATCH 2/5] Remove collection view from hierarchy during ListView deallocation During ListView.deinit, the embedded UICollectionView could remain in the view hierarchy briefly while its internal UICollectionViewData was being torn down. If a layout pass (e.g. from layoutBelowIfNeeded) hit the window during this window, UIKit would call layoutSubviews on the collection view, accessing the already-freed UICollectionViewData and crashing in objc_loadWeakRetained with a garbage weak ref. Explicitly removing the collection view from its superview in deinit ensures UIKit won't attempt to lay it out after ListView begins deallocation. --- ListableUI/Sources/ListView/ListView.swift | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ListableUI/Sources/ListView/ListView.swift b/ListableUI/Sources/ListView/ListView.swift index 651b6b30c..98b67ab60 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) From 0662da72ad49ec32468691befe3e7a009ab6a668 Mon Sep 17 00:00:00 2001 From: Alex Odawa Date: Sun, 3 May 2026 01:20:03 +0200 Subject: [PATCH 3/5] Add test for delegate deallocation during deferred dispatch Covers the OperationQueue.main path in sendEndQueuingEditsAfterDelay, which was the original crash motivating the weak-delegate fix. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../Sources/Layout/CollectionViewLayout.swift | 2 +- .../Layout/CollectionViewLayoutTests.swift | 30 +++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/ListableUI/Sources/Layout/CollectionViewLayout.swift b/ListableUI/Sources/Layout/CollectionViewLayout.swift index c26e27649..c6c1b334a 100644 --- a/ListableUI/Sources/Layout/CollectionViewLayout.swift +++ b/ListableUI/Sources/Layout/CollectionViewLayout.swift @@ -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. diff --git a/ListableUI/Tests/Layout/CollectionViewLayoutTests.swift b/ListableUI/Tests/Layout/CollectionViewLayoutTests.swift index a9956657e..a9956d49b 100644 --- a/ListableUI/Tests/Layout/CollectionViewLayoutTests.swift +++ b/ListableUI/Tests/Layout/CollectionViewLayoutTests.swift @@ -32,6 +32,36 @@ final class CollectionViewLayoutTests : XCTestCase 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) + } } From e2d6b7780f13b1b78e9d3ec7dd8f0aa85cf80785 Mon Sep 17 00:00:00 2001 From: Alex Odawa Date: Sun, 3 May 2026 15:19:21 +0200 Subject: [PATCH 4/5] Bust Bazel cache for ios-register CI From 3aa62e6311b61b485d4ad4c43d380ea10b5d2799 Mon Sep 17 00:00:00 2001 From: Alex Odawa Date: Sun, 3 May 2026 20:53:26 +0200 Subject: [PATCH 5/5] Move collection view cleanup to didMoveToWindow for earlier teardown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The removeFromSuperview in deinit fires too late — the ListView may still be retained while KIF traverses the view hierarchy during test teardown. Moving cleanup to didMoveToWindow(nil) detaches the collection view as soon as the ListView leaves the window, preventing UICollectionViewData from accessing a corrupted weak reference during layout. --- ListableUI/Sources/ListView/ListView.swift | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/ListableUI/Sources/ListView/ListView.swift b/ListableUI/Sources/ListView/ListView.swift index 98b67ab60..6d44541e6 100644 --- a/ListableUI/Sources/ListView/ListView.swift +++ b/ListableUI/Sources/ListView/ListView.swift @@ -1113,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() } }