Attach shared workers through browser-level auto-attach - #424
Conversation
An attachToTarget session keeps an ended shared worker's DevTools host alive, and another client's browser-level Target.setAutoAttach then attaches to that host and crashes the browser process on stock Chromium. Chrome detaches auto-attached shared worker sessions when the worker ends, so attach them that way instead of through discovery.
The monitor attaches to a shared worker, the worker closes, and a new client sends browser-level Target.setAutoAttach. The browser must keep answering. Run it in the CDP telemetry browser regression step. Co-authored-by: hiroTamada <88675973+hiroTamada@users.noreply.github.com>
hiroTamada
left a comment
There was a problem hiding this comment.
reviewed — the attachment change looks sound. two follow-ups worth addressing:
server/lib/cdpmonitor/README.md:148-152— please update the discovery description: it still says shared workers are explicitly attached, but they now use browser-level auto-attach.server/lib/cdpmonitor/shared_worker_attach_chrome_e2e_test.go:51-64,81-83— consider distinguishing a failed or malformedTarget.getTargetsresponse from an absent worker, so the termination assertion only passes after a successful listing.
…isting The README now says shared workers attach through browser-level auto-attach. The regression test only treats the worker as ended after a successful Target.getTargets listing, not after a failed or malformed one.
|
Thanks for the review. I pushed a commit that addresses both points.
I reran the test locally on stock Chrome 153. It passes 5 out of 5 with the fix and still fails 3 out of 3 without it, and the failure is still at the setAutoAttach step. No change to the tracker code. |
…" (#432) Reverts #424 (`7cf9c46`). ## Why #424 made the telemetry monitor send a browser-level `Target.setAutoAttach` (filtered to `shared_worker`, `waitForDebuggerOnStart: false`) in every browser. Chromium treats *any* client with browser-level auto-attach as a debugger that wants new popups paused: `TargetHandler::ShouldThrottlePopups()` returns `auto_attach_` without consulting the filter or the wait flag. Every `window.open` popup is then created with `wait_for_debugger`, and the renderer sits in a nested pause loop (`WebDevToolsAgentImpl::WaitForDebuggerWhenShown`) until a session attached to the popup sends `Runtime.runIfWaitingForDebugger`. The monitor never attaches to popups, so it never resumes them. Clients that auto-attach and resume themselves (Playwright, Puppeteer) still work; anything else — live view input, the computer API, Selenium, raw CDP — gets a popup frozen at `about:blank` and an opener frozen with the "Debugger paused in another tab" infobar, since the pause disables input for the whole browsing context group. Reproduced on a fresh headful browser with no CDP client attached: one OS-level click on a sign-in-with-Google button, and also a plain `window.open('https://example.com')`. A single `Runtime.runIfWaitingForDebugger` on the popup target unfreezes it. `target="_blank"` links are unaffected. ## What this changes Straight `git revert`: the monitor goes back to `Target.attachToTarget` for shared workers, and the regression test and CI entry from #424 are removed with it, since they asserted the reverted behaviour. The crash #424 targeted (a new client's auto-attach walking an ended shared worker's DevTools host) remains covered by patch 0035 in the Chromium fork. ## Tests - `go build ./...`, `go vet`, and `go test ./lib/browsersurface ./lib/cdpmonitor` pass. - With the reverted tracker, a local real-Chromium check (monitor attached, no other client, opener calls `window.open`) has the popup navigate normally; on `main` the same check hangs inside `window.open`. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes CDP target attachment for telemetry shared workers and reintroduces the ended-worker DevTools host issue #424 addressed (mitigated separately in the Chromium fork). > > **Overview** > Reverts browser-level **`Target.setAutoAttach`** for **`shared_worker`** targets introduced in #424. Shared workers are tracked again through normal discovery and **`Target.attachToTarget`**, alongside service workers and OOPIFs; only dedicated **`worker`** targets stay parent/auto-attach only. > > The Chrome E2E regression **`TestNewClientAutoAttachAfterMonitoredSharedWorkerEnds`** and its CI filter entry are removed, and **`browsersurface`** unit tests no longer assert the shared-worker auto-attach path. **`cdpmonitor` README** is updated to match. > > This trades the #424 crash workaround for fixing a Chromium side effect where any browser-level auto-attach throttles **`window.open`** popups (frozen at `about:blank` until another client resumes them), which broke live view, computer API, and non-Playwright CDP clients. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 3b77c93. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> Co-authored-by: dcruzeneil2 <247271309+dcruzeneil2@users.noreply.github.com>
Summary
The telemetry monitor now attaches to shared workers with a browser-level
Target.setAutoAttachfiltered toshared_worker. Before this change it discovered each one and attached withTarget.attachToTarget. This PR also adds a real-Chromium regression test for the crash that the old behavior set up, and runs it in CI.Problem
When a shared worker ends, Chromium keeps its DevTools host alive with
worker_host_set to null for as long as anyattachToTargetsession holds it. The monitor held such a session, and only ever receivedInspector.targetCrashedfor it, neverdetachedFromTargetortargetDestroyed. So it kept every ended shared worker's host alive until Chromium restarted.A new client's browser-level
Target.setAutoAttachthen walks every live host, including the ended one. Playwright'sconnectOverCDPsends this first.SharedWorkerDevToolsAgentHost::AttachSessiondereferences the nullworker_host_, and the browser process segfaults, dropping every CDP connection. This null dereference has been in upstream Chromium since M143.Change
Tracker.StartsendsTarget.setAutoAttach {autoAttach: true, flatten: true, waitForDebuggerOnStart: false, filter: [{type: "shared_worker"}]}on the root session when the tracker tracksshared_worker. Only the telemetry monitor does; the WebMCP tracker is unaffected.trackNonPageTargetskipsshared_worker, the same way it already skips dedicated workers. Discovery still reports shared worker targets; they just aren't attached explicitly anymore.Target.detachedFromTarget, which the tracker already handles. It also attaches them as soon as the worker is created, so the monitor no longer misses a worker's first requests.Caveats
setAutoAttachitself. On stock Chromium, that request would crash the browser if some other client held anattachToTargetsession on an ended shared worker. That is far rarer than today, where the monitor holds one for every ended shared worker.attachToTargetand a worker with the same URL and name starts again, Chromium reuses the old host without notifying auto-attach clients. The monitor would not attach to the restarted worker until it reconnects.Tests
TestNewClientAutoAttachAfterMonitoredSharedWorkerEnds(new, real Chromium): the real monitor attaches to a shared worker, the worker closes, and a second client sendsTarget.setAutoAttach. The test asserts the browser keeps answering. It is added to the "Run CDP telemetry browser regressions" step so CI runs it.TestWorkerDiscoveryIsOptInnow checks that shared workers get exactly one browser-level auto-attach and noattachToTarget.Run locally on stock Chrome for Testing 153.0.8010.53, headless:
-race.KERNEL_CDPMONITOR_CHROME_E2E=1cdpmonitorsuite passes under-race, includingTestNetworkCaptureFromWorkers/shared_worker.go test -racepasses for./lib/browsersurface,./lib/cdpmonitorand./lib/webmcpclient.go vetandgofmtare clean.Note
Medium Risk
Changes CDP target attachment for shared workers in the telemetry monitor; wrong behavior could miss worker network events or reintroduce the browser crash on multi-client CDP.
Overview
Fixes a Chromium segfault when a second CDP client (e.g. Playwright
connectOverCDP) sends browser-levelTarget.setAutoAttachafter the telemetry monitor had held anattachToTargetsession on a shared worker that already ended. Those sessions kept stale DevTools hosts alive until restart.browsersurface.Trackernow registersTarget.setAutoAttachon the root session with ashared_workerfilter when that target type is tracked (telemetry only), andtrackNonPageTargetno longer callsattachToTargetfor shared workers—matching dedicated workers. Chrome can detach auto-attached sessions when the worker ends instead of leaving zombie hosts.Adds real-Chromium regression
TestNewClientAutoAttachAfterMonitoredSharedWorkerEnds, extendsTestWorkerDiscoveryIsOptInfor the new attach path, updatescdpmonitorREADME, and runs the new test in the CDP telemetry browser regressions CI step.Reviewed by Cursor Bugbot for commit 8447494. Bugbot is set up for automated code reviews on this repo. Configure here.