Skip to content

Stop WindowAccessor storing a deallocating window - #14946

Merged
teamleaderleo merged 1 commit into
mainfrom
fix/window-accessor-deallocating-window
Sep 27, 2026
Merged

teamleaderleo merged 1 commit into
mainfrom
fix/window-accessor-deallocating-window

Conversation

@teamleaderleo

@teamleaderleo teamleaderleo commented Sep 27, 2026 •

Copy link
Copy Markdown
Collaborator

App-host shards on main abort with:

objc[97896]: Cannot form weak reference to instance (0x...) of class NSKVONotifying__TtCC9cmuxTests24WindowDragHandleHitTests...RecordingTitlebarActionWindow. It is possible that this object was over-released, or is in the process of deallocation.
*** Program crashed: Aborted

Symbolicated against the run's own product (run 36295033926, shard 5), the abort comes from a SwiftUI update:

NSViewRepresentable.updateNSView → WindowAccessor.updateNSView → the installWindowHandler closure → WindowAccessor.Coordinator.shouldInvoke → weak store → abort

updateNSView reads nsView.window. NSView.window does not retain, so during a SwiftUI update it can return a window that is already deallocating. The coordinator then stores it in weak var lastWindow, and the objc runtime aborts. The window here is a test window that a different test dropped, which is why the crash shows up in unrelated tests. An app window whose last owner lets go during an update hits the same path.

WindowObservingView now keeps the window it gets in viewDidMoveToWindow, where AppKit hands over a live one, in its own weak property, and updateNSView reads that. While the view is attached it is the same window as before. A deallocating window reads nil, so nothing is stored and no handler runs.

This is the pattern #14925 used for the titlebar accessory (TitlebarControlsAccessoryViewController.updateObservedWindowIfNeeded), which caused shard 4 of the same run. It is already on main.

Verification

  • Stack symbolicated with atos against cmux DEV.debug.dylib from that run's app-host-layer-app-cli artifact; the dylib UUID matches the crash.
  • No local build on this Mac. This PR's CI app-host shards exercise WindowAccessor through every window SwiftUI hosts.
  • There's no focused regression test: the abort needs AppKit to deallocate a window in the middle of a SwiftUI update, which can't be triggered on demand.

Other weak var ...: NSWindow? sites are safe as long as they take the window from viewDidMoveToWindow. If the abort shows up again after this merges, symbolicating that stack will show which site it came from.

🤖 Generated with Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.


Summary by cubic

Stops WindowAccessor from aborting when SwiftUI updates the view while its window is deallocating, which was crashing app-host shards with "Cannot form weak reference".

WindowAccessor now reads the window from a new weak property on WindowObservingView, set in viewDidMoveToWindow, instead of nsView.window during updateNSView. A deallocating window reads nil, so the coordinator never stores it and the handler doesn't run.

Written for commit 3be1bbc. Summary will update on new commits.

Review in cubic

SwiftUI can run WindowAccessor.updateNSView while the view's window is
deallocating. NSView.window does not retain, so updateNSView handed that
window to the coordinator, whose weak store aborted with "Cannot form
weak reference to instance ... of class NSKVONotifying_...". App-host
shards crashed this way on main (a test window, symbolicated from run
36295033926 shard 5).

The view now keeps the window it got in viewDidMoveToWindow, where AppKit
hands over a live one, in its own weak property, and updateNSView reads
that. A deallocating window reads nil there instead of aborting. This is
the pattern #14925 used for the titlebar accessory.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Sep 27, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 16 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: manaflow-ai/cmux/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: ce5e9877-bed3-4873-93f2-1ee5a3bf6475

📥 Commits

Reviewing files that changed from the base of the PR and between 75caaa5 and 3be1bbc.

📒 Files selected for processing (1)
  • Sources/WindowAccessor.swift

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@teamleaderleo
teamleaderleo merged commit 533a5b9 into main Sep 27, 2026
61 of 62 checks passed
@teamleaderleo
teamleaderleo deleted the fix/window-accessor-deallocating-window branch September 27, 2026 10:38
@github-actions

Copy link
Copy Markdown
Contributor

Merge receipt for 3be1bbcc68: every check was green at merge (16 verified; 14 skipped by policy). Full suite runs on main after merge.

rustybret pushed a commit to rustybret/bmux that referenced this pull request Sep 27, 2026
5090403 UI test frames: sample XCTest screen recordings; SIGKILL stuck prompts (manaflow-ai#14956)
9943115 Canvas: keep agent panes from moving the viewport; honor Reduce Motion (manaflow-ai#14939)
f873b5a Fix duplicate-instance handler terminating unrelated helpers (manaflow-ai#13845)
0c151d1 Open Settings panes at their natural top (manaflow-ai#14950)
cfdde0b cmux-tui: inject Claude hooks through a PATH shim, including under sr (manaflow-ai#14908)
320a966 ci: correct the producer rpath length in the relocation docstring (manaflow-ai#14947)
8f79066 Hover never outshouts selection; focus, badge, and feed pill edges (manaflow-ai#14941)
5617ac3 cmux-tui: publish the agent's session id on the agents roster (manaflow-ai#14904)
533a5b9 fix: stop WindowAccessor storing a deallocating window (manaflow-ai#14946)
9546e06 reloadp.sh: exclude only this build's own bundle from the stable check (manaflow-ai#14889)
e27f361 docs: say full-ci runs only selected cmuxUITests targets (manaflow-ai#14945)
28d1eaf ci: point restored products at their own package frameworks (manaflow-ai#14930)
75caaa5 Land hot-path sidebar, feed, palette and notification state changes in the next frame (manaflow-ai#14927)
6b58884 docs: tighten CLAUDE.md and CONTRIBUTING.md; move procedures to skills (manaflow-ai#14920)
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