diff --git a/packages/playwright-core/src/tools/backend/browserBackend.ts b/packages/playwright-core/src/tools/backend/browserBackend.ts index 1c4dd4d63419b..ac23adda870e3 100644 --- a/packages/playwright-core/src/tools/backend/browserBackend.ts +++ b/packages/playwright-core/src/tools/backend/browserBackend.ts @@ -38,7 +38,6 @@ export class BrowserBackend extends EventEmitter<{ disconnected: [] }> implement private _disposed = false; private _browserContext: playwright.BrowserContext; private _disposeCallback: (() => Promise) | undefined; - private _markDisconnected: () => void; constructor(config: ContextConfig, browserContext: playwright.BrowserContext, tools: Tool[], disposeCallback?: () => Promise) { super(); @@ -46,15 +45,15 @@ export class BrowserBackend extends EventEmitter<{ disconnected: [] }> implement 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 { @@ -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 } = {}, signal?: AbortSignal): Promise { diff --git a/packages/playwright-core/src/tools/mcp/index.ts b/packages/playwright-core/src/tools/mcp/index.ts index 5f259b3fe31b2..d720e1c93890a 100644 --- a/packages/playwright-core/src/tools/mcp/index.ts +++ b/packages/playwright-core/src/tools/mcp/index.ts @@ -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); diff --git a/tests/mcp/http.spec.ts b/tests/mcp/http.spec.ts index 6952eef2e8f03..f24b65e28f13c 100644 --- a/tests/mcp/http.spec.ts +++ b/tests/mcp/http.spec.ts @@ -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); diff --git a/tests/mcp/library.spec.ts b/tests/mcp/library.spec.ts index 176493f5eaf51..c853b530978e5 100644 --- a/tests/mcp/library.spec.ts +++ b/tests/mcp/library.spec.ts @@ -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'); @@ -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(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(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 { - 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; -}