Repository navigation
Conversation
One oversized preview CDP event could exhaust V8 memory inside JSON.stringify in the desktop main process and take down the app with every thread it hosts (pingdotgg#20395). The relay now checks an upper bound on each message's serialized size before calling JSON.stringify. A reply over the bound becomes a CDP -32000 error for that command. An event over it closes the relay and detaches that tab, since dropping it would leave Playwright out of step with the page. Co-Authored-By: GPT-6.1 Sol <noreply@openai.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedOnly developers with an assigned seat can use this organization's usage-based review budget, and seats here are assigned manually. Ask an admin to assign a seat, or change the review continuation mode in Billing. Next included review available in 9 minutes. View limit details
Comment |
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| // Announcing the tab again would reconnect Playwright, and the page could | ||
| // replay the same oversized event, so the tab stays detached. | ||
| () => { | ||
| if (tab.relay === relay && tabs.get(keyOf(tab.key)) === tab) detach(tab.key); |
There was a problem hiding this comment.
🟠 High preview/DesktopBrowserHost.ts:115
After an oversized event, newly created iframe and worker targets remain paused, so page activity that depends on them hangs. This callback removes the tab and relay listener but leaves the shared debugger's Target.setAutoAttach configuration active, so attach events and resume commands are no longer handled; disable auto-attach and resume child sessions before abandoning the relay, without detaching the shared debugger.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/desktop/src/preview/DesktopBrowserHost.ts around line 115:
After an oversized event, newly created iframe and worker targets remain paused, so page activity that depends on them hangs. This callback removes the tab and relay listener but leaves the shared debugger's `Target.setAutoAttach` configuration active, so attach events and resume commands are no longer handled; disable auto-attach and resume child sessions before abandoning the relay, without detaching the shared debugger.
ApprovabilityVerdict: Would Approve Macroscope's review found this PR approvable — This is a focused desktop bug fix that prevents oversized CDP serialization from crashing the app and includes targeted coverage for reply and event handling. An unresolved high-severity finding separately flags cleanup of paused debugger targets after detachment. Needs human approval because:
Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more. |
Problem
When an agent drives a browser tab in the desktop app, the CDP relay calls
JSON.stringifyon every protocol event in the Electron main process, with no size limit. One very large event takes the app down. On macOS, the reporters in #20395 saw V8 abort with an out-of-memory error inside thatJSON.stringify, which also stops the bundled server and every thread. On Linux the same call throwsRangeError: Invalid string lengthout of the debugger listener. Electron then stops the main process at an error dialog, and every preview call times out.To reproduce on a production build, open an agent preview tab on a page that runs
console.log("\u0001".repeat(100_000_000)). JSON escapes each of those characters as six, which is past V8's string limit.Change
CdpRelaychecks an upper bound on each message's serialized size before it callsJSON.stringify. A string counts at its length plus quotes, or at six units per character when it holds a character JSON may escape. Every other value counts 24 units. The limit is 64 Mi UTF-16 code units, well under V8's string limit and far above ordinary CDP traffic. The check allocates nothing the size of the message.-32000"CDP payload too large" error, so only that command fails.DesktopBrowserHostdetaches that tab. Dropping the event would leave Playwright out of step with the page. Announcing the tab again would reconnect Playwright, and the page could replay the same event, so the tab stays detached until its page attaches again, for example when the tab is reopened. The page stays open, and other tabs keep working.Base64 has no escapable characters, so screenshots count at their length. A 16 MiB screenshot reply passes in the tests.
Scope and approval
Bug #20395. Maintainer triage confirmed the cause: the relay "serializes every preview event in the Electron main process with no size limit" (comment). This PR changes only the relay's serialization and the failing tab's detach path.
Verification
pnpm exec vp test run apps/desktop/src/preview/CdpRelay.test.ts apps/desktop/src/preview/DesktopBrowserHost.test.ts: 13 passed on the branch. With base'sCdpRelay.tsandDesktopBrowserHost.ts, the 3 new tests fail and the 10 existing ones pass.pnpm exec vp run check:changed bd6940beab: format, lint, knip, typecheck of both affected packages, and both test files pass.build:desktopof base and branch, isolated state, the repo's mock ACP agent as the provider. Through that agent's MCP session I calledpreview_openon a local page, thenpreview_evaluatewithlogLarge(100, 1, 1), which logs 100,000,000 U+0001 characters.RangeError: Invalid string lengthatJSON.stringify,send,Object.event,Debugger.onMessage.preview_evaluateand a laterpreview_statusboth timed out after 15 s.preview_evaluatefailed after 9 s with "Target page, context or browser has been closed", because the tab detached.preview_statusanswered at once, and the app switched threads on click.Base, 30 s after the event. Local install paths in the dialog are shortened to
…/dist-electron.Branch, 30 s after the same event: no dialog, and the page is still open in the corner preview.
The run logs for both builds.
The new tests against base sources, then on the branch.
Not checked:
JSON.stringifyand fails with the dialog instead.Fatal error in V8: v8::ToLocalChecked Empty MaybeLocal. The branch's relay never stringifies that message, so the crash happens elsewhere in the main process. I did not find where, and this PR does not cover that size.Prior PRs: none found for this fix. #21514 by @maria-rcks only edits comments in these files. Thanks to @Maxymvs for the report and to @kestereverts for confirming it on a later nightly.
Production fix and tests written with GPT-6.1 Sol and Claude Opus 5.5; PR description and images by Claude Opus 5.5, both in T3 Code Nightly.
🤖 Generated with GPT-6.1 Sol and Claude Opus 5.5 in T3 Code Nightly