Skip to content

Fix CollectionViewLayout delegate lifetime#611

Merged
RoyalPineapple merged 1 commit intomainfrom
RoyalPineapple/fix-collection-layout-delegate-lifetime
May 2, 2026
Merged

Fix CollectionViewLayout delegate lifetime#611
RoyalPineapple merged 1 commit intomainfrom
RoyalPineapple/fix-collection-layout-delegate-lifetime

Conversation

@RoyalPineapple
Copy link
Copy Markdown
Collaborator

@RoyalPineapple RoyalPineapple commented May 2, 2026

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

  • Ensure any public-facing changes are reflected in the changelog. Include them in the Main section.

@RoyalPineapple RoyalPineapple force-pushed the RoyalPineapple/fix-collection-layout-delegate-lifetime branch from 2fcf7ae to 4703f72 Compare May 2, 2026 15:05
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) <noreply@anthropic.com>
@RoyalPineapple RoyalPineapple force-pushed the RoyalPineapple/fix-collection-layout-delegate-lifetime branch from 4703f72 to 99bfb40 Compare May 2, 2026 15:11
@RoyalPineapple RoyalPineapple marked this pull request as ready for review May 2, 2026 15:14
@RoyalPineapple RoyalPineapple enabled auto-merge (squash) May 2, 2026 15:14
@RoyalPineapple RoyalPineapple merged commit c3245d3 into main May 2, 2026
10 checks passed
@RoyalPineapple RoyalPineapple deleted the RoyalPineapple/fix-collection-layout-delegate-lifetime branch May 2, 2026 15:17
@RoyalPineapple RoyalPineapple restored the RoyalPineapple/fix-collection-layout-delegate-lifetime branch May 2, 2026 15:21
RoyalPineapple added a commit that referenced this pull request May 2, 2026
RoyalPineapple added a commit that referenced this pull request May 2, 2026
Reverts #611 Which was accidentally automerged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant