From 905df6eec5ef10610b1ec5e6418a9c8fc26987e2 Mon Sep 17 00:00:00 2001 From: "Tommy D. Rossi" Date: Wed, 4 Mar 2026 21:57:27 +0100 Subject: [PATCH] fix: route Target.detachFromTarget on root CDP session (#40) When Playwright sends Target.detachFromTarget via the root browser session (no top-level sessionId), the extension couldn't find the target tab because it only checked msg.params.sessionId for routing. This caused 'No tab found' errors that cascaded into disconnects and instability. - Add getTabForCommand() helper with params.sessionId fallback so any command referencing a session in its params can be routed when the top-level sessionId is absent - No-op Target.detachFromTarget for stale/unknown sessions instead of throwing - Always re-apply tab group color on every sync to prevent Chrome resetting it to white - Replace silent .catch() with error log in aria-snapshot OOPIF detach - Add regression test using raw WebSocket to verify routing without sessionId Extension bumped to 0.0.74. --- extension/CHANGELOG.md | 8 ++ extension/manifest.json | 2 +- extension/src/background.ts | 99 ++++++++++++------ playwriter/src/aria-snapshot.ts | 4 +- playwriter/src/relay-navigation.test.ts | 132 ++++++++++++++++++++++++ 5 files changed, 212 insertions(+), 33 deletions(-) diff --git a/extension/CHANGELOG.md b/extension/CHANGELOG.md index e57d633..b93a2a1 100644 --- a/extension/CHANGELOG.md +++ b/extension/CHANGELOG.md @@ -1,5 +1,13 @@ # Changelog +## 0.0.74 + +### Bug Fixes + +- **Fix Target.detachFromTarget routing on root CDP session**: Commands sent without a top-level sessionId (e.g. from Playwright's root browser session) now resolve the target tab via `params.sessionId` fallback. Previously the extension threw "No tab found" which caused cascading disconnects and instability. (#40) +- **No-op stale Target.detachFromTarget**: Unknown or already-cleaned-up sessions return `{}` instead of throwing, preventing error cascading during rapid connect/disconnect cycles. +- **Always re-apply tab group color**: Tab group title and color are now re-applied on every sync to prevent Chrome from resetting them to white/unlabeled. + ## 0.0.73 ### Bug Fixes diff --git a/extension/manifest.json b/extension/manifest.json index a5a4d19..8b3b4f7 100644 --- a/extension/manifest.json +++ b/extension/manifest.json @@ -1,7 +1,7 @@ { "manifest_version": 3, "name": "Playwriter", - "version": "0.0.73", + "version": "0.0.74", "description": "Automate your Browser using Cursor, Claude, VS Code. More capable and context efficient than Playwright MCP.", "permissions": [ "alarms", diff --git a/extension/src/background.ts b/extension/src/background.ts index 2924af4..faa58d5 100644 --- a/extension/src/background.ts +++ b/extension/src/background.ts @@ -102,6 +102,9 @@ async function getExtensionIdentity(): Promise { return identityPromise } +const TAB_GROUP_COLOR: chrome.tabGroups.ColorEnum = 'green' +const TAB_GROUP_TITLE = 'playwriter' + let childSessions: Map = new Map() let nextSessionId = 1 let tabGroupQueue: Promise = Promise.resolve() @@ -722,7 +725,7 @@ async function syncTabGroup(): Promise { .map(([tabId]) => tabId) // Always query by title - no cached ID that can go stale - const existingGroups = await chrome.tabGroups.query({ title: 'playwriter' }) + const existingGroups = await chrome.tabGroups.query({ title: TAB_GROUP_TITLE }) // If no connected tabs, clear any existing playwriter groups if (connectedTabIds.length === 0) { @@ -771,12 +774,17 @@ async function syncTabGroup(): Promise { if (tabsToAdd.length > 0) { if (groupId === undefined) { const newGroupId = await chrome.tabs.group({ tabIds: tabsToAdd }) - await chrome.tabGroups.update(newGroupId, { title: 'playwriter', color: 'green' }) + await chrome.tabGroups.update(newGroupId, { title: TAB_GROUP_TITLE, color: TAB_GROUP_COLOR }) logger.debug('Created tab group:', newGroupId, 'with tabs:', tabsToAdd) } else { await chrome.tabs.group({ tabIds: tabsToAdd, groupId }) + await chrome.tabGroups.update(groupId, { title: TAB_GROUP_TITLE, color: TAB_GROUP_COLOR }) logger.debug('Added tabs to existing group:', tabsToAdd) } + } else if (groupId !== undefined) { + // No tabs to add, but ensure the existing group keeps the right color/title. + // Chrome can reset these on group collapse/expand or tab moves. + await chrome.tabGroups.update(groupId, { title: TAB_GROUP_TITLE, color: TAB_GROUP_COLOR }) } } catch (error: any) { logger.debug('Failed to sync tab group:', error.message) @@ -820,37 +828,61 @@ function emitChildDetachesForTab(tabId: number): void { }) } +// Resolve which tab a CDP command targets by checking sessionId sources in priority order: +// 1. Top-level sessionId (the CDP session the command was sent on) +// 2. params.sessionId (e.g. Target.detachFromTarget on the root session, see #40) +// 3. params.targetId (e.g. Target.closeTarget) +function getTabForCommand(msg: ExtensionCommandMessage): { tabId: number; tab: TabInfo } | undefined { + const sessionId = msg.params.sessionId + if (sessionId) { + const found = getTabBySessionId(sessionId) + if (found) { + return found + } + const child = childSessions.get(sessionId) + if (child) { + const tab = store.getState().tabs.get(child.tabId) + if (tab) { + return { tabId: child.tabId, tab } + } + } + } + + const paramsSessionId = + msg.params.params && 'sessionId' in msg.params.params && typeof msg.params.params.sessionId === 'string' + ? msg.params.params.sessionId + : undefined + if (paramsSessionId) { + const found = getTabBySessionId(paramsSessionId) + if (found) { + return found + } + const child = childSessions.get(paramsSessionId) + if (child) { + const tab = store.getState().tabs.get(child.tabId) + if (tab) { + return { tabId: child.tabId, tab } + } + } + } + + const targetId = + msg.params.params && 'targetId' in msg.params.params && typeof msg.params.params.targetId === 'string' + ? msg.params.params.targetId + : undefined + if (targetId) { + return getTabByTargetId(targetId) + } + + return undefined +} + async function handleCommand(msg: ExtensionCommandMessage): Promise { if (msg.method !== 'forwardCDPCommand') return - let targetTabId: number | undefined - let targetTab: TabInfo | undefined - - if (msg.params.sessionId) { - const found = getTabBySessionId(msg.params.sessionId) - if (found) { - targetTabId = found.tabId - targetTab = found.tab - } - } - - if (!targetTab && msg.params.sessionId) { - const childSession = childSessions.get(msg.params.sessionId) - if (childSession) { - targetTabId = childSession.tabId - targetTab = store.getState().tabs.get(childSession.tabId) - logger.debug('Found parent tab for child session:', msg.params.sessionId, 'tabId:', childSession.tabId) - } - } - - if (!targetTab && msg.params.params && 'targetId' in msg.params.params && msg.params.params.targetId) { - const found = getTabByTargetId(msg.params.params.targetId as string) - if (found) { - targetTabId = found.tabId - targetTab = found.tab - logger.debug('Found tab for targetId:', msg.params.params.targetId, 'tabId:', targetTabId) - } - } + const resolved = getTabForCommand(msg) + let targetTabId = resolved?.tabId + let targetTab = resolved?.tab const debuggee = targetTabId ? { tabId: targetTabId } : undefined @@ -936,6 +968,11 @@ async function handleCommand(msg: ExtensionCommandMessage): Promise { } if (!debuggee || !targetTab) { + // Target.detachFromTarget is best-effort — no-op if the session is already gone (#40). + if (msg.params.method === 'Target.detachFromTarget') { + return {} + } + throw new Error( `No tab found for method ${msg.params.method} sessionId: ${msg.params.sessionId} params: ${JSON.stringify(msg.params.params || null)}`, ) @@ -1601,7 +1638,7 @@ chrome.tabs.onUpdated.addListener((tabId, changeInfo, tab) => { tabGroupQueue = tabGroupQueue .then(async () => { // Query for playwriter group by title - no stale cached ID - const existingGroups = await chrome.tabGroups.query({ title: 'playwriter' }) + const existingGroups = await chrome.tabGroups.query({ title: TAB_GROUP_TITLE }) const groupId = existingGroups[0]?.id if (groupId === undefined) { return diff --git a/playwriter/src/aria-snapshot.ts b/playwriter/src/aria-snapshot.ts index cdee731..c2ff677 100644 --- a/playwriter/src/aria-snapshot.ts +++ b/playwriter/src/aria-snapshot.ts @@ -1260,7 +1260,9 @@ export async function getAriaSnapshot({ }, scopeAttr) } if (oopifSessionId) { - await session.send('Target.detachFromTarget', { sessionId: oopifSessionId }).catch(() => {}) + await session.send('Target.detachFromTarget', { sessionId: oopifSessionId }).catch((e) => { + console.error('[aria-snapshot] Failed to detach OOPIF session:', oopifSessionId, e) + }) } if (!cdp) { await session.detach() diff --git a/playwriter/src/relay-navigation.test.ts b/playwriter/src/relay-navigation.test.ts index 015a036..84ff0ec 100644 --- a/playwriter/src/relay-navigation.test.ts +++ b/playwriter/src/relay-navigation.test.ts @@ -1,5 +1,6 @@ import { describe, it, expect, beforeAll, afterAll } from 'vitest' import { chromium, type Page } from '@xmorse/playwright-core' +import WebSocket from 'ws' import { getCdpUrl } from './utils.js' import { setupTestContext, @@ -625,4 +626,135 @@ describe('Relay Navigation Tests', () => { fs.unlinkSync(outputPath) fs.unlinkSync(demoPath) }, 60000) + + // Regression test for https://github.com/remorses/playwriter/issues/40 + // When Playwright sends Target.detachFromTarget on the root CDP session (no top-level + // sessionId), the extension must still route the command by looking at params.sessionId. + // Previously the extension threw "No tab found for method Target.detachFromTarget" + // because it only checked the top-level sessionId for routing, which is absent on root + // session commands. This caused cascading disconnects and instability. + it('should route Target.detachFromTarget without top-level sessionId (issue #40)', async () => { + const browserContext = getBrowserContext() + const serviceWorker = await getExtensionServiceWorker(browserContext) + + const server = await createSimpleServer({ + routes: { '/': 'detach test' }, + }) + + const page = await browserContext.newPage() + try { + await page.goto(server.baseUrl, { waitUntil: 'domcontentloaded' }) + await page.bringToFront() + + await withTimeout({ + promise: serviceWorker.evaluate(async () => { + await globalThis.toggleExtensionForActiveTab() + }), + timeoutMs: 5000, + errorMessage: 'Timed out toggling extension for detach test', + }) + await new Promise((r) => { + setTimeout(r, 400) + }) + + // Connect a raw WebSocket to the relay — this lets us send CDP messages + // exactly as they appear on the wire, without Playwright adding sessionId. + const ws = new WebSocket(`ws://localhost:${TEST_PORT}/cdp/test-detach-raw`) + await new Promise((resolve, reject) => { + ws.on('open', () => { + resolve() + }) + ws.on('error', reject) + }) + + let nextId = 1 + const sendCdp = (msg: Record): Promise => { + return new Promise((resolve, reject) => { + const id = nextId++ + const timeout = setTimeout(() => { + ws.off('message', handler) + reject(new Error(`CDP response timeout for id ${id}`)) + }, 5000) + + const handler = (data: WebSocket.RawData) => { + const parsed = JSON.parse(data.toString()) + if (parsed.id === id) { + ws.off('message', handler) + clearTimeout(timeout) + resolve(parsed as T) + } + } + ws.on('message', handler) + ws.send(JSON.stringify({ id, ...msg })) + }) + } + + // Collect async events from the relay + const events: Array<{ method: string; params: Record; sessionId?: string }> = [] + ws.on('message', (data) => { + const msg = JSON.parse(data.toString()) + if (!msg.id && msg.method) { + events.push(msg) + } + }) + + // Trigger Target.setAutoAttach so the relay sends Target.attachedToTarget for + // all connected tabs. This gives us the page's pw-tab-* sessionId. + await sendCdp({ + method: 'Target.setAutoAttach', + params: { autoAttach: true, waitForDebuggerOnStart: false, flatten: true }, + }) + + // Wait for events to arrive + await new Promise((r) => { + setTimeout(r, 500) + }) + + // Filter for the specific page target by URL to avoid grabbing wrong sessions + // (welcome tab, extension pages, etc.) + type AttachParams = { sessionId?: string; targetInfo?: { type?: string; url?: string } } + const attachEvent = events.find((e) => { + if (e.method !== 'Target.attachedToTarget') { + return false + } + const p = e.params as AttachParams + return p.targetInfo?.type === 'page' && p.targetInfo?.url?.startsWith(server.baseUrl) + }) + expect(attachEvent).toBeDefined() + const pageSessionId = (attachEvent!.params as AttachParams).sessionId + expect(pageSessionId).toBeTruthy() + + // Verify the session is usable before detach — send a command that requires routing. + const evalBefore = await sendCdp<{ id: number; error?: { message: string }; result?: unknown }>({ + method: 'Runtime.evaluate', + sessionId: pageSessionId, + params: { expression: '1 + 1', returnByValue: true }, + }) + expect(evalBefore.error).toBeUndefined() + expect((evalBefore.result as { result?: { value?: number } })?.result?.value).toBe(2) + + // NOW: send Target.detachFromTarget WITHOUT a top-level sessionId. + // This is the exact wire format Playwright uses when sending on the root session + // (e.g. from CRSession.detach() where _parentSession is the root browser session). + // The extension must route this by looking at params.sessionId. + const detachResult = await sendCdp<{ id: number; error?: { message: string }; result?: unknown }>({ + method: 'Target.detachFromTarget', + // Intentionally NO sessionId field — this is the root session + params: { sessionId: pageSessionId }, + }) + + // Must not fail with extension routing error — the command must reach Chrome. + // Chrome returns "No session with given id" because pw-tab-* is a virtual session + // managed by the relay, not a real Chrome CDP session. This is expected — the key + // proof is that the extension routed the command to Chrome instead of throwing + // "No tab found" at the routing layer. + expect(detachResult.error?.message).not.toContain('No tab found') + expect(detachResult.error?.message).toContain('No session with given id') + + ws.close() + } finally { + await page.close() + await server.close() + } + }, 30000) })