Repository navigation
fix(macos): order windows out on exit to prevent KVO crash - #820
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdded a new platform abstraction feature that orders out (hides) all application windows on macOS before application shutdown. The functionality is triggered during app exit via a cross-platform interface that delegates to platform-specific implementations. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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. Comment |
On macOS, winit's event handler teardown triggers a display cycle flush that finds _NSTouchBarFinderObservation KVO observers on objects in an inconsistent state, causing a SIGILL crash via _crashOnException:. Fix: call [window orderOut:nil] on all windows as the first action in on_exit(), before returning control to eframe/winit. This lets AppKit properly tear down its display-related observations while the windows and their views are still alive. Co-Authored-By: Claude Opus 4.6 <[email protected]>
ebf5e94 to
2a97da2
Compare
Review GateCommit:
|
There was a problem hiding this comment.
Pull request overview
Fixes a macOS-specific shutdown crash by hiding (ordering out) all AppKit windows before winit/eframe teardown, allowing AppKit to detach TouchBar-related KVO observers while the window/view graph is still valid.
Changes:
- Add
platform::order_out_all_windows()as a cross-platform (macOS-only implementation) shutdown helper. - Implement macOS window enumeration +
[window orderOut:nil]viaobjc2/msg_send!. - Invoke the new helper at the start of
AppState::on_exit().
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/platform/mod.rs | Adds a public, macOS-gated order_out_all_windows() entry point. |
| src/platform/macos.rs | Implements ordering out all NSApplication windows using raw AppKit selectors. |
| src/app.rs | Calls order_out_all_windows() first in on_exit() to mitigate macOS teardown crash. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…the process On a Mac with a Touch Bar, `_NSTouchBarFinder` observes `nextResponder` on every responder in the active window's chain — the window, its views, and the transient views AppKit puts there while editing — and retracts those observations later, from its own display-cycle block. When a retraction is not the first one for the same observation, Foundation raises `NSRangeException`, AppKit catches it and calls `-[NSApplication _crashOnException:]`, whose entire purpose is to abort: the process dies with SIGILL, with no Rust frame on the stack and nothing for us to catch. That abort is not caused by anything we release. Retiring the native window instead of releasing it, and a grace period before the release, were both shipped and both still aborted; the same exception is reported by apps that share neither GPUI nor our window lifetime, and one of them root-caused it to queued invalidations retracting an observation that was already gone: - longbridge/gpui-kit#3192 — intermittent, on switching the frontmost app - kodezine/RustyCAN#95 — root cause: queued invalidations, already-removed observer - dashpay/dash-evo-tool#820 — mitigated by ordering windows out before exit - johnlindquist/kit#1550 — Electron, reported on the window object - emilk/egui#2768 — eframe, on quitting the app - feigeCode/navop#268 and feigeCode/navop#308 — the reports this exists for `touch_bar_guard` takes over `-[NSApplication _crashOnException:]` and swallows exactly one exception: an `NSRangeException` whose reason names `_NSTouchBarFinderObservation`. Nothing is left to clean up in that case — the retraction it failed on had already happened, so the state AppKit wanted is the state it has. Every other exception is forwarded to the original implementation, which aborts exactly as AppKit intended. Release Notes: - Fixed a crash on Macs with a Touch Bar, where AppKit aborted the process for an exception its own Touch Bar finder raised while retracting its observations.
Summary
_NSTouchBarFinderObservationKVO observers on inconsistent objects[window orderOut:nil]on all windows as the first action inon_exit(), giving AppKit a chance to properly tear down display-related observations while windows and views are still aliveRoot cause
During shutdown, winit's event handler teardown triggers a display cycle flush (
CATransaction commit). This causes_NSTouchBarFinderObservation invalidateto remove KVO observers from objects that are already in an inconsistent state, throwing an NSException that AppKit catches and deliberately crashes on via_crashOnException:.Fix
By ordering all windows out before returning from
on_exit(), AppKit processes the window removal while everything is still alive and consistent — the KVO observers are properly detached during this orderly removal rather than during the chaotic winit teardown.Test plan
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit