diff --git a/packages/playwright-core/src/tools/cli-daemon/program.ts b/packages/playwright-core/src/tools/cli-daemon/program.ts index 40b5f5337765a..6682efbee19ff 100644 --- a/packages/playwright-core/src/tools/cli-daemon/program.ts +++ b/packages/playwright-core/src/tools/cli-daemon/program.ts @@ -62,9 +62,7 @@ export function decorateProgram(program: Command) { }; try { - const { browser, browserInfo, canBind, ownership } = await createBrowserWithInfo(mcpConfig, mcpClientInfo, options); - if (canBind) - await browser.bind(sessionName, { workspaceDir: clientInfo.workspaceDir }); + const { browser, browserInfo, ownership } = await createBrowserWithInfo(mcpConfig, mcpClientInfo, options, { title: sessionName, workspaceDir: clientInfo.workspaceDir }); const browserContext = mcpConfig.browser.isolated ? await browser.newContext(mcpConfig.browser.contextOptions) : browser.contexts()[0]; if (!browserContext) throw new Error('Error: unable to connect to a browser that does not have any contexts'); diff --git a/packages/playwright-core/src/tools/mcp/browserFactory.ts b/packages/playwright-core/src/tools/mcp/browserFactory.ts index 113370799d96f..c03b4caa217c9 100644 --- a/packages/playwright-core/src/tools/mcp/browserFactory.ts +++ b/packages/playwright-core/src/tools/mcp/browserFactory.ts @@ -23,7 +23,7 @@ import { defaultCacheDirectory } from '../../server/registry/index'; import { testDebug } from './log'; import { outputDir } from '../backend/context'; import { createExtensionBrowser } from './extensionContextFactory'; -import { connectToBrowserAcrossVersions } from '../utils/connect'; +import { connectToBrowserAcrossVersions, descriptorEndpoint } from '../utils/connect'; import { serverRegistry } from '../../serverRegistry'; import { resolveExtensionOptions } from './config'; // eslint-disable-next-line no-restricted-imports @@ -36,27 +36,29 @@ import type { Playwright } from '../../client/playwright'; import type * as playwrightTypes from '../../..'; import type { BrowserInfo } from '../../serverRegistry'; -type BrowserWithInfo = { +export type BrowserWithInfo = { browser: playwrightTypes.Browser, browserInfo: BrowserInfo, - canBind: boolean, + endpoint: string, ownership: 'attached' | 'own', }; -export async function createBrowserWithInfo(config: FullConfig, clientInfo: ClientInfo, cliOptions: CLIOptions): Promise { +export type BindOptions = { + title: string, + workspaceDir?: string, +}; + +export async function createBrowserWithInfo(config: FullConfig, clientInfo: ClientInfo, cliOptions: CLIOptions, bindOptions: BindOptions): Promise { if (config.browser.remoteEndpoint) return await createRemoteBrowser(config); let browser: playwrightTypes.Browser; - let canBind = false; let ownership: 'attached' | 'own' = 'own'; if (config.browser.cdpEndpoint) { browser = await createCDPBrowser(config, clientInfo); - canBind = true; ownership = 'attached'; } else if (config.browser.isolated) { browser = await createIsolatedBrowser(config, clientInfo); - canBind = true; ownership = 'own'; } else if (config.extension) { const { channel, executablePath, profileDirName } = resolveExtensionOptions(cliOptions); @@ -64,11 +66,21 @@ export async function createBrowserWithInfo(config: FullConfig, clientInfo: Clie ownership = 'attached'; } else { browser = await createPersistentBrowser(config, clientInfo); - canBind = true; ownership = 'own'; } - return { browser, browserInfo: browserInfo(browser, config), canBind, ownership }; + try { + const { endpoint } = await browser.bind(bindOptions.title, { workspaceDir: bindOptions.workspaceDir }); + return { browser, browserInfo: browserInfo(browser, config), endpoint, ownership }; + } catch (error) { + await browser.close().catch(() => {}); + throw error; + } +} + +export async function connectToBrowserEndpoint(config: FullConfig, browser: playwrightTypes.Browser, endpoint: string): Promise { + const options = config.browser.remoteEndpoint ? remoteConnectOptions(config).options : {}; + return await browser.browserType().connect(endpoint, options); } export interface BrowserContextFactory { @@ -114,21 +126,25 @@ async function createCDPBrowser(config: FullConfig, clientInfo: ClientInfo): Pro return browser; } -async function createRemoteBrowser(config: FullConfig): Promise { - testDebug('create browser (remote)'); - // `remoteEndpoint` may be a plain URL string or a ConnectOptions object that - // carries additional fields such as `exposeNetwork`, `headers`, `slowMo`, and - // `timeout`. Normalize once so the rest of the function deals with a single - // shape. +// `remoteEndpoint` may be a plain URL string or a ConnectOptions object that +// carries additional fields such as `exposeNetwork`, `headers`, `slowMo`, and +// `timeout`. Normalize once so every connect deals with a single shape. +function remoteConnectOptions(config: FullConfig): { endpoint: string, options: playwrightTypes.ConnectOptions } { const remote = config.browser.remoteEndpoint!; // `remoteHeaders` is for back-compat, `remoteEndpoint.headers` takes precedence. // eslint-disable-next-line no-restricted-syntax const remoteHeaders = (config.browser as any).remoteHeaders as Record | undefined; - const remoteOptions = typeof remote === 'string' - ? { endpoint: remote, headers: remoteHeaders } - : { ...remote, headers: { ...remoteHeaders, ...remote.headers } }; + if (typeof remote === 'string') + return { endpoint: remote, options: { headers: remoteHeaders } }; + const { endpoint, ...options } = remote; + return { endpoint, options: { ...options, headers: { ...remoteHeaders, ...remote.headers } } }; +} + +async function createRemoteBrowser(config: FullConfig): Promise { + testDebug('create browser (remote)'); + const { endpoint, options } = remoteConnectOptions(config); - const descriptor = await serverRegistry.find(remoteOptions.endpoint); + const descriptor = await serverRegistry.find(endpoint); if (descriptor) { const browser = await connectToBrowserAcrossVersions(descriptor); return { @@ -139,20 +155,20 @@ async function createRemoteBrowser(config: FullConfig): Promise launchOptions: descriptor.browser.launchOptions, userDataDir: descriptor.browser.userDataDir }, - canBind: false, + endpoint: descriptorEndpoint(descriptor), ownership: 'attached' }; } const playwrightObject = playwright as Playwright; // Use connectToBrowser instead of playwright[browserName].connect because we don't have browserName. - const browser = await connectToBrowser(playwrightObject, remoteOptions); + const browser = await connectToBrowser(playwrightObject, { endpoint, ...options }); browser._connectToBrowserType(playwrightObject[browser._browserName], {}, undefined); // A browser started via `launchServer` exposes no contexts until one is // created, so create one when attaching to such a server. if (!browser.contexts().length) await browser.newContext(config.browser.contextOptions); - return { browser, browserInfo: { ...browserInfo(browser, config), browserName: browser._browserName }, canBind: false, ownership: 'attached' }; + return { browser, browserInfo: { ...browserInfo(browser, config), browserName: browser._browserName }, endpoint, ownership: 'attached' }; } async function createPersistentBrowser(config: FullConfig, clientInfo: ClientInfo): Promise { diff --git a/packages/playwright-core/src/tools/mcp/index.ts b/packages/playwright-core/src/tools/mcp/index.ts index d720e1c93890a..f0be3c3596a1a 100644 --- a/packages/playwright-core/src/tools/mcp/index.ts +++ b/packages/playwright-core/src/tools/mcp/index.ts @@ -37,7 +37,7 @@ export async function createConnection(userConfig: Config = {}, contextGetter?: create: async (clientInfo: ClientInfo) => { const browser = contextGetter ? new SimpleBrowser(await contextGetter()) - : (await createBrowserWithInfo(config, clientInfo, {})).browser; + : (await createBrowserWithInfo(config, clientInfo, {}, { title: clientInfo.clientName, workspaceDir: clientInfo.cwd })).browser; const context = config.browser.isolated ? await browser.newContext(config.browser.contextOptions) : browser.contexts()[0]; return new BrowserBackend(config, context, tools); }, diff --git a/packages/playwright-core/src/tools/mcp/program.ts b/packages/playwright-core/src/tools/mcp/program.ts index 8b59d778f5700..9b9213a7bab9e 100644 --- a/packages/playwright-core/src/tools/mcp/program.ts +++ b/packages/playwright-core/src/tools/mcp/program.ts @@ -18,7 +18,7 @@ import { Option as ProgramOption } from 'commander'; import * as mcpServer from '../utils/mcp/server'; import { commaSeparatedList, defaultCodegenLanguage, dotenvFileLoader, enumParser, headerParser, numberParser, resolutionParser, resolveCLIConfigForMCP, semicolonSeparatedList } from './config'; import { setupExitWatchdog } from './watchdog'; -import { createBrowserWithInfo } from './browserFactory'; +import { connectToBrowserEndpoint, createBrowserWithInfo } from './browserFactory'; import { BrowserBackend } from '../backend/browserBackend'; import { filteredTools } from '../backend/tools'; import { testDebug } from './log'; @@ -26,6 +26,7 @@ import { packageJSON } from '../../package'; import type { Command } from 'commander'; import type { ClientInfo } from '../utils/mcp/server'; +import type { BrowserWithInfo } from './browserFactory'; import type * as playwright from '../../..'; const version = packageJSON.version; @@ -97,7 +98,7 @@ export function decorateMCPCommand(command: Command) { const config = await resolveCLIConfigForMCP(options); const tools = filteredTools(config); const useSharedBrowser = config.sharedBrowserContext || config.browser.isolated; - let sharedBrowserPromise: Promise | undefined; + let sharedBrowserPromise: Promise | undefined; let clientCount = 0; const clientNameCounters = new Map(); @@ -108,16 +109,13 @@ export function decorateMCPCommand(command: Command) { toolSchemas: tools.map(tool => tool.schema), create: async (clientInfo: ClientInfo) => { if (useSharedBrowser && !sharedBrowserPromise) { - const promise = (async () => { - const { browser, canBind } = await createBrowserWithInfo(config, clientInfo, options); - if (canBind) - await browser.bind(clientInfo.clientName, { workspaceDir: clientInfo.cwd }); - browser.once('disconnected', () => { + const promise = createBrowserWithInfo(config, clientInfo, options, { title: clientInfo.clientName, workspaceDir: clientInfo.cwd }).then(shared => { + shared.browser.once('disconnected', () => { if (sharedBrowserPromise === promise) sharedBrowserPromise = undefined; }); - return browser; - })().catch(error => { + return shared; + }, error => { if (sharedBrowserPromise === promise) sharedBrowserPromise = undefined; throw error; @@ -125,38 +123,58 @@ export function decorateMCPCommand(command: Command) { sharedBrowserPromise = promise; } clientCount++; + const promise = sharedBrowserPromise; + let shared: BrowserWithInfo | undefined; + let browser: BrowserWithInfo['browser']; try { - const promise = sharedBrowserPromise; - const { browser, canBind } = promise ? { browser: await promise, canBind: false } : await createBrowserWithInfo(config, clientInfo, options); - if (canBind) { + shared = await promise; + if (shared) { + testDebug('connect to shared browser'); + browser = await connectToBrowserEndpoint(config, shared.browser, shared.endpoint); + } else { const count = (clientNameCounters.get(clientInfo.clientName) ?? 0) + 1; clientNameCounters.set(clientInfo.clientName, count); const sessionName = count > 1 ? `${clientInfo.clientName} (${count})` : clientInfo.clientName; - await browser.bind(sessionName, { workspaceDir: clientInfo.cwd }); + browser = (await createBrowserWithInfo(config, clientInfo, options, { title: sessionName, workspaceDir: clientInfo.cwd })).browser; } - const browserContext = config.browser.isolated ? await browser.newContext(config.browser.contextOptions) : browser.contexts()[0]; - return new BrowserBackend(config, browserContext, tools, async () => { - clientCount--; - - if (sharedBrowserPromise && clientCount > 0) { - if (config.browser.isolated) { - testDebug('close context'); - await browserContext.close().catch(() => { }); - } - return; - } - - testDebug('close browser'); - if (sharedBrowserPromise === promise) - sharedBrowserPromise = undefined; - await browserContext.close().catch(() => { }); - await browser.close().catch(() => { }); - }); } catch (error) { // The dispose callback never runs for a failed create. clientCount--; throw error; } + + let browserContext: playwright.BrowserContext; + try { + // A `launchServer` remote isolates contexts per connection, so a fresh connection may see none. + browserContext = config.browser.isolated ? await browser.newContext(config.browser.contextOptions) : browser.contexts()[0] ?? await browser.newContext(config.browser.contextOptions); + } catch (error) { + clientCount--; + await browser.close().catch(() => { }); + throw error; + } + + return new BrowserBackend(config, browserContext, tools, async () => { + clientCount--; + const last = !shared || !clientCount; + if (last && sharedBrowserPromise === promise) + sharedBrowserPromise = undefined; + + if (!last) { + if (config.browser.isolated) { + testDebug('close context'); + await browserContext.close().catch(() => { }); + } else { + testDebug('disconnect from shared browser'); + } + await browser.close().catch(() => { }); + return; + } + + testDebug('close browser'); + await browserContext.close().catch(() => { }); + await browser.close().catch(() => { }); + await shared?.browser.close().catch(() => { }); + }); }, }; await mcpServer.start(factory, config.server); diff --git a/packages/playwright-core/src/tools/utils/connect.ts b/packages/playwright-core/src/tools/utils/connect.ts index 915c4061ea0c0..0479b5b587258 100644 --- a/packages/playwright-core/src/tools/utils/connect.ts +++ b/packages/playwright-core/src/tools/utils/connect.ts @@ -20,6 +20,10 @@ import type { BrowserDescriptor } from '../../serverRegistry'; export async function connectToBrowserAcrossVersions(descriptor: BrowserDescriptor): Promise { const pw = require(descriptor.playwrightLib); const browserType = pw[descriptor.browser.browserName] as playwright.BrowserType; + return await browserType.connect(descriptorEndpoint(descriptor)); +} + +export function descriptorEndpoint(descriptor: BrowserDescriptor): string { // eslint-disable-next-line no-restricted-syntax - return await browserType.connect(descriptor.endpoint ?? (descriptor as any).pipeName); + return descriptor.endpoint ?? (descriptor as any).pipeName; } diff --git a/tests/mcp/http.spec.ts b/tests/mcp/http.spec.ts index f47e3d3b31aa9..e11e0c3ae99ba 100644 --- a/tests/mcp/http.spec.ts +++ b/tests/mcp/http.spec.ts @@ -136,6 +136,7 @@ test('http transport browser lifecycle (isolated)', async ({ serverEndpoint, ser 'create http session': 2, 'delete http session': 2, 'create browser \(isolated\)': 2, + 'connect to shared browser': 2, 'create context': 2, 'close browser': 2, }); @@ -156,6 +157,7 @@ test('http transport browser sigint', async ({ serverEndpoint, server }) => { await expect.poll(() => formatLog(stderr())).toEqual({ 'create browser (isolated)': 1, + 'connect to shared browser': 1, 'create context': 1, 'create http session': 1, 'gracefully closing 1': 1, @@ -202,6 +204,7 @@ test('http transport browser lifecycle (isolated, multiclient)', { annotation: { 'delete http session': 3, 'create context': 3, 'create browser (isolated)': 1, + 'connect to shared browser': 3, 'close context': 2, 'close browser': 1, }); @@ -232,6 +235,7 @@ test('http transport browser lifecycle (isolated, concurrent clients)', { annota 'delete http session': 3, 'create context': 3, 'create browser (isolated)': 1, + 'connect to shared browser': 3, 'close context': 2, 'close browser': 1, }); @@ -292,6 +296,7 @@ test('http transport isolated multiclient relaunches a crashed shared browser', 'create http session': 2, 'delete http session': 2, 'create browser (isolated)': 2, + 'connect to shared browser': 4, 'create context': 4, 'close browser': 2, 'close context': 2, @@ -329,6 +334,7 @@ test('http transport isolated closes the browser despite an earlier failed backe 'create http session': 1, 'delete http session': 1, 'create browser (isolated)': 1, + 'connect to shared browser': 2, 'create context': 1, 'close browser': 1, }); @@ -431,6 +437,8 @@ test('http transport shared context', async ({ serverEndpoint, server }) => { await expect.poll(() => formatLog(stderr())).toEqual({ 'create browser (persistent)': 1, + 'connect to shared browser': 2, + 'disconnect from shared browser': 1, 'create http session': 2, 'delete http session': 2, 'create context': 2, @@ -489,6 +497,8 @@ test('http transport shared context refuses browser_close', { annotation: { type await expect.poll(() => formatLog(stderr())).toEqual({ 'create browser (persistent)': 1, + 'connect to shared browser': 2, + 'disconnect from shared browser': 1, 'create http session': 2, 'delete http session': 2, 'create context': 2, diff --git a/tests/mcp/sse.spec.ts b/tests/mcp/sse.spec.ts index 2ea74f488e1d6..8873368513849 100644 --- a/tests/mcp/sse.spec.ts +++ b/tests/mcp/sse.spec.ts @@ -111,6 +111,7 @@ test('sse transport browser lifecycle (isolated)', async ({ serverEndpoint, serv 'delete SSE session': 2, 'create context': 2, 'create browser (isolated)': 2, + 'connect to shared browser': 2, 'close browser': 2, }); }); @@ -161,6 +162,7 @@ test('sse transport browser lifecycle (isolated, multiclient)', async ({ serverE 'delete SSE session': 3, 'create context': 3, 'create browser (isolated)': 1, + 'connect to shared browser': 3, 'close context': 2, 'close browser': 1, }); @@ -271,7 +273,9 @@ test('sse transport shared context', async ({ serverEndpoint, server }) => { 'create SSE session': 2, 'delete SSE session': 2, 'create browser (persistent)': 1, + 'connect to shared browser': 2, 'create context': 2, + 'disconnect from shared browser': 1, 'close browser': 1, }); });