Repository navigation
Close Web Inspector before browser host teardown - #3835
Merged
Merged
Conversation
The WebKit crash occurs when cmux enters browser pane or window teardown while the inspector remains visible, so the regression locks the expected ordering at both close surfaces before changing production code. Constraint: Regression must use the production BrowserPanel and CmuxWebView path rather than a stripped-down WKWebView Confidence: high Scope-risk: narrow Tested: Not run locally; direct xcodebuild is forbidden for this issue Not-tested: CI failure evidence for the test-only commit is pending
WebKit can leave an inspector frontend observing key-window notifications after its parent inspector proxy starts closing. The fix establishes one synchronous teardown helper and invokes it from the pane, popup, main-window, and app-quit close paths before delegates and windows enter their normal teardown cascade. Constraint: Direct xcodebuild is forbidden for this issue; verification must use sanctioned reload and CI paths Rejected: Add delayed or defensive cleanup after windowWillClose | the crash happens inside the same AppKit close cascade Rejected: Only patch BrowserPanel.close | main-window close can deliver windowWillClose before pane teardown runs Confidence: high Scope-risk: moderate Directive: Keep WebViewInspectorTeardown synchronous; do not replace it with DispatchQueue, Task, or delayed retry cleanup Tested: git diff --check Not-tested: Local unit tests and app build pending sanctioned reload/CI workflow
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe PR adds synchronous Web Inspector teardown across panel, popup, main window close, and app termination paths. It introduces WebViewInspectorTeardown utilities and calls them from BrowserPanel, BrowserPopupWindowController, and AppDelegate; tests validate inspector close ordering. ChangesWeb Inspector Teardown on Window/Pane Close
Sequence Diagram(s)sequenceDiagram
participant User
participant BrowserPanel
participant BrowserPopupWindowController
participant AppDelegate
participant WebViewInspectorTeardown
User->>BrowserPanel: trigger pane close / popup close
BrowserPanel->>WebViewInspectorTeardown: closeInspector(for: webView)
BrowserPopupWindowController->>WebViewInspectorTeardown: closeAllInspectors(in: panel)
User->>AppDelegate: quit
AppDelegate->>WebViewInspectorTeardown: closeAllInspectors(in: NSApp.windows) (via helper)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Contributor
Greptile SummaryThis PR fixes a
Confidence Score: 5/5Safe to merge; the fix is narrowly scoped to closing the inspector synchronously before every window/pane/app teardown path and does not change any other AppKit or WebKit interaction. All three teardown entry points (pane, window, app) receive the synchronous close before AppKit begins tearing down; the regression tests verify the critical ordering invariant without timing dependencies on the close-count assertion; no existing lifecycle contracts are modified. No files require special attention beyond the cosmetic notes on AppDelegate.swift and BrowserPanel.swift. Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User / AppKit
participant WD as WindowDelegate / AppDelegate
participant TD as WebViewInspectorTeardown
participant WK as WebKit (_inspector SPI)
participant AK as AppKit close cascade
Note over U,AK: Pane close (BrowserPanel.close)
U->>WD: close()
WD->>TD: closeInspector(for: webView)
TD->>WK: inspector.close() [same runloop turn]
WK-->>TD: observer unregistered
WD->>AK: super.close() to windowWillClose
AK-->>U: window torn down safely
Note over U,AK: Main-window close (windowShouldClose)
U->>WD: windowShouldClose
WD->>TD: closeAllInspectors(in: window)
TD->>WK: inspector.close() [same runloop turn]
WK-->>TD: observer unregistered
WD-->>AK: return true to windowWillClose
AK-->>U: window torn down safely
Note over U,AK: App termination
U->>WD: applicationShouldTerminate
WD->>TD: closeAllInspectors(in: NSApp.windows)
TD->>WK: inspector.close() per window
WK-->>TD: all observers unregistered
WD-->>U: .terminateNow
U->>WD: applicationWillTerminate
WD->>TD: closeAllWebInspectorsBeforeAppTeardown (no-op)
Reviews (2): Last reviewed commit: "Preserve synchronous inspector teardown ..." | Re-trigger Greptile |
Comment on lines
+5901
to
+5904
| enum WebViewInspectorTeardown { | ||
| @discardableResult | ||
| static func closeAllInspectors(in window: NSWindow) -> Int { | ||
| assert(Thread.isMainThread) |
Contributor
There was a problem hiding this comment.
The
WebViewInspectorTeardown enum enforces main-thread execution through assert(Thread.isMainThread) at runtime, but the multi-window overload closeAllInspectors(in windows: [NSWindow]) has no assertion at all — it only inherits the check from the inner call. Without an @MainActor annotation on the enum, Swift 6 strict concurrency allows any non-isolated context to call these methods without a compiler error. The crash fix's correctness depends on inspector teardown happening synchronously on the main runloop turn; making that a compile-time guarantee rather than a runtime trap aligns with the fix's stated design invariant.
Suggested change
| enum WebViewInspectorTeardown { | |
| @discardableResult | |
| static func closeAllInspectors(in window: NSWindow) -> Int { | |
| assert(Thread.isMainThread) | |
| @MainActor | |
| enum WebViewInspectorTeardown { | |
| @discardableResult | |
| static func closeAllInspectors(in window: NSWindow) -> Int { | |
| assert(Thread.isMainThread) |
Rule Used: Flag new or materially worsened Swift 6 actor isol... (source)
Comment on lines
+2144
to
+2147
| override func setUp() { | ||
| super.setUp() | ||
| setenv("NSZombieEnabled", "YES", 1) | ||
| } |
Contributor
There was a problem hiding this comment.
setenv("NSZombieEnabled", "YES", 1) in the instance setUp() runs before every test case in the class and permanently enables zombie mode for the entire test-process lifetime — it cannot be undone once set. The class already has a class setUp() override, which is the correct home for a one-time process-wide setting. Repeated re-activation in the instance setUp() is redundant and signals to future readers that this is per-test scaffolding rather than a deliberate process-level choice, risking confusion about when zombie mode is actually active.
Suggested change
| override func setUp() { | |
| super.setUp() | |
| setenv("NSZombieEnabled", "YES", 1) | |
| } | |
| // NSZombieEnabled moved to class setUp() — zombie mode is process-global and | |
| // cannot be unset; activating it once at class setup is the correct scope. | |
| // See override class func setUp() above. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/BrowserConfigTests.swift`:
- Around line 2195-2207: The test currently advances the run loop before
asserting inspector.closeCount, which permits async teardown; update
testPaneCloseClosesVisibleInspectorSynchronouslyBeforeWebViewTeardown to assert
inspector.closeCount == 1 immediately after calling panel.close() (i.e., add an
immediate XCTAssertEqual(inspector.closeCount, 1) directly after panel.close())
to enforce synchronous behavior, then keep the existing spinRunLoopOneTick() and
subsequent XCTAssertFalse(panel.isDeveloperToolsVisible()) check to verify
post-teardown state.
- Around line 2144-2147: The test currently calls setenv("NSZombieEnabled",
"YES", 1) inside override func setUp(), which is ineffective after process
launch and mutates global state; remove the setenv call from setUp (and any
related teardown) and instead configure NSZombieEnabled as an environment
variable before the process starts (e.g. in the Xcode test scheme or the test
runner environment), ensuring test isolation and reliable zombie detection for
the process; search for setenv("NSZombieEnabled" or usages in setUp/tearDown to
locate and remove the change.
In `@Sources/Panels/BrowserPopupWindowController.swift`:
- Around line 282-285: The programmatic close path skips windowShouldClose(_:)
so inspectors for all webViews in the popup's window are not torn down; update
the closePopup() implementation to explicitly call
WebViewInspectorTeardown.closeAllInspectors(in: panel) (using the same
panel/window passed to panel.close()) before invoking panel.close(), ensuring
the same semantics as windowShouldClose(_:). Keep windowWillClose(_:)'s
closeInspector(for:) intact as a per-webView fallback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 86f8ad28-350d-4367-aa85-7bb7daf9693a
📒 Files selected for processing (4)
Sources/AppDelegate.swiftSources/Panels/BrowserPanel.swiftSources/Panels/BrowserPopupWindowController.swiftcmuxTests/BrowserConfigTests.swift
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 228390f. Configure here.
Review feedback exposed two gaps around the teardown contract: the regression allowed a runloop turn before observing the close, and popup programmatic closes could bypass windowShouldClose. This tightens the test to assert the close immediately, routes closePopup through the shared inspector teardown path, removes an ineffective runtime NSZombie environment mutation, and makes the helper main-actor isolated so the compiler preserves the UI-thread boundary. Constraint: WebKit inspector teardown must complete before AppKit starts host window teardown. Rejected: Rely on windowShouldClose for popup closePopup | panel.close skips that delegate path. Confidence: high Scope-risk: narrow Tested: git diff --check Tested: ./scripts/reload.sh --tag issue3828-reviewfix Tested: ./scripts/reload.sh --tag issue3828-reviewfix --launch Tested: Manual tagged app repro with Web Inspector open, parent close leaves no inspector window and no new DiagnosticReports crash. Not-tested: Local cmux-unit run per repo testing policy; CI must rerun on pushed commit.
This was referenced May 12, 2026
This was referenced May 14, 2026
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3828
Crash signature
cmux 0.64.3 can crash on the main thread when closing a window or pane containing a browser
WKWebViewwhose Web Inspector is open:Repro notes
Attempted the requested baseline command before code changes:
./scripts/reload.sh --tag issue-3828-web-inspector-crash-baseline --launch. The first attempt was interrupted by the shared build queue before launch; a retry was still waiting on the global reload lock, so I stopped only that queued retry before editing to avoid it launching a non-baseline build after changes. A fresh local crash report at~/Library/Logs/DiagnosticReports/cmux-2026-05-10-164628.ipsmatched the exact signature above, including the0x170read,WKInspectorWKWebView _handleWindowDidBecomeKey, andNSWindow.cmux_sendEventframe.Manual repro sequence used for the live smoke path:
DiagnosticReportsfor a new crash.Design memo
The fix closes Web Inspector synchronously before AppKit starts tearing down the parent host.
WebViewInspectorTeardown.closeAllInspectors(in:)walks the closing window's view tree for productionWKWebViewinstances and closes any visible or attached inspector through the same WebKit_inspectorobject cmux already uses for DevTools. The helper is called fromwindowShouldClosefor main windows, pane teardown, popup teardown, and app termination. It does not scheduleDispatchQueue,Task, or delayed retries, because the failing observer must be removed on the same runloop turn beforewindowWillClose:and key-window notifications cascade through WebKit.NSWindow.cmux_sendEventaudit: the swizzle only performs event routing, focus guard context, key timing, and folder-drag/window-move suppression before forwarding to AppKit. It does not perform synchronous teardown or pre-empt normal close ordering, so the fix belongs in the close lifecycle rather than the event swizzle.Regression tests
Two-commit structure:
8d43a77f3adds failing regression coverage only.228390fa7adds the synchronous inspector teardown fix.Added
@MainActortests inBrowserDeveloperToolsVisibilityPersistenceTeststhat use the productionBrowserPanel/CmuxWebViewpath and the same inspector SPI hook used by existing DevTools tests:testPaneCloseClosesVisibleInspectorSynchronouslyBeforeWebViewTeardowntestWindowCloseClosesContainedBrowserInspectorBeforeWindowWillCloseLocal unit tests were not run because this issue forbids direct
xcodebuild; CI is the validation path.Manual verification
applicationShouldTerminate/applicationWillTerminatehook; not exercised manually to preserve final launched dev app~/Library/Logs/DiagnosticReports/cmux-*.ipscmux-2026-05-10-164628.ipsafter launch/smoke interactionValidation
git diff --checkpassed.workflow-guard-tests,remote-daemon-tests,web-typecheck,web-db-migrations,activation-session.macos-debug-build,macos-release-build,macos-unit-tests.origin(manaflow-ai/cmux) only../scripts/reload.sh --tag issue-3828-web-inspector-crash --launchsucceeded after freeing this issue's stale baseline DerivedData and launchedcmux DEV issue-3828-web-inspector-crash.Note
Medium Risk
Touches window-close and app-termination paths and relies on WebKit private inspector SPI; incorrect ordering could regress close behavior or DevTools state, but changes are localized and covered by new tests.
Overview
Closes any open WebKit Web Inspector synchronously before AppKit begins tearing down windows, panes, or popups to prevent a crash during close.
Introduces
WebViewInspectorTeardownto walk a window’s view tree, find non-inspectorWKWebViews, and invoke the inspectorcloseSPI when visible/attached. This teardown is now triggered from main windowwindowShouldClose,BrowserPanel.close(), popup close/delegate hooks, and app quit/terminate (applicationShouldTerminate/applicationWillTerminate).Adds unit tests asserting inspectors are closed before
WKWebViewteardown and beforeNSWindow.willClosefires during window close.Reviewed by Cursor Bugbot for commit e1c31b8. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Bug Fixes
Tests