Isolate browser WebKit process pools#4987
Open
lawrencecchen wants to merge 2 commits into
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThis PR gives each BrowserPanel its own WKProcessPool, threads that pool through all WebView creation and replacement code paths, adds a manual recovery state/API for WebContent process termination, adds a recovery UI and reload interception, and closes popup windows when their web content process terminates. ChangesPer-panel WebKit process isolation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning, 3 inconclusive)
✅ Passed checks (13 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
Greptile SummaryReplaces the app-wide shared
Confidence Score: 5/5Safe to merge; the process isolation and popup-close mechanics are correctly wired end-to-end, and the new recovery overlay path has targeted test coverage. The crash-isolation goal is achieved cleanly: the stored processPool constant is threaded through every makeWebView call site, popup configurations are overwritten with the opener's pool, and the popup termination delegate correctly tears down the window. The two P2 observations (two-flag recovery state and the missing blankURLString guard in the manual-recovery path) are edge cases with minimal user-visible impact in practice. Sources/Panels/BrowserPanel.swift — specifically the dual hasRecoverableWebContentTermination / pendingWebContentRecoveryURL state and the recovery navigation path around line 5019. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[WebContent Process Terminates] --> B{Is popup?}
B -- yes --> C[PopupNavigationDelegate\nwebViewWebContentProcessDidTerminate]
C --> D[closePopup → panel.close\n→ windowWillClose\n→ nil delegates]
B -- no --> E[BrowserPanel\nreplaceWebViewAfterContentProcessTermination]
E --> F{wasRenderable?}
F -- no blank tab --> G[clearWebContentTerminationRecovery\nno overlay shown]
F -- yes --> H[New blank WKWebView\ncreated with same processPool]
H --> I[hasRecoverableWebContentTermination = true\npendingWebContentRecoveryURL = restoreURL]
I --> J[Recovery overlay shown]
J --> K{User action}
K -- clicks Reload or toolbar reload --> L[recoverTerminatedWebContent\nclearWebContentTerminationRecovery]
L --> M{pendingWebContentRecoveryURL?}
M -- non-nil URL --> N[navigateWithoutInsecureHTTPPrompt]
M -- nil --> O[refreshNavigationAvailability\noverlay dismissed, no navigation]
Reviews (2): Last reviewed commit: "Isolate browser WebKit process pools" | Re-trigger Greptile |
lawrencecchen
force-pushed
the
task-webview-crash-isolation
branch
from
fddf122 to
99dfbd4
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 99dfbd4. Configure here.
| profileID: profileID, | ||
| websiteDataStore: websiteDataStore | ||
| websiteDataStore: websiteDataStore, | ||
| processPool: processPool |
There was a problem hiding this comment.
Context reset doesn't clear web content recovery state
Medium Severity
resetForWorkspaceContextChange resets the panel to a blank new-tab state (shouldRenderWebView = false, currentURL = nil, etc.) but never calls clearWebContentTerminationRecovery(). If a panel had hasRecoverableWebContentTermination == true when the context reset fires, the recovery overlay (with the "Reload" button) remains visible on top of an otherwise blank new-tab placeholder. The stale pendingWebContentRecoveryURL would also attempt to navigate to the old URL if the user clicks "Reload." Other reset paths like performNavigation and replaceWebViewPreservingState correctly clear this state.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 99dfbd4. Configure here.
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 `@Sources/Panels/BrowserPanel.swift`:
- Around line 4921-4922: The manual-recovery gate currently uses only
waitForManualRecovery && wasRenderable, which can mark an about:blank/URL-less
view as recoverable; update the shouldShowManualRecovery condition to also
verify there is a real navigation target from sessionNavigationHistorySnapshot()
(e.g., check history has a current entry/url or hasEntries/canNavigate flag)
before presenting recovery UI, and apply the same check in the other occurrence
(around lines referenced 4976-4984) so recoverTerminatedWebContent() only runs
when an actual recoverable URL exists.
- Around line 4914-4918: When building restoreURL in BrowserPanel.swift, prefer
the attempted URL if a provisional main-frame navigation is active: check
navigationDelegate?.isMainFrameProvisionalNavigationActive and, when true, place
navigationDelegate?.lastAttemptedURL (and its remoteProxyDisplayURL) before
currentURL in the fallback chain used to compute restoreURL; otherwise keep the
existing ordering. Update the restoreURL expression that calls
Self.remoteProxyDisplayURL(for:), currentURL,
navigationDelegate?.lastAttemptedURL, and resolvedCurrentSessionHistoryURL() to
branch on isMainFrameProvisionalNavigationActive and swap the attempted-URL
entries into the first fallback positions.
In `@Sources/Panels/BrowserPanelView.swift`:
- Around line 1661-1681: The recovery overlay's Label uses the localized key
"browser.error.reload" (seen in webContentRecoveryOverlay) but the Khmer locale
is missing in Resources/Localizable.xcstrings; add a "km" entry for
"browser.error.reload" with the correct Khmer translation (matching the format
used for other locales), ensure the string encoding/quoting matches existing
.xcstrings entries and include the plural/variant if applicable so the Label
shows Khmer instead of falling back to English.
🪄 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: 94a99af9-8c0f-45cf-9315-27954b49ca52
📒 Files selected for processing (6)
Sources/Panels/BrowserPanel.swiftSources/Panels/BrowserPanelView.swiftSources/Panels/BrowserPopupWindowController.swiftcmuxTests/BrowserConfigTests.swiftcmuxTests/BrowserPanelTests.swiftcmuxTests/GhosttyConfigTests.swift
Comment on lines
+4914
to
+4918
| let restoreURL = Self.remoteProxyDisplayURL(for: oldWebView.url) | ||
| ?? currentURL | ||
| ?? Self.remoteProxyDisplayURL(for: navigationDelegate?.lastAttemptedURL) | ||
| ?? navigationDelegate?.lastAttemptedURL | ||
| ?? resolvedCurrentSessionHistoryURL() |
There was a problem hiding this comment.
Prefer the attempted URL when termination interrupts a provisional load.
currentURL still points to the last committed page while a provisional navigation is in flight, so this ordering can recover page A after page B crashes during load. Use navigationDelegate?.lastAttemptedURL first when isMainFrameProvisionalNavigationActive is true.
Suggested fix
- let restoreURL = Self.remoteProxyDisplayURL(for: oldWebView.url)
- ?? currentURL
- ?? Self.remoteProxyDisplayURL(for: navigationDelegate?.lastAttemptedURL)
- ?? navigationDelegate?.lastAttemptedURL
- ?? resolvedCurrentSessionHistoryURL()
+ let attemptedURL = Self.remoteProxyDisplayURL(for: navigationDelegate?.lastAttemptedURL)
+ ?? navigationDelegate?.lastAttemptedURL
+ let liveURL = Self.remoteProxyDisplayURL(for: oldWebView.url)
+ ?? currentURL
+ let restoreURL = (isMainFrameProvisionalNavigationActive ? attemptedURL : nil)
+ ?? liveURL
+ ?? attemptedURL
+ ?? resolvedCurrentSessionHistoryURL()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let restoreURL = Self.remoteProxyDisplayURL(for: oldWebView.url) | |
| ?? currentURL | |
| ?? Self.remoteProxyDisplayURL(for: navigationDelegate?.lastAttemptedURL) | |
| ?? navigationDelegate?.lastAttemptedURL | |
| ?? resolvedCurrentSessionHistoryURL() | |
| let attemptedURL = Self.remoteProxyDisplayURL(for: navigationDelegate?.lastAttemptedURL) | |
| ?? navigationDelegate?.lastAttemptedURL | |
| let liveURL = Self.remoteProxyDisplayURL(for: oldWebView.url) | |
| ?? currentURL | |
| let restoreURL = (isMainFrameProvisionalNavigationActive ? attemptedURL : nil) | |
| ?? liveURL | |
| ?? attemptedURL | |
| ?? resolvedCurrentSessionHistoryURL() |
🤖 Prompt for 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.
In `@Sources/Panels/BrowserPanel.swift` around lines 4914 - 4918, When building
restoreURL in BrowserPanel.swift, prefer the attempted URL if a provisional
main-frame navigation is active: check
navigationDelegate?.isMainFrameProvisionalNavigationActive and, when true, place
navigationDelegate?.lastAttemptedURL (and its remoteProxyDisplayURL) before
currentURL in the fallback chain used to compute restoreURL; otherwise keep the
existing ordering. Update the restoreURL expression that calls
Self.remoteProxyDisplayURL(for:), currentURL,
navigationDelegate?.lastAttemptedURL, and resolvedCurrentSessionHistoryURL() to
branch on isMainFrameProvisionalNavigationActive and swap the attempted-URL
entries into the first fallback positions.
Comment on lines
+4921
to
4922
| let shouldShowManualRecovery = waitForManualRecovery && wasRenderable | ||
| let history = sessionNavigationHistorySnapshot() |
There was a problem hiding this comment.
Only surface manual recovery when there is an actual recovery target.
shouldShowManualRecovery only checks wasRenderable, so a terminated about:blank/URL-less view still becomes “recoverable”. recoverTerminatedWebContent() then swallows Reload, clears the flag, and leaves the replacement web view blank.
Suggested fix
- let shouldShowManualRecovery = waitForManualRecovery && wasRenderable
+ let shouldShowManualRecovery = waitForManualRecovery && shouldRestoreURL
@@
- if shouldShowManualRecovery {
- pendingWebContentRecoveryURL = restoreURL
+ if shouldShowManualRecovery, let restoreURL {
+ pendingWebContentRecoveryURL = restoreURL
hasRecoverableWebContentTermination = true
refreshNavigationAvailability()
} else {
clearWebContentTerminationRecovery()
}Also applies to: 4976-4984
🤖 Prompt for 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.
In `@Sources/Panels/BrowserPanel.swift` around lines 4921 - 4922, The
manual-recovery gate currently uses only waitForManualRecovery && wasRenderable,
which can mark an about:blank/URL-less view as recoverable; update the
shouldShowManualRecovery condition to also verify there is a real navigation
target from sessionNavigationHistorySnapshot() (e.g., check history has a
current entry/url or hasEntries/canNavigate flag) before presenting recovery UI,
and apply the same check in the other occurrence (around lines referenced
4976-4984) so recoverTerminatedWebContent() only runs when an actual recoverable
URL exists.
Comment on lines
+1661
to
+1681
| private var webContentRecoveryOverlay: some View { | ||
| ZStack { | ||
| Color(nsColor: browserChromeBackgroundColor) | ||
| .opacity(0.92) | ||
| Button(action: { | ||
| panel.recoverTerminatedWebContent(reason: "overlayButton") | ||
| }) { | ||
| Label( | ||
| String(localized: "browser.error.reload", defaultValue: "Reload"), | ||
| systemImage: "arrow.clockwise" | ||
| ) | ||
| .font(.system(size: 13, weight: .medium)) | ||
| .padding(.horizontal, 6) | ||
| } | ||
| .buttonStyle(.borderedProminent) | ||
| .controlSize(.regular) | ||
| .safeHelp(String(localized: "browser.reload", defaultValue: "Reload")) | ||
| .accessibilityIdentifier("BrowserWebContentRecoveryButton") | ||
| } | ||
| .frame(maxWidth: .infinity, maxHeight: .infinity) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import json
import pathlib
key = "browser.error.reload"
xcstrings_files = sorted(pathlib.Path(".").rglob("*.xcstrings"))
if not xcstrings_files:
print("No .xcstrings files found.")
raise SystemExit(0)
found = False
for path in xcstrings_files:
try:
data = json.loads(path.read_text())
except Exception as exc:
print(f"{path}: failed to parse JSON: {exc}")
continue
strings = data.get("strings", {})
if key not in strings:
continue
found = True
entry = strings[key]
key_locales = set((entry.get("localizations") or {}).keys())
catalog_locales = set()
for value in strings.values():
if isinstance(value, dict):
catalog_locales.update((value.get("localizations") or {}).keys())
missing = sorted(catalog_locales - key_locales)
print(f"{path}:")
print(f" key locales = {sorted(key_locales)}")
print(f" catalog locales = {sorted(catalog_locales)}")
print(f" missing locales = {missing}")
if not found:
print(f"Missing localization key: {key}")
PYRepository: manaflow-ai/cmux
Length of output: 422
Fix missing Khmer translation for browser.error.reload.
browser.error.reload is present in Resources/Localizable.xcstrings, but the km locale entry is missing—this recovery overlay will fall back to English for Khmer. Add a km translation for this key.
🤖 Prompt for 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.
In `@Sources/Panels/BrowserPanelView.swift` around lines 1661 - 1681, The recovery
overlay's Label uses the localized key "browser.error.reload" (seen in
webContentRecoveryOverlay) but the Khmer locale is missing in
Resources/Localizable.xcstrings; add a "km" entry for "browser.error.reload"
with the correct Khmer translation (matching the format used for other locales),
ensure the string encoding/quoting matches existing .xcstrings entries and
include the plural/variant if applicable so the Label shows Khmer instead of
falling back to English.
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.
Summary
WKProcessPoolso one bad page is less likely to take sibling browser panels down with itTesting
./scripts/reload.sh --tag webcrash./scripts/reload.sh --tag webcrash --launchhttp://127.0.0.1:18787/?cap=4096; it pushed the WebContent process to roughly 2.1 GiB RSS while cmux stayed around 430 MiB RSS.SIGKILLto the probe WebContent PID; cmux stayed alive, switching to a terminal tab worked, andecho cmux survived WebContent SIGKILLran successfully.cmux-aws-m4prohad only 363 MiB free on/after cleanup.Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Changes core WebKit lifecycle (process pools, crash recovery, popup teardown); behavior is well-tested but affects all browser tabs and popups.
Overview
Replaces the shared
WKProcessPoolacross browser panels with a per-panel pool so a WebContent crash is confined to that tab, while profileWKWebsiteDataStorestays shared for cookies/storage. Popups still use the opener’s pool viapopupBrowserContext.After WebContent termination, the panel no longer auto-reloads the page: it swaps in a new
WKWebView(same pool), setshasRecoverableWebContentTermination, and shows a Reload overlay;recoverTerminatedWebContent(toolbar reload, navigation, etc.) performs the restore. Popup windows close when their WebContent process terminates.Adds regression tests for separate pools, recovery on reload, and popup cleanup.
Reviewed by Cursor Bugbot for commit 99dfbd4. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Isolated each browser panel into its own WebKit process pool to contain crashes, and added a simple reload overlay to recover after WebContent termination. Popups stay in the opener’s context and auto-close if their WebContent process dies.
WKProcessPool, while sharing the profile/workspaceWKWebsiteDataStorefor cookies and storage.WKWebViewrebuilds, manual recovery via reload, and popup cleanup.Written for commit 99dfbd4. Summary will update on new commits.
Review in cubic
Summary by CodeRabbit
Bug Fixes
New Features
Tests