From 3b77c931e8b2cc8adf77c6a3d53a11be6981ea35 Mon Sep 17 00:00:00 2001 From: dcruzeneil2 <247271309+dcruzeneil2@users.noreply.github.com> Date: Thu, 1 Oct 2026 21:08:20 +0000 Subject: [PATCH] Revert "Attach shared workers through browser-level auto-attach (#424)" This reverts commit 7cf9c4691844ab51e90181aa2baaf0d270207b9b. --- .github/workflows/server-test.yaml | 2 +- server/lib/browsersurface/sessions_test.go | 13 --- server/lib/browsersurface/targets.go | 3 +- server/lib/browsersurface/tracker.go | 13 --- server/lib/cdpmonitor/README.md | 6 +- .../shared_worker_attach_chrome_e2e_test.go | 101 ------------------ 6 files changed, 4 insertions(+), 134 deletions(-) delete mode 100644 server/lib/cdpmonitor/shared_worker_attach_chrome_e2e_test.go diff --git a/.github/workflows/server-test.yaml b/.github/workflows/server-test.yaml index 44f7e95c..6e38fd03 100644 --- a/.github/workflows/server-test.yaml +++ b/.github/workflows/server-test.yaml @@ -103,7 +103,7 @@ jobs: cache-dependency-path: server/go.sum - name: Run CDP telemetry browser regressions - run: go test -race ./lib/cdpmonitor -run '^(TestNetworkCapture|TestTelemetryConnection|TestProxyErrorE2E|TestNewClientAutoAttachAfterMonitoredSharedWorkerEnds)' -count=1 -v + run: go test -race ./lib/cdpmonitor -run '^(TestNetworkCapture|TestTelemetryConnection|TestProxyErrorE2E)' -count=1 -v working-directory: server env: KERNEL_CDPMONITOR_CHROME_E2E: "1" diff --git a/server/lib/browsersurface/sessions_test.go b/server/lib/browsersurface/sessions_test.go index a7637f4f..c4f2ea41 100644 --- a/server/lib/browsersurface/sessions_test.go +++ b/server/lib/browsersurface/sessions_test.go @@ -36,9 +36,6 @@ func TestWorkerDiscoveryIsOptIn(t *testing.T) { protocol.emitTarget("Target.attachedToTarget", map[string]any{ "sessionId": "worker-session", "targetInfo": targets[0], }) - protocol.emitTarget("Target.attachedToTarget", map[string]any{ - "sessionId": "shared-worker-session", "targetInfo": targets[1], - }) for _, target := range targets { // Repeated discovery must not create another session. protocol.emitTarget("Target.targetCreated", map[string]any{"targetInfo": target}) @@ -70,14 +67,6 @@ func TestWorkerDiscoveryIsOptIn(t *testing.T) { }, time.Second, time.Millisecond, "nested dedicated-worker discovery must be enabled") } protocol.mu.Lock() - browserAutoAttach := protocol.autoAttachCalls[""] - protocol.mu.Unlock() - if enabled { - require.Equal(t, 1, browserAutoAttach, "shared workers attach through browser-level auto-attach") - } else { - require.Zero(t, browserAutoAttach) - } - protocol.mu.Lock() calls := maps.Clone(protocol.attachCalls) types := slices.Clone(protocol.discoveredTypes) pageCalls := maps.Clone(protocol.pageEnableCalls) @@ -86,8 +75,6 @@ func TestWorkerDiscoveryIsOptIn(t *testing.T) { if enabled { if target.Type == "worker" { require.Zero(t, calls[target.TargetID], "dedicated workers attach only through their parent") - } else if target.Type == "shared_worker" { - require.Zero(t, calls[target.TargetID], "shared workers attach only through auto-attach") } else { require.Equal(t, 1, calls[target.TargetID]) } diff --git a/server/lib/browsersurface/targets.go b/server/lib/browsersurface/targets.go index f5efc130..c8698c16 100644 --- a/server/lib/browsersurface/targets.go +++ b/server/lib/browsersurface/targets.go @@ -115,8 +115,7 @@ func (t *Tracker) attachTarget(target targetInfo) error { func (t *Tracker) trackNonPageTarget(target targetInfo) { // Dedicated workers are attached through their parent's Target domain; // browser-wide enumeration/discovery does not reliably expose them. - // Shared workers are attached by browser-level auto-attach; see Start. - if target.Type == "worker" || target.Type == "shared_worker" { + if target.Type == "worker" { return } t.stateMu.Lock() diff --git a/server/lib/browsersurface/tracker.go b/server/lib/browsersurface/tracker.go index 40358599..f55d2a89 100644 --- a/server/lib/browsersurface/tracker.go +++ b/server/lib/browsersurface/tracker.go @@ -130,19 +130,6 @@ func (t *Tracker) Start(ctx context.Context) error { t.startErr = fmt.Errorf("start browser surface discovery: %w", err) return t.startErr } - // Chrome detaches auto-attached shared worker sessions when the worker - // ends. An attachToTarget session would instead keep the ended worker's - // DevTools host alive, and another client's browser-level auto-attach - // crashes stock Chromium on that host. - if t.tracksTarget("shared_worker") { - if _, err := t.protocol.Send(ctx, "Target.setAutoAttach", map[string]any{ - "autoAttach": true, "flatten": true, "waitForDebuggerOnStart": false, - "filter": []map[string]any{{"type": "shared_worker"}}, - }, ""); err != nil { - t.startErr = fmt.Errorf("start shared worker auto-attach: %w", err) - return t.startErr - } - } raw, err := t.protocol.Send(ctx, "Target.getTargets", nil, "") if err != nil { t.startErr = fmt.Errorf("list browser tabs: %w", err) diff --git a/server/lib/cdpmonitor/README.md b/server/lib/cdpmonitor/README.md index e6ecade3..78c4c793 100644 --- a/server/lib/cdpmonitor/README.md +++ b/server/lib/cdpmonitor/README.md @@ -145,10 +145,8 @@ CDP session, or tracker state is shared with WebMCP. Telemetry adds worker and background-page targets with `WithAdditionalTargets` and uses `WithoutLocations`: it receives attachment events without waiting for window lookup or frame-tree initialization, and enables its own capture domains. -The tracker explicitly discovers and attaches pages, OOPIFs, service workers, and -extension background pages. Shared workers attach through a browser-level -`Target.setAutoAttach` filtered to `shared_worker`, so Chrome detaches their sessions -when the worker ends. Dedicated workers require parent-session `Target.setAutoAttach`; +The tracker explicitly discovers and attaches pages, OOPIFs, shared workers, and +service workers (and extension background pages). Dedicated workers require parent-session `Target.setAutoAttach`; that subscription is also installed on worker sessions to discover nested workers. Only `worker` targets match this auto-attach filter, avoiding duplicate attachment of explicitly discovered OOPIFs. WebMCP's default tracker still tracks page/frame diff --git a/server/lib/cdpmonitor/shared_worker_attach_chrome_e2e_test.go b/server/lib/cdpmonitor/shared_worker_attach_chrome_e2e_test.go deleted file mode 100644 index a9b2e9e9..00000000 --- a/server/lib/cdpmonitor/shared_worker_attach_chrome_e2e_test.go +++ /dev/null @@ -1,101 +0,0 @@ -package cdpmonitor - -import ( - "context" - "encoding/json" - "fmt" - "net/http" - "net/http/httptest" - "os" - "testing" - "time" - - "github.com/stretchr/testify/require" -) - -// A client that connects after a monitored SharedWorker has ended must not -// crash the browser. Chromium keeps the worker's DevTools host for existing -// sessions, and browser-level auto-attach on that host dereferences the -// destroyed worker. -func TestNewClientAutoAttachAfterMonitoredSharedWorkerEnds(t *testing.T) { - if os.Getenv("KERNEL_CDPMONITOR_CHROME_E2E") == "" { - t.Skip("set KERNEL_CDPMONITOR_CHROME_E2E=1 to run real-Chromium worker tests") - } - ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) - defer cancel() - stub := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path == "/shared.js" { - w.Header().Set("Content-Type", "text/javascript") - fmt.Fprint(w, `self.onconnect = event => { - const port = event.ports[0]; port.onmessage = event => event.data === 'close' && self.close(); port.start(); - };`) - return - } - w.Header().Set("Content-Type", "text/html") - fmt.Fprint(w, "shared worker") - })) - defer stub.Close() - browserWS := launchChromium(t, ctx, findChromium(t)) - cdp := dialCDP(t, ctx, browserWS) - defer cdp.close() - m := New(&staticUpstream{url: browserWS}, newEventCollector().publishFn(), 99, discardLogger, nil) - require.NoError(t, m.Start(ctx)) - defer m.Stop() - targetID := cdp.call(t, ctx, "", "Target.createTarget", map[string]any{"url": stub.URL}).targetID(t) - sessionID := cdp.call(t, ctx, "", "Target.attachToTarget", map[string]any{"targetId": targetID, "flatten": true}).sessionID(t) - require.Eventually(t, func() bool { - return cdp.evalBool(ctx, sessionID, fmt.Sprintf(`location.href === %q && document.readyState === 'complete' && window.__kernelEventInjected === true`, stub.URL+"/")) - }, 5*time.Second, 50*time.Millisecond) - - // listed is false when the target list could not be read, so callers never - // mistake a failed listing for an absent worker. - sharedWorker := func() (listed, found, attached bool) { - raw, err := cdp.roundtrip(ctx, "", "Target.getTargets", nil) - if err != nil { - return false, false, false - } - var result struct { - Result struct { - Targets []struct { - Type string `json:"type"` - Attached bool `json:"attached"` - } `json:"targetInfos"` - } `json:"result"` - } - if json.Unmarshal(raw, &result) != nil || result.Result.Targets == nil { - return false, false, false - } - for _, target := range result.Result.Targets { - if target.Type == "shared_worker" { - return true, true, target.Attached - } - } - return true, false, false - } - evaluateNetworkScript(t, ctx, cdp, sessionID, `(() => { window.sharedWorker = new SharedWorker('/shared.js'); sharedWorker.port.start(); return true; })()`) - // The test client never attaches to the worker, so only the monitor can. - require.Eventually(t, func() bool { - _, _, attached := sharedWorker() - return attached - }, 5*time.Second, 50*time.Millisecond, "monitor did not attach to the shared worker") - - evaluateNetworkScript(t, ctx, cdp, sessionID, `(() => { sharedWorker.port.postMessage('close'); return true; })()`) - require.Eventually(t, func() bool { - listed, found, _ := sharedWorker() - return listed && !found - }, 5*time.Second, 50*time.Millisecond, "shared worker did not end") - - // Playwright's connectOverCDP sends this first. - client := dialCDP(t, ctx, browserWS) - defer client.close() - attachCtx, attachCancel := context.WithTimeout(ctx, 5*time.Second) - defer attachCancel() - _, err := client.roundtrip(attachCtx, "", "Target.setAutoAttach", map[string]any{ - "autoAttach": true, "waitForDebuggerOnStart": false, "flatten": true, - }) - require.NoError(t, err, "browser exited or did not answer Target.setAutoAttach") - versionCtx, versionCancel := context.WithTimeout(ctx, 5*time.Second) - defer versionCancel() - _, err = cdp.roundtrip(versionCtx, "", "Browser.getVersion", nil) - require.NoError(t, err, "browser stopped responding after a new client auto-attached") -}