Skip to content

Commit c3245d3

Browse files
Fix CollectionViewLayout delegate lifetime (#611)
## Problem `CollectionViewLayout` held its delegate as `unowned let`. In normal use the `ListView` ownership graph keeps the delegate alive for the layout's whole lifetime, but a layout swap (`LayoutManager.set(layout:)`) or test setup that constructs a layout in isolation can deallocate the delegate first, leaving a dangling `unowned` reference and a hard crash on next access. ## Fix Change `delegate` from `unowned let` to `private(set) weak var`, and tighten the surrounding call sites: - `prepare()` reads the delegate once at the top, returns early if it's gone, and threads the unwrapped reference through `performLayout`, `performRebuild`, and `performLayoutUpdate` to avoid per-call weak loads. - `init` keeps a non-optional delegate parameter; the weak storage already conveys "may vanish later," and the parameter keeps construction honest. - `LayoutManager.set(layout:)` uses `listableInternalFatal` if the outgoing layout's delegate has been deallocated, since silently dropping a layout swap is worse than crashing on a "should never happen" state. - `sendEndQueuingEditsAfterDelay()` snapshots the delegate locally and weak-captures it, so the closure neither pins `self` nor extends the delegate's lifetime past its natural end. ## Test Adds `CollectionViewLayoutTests.test_prepare_whenDelegateHasBeenDeallocated`, which constructs a layout with a mock delegate, drops the strong reference, asserts the weak references are nil, and calls `prepare()` to verify it doesn't crash. Verified locally that the test fails against `origin/main`'s source and passes with this PR's fix. **Before** (against `origin/main`'s `unowned let delegate`): ``` Test Case '-[ListableTests.CollectionViewLayoutTests test_prepare_whenDelegateHasBeenDeallocated]' started. Fatal error: Attempted to read an unowned reference but object 0x1027ed7a0 was already destroyed ** TEST FAILED ** ``` **After** (this PR): ``` Test Case '-[ListableTests.CollectionViewLayoutTests test_prepare_whenDelegateHasBeenDeallocated]' started. Test Case '-[ListableTests.CollectionViewLayoutTests test_prepare_whenDelegateHasBeenDeallocated]' passed (0.001 seconds). ** TEST SUCCEEDED ** ``` ## Risk Low. `ListView` retains its `Delegate` for its entire lifetime, so in normal flows the delegate is alive every time the layout uses it. The fix is defense-in-depth for layout swaps and isolated-construction paths. ### Checklist - [x] Ensure any public-facing changes are reflected in the [changelog](https://github.com/square/Listable/blob/main/CHANGELOG.md). Include them in the `Main` section. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent e3a2a6f commit c3245d3

4 files changed

Lines changed: 83 additions & 16 deletions

File tree

CHANGELOG.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@
22

33
### Fixed
44

5+
- Fixed a crash in `CollectionViewLayout` when the delegate was deallocated before the layout. The delegate is now held weakly rather than unowned.
6+
57
### Added
68

79
### Removed

ListableUI/Sources/Layout/CollectionViewLayout.swift

Lines changed: 21 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ final class CollectionViewLayout : UICollectionViewLayout
1414
// MARK: Properties
1515
//
1616

17-
unowned let delegate : CollectionViewLayoutDelegate
17+
private(set) weak var delegate : CollectionViewLayoutDelegate?
1818

1919
var layoutDescription : LayoutDescription
2020

@@ -221,8 +221,10 @@ final class CollectionViewLayout : UICollectionViewLayout
221221
/// processing any further updates 🥴.
222222
///
223223

224-
OperationQueue.main.addOperation {
225-
self.delegate.listViewShouldEndQueueingEditsForReorder()
224+
let delegate = self.delegate
225+
226+
OperationQueue.main.addOperation { [weak delegate] in
227+
delegate?.listViewShouldEndQueueingEditsForReorder()
226228
}
227229
}
228230

@@ -390,6 +392,10 @@ final class CollectionViewLayout : UICollectionViewLayout
390392

391393
self.changesDuringCurrentUpdate = UpdateItems(with: [])
392394

395+
guard let delegate = self.delegate else {
396+
return
397+
}
398+
393399
let size = self.collectionView?.bounds.size ?? .zero
394400

395401
self.neededLayoutType.update(with: {
@@ -402,18 +408,18 @@ final class CollectionViewLayout : UICollectionViewLayout
402408
case .none:
403409
return true
404410
case .relayout:
405-
self.performLayout()
411+
self.performLayout(delegate: delegate)
406412
case .rebuild:
407-
self.performRebuild(andLayout: shouldLayout)
413+
self.performRebuild(andLayout: shouldLayout, delegate: delegate)
408414
}
409415

410416
return true
411417
}())
412418

413-
self.performLayoutUpdate()
419+
self.performLayoutUpdate(delegate: delegate)
414420

415421
if self.isReordering == false {
416-
self.delegate.listViewLayoutDidLayoutContents()
422+
delegate.listViewLayoutDidLayoutContents()
417423
}
418424
}
419425

@@ -439,15 +445,15 @@ final class CollectionViewLayout : UICollectionViewLayout
439445
// MARK: Performing Layouts
440446
//
441447

442-
private func performRebuild(andLayout layout : Bool)
448+
private func performRebuild(andLayout layout : Bool, delegate : CollectionViewLayoutDelegate)
443449
{
444450
self.previousLayout = self.layout
445451

446452
self.layout = self.layoutDescription.configuration.createPopulatedLayout(
447453
appearance: self.appearance,
448454
behavior: self.behavior,
449455
content: {
450-
self.delegate.listLayoutContent(defaults: $0)
456+
delegate.listLayoutContent(defaults: $0)
451457
}
452458
)
453459

@@ -459,34 +465,34 @@ final class CollectionViewLayout : UICollectionViewLayout
459465
)
460466

461467
if layout {
462-
self.performLayout()
468+
self.performLayout(delegate: delegate)
463469
}
464470
}
465471

466-
private func performLayout()
472+
private func performLayout(delegate : CollectionViewLayoutDelegate)
467473
{
468474
let view = self.collectionView!
469475

470476
let context = ListLayoutLayoutContext(
471477
collectionView: view,
472-
environment: self.delegate.listViewLayoutCurrentEnvironment()
478+
environment: delegate.listViewLayoutCurrentEnvironment()
473479
)
474480

475481
self.layout.performLayout(
476-
with: self.delegate,
482+
with: delegate,
477483
in: context
478484
)
479485

480486
self.viewProperties = CollectionViewLayoutProperties(collectionView: view)
481487
}
482488

483-
private func performLayoutUpdate()
489+
private func performLayoutUpdate(delegate : CollectionViewLayoutDelegate)
484490
{
485491
let view = self.collectionView!
486492

487493
let context = ListLayoutLayoutContext(
488494
collectionView: view,
489-
environment: self.delegate.listViewLayoutCurrentEnvironment()
495+
environment: delegate.listViewLayoutCurrentEnvironment()
490496
)
491497

492498
self.layout.positionStickyListHeaderIfNeeded(in: context)

ListableUI/Sources/ListView/ListView.LayoutManager.swift

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,8 +45,12 @@ extension ListView
4545
self.collectionViewLayout.setNeedsRebuild(animated: animated)
4646
}
4747
} else {
48+
guard let delegate = self.collectionViewLayout.delegate else {
49+
listableInternalFatal("Cannot swap layout when the previous layout's delegate has been deallocated.")
50+
}
51+
4852
self.collectionViewLayout = CollectionViewLayout(
49-
delegate: self.collectionViewLayout.delegate,
53+
delegate: delegate,
5054
layoutDescription: layout,
5155
appearance: self.collectionViewLayout.appearance,
5256
behavior: self.collectionViewLayout.behavior
Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
//
2+
// CollectionViewLayoutTests.swift
3+
// ListableUI-Unit-Tests
4+
//
5+
// Created by Alex Odawa on 5/2/26.
6+
//
7+
8+
import XCTest
9+
@testable import ListableUI
10+
11+
12+
final class CollectionViewLayoutTests : XCTestCase
13+
{
14+
func test_prepare_whenDelegateHasBeenDeallocated()
15+
{
16+
weak var weakDelegate : CollectionViewLayoutDelegateMock?
17+
18+
let layout : CollectionViewLayout = {
19+
let delegate = CollectionViewLayoutDelegateMock()
20+
weakDelegate = delegate
21+
22+
return CollectionViewLayout(
23+
delegate: delegate,
24+
layoutDescription: .table(),
25+
appearance: Appearance(),
26+
behavior: Behavior()
27+
)
28+
}()
29+
30+
XCTAssertNil(weakDelegate)
31+
XCTAssertNil(layout.delegate)
32+
33+
layout.prepare()
34+
}
35+
}
36+
37+
38+
private final class CollectionViewLayoutDelegateMock : CollectionViewLayoutDelegate
39+
{
40+
func listViewLayoutUpdatedItemPositions() {}
41+
42+
func listLayoutContent(
43+
defaults: ListLayoutDefaults
44+
) -> ListLayoutContent {
45+
ListLayoutContent()
46+
}
47+
48+
func listViewLayoutCurrentEnvironment() -> ListEnvironment {
49+
.empty
50+
}
51+
52+
func listViewLayoutDidLayoutContents() {}
53+
54+
func listViewShouldEndQueueingEditsForReorder() {}
55+
}

0 commit comments

Comments
 (0)