Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions packages/playwright-core/src/tools/cli-daemon/program.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
60 changes: 38 additions & 22 deletions packages/playwright-core/src/tools/mcp/browserFactory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -36,39 +36,51 @@ 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<BrowserWithInfo> {
export type BindOptions = {
title: string,
workspaceDir?: string,
};

export async function createBrowserWithInfo(config: FullConfig, clientInfo: ClientInfo, cliOptions: CLIOptions, bindOptions: BindOptions): Promise<BrowserWithInfo> {
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);
browser = await createExtensionBrowser(channel, executablePath, config.browser.userDataDir, profileDirName, clientInfo.clientName);
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 });

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now binds in extension mode too. Do we really want that?

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<playwrightTypes.Browser> {
const options = config.browser.remoteEndpoint ? remoteConnectOptions(config).options : {};
return await browser.browserType().connect(endpoint, options);
}

export interface BrowserContextFactory {
Expand Down Expand Up @@ -114,21 +126,25 @@ async function createCDPBrowser(config: FullConfig, clientInfo: ClientInfo): Pro
return browser;
}

async function createRemoteBrowser(config: FullConfig): Promise<BrowserWithInfo> {
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<string, string> | 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<BrowserWithInfo> {
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 {
Expand All @@ -139,20 +155,20 @@ async function createRemoteBrowser(config: FullConfig): Promise<BrowserWithInfo>
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<playwrightTypes.Browser> {
Expand Down
2 changes: 1 addition & 1 deletion packages/playwright-core/src/tools/mcp/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
},
Expand Down
80 changes: 49 additions & 31 deletions packages/playwright-core/src/tools/mcp/program.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,15 @@ 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';
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;
Expand Down Expand Up @@ -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<playwright.Browser> | undefined;
let sharedBrowserPromise: Promise<BrowserWithInfo> | undefined;
let clientCount = 0;
const clientNameCounters = new Map<string, number>();

Expand All @@ -108,55 +109,72 @@ 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;
});
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(() => { });

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we call shared?.browser.close() here as well, similar to lines 174-176? Ideally, we call the onClose callback we pass on line 156, teaching it to work without defined browserContext?

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);
Expand Down
6 changes: 5 additions & 1 deletion packages/playwright-core/src/tools/utils/connect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@ import type { BrowserDescriptor } from '../../serverRegistry';
export async function connectToBrowserAcrossVersions(descriptor: BrowserDescriptor): Promise<playwright.Browser> {
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;
}
10 changes: 10 additions & 0 deletions tests/mcp/http.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
});
Expand All @@ -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,
Expand Down Expand Up @@ -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,
});
Expand Down Expand Up @@ -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,
});
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
});
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
4 changes: 4 additions & 0 deletions tests/mcp/sse.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
});
});
Expand Down Expand Up @@ -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,
});
Expand Down Expand Up @@ -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,
});
});