Skip to content

Revert "fix(mcp): drop the backend after browser_close with a shared browser" #42492

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Yury Semikhatsky (yury-s) merged 1 commit into from
Sep 1, 2026
Merged

Revert "fix(mcp): drop the backend after browser_close with a shared browser" #42492

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
17 changes: 3 additions & 14 deletions packages/playwright-core/src/tools/backend/browserBackend.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,23 +38,22 @@ export class BrowserBackend extends EventEmitter<{ disconnected: [] }> implement
private _disposed = false;
private _browserContext: playwright.BrowserContext;
private _disposeCallback: (() => Promise<void>) | undefined;
private _markDisconnected: () => void;

constructor(config: ContextConfig, browserContext: playwright.BrowserContext, tools: Tool[], disposeCallback?: () => Promise<void>) {
super();
this._config = config;
this._tools = tools;
this._browserContext = browserContext;
this._disposeCallback = disposeCallback;
this._markDisconnected = () => {
const markDisconnected = () => {
if (this._disconnected)
return;
backendDebug('browser disconnected');
this._disconnected = true;
this.emit('disconnected');
};
this._browserContext.once('close', this._markDisconnected);
this._browserContext.browser()?.once('disconnected', this._markDisconnected);
this._browserContext.once('close', markDisconnected);
this._browserContext.browser()?.once('disconnected', markDisconnected);
}

async initialize(clientInfo: ClientInfo): Promise<void> {
Expand All @@ -70,18 +69,8 @@ export class BrowserBackend extends EventEmitter<{ disconnected: [] }> implement
if (this._disposed)
return;
this._disposed = true;
// These listeners would otherwise keep every disposed backend reachable for
// as long as the context and the browser live.
this._browserContext.off('close', this._markDisconnected);
this._browserContext.browser()?.off('disconnected', this._markDisconnected);
await this._context?.dispose().catch(e => debug('pw:tools:error')(e));
await this._disposeCallback?.().catch(e => debug('pw:tools:error')(e));
// The browser may outlive this backend, e.g. when other clients share it,
// so tell the server to stop handing out the disposed backend.
if (!this._disconnected) {
this._disconnected = true;
this.emit('disconnected');
}
}

async callTool(name: string, rawArguments: mcpServer.CallToolRequest['params']['arguments'] & { _meta?: Record<string, any> } = {}, signal?: AbortSignal): Promise<mcpServer.CallToolResult> {
Expand Down
21 changes: 4 additions & 17 deletions packages/playwright-core/src/tools/mcp/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,24 +35,11 @@ export async function createConnection(userConfig: Config = {}, contextGetter?:
version: packageJSON.version,
toolSchemas: tools.map(tool => tool.schema),
create: async (clientInfo: ClientInfo) => {
if (contextGetter) {
const browser = new SimpleBrowser(await contextGetter());
const context = config.browser.isolated ? await browser.newContext() : browser.contexts()[0];
// The caller owns the context it handed us, so it closes it too.
return new BrowserBackend(config, context, tools);
}

const { browser } = await createBrowserWithInfo(config, clientInfo, {});
const browser = contextGetter
? new SimpleBrowser(await contextGetter())
: (await createBrowserWithInfo(config, clientInfo, {})).browser;
const context = config.browser.isolated ? await browser.newContext(config.browser.contextOptions) : browser.contexts()[0];
// Close the browser in both ownership modes. A browser this factory
// launched shuts down here. For an attached one (a CDP or remote
// endpoint, or the extension) close() only drops the connection this
// factory made while the external browser keeps running, so skipping it
// would leak that connection. The next call attaches again.
return new BrowserBackend(config, context, tools, async () => {
await context.close().catch(() => {});
await browser.close().catch(() => {});
});
return new BrowserBackend(config, context, tools);
},
};
return createServer('api', packageJSON.version, backendFactory, Promise.resolve(), false);
Expand Down
56 changes: 0 additions & 56 deletions tests/mcp/http.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -438,62 +438,6 @@ test('http transport shared context', async ({ serverEndpoint, server }) => {
});
});

test('http transport shared context survives browser_close', { annotation: { type: 'issue', description: 'https://github.com/microsoft/playwright/issues/42363' } }, async ({ serverEndpoint, server }) => {
const { url, stderr } = await serverEndpoint({ args: ['--shared-browser-context'] });

const transport1 = new StreamableHTTPClientTransport(new URL('/mcp', url));
const client1 = new Client({ name: 'test1', version: '1.0.0' });
await client1.connect(transport1);
await client1.callTool({
name: 'browser_navigate',
arguments: { url: server.HELLO_WORLD },
});

const transport2 = new StreamableHTTPClientTransport(new URL('/mcp', url));
const client2 = new Client({ name: 'test2', version: '1.0.0' });
await client2.connect(transport2);
await client2.callTool({
name: 'browser_navigate',
arguments: { url: server.HELLO_WORLD },
});

// The second client keeps the shared browser alive, so closing only
// disposes the first client's backend.
await client1.callTool({
name: 'browser_close',
arguments: {},
});

// The next call from the first client must get a fresh backend.
expect(await client1.callTool({
name: 'browser_tabs',
arguments: { action: 'new', url: server.HELLO_WORLD },
})).toHaveResponse({
snapshot: expect.stringContaining(`Hello, world!`),
});

// The second client is unaffected.
expect(await client2.callTool({
name: 'browser_snapshot',
arguments: {},
})).toHaveResponse({
inlineSnapshot: expect.stringContaining(`Hello, world!`),
});

await transport1.terminateSession();
await client1.close();
await transport2.terminateSession();
await client2.close();

await expect.poll(() => formatLog(stderr())).toEqual({
'create browser (persistent)': 1,
'create http session': 2,
'delete http session': 2,
'create context': 3,
'close browser': 1,
});
});

test('http transport (default)', async ({ serverEndpoint }) => {
const { url } = await serverEndpoint();
const transport = new StreamableHTTPClientTransport(url);
Expand Down
153 changes: 0 additions & 153 deletions tests/mcp/library.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,7 @@
*/
import child_process from 'child_process';
import fs from 'fs/promises';
import net from 'net';

import * as playwright from 'playwright';
import { Client } from '@modelcontextprotocol/sdk/client/index.js';
import { InMemoryTransport } from '@modelcontextprotocol/sdk/inMemory.js';
import { test, expect } from './fixtures';
import { tools } from '../../packages/playwright-core/lib/coreBundle';

import type { Server } from '@modelcontextprotocol/sdk/server/index.js';

const { createConnection } = tools;

test('library can be used from CommonJS', { annotation: { type: 'issue', description: 'https://github.com/microsoft/playwright-mcp/issues/456' } }, async ({}, testInfo) => {
const file = testInfo.outputPath('main.cjs');
Expand All @@ -36,146 +26,3 @@ test('library can be used from CommonJS', { annotation: { type: 'issue', descrip
`);
expect(child_process.execSync(`node ${file}`, { encoding: 'utf-8' })).toContain('OK');
});

test('createConnection detaches backend listeners from a caller owned context', async ({ mcpBrowser, mcpHeadless, server }, testInfo) => {
const { browserName, channel } = browserForProject(mcpBrowser);
const browser = await playwright[browserName].launch({ channel, headless: mcpHeadless });
const browserContext = await browser.newContext();

const client = await connectClient(await createConnection({
outputDir: testInfo.outputPath('output'),
}, async () => browserContext));

const contextListeners = listenerCount(browserContext, 'close');
const browserListeners = listenerCount(browser, 'disconnected');

// browser_close disposes the backend while the caller keeps the context, so
// the next tool call builds another backend over the same two objects.
for (let i = 0; i < 3; i++) {
await client.callTool({ name: 'browser_navigate', arguments: { url: server.HELLO_WORLD } });
// A live backend listens on both objects.
expect(listenerCount(browserContext, 'close')).toBe(contextListeners + 1);
expect(listenerCount(browser, 'disconnected')).toBe(browserListeners + 1);

await client.callTool({ name: 'browser_close', arguments: {} });
// A disposed one gives both back, so they do not pile up.
expect(listenerCount(browserContext, 'close')).toBe(contextListeners);
expect(listenerCount(browser, 'disconnected')).toBe(browserListeners);
}

await client.close();
await browser.close();
});

test('createConnection closes the browser it launched when the backend is disposed', async ({ mcpBrowser, mcpHeadless, server }, testInfo) => {
test.skip(!['chrome', 'msedge', 'chromium'].includes(mcpBrowser!), 'Profile locking is a Chromium behavior');

const { browserName, channel } = browserForProject(mcpBrowser);
const userDataDir = testInfo.outputPath('user-data-dir');

const client = await connectClient(await createConnection({
// Match what BrowserType.launch does by default. Resolving this config
// turns the sandbox on for a chromium without a channel, and the bundled
// build cannot sandbox on a linux runner.
browser: { browserName, userDataDir, launchOptions: { channel, chromiumSandbox: false, headless: mcpHeadless } },
outputDir: testInfo.outputPath('output'),
}));

expect(await client.callTool({
name: 'browser_navigate',
arguments: { url: server.HELLO_WORLD },
})).toHaveResponse({
page: expect.stringContaining(`Page URL: ${server.HELLO_WORLD}`),
});

// Nothing else holds this browser, so disposing the backend has to close it.
// A leaked browser keeps the profile, and nothing else can open it. Claiming
// the profile from the test rather than through another tool call keeps this
// about the browser process and not about how the server rebuilds a backend.
await client.callTool({ name: 'browser_close', arguments: {} });
await client.close();

await expect.poll(async () => {
try {
const context = await playwright[browserName].launchPersistentContext(userDataDir, { channel, headless: mcpHeadless });
await context.close();
return 'free';
} catch (e: any) {
return String(e.message);
}
}, { timeout: 15000 }).toBe('free');
});

test('createConnection drops its connection to an attached browser when the backend is disposed', async ({ mcpBrowser, mcpHeadless, server }, testInfo) => {
const { browserName, channel } = browserForProject(mcpBrowser);
const browserServer = await playwright[browserName].launchServer({ channel, headless: mcpHeadless });
const wsUrl = new URL(browserServer.wsEndpoint());
// URL keeps the brackets around an IPv6 host, net.connect does not take them.
const wsHost = wsUrl.hostname.replace(/^\[|\]$/g, '');

// A counting TCP proxy in front of the browser server makes the connection
// the factory holds observable from the outside.
let openConnections = 0;
const proxy = net.createServer(socket => {
openConnections++;
const upstream = net.connect(Number(wsUrl.port), wsHost);
socket.pipe(upstream);
upstream.pipe(socket);
socket.once('close', () => {
openConnections--;
upstream.destroy();
});
upstream.once('close', () => socket.destroy());
socket.on('error', () => socket.destroy());
upstream.on('error', () => upstream.destroy());
});
await new Promise<void>(resolve => proxy.listen(0, '127.0.0.1', resolve));
const proxyPort = (proxy.address() as net.AddressInfo).port;

const client = await connectClient(await createConnection({
browser: { remoteEndpoint: `ws://127.0.0.1:${proxyPort}${wsUrl.pathname}` },
outputDir: testInfo.outputPath('output'),
}));

expect(await client.callTool({
name: 'browser_navigate',
arguments: { url: server.HELLO_WORLD },
})).toHaveResponse({
page: expect.stringContaining(`Page URL: ${server.HELLO_WORLD}`),
});
expect(openConnections).toBe(1);

// The factory attached to this browser instead of launching it, so closing
// the browser on dispose only drops the connection the factory made while
// the browser server keeps running. Skipping the close would leak that
// connection for as long as the server stays up.
await client.callTool({ name: 'browser_close', arguments: {} });
await expect.poll(() => openConnections).toBe(0);

// The external browser survived the disconnect.
const probe = await playwright[browserName].connect(browserServer.wsEndpoint());
expect(probe.isConnected()).toBe(true);
await probe.close();

await client.close();
await new Promise<void>(resolve => proxy.close(() => resolve()));
await browserServer.close();
});

function browserForProject(mcpBrowser: string | undefined): { browserName: 'chromium' | 'firefox' | 'webkit', channel: string | undefined } {
const channel = mcpBrowser === 'chrome' || mcpBrowser === 'msedge' ? mcpBrowser : undefined;
const browserName = channel || mcpBrowser === 'chromium' ? 'chromium' : mcpBrowser as 'firefox' | 'webkit';
return { browserName, channel };
}

function listenerCount(emitter: object, event: string): number {
return (emitter as unknown as { listenerCount(event: string): number }).listenerCount(event);
}

async function connectClient(server: Server): Promise<Client> {
const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair();
await server.connect(serverTransport);
const client = new Client({ name: 'test', version: '1.0.0' });
await client.connect(clientTransport);
return client;
}
Loading