Ade Browser Link Crashes#674
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
@copilot review but do not make fixes |
📝 WalkthroughWalkthroughAdds async renderer crash recovery to Built-in Browser Service
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@apps/desktop/src/main/services/builtInBrowser/builtInBrowserService.ts`:
- Around line 1167-1172: The builtInBrowserService logger currently records the
full browser URL in builtInBrowserService.ts, which can leak sensitive query
data; update the built_in_browser.render_process_gone payload to log a redacted
value or origin only instead of tab.webContents.getURL(), and keep the same
tabId/reason/exitCode fields. Make the change in the code path that uses
logger()?.warn and adjust the corresponding test expectation so it asserts the
sanitized URL shape rather than the full arbitrary URL.
- Around line 1065-1095: Recover the crashed WebContents instance directly
instead of re-reading tab.webContents after an await in builtInBrowserService’s
render-process recovery flow. Capture the current tab.webContents in a local
variable before calling stopInspectQuietly, then use that saved reference for
isDestroyed() checks and loadURL("about:blank") so attachWebview cannot swap in
a different WebContents and get blanked by mistake.
🪄 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: CHILL
Plan: Pro
Run ID: 03213df6-6b47-4c9d-a431-bdf199357525
📒 Files selected for processing (2)
apps/desktop/src/main/services/builtInBrowser/builtInBrowserService.test.tsapps/desktop/src/main/services/builtInBrowser/builtInBrowserService.ts
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 446f9a754a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (tab) { | ||
| const popupUrl = stringOrNull(url) ?? "about:blank"; | ||
| // Own popup navigation; Electron cannot attach an existing WebContentsView as a native guest window. | ||
| void tab.webContents.loadURL(popupUrl).catch((error) => { |
There was a problem hiding this comment.
When the popup request comes from a <form target="_blank" method="post">, Electron passes the POST payload/referrer on the window-open details, but this manual loadURL(popupUrl) turns the new tab navigation into a plain GET. Before this change, returning allow with createWindow let Electron drive the original popup navigation into the returned WebContents, including the form body; with the new path, login/payment flows that submit popups by POST will open the right tab with the wrong request.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 830c2415d2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| void tab.webContents.loadURL(popupUrl, popupLoadOptions).catch((error) => { | ||
| emitError(new Error(`Could not open browser popup: ${errorMessage(error)}`)); | ||
| emitStatus(); | ||
| }); | ||
| } | ||
| return { action: "deny" }; |
There was a problem hiding this comment.
When a page opens an OAuth/SSO popup with window.open or a target=_blank form and expects the returned WindowProxy/window.opener relationship, returning action: "deny" makes Chromium treat the popup as blocked while ADE separately loads the URL into an unrelated tab. That breaks popup-based providers such as Google sign-in flows that complete by posting back to or closing the opener; the previous createWindow path preserved that child-window relationship while still using ADE-managed webContents.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a6c4c25a1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const createTabState = (): BrowserTabState => { | ||
| configureBrowserSession(); | ||
| const browserWebPreferences = (overrides: Electron.WebPreferences = {}): Electron.WebPreferences => ({ | ||
| ...overrides, |
There was a problem hiding this comment.
When a visited page calls window.open(..., ..., "javascript=no,webviewTag=yes"), Electron passes those feature-derived values in options.webPreferences (docs), and this spread carries them into the ADE tab. That lets arbitrary page content change the popup tab's preferences, e.g. disabling JavaScript in an auth popup or enabling <webview> despite ADE's default of keeping it off; the previous createTabState() path ignored popup-supplied preferences. Please whitelist only the preferences ADE intentionally preserves, or drop these overrides entirely.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
Describe the change.
What Changed
Key files and behaviors.
Validation
How you tested.
Risks
Anything to watch.
Summary by CodeRabbit
New Features
Bug Fixes
Greptile Summary
This PR fixes two Electron browser crash issues: renderer process crashes now recover the tab to
about:blankinstead of leaving it in a broken state, and popup link handling now correctly creates tabs inside Electron'screateWindowcallback rather than pre-creating them insetWindowOpenHandler.recoverTabAfterRenderProcessGonewith idempotency viarenderProcessRecoveryTabsSet, proper cleanup in afinallyblock (notifyTabActivity+emitStatus), and clear distinction between a clean exit and a real crash.WebContentsViewcreation into thecreateWindowcallback and ignores caller-suppliedwebPreferencesin favor ofbrowserWebPreferences(), closing a potential security gap where a page could influence popup sandbox settings.Confidence Score: 5/5
Safe to merge — both crash recovery and popup handling are well-guarded with finally-block cleanup, idempotency, and secure web preferences enforced on every popup tab.
The crash recovery path uses a finally block that always calls notifyTabActivity and emitStatus regardless of success or failure, and the renderProcessRecoveryTabs Set prevents re-entrant recovery. The popup change correctly defers WebContentsView creation into createWindow and ignores caller-supplied web preferences, which is both the correct Electron API pattern and a hardening of the security boundary. Tests cover both code paths including the URL-redaction behaviour on the logger.
No files require special attention.
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant E as Electron participant H as render-process-gone handler participant R as recoverTabAfterRenderProcessGone participant WC as WebContents E->>H: render-process-gone (crashed, exitCode) H->>H: tabForWebContents(wc) → tab alt tab not found H->>H: emitStatus() else tab found H->>R: recoverTabAfterRenderProcessGone(tab, details) R->>R: check renderProcessRecoveryTabs / isDestroyed → early return if true alt "reason == clean-exit" R->>R: notifyTabActivity + emitStatus else real crash R->>R: renderProcessRecoveryTabs.add(tab.id) R->>R: clearPendingRequests + pushNetworkDiagnostic R->>WC: loadURL(about:blank) WC-->>R: resolved R->>R: emitError(Recovered the tab to a blank page) R->>R: [finally] renderProcessRecoveryTabs.delete + notifyTabActivity + emitStatus end end%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant E as Electron participant H as render-process-gone handler participant R as recoverTabAfterRenderProcessGone participant WC as WebContents E->>H: render-process-gone (crashed, exitCode) H->>H: tabForWebContents(wc) → tab alt tab not found H->>H: emitStatus() else tab found H->>R: recoverTabAfterRenderProcessGone(tab, details) R->>R: check renderProcessRecoveryTabs / isDestroyed → early return if true alt "reason == clean-exit" R->>R: notifyTabActivity + emitStatus else real crash R->>R: renderProcessRecoveryTabs.add(tab.id) R->>R: clearPendingRequests + pushNetworkDiagnostic R->>WC: loadURL(about:blank) WC-->>R: resolved R->>R: emitError(Recovered the tab to a blank page) R->>R: [finally] renderProcessRecoveryTabs.delete + notifyTabActivity + emitStatus end endReviews (5): Last reviewed commit: "Ignore untrusted popup web preferences" | Re-trigger Greptile