From 67d017237e75f302c28af8fd1d4b01086bda5ae3 Mon Sep 17 00:00:00 2001 From: "Tommy D. Rossi" Date: Tue, 27 Jan 2026 16:05:15 +0100 Subject: [PATCH] fix(cdp): isolate extension builds and keep oopif auto-attach --- .gitignore | 1 + AGENTS.md | 2 +- PLAYWRITER_AGENTS.md | 2 +- extension/src/background.ts | 36 +++++++++ extension/vite.config.mts | 9 ++- playwriter/src/cdp-relay.ts | 6 ++ playwriter/src/mcp.test.ts | 153 ++++++++++++++++++++++++++++++++++- playwriter/src/test-utils.ts | 32 +++++--- 8 files changed, 224 insertions(+), 17 deletions(-) diff --git a/.gitignore b/.gitignore index df493f3..ead5d14 100644 --- a/.gitignore +++ b/.gitignore @@ -1,6 +1,7 @@ node_modules .vercel dist +extension/dist* esm .DS_Store *.tsbuildinfo diff --git a/AGENTS.md b/AGENTS.md index cfbd05a..76afa13 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -130,7 +130,7 @@ each test should reset the extension connection. NEVER call `browser.close()` in remember: toggling extension on a tab adds it to available pages. if you toggle then call `context.newPage()`, you'll have 2 pages. -IMPORTANT: set bash timeout to at least 90000ms when running `pnpm test` +IMPORTANT: set bash timeout to at least 300000ms (5 minutes) when running `pnpm test` to debug test failures, inspect the relay server log file. during tests, logs are written to `./relay-server.log` in the playwriter folder (not the system temp directory). contains extension, MCP and WS server logs with all CDP events. diff --git a/PLAYWRITER_AGENTS.md b/PLAYWRITER_AGENTS.md index ac0c650..9e8a2c1 100644 --- a/PLAYWRITER_AGENTS.md +++ b/PLAYWRITER_AGENTS.md @@ -128,7 +128,7 @@ each test should reset the extension connection. NEVER call `browser.close()` in remember: toggling extension on a tab adds it to available pages. if you toggle then call `context.newPage()`, you'll have 2 pages. -IMPORTANT: set bash timeout to at least 90000ms when running `pnpm test` +IMPORTANT: set bash timeout to at least 300000ms (5 minutes) when running `pnpm test` to debug test failures, inspect the relay server log file. during tests, logs are written to `./relay-server.log` in the playwriter folder (not the system temp directory). contains extension, MCP and WS server logs with all CDP events. diff --git a/extension/src/background.ts b/extension/src/background.ts index 12ebecf..bd96057 100644 --- a/extension/src/background.ts +++ b/extension/src/background.ts @@ -23,6 +23,9 @@ function sleep(ms: number): Promise { let childSessions: Map = new Map() let nextSessionId = 1 let tabGroupQueue: Promise = Promise.resolve() +// Cache Target.setAutoAttach params so existing and future tabs enable OOPIF target events. +// This ensures Playwright can build the iframe frame tree when connecting over CDP. +let autoAttachParams: Protocol.Target.SetAutoAttachRequest | null = null class ConnectionManager { ws: WebSocket | null = null @@ -673,6 +676,30 @@ async function handleCommand(msg: ExtensionCommandMessage): Promise { const debuggee = targetTabId ? { tabId: targetTabId } : undefined + // Root-level Target.setAutoAttach must apply to all connected tabs since + // CDP auto-attach is per-debugger-session. Without this, OOPIF targets never attach. + if (msg.params.method === 'Target.setAutoAttach' && !msg.params.sessionId) { + const params = msg.params.params as Protocol.Target.SetAutoAttachRequest | undefined + if (!params) { + return {} + } + + autoAttachParams = params + const connectedTabIds = Array.from(store.getState().tabs.entries()) + .filter(([_, info]) => info.state === 'connected') + .map(([tabId]) => tabId) + + await Promise.all(connectedTabIds.map(async (tabId) => { + try { + await chrome.debugger.sendCommand({ tabId }, 'Target.setAutoAttach', params) + } catch (error) { + logger.debug('Failed to set auto-attach for tab:', tabId, error) + } + })) + + return {} + } + // TODO disable network things? // if (msg.params.method === 'Network.enable' && msg.params.source !== 'playwriter') { // logger.debug('Skipping Network.enable from non-playwriter CDP client:', msg.params.sessionId) @@ -831,6 +858,15 @@ async function attachTab(tabId: number, { skipAttachedEvent = false }: { skipAtt await chrome.debugger.sendCommand(debuggee, 'Page.enable') + // Reapply cached auto-attach for new tabs so OOPIF targets are reported immediately. + if (autoAttachParams) { + try { + await chrome.debugger.sendCommand(debuggee, 'Target.setAutoAttach', autoAttachParams) + } catch (error) { + logger.debug('Failed to apply auto-attach for tab:', tabId, error) + } + } + const contextMenuScript = ` document.addEventListener('contextmenu', (e) => { window.__playwriter_lastRightClicked = e.target; diff --git a/extension/vite.config.mts b/extension/vite.config.mts index 9ca83d6..5b9bf2e 100644 --- a/extension/vite.config.mts +++ b/extension/vite.config.mts @@ -1,5 +1,5 @@ -import { fileURLToPath } from 'url'; -import { dirname, resolve } from 'path'; +import { fileURLToPath } from 'node:url'; +import { dirname, resolve } from 'node:path'; import { defineConfig } from 'vite'; import { viteStaticCopy } from 'vite-plugin-static-copy'; @@ -13,6 +13,9 @@ if (process.env.TESTING) { defineEnv['import.meta.env.TESTING'] = 'true'; } +// Allow tests to build per-port extension outputs to avoid parallel run conflicts. +const outDir = process.env.PLAYWRITER_EXTENSION_DIST || 'dist'; + export default defineConfig({ plugins: [ viteStaticCopy({ @@ -48,7 +51,7 @@ export default defineConfig({ ], build: { - outDir: 'dist', + outDir, emptyOutDir: false, minify: false, rollupOptions: { diff --git a/playwriter/src/cdp-relay.ts b/playwriter/src/cdp-relay.ts index cf81fc6..b89dc24 100644 --- a/playwriter/src/cdp-relay.ts +++ b/playwriter/src/cdp-relay.ts @@ -347,6 +347,12 @@ export async function startPlayWriterCDPRelayServer({ break } await maybeAutoCreateInitialTab() + // Forward auto-attach so Chrome emits iframe Target.attachedToTarget events. + // Playwright relies on these (with parentFrameId) when reconnecting over CDP. + await sendToExtension({ + method: 'forwardCDPCommand', + params: { method, params, source } + }) return {} } diff --git a/playwriter/src/mcp.test.ts b/playwriter/src/mcp.test.ts index 84b2e14..81b9274 100644 --- a/playwriter/src/mcp.test.ts +++ b/playwriter/src/mcp.test.ts @@ -4,6 +4,8 @@ import { chromium } from 'playwright-core' import path from 'node:path' import fs from 'node:fs' import os from 'node:os' +import http from 'node:http' +import net from 'node:net' import { getCdpUrl } from './utils.js' import type { ExtensionState } from 'mcp-extension/src/types.js' import type { Protocol } from 'devtools-protocol' @@ -28,6 +30,72 @@ function js(strings: TemplateStringsArray, ...values: any[]): string { ) } +type SimpleServer = { + baseUrl: string + close: () => Promise +} + +// Minimal local servers to create a cross-origin iframe without external dependencies. +async function createSimpleServer({ routes }: { routes: Record }): Promise { + const openSockets: Set = new Set() + const server = http.createServer((req, res) => { + const url = req.url || '/' + const body = routes[url] + if (!body) { + res.writeHead(404, { 'Content-Type': 'text/plain' }) + res.end('not found') + return + } + res.writeHead(200, { 'Content-Type': 'text/html' }) + res.end(body) + }) + + server.on('connection', (socket) => { + openSockets.add(socket) + socket.on('close', () => { + openSockets.delete(socket) + }) + }) + + await new Promise((resolve) => { + server.listen(0, '127.0.0.1', () => { + resolve() + }) + }) + + const address = server.address() + if (!address || typeof address === 'string') { + await new Promise((resolve, reject) => { + server.close((error) => { + if (error) { + reject(error) + return + } + resolve() + }) + }) + throw new Error('Failed to start test server') + } + + return { + baseUrl: `http://127.0.0.1:${address.port}`, + close: async () => { + for (const socket of openSockets) { + socket.destroy() + } + await new Promise((resolve, reject) => { + server.close((error) => { + if (error) { + reject(error) + return + } + resolve() + }) + }) + }, + } +} + declare global { var toggleExtensionForActiveTab: () => Promise<{ isConnected: boolean; state: ExtensionState }>; @@ -61,7 +129,11 @@ describe('MCP Server Tests', () => { it('should inject script via addScriptTag through CDP relay', async () => { const browserContext = getBrowserContext() - const serviceWorker = await getExtensionServiceWorker(browserContext) + const serviceWorker = await withTimeout({ + promise: getExtensionServiceWorker(browserContext), + timeoutMs: 5000, + errorMessage: 'Timed out waiting for extension service worker for iframe test', + }) const page = await browserContext.newPage() await page.setContent('') @@ -1277,6 +1349,85 @@ describe('MCP Server Tests', () => { await page.close() }, 60000) + // Reproduces the CDP reconnect issue: without auto-attach, Playwright only sees the main frame. + it('should expose iframe frames when connecting to an existing page over CDP', async () => { + const browserContext = getBrowserContext() + const serviceWorker = await getExtensionServiceWorker(browserContext) + + const childServer = await createSimpleServer({ + routes: { + '/child.html': 'child', + }, + }) + const childUrl = `${childServer.baseUrl}/child.html` + + const parentServer = await createSimpleServer({ + routes: { + '/': ``, + }, + }) + + const page = await browserContext.newPage() + try { + await withTimeout({ + promise: page.goto(parentServer.baseUrl, { waitUntil: 'domcontentloaded', timeout: 5000 }), + timeoutMs: 6000, + errorMessage: 'Timed out loading parent page for iframe test', + }) + await withTimeout({ + promise: page.frameLocator('iframe').locator('body').waitFor({ timeout: 5000 }), + timeoutMs: 6000, + errorMessage: 'Timed out waiting for iframe to attach in parent page', + }) + expect(page.frames().map((frame) => frame.url())).toContain(childUrl) + await page.bringToFront() + + await withTimeout({ + promise: serviceWorker.evaluate(async () => { + await globalThis.toggleExtensionForActiveTab() + }), + timeoutMs: 5000, + errorMessage: 'Timed out toggling extension for iframe test', + }) + await new Promise((r) => { setTimeout(r, 400) }) + + const browser = await withTimeout({ + promise: chromium.connectOverCDP(getCdpUrl({ port: TEST_PORT })), + timeoutMs: 5000, + errorMessage: 'Timed out connecting over CDP for iframe test', + }) + const context = browser.contexts()[0] + const cdpPage = context.pages().find((candidate) => { + return candidate.url().startsWith(parentServer.baseUrl) + }) + expect(cdpPage).toBeDefined() + + const frames = cdpPage!.frames() + const childFrame = frames.find((frame) => { + return frame.url() === childUrl + }) + + expect(frames.length).toBe(2) + expect(childFrame).toBeDefined() + + await withTimeout({ + promise: browser.close(), + timeoutMs: 5000, + errorMessage: 'Timed out closing CDP browser for iframe test', + }) + } finally { + await withTimeout({ + promise: page.close(), + timeoutMs: 5000, + errorMessage: 'Timed out closing page for iframe test', + }) + await Promise.all([ + parentServer.close(), + childServer.close(), + ]) + } + }, 60000) + it('should have non-empty URLs when connecting to already-loaded pages', async () => { // This test validates that when we connect to a browser with already-loaded pages, // all pages have non-empty URLs. Empty URLs break Playwright permanently. diff --git a/playwriter/src/test-utils.ts b/playwriter/src/test-utils.ts index 8753a0f..b96cf35 100644 --- a/playwriter/src/test-utils.ts +++ b/playwriter/src/test-utils.ts @@ -11,7 +11,22 @@ import { createFileLogger } from './create-logger.js' import { killPortProcess } from 'kill-port-process' const execAsync = promisify(exec) -let extensionBuildQueue: Promise = Promise.resolve() +const extensionBuildQueues: Map> = new Map() + +async function buildExtension({ port, distDir }: { port: number; distDir: string }): Promise { + const previous = extensionBuildQueues.get(distDir) || Promise.resolve() + const buildPromise = previous + .catch((error) => { + console.error('Previous extension build failed:', error) + }) + .then(async () => { + // Build into a per-port dist to avoid parallel test runs overwriting each other. + await execAsync(`TESTING=1 PLAYWRITER_PORT=${port} PLAYWRITER_EXTENSION_DIST=${distDir} pnpm build`, { cwd: '../extension' }) + }) + + extensionBuildQueues.set(distDir, buildPromise.finally(() => {})) + await buildPromise +} export async function getExtensionServiceWorker(context: BrowserContext) { let serviceWorkers = context.serviceWorkers().filter((sw) => sw.url().startsWith('chrome-extension://')) @@ -54,16 +69,11 @@ export async function setupTestContext({ }): Promise { await killPortProcess(port).catch(() => {}) + // Use a port-scoped dist folder so parallel tests don't replace each other's extension builds. + const distDir = `dist-${port}` + console.log('Building extension...') - const buildPromise = extensionBuildQueue - .catch((error) => { - console.error('Previous extension build failed:', error) - }) - .then(async () => { - await execAsync(`TESTING=1 PLAYWRITER_PORT=${port} pnpm build`, { cwd: '../extension' }) - }) - extensionBuildQueue = buildPromise.finally(() => {}) - await buildPromise + await buildExtension({ port, distDir }) console.log('Extension built') const localLogPath = path.join(process.cwd(), 'relay-server.log') @@ -71,7 +81,7 @@ export async function setupTestContext({ const relayServer = await startPlayWriterCDPRelayServer({ port, logger }) const userDataDir = fs.mkdtempSync(path.join(os.tmpdir(), tempDirPrefix)) - const extensionPath = path.resolve('../extension/dist') + const extensionPath = path.resolve('../extension', distDir) const browserContext = await chromium.launchPersistentContext(userDataDir, { channel: 'chromium',