diff --git a/src/browserServerImpl.ts b/src/browserServerImpl.ts index 2dddeaf9dd..f58c102e16 100644 --- a/src/browserServerImpl.ts +++ b/src/browserServerImpl.ts @@ -18,8 +18,7 @@ import { LaunchServerOptions, Logger } from './client/types'; import { BrowserType } from './server/browserType'; import { Browser } from './server/browser'; import { EventEmitter } from 'ws'; -import { DispatcherScope } from './dispatchers/dispatcher'; -import { BrowserDispatcher } from './dispatchers/browserDispatcher'; +import { Dispatcher, DispatcherScope } from './dispatchers/dispatcher'; import { BrowserContextDispatcher } from './dispatchers/browserContextDispatcher'; import * as channels from './protocol/channels'; import { BrowserServerLauncher, BrowserServer } from './client/browserType'; @@ -32,6 +31,10 @@ import { CallMetadata, internalCallMetadata } from './server/instrumentation'; import { Playwright } from './server/playwright'; import { PlaywrightDispatcher } from './dispatchers/playwrightDispatcher'; import { PlaywrightServer, PlaywrightServerDelegate } from './remote/playwrightServer'; +import { BrowserContext } from './server/browserContext'; +import { CRBrowser } from './server/chromium/crBrowser'; +import { CDPSessionDispatcher } from './dispatchers/cdpSessionDispatcher'; +import { PageDispatcher } from './dispatchers/pageDispatcher'; export class BrowserServerLauncherImpl implements BrowserServerLauncher { private _playwright: Playwright; @@ -67,6 +70,7 @@ export class BrowserServerLauncherImpl implements BrowserServerLauncher { browserServer.wsEndpoint = () => wsEndpoint; browserServer.close = () => browser.options.browserProcess.close(); browserServer.kill = () => browser.options.browserProcess.kill(); + (browserServer as any)._disconnectForTest = () => server.close(); browser.options.browserProcess.onclose = async (exitCode, signal) => { server.close(); browserServer.emit('close', exitCode, signal); @@ -74,55 +78,78 @@ export class BrowserServerLauncherImpl implements BrowserServerLauncher { return browserServer; } - private _onConnect(browser: Browser, scope: DispatcherScope) { + private _onConnect(browser: Browser, scope: DispatcherScope, forceDisconnect: () => void) { const selectors = new Selectors(); const selectorsDispatcher = new SelectorsDispatcher(scope, selectors); - const browserDispatcher = new ConnectedBrowser(scope, browser, selectors); + const browserDispatcher = new ConnectedBrowserDispatcher(scope, browser, selectors); + browser.on(Browser.Events.Disconnected, () => { + // Underlying browser did close for some reason - force disconnect the client. + forceDisconnect(); + }); new PlaywrightDispatcher(scope, this._playwright, selectorsDispatcher, browserDispatcher); return () => { // Cleanup contexts upon disconnect. - browserDispatcher.close().catch(e => {}); + browserDispatcher.cleanupContexts().catch(e => {}); }; } } -// This class implements multiplexing multiple BrowserDispatchers over a single Browser instance. -class ConnectedBrowser extends BrowserDispatcher { - private _contexts: BrowserContextDispatcher[] = []; +// This class implements multiplexing browser dispatchers over a single Browser instance. +class ConnectedBrowserDispatcher extends Dispatcher implements channels.BrowserChannel { + private _contexts = new Set(); private _selectors: Selectors; - private _closed = false; constructor(scope: DispatcherScope, browser: Browser, selectors: Selectors) { - super(scope, browser); + super(scope, browser, 'Browser', { version: browser.version(), name: browser.options.name }, true); this._selectors = selectors; } - async newContext(params: channels.BrowserNewContextParams, metadata: CallMetadata): Promise<{ context: channels.BrowserContextChannel }> { + async newContext(params: channels.BrowserNewContextParams, metadata: CallMetadata): Promise { if (params.recordVideo) { // TODO: we should create a separate temp directory or accept a launchServer parameter. params.recordVideo.dir = this._object.options.downloadsPath!; } - const result = await super.newContext(params, metadata); - const dispatcher = result.context as BrowserContextDispatcher; - dispatcher._object._setSelectors(this._selectors); - this._contexts.push(dispatcher); - return result; + const context = await this._object.newContext(params); + this._contexts.add(context); + context._setSelectors(this._selectors); + context.on(BrowserContext.Events.Close, () => this._contexts.delete(context)); + if (params.storageState) + await context.setStorageState(metadata, params.storageState); + return { context: new BrowserContextDispatcher(this._scope, context) }; } async close(): Promise { - // Only close our own contexts. - await Promise.all(this._contexts.map(context => context.close({}, internalCallMetadata()))); - this._didClose(); + // Client should not send us Browser.close. } - _didClose() { - if (!this._closed) { - // We come here multiple times: - // - from ConnectedBrowser.close(); - // - from underlying Browser.on('close'). - this._closed = true; - super._didClose(); - } + async killForTests(): Promise { + // Client should not send us Browser.killForTests. + } + + async newBrowserCDPSession(): Promise { + if (!this._object.options.isChromium) + throw new Error(`CDP session is only available in Chromium`); + const crBrowser = this._object as CRBrowser; + return { session: new CDPSessionDispatcher(this._scope, await crBrowser.newBrowserCDPSession()) }; + } + + async startTracing(params: channels.BrowserStartTracingParams): Promise { + if (!this._object.options.isChromium) + throw new Error(`Tracing is only available in Chromium`); + const crBrowser = this._object as CRBrowser; + await crBrowser.startTracing(params.page ? (params.page as PageDispatcher)._object : undefined, params); + } + + async stopTracing(): Promise { + if (!this._object.options.isChromium) + throw new Error(`Tracing is only available in Chromium`); + const crBrowser = this._object as CRBrowser; + const buffer = await crBrowser.stopTracing(); + return { binary: buffer.toString('base64') }; + } + + async cleanupContexts() { + await Promise.all(Array.from(this._contexts).map(context => context.close(internalCallMetadata()))); } } diff --git a/src/client/browser.ts b/src/client/browser.ts index 9367b0baf5..a059796372 100644 --- a/src/client/browser.ts +++ b/src/client/browser.ts @@ -28,7 +28,7 @@ export class Browser extends ChannelOwner(); private _isConnected = true; private _closedPromise: Promise; - _isRemote = false; + _remoteType: 'owns-connection' | 'uses-connection' | null = null; readonly _name: string; static from(browser: channels.BrowserChannel): Browser { @@ -98,7 +98,10 @@ export class Browser extends ChannelOwner { try { await this._wrapApiCall('browser.close', async (channel: channels.BrowserChannel) => { - await channel.close(); + if (this._remoteType === 'owns-connection') + this._connection.close(); + else + await channel.close(); await this._closedPromise; }); } catch (e) { diff --git a/src/client/browserType.ts b/src/client/browserType.ts index 74cf140ed1..1d17b9461f 100644 --- a/src/client/browserType.ts +++ b/src/client/browserType.ts @@ -21,7 +21,6 @@ import { ChannelOwner } from './channelOwner'; import { LaunchOptions, LaunchServerOptions, ConnectOptions, LaunchPersistentContextOptions } from './types'; import WebSocket from 'ws'; import { Connection } from './connection'; -import { serializeError } from '../protocol/serializers'; import { Events } from './events'; import { TimeoutSettings } from '../utils/timeoutSettings'; import { ChildProcess } from 'child_process'; @@ -111,32 +110,32 @@ export class BrowserType extends ChannelOwner { const logger = params.logger; return this._wrapApiCall('browserType.connect', async () => { - const connection = new Connection(); - const ws = new WebSocket(params.wsEndpoint, [], { perMessageDeflate: false, maxPayload: 256 * 1024 * 1024, // 256Mb, handshakeTimeout: this._timeoutSettings.timeout(params), headers: params.headers, }); + const connection = new Connection(() => ws.close()); // The 'ws' module in node sometimes sends us multiple messages in a single task. const waitForNextTask = params.slowMo ? (cb: () => any) => setTimeout(cb, params.slowMo) : makeWaitForNextTask(); connection.onmessage = message => { - if (ws.readyState !== WebSocket.OPEN) { - setTimeout(() => { - connection.dispatch({ id: (message as any).id, error: serializeError(new Error(kBrowserClosedError)) }); - }, 0); + // Connection should handle all outgoing message in disconnected(). + if (ws.readyState !== WebSocket.OPEN) return; - } ws.send(JSON.stringify(message)); }; ws.addEventListener('message', event => { waitForNextTask(() => { try { - connection.dispatch(JSON.parse(event.data)); + // Since we may slow down the messages, but disconnect + // synchronously, we might come here with a message + // after disconnect. + if (!connection.isDisconnected()) + connection.dispatch(JSON.parse(event.data)); } catch (e) { ws.close(); } @@ -165,7 +164,7 @@ export class BrowserType extends ChannelOwner { // Emulate all pages, contexts and the browser closing upon disconnect. for (const context of browser.contexts()) { @@ -174,6 +173,7 @@ export class BrowserType extends ChannelOwner extends EventEmitter { - private _connection: Connection; + protected _connection: Connection; private _parent: ChannelOwner | undefined; private _objects = new Map(); diff --git a/src/client/connection.ts b/src/client/connection.ts index 5c30bc6125..a634f68af5 100644 --- a/src/client/connection.ts +++ b/src/client/connection.ts @@ -52,9 +52,12 @@ export class Connection { private _lastId = 0; private _callbacks = new Map void, reject: (a: Error) => void }>(); private _rootObject: ChannelOwner; + private _disconnectedErrorMessage: string | undefined; + private _onClose?: () => void; - constructor() { + constructor(onClose?: () => void) { this._rootObject = new Root(this); + this._onClose = onClose; } async waitForObjectWithKnownName(guid: string): Promise { @@ -75,6 +78,8 @@ export class Connection { debugLogger.log('channel:command', converted); this.onmessage({ ...converted, metadata: { stack: frames, apiName } }); try { + if (this._disconnectedErrorMessage) + throw new Error(this._disconnectedErrorMessage); return await new Promise((resolve, reject) => this._callbacks.set(id, { resolve, reject })); } catch (e) { const innerStack = ((process.env.PWDEBUGIMPL || isUnderTest()) && e.stack) ? e.stack.substring(e.stack.indexOf(e.message) + e.message.length) : ''; @@ -120,6 +125,22 @@ export class Connection { object._channel.emit(method, this._replaceGuidsWithChannels(params)); } + close() { + if (this._onClose) + this._onClose(); + } + + didDisconnect(errorMessage: string) { + this._disconnectedErrorMessage = errorMessage; + for (const callback of this._callbacks.values()) + callback.reject(new Error(errorMessage)); + this._callbacks.clear(); + } + + isDisconnected() { + return !!this._disconnectedErrorMessage; + } + private _replaceGuidsWithChannels(payload: any): any { if (!payload) return payload; diff --git a/src/client/frame.ts b/src/client/frame.ts index dec8c1140e..7face16fac 100644 --- a/src/client/frame.ts +++ b/src/client/frame.ts @@ -102,6 +102,8 @@ export class Frame extends ChannelOwner(this._page!, Events.Page.FrameDetached, new Error('Navigating frame was detached!'), frame => frame === this); diff --git a/src/client/page.ts b/src/client/page.ts index 9df2fe60af..e7511318ad 100644 --- a/src/client/page.ts +++ b/src/client/page.ts @@ -124,7 +124,7 @@ export class Page extends ChannelOwner this.emit(Events.Page.DOMContentLoaded, this)); this._channel.on('download', ({ url, suggestedFilename, artifact }) => { const artifactObject = Artifact.from(artifact); - artifactObject._isRemote = !!this._browserContext._browser && this._browserContext._browser._isRemote; + artifactObject._isRemote = !!this._browserContext._browser && !!this._browserContext._browser._remoteType; this.emit(Events.Page.Download, new Download(url, suggestedFilename, artifactObject)); }); this._channel.on('fileChooser', ({ element, isMultiple }) => this.emit(Events.Page.FileChooser, new FileChooser(this, ElementHandle.from(element), isMultiple))); diff --git a/src/client/tracing.ts b/src/client/tracing.ts index 21f7890963..198ef53c50 100644 --- a/src/client/tracing.ts +++ b/src/client/tracing.ts @@ -42,7 +42,7 @@ export class Tracing { return await channel.tracingExport(); }); const artifact = Artifact.from(result.artifact); - if (this._context.browser()?._isRemote) + if (this._context.browser()?._remoteType) artifact._isRemote = true; await artifact.saveAs(path); await artifact.delete(); diff --git a/src/client/video.ts b/src/client/video.ts index b6f8cfe3a0..8ae5034ab8 100644 --- a/src/client/video.ts +++ b/src/client/video.ts @@ -25,7 +25,7 @@ export class Video implements api.Video { constructor(page: Page) { const browser = page.context()._browser; - this._isRemote = !!browser && browser._isRemote; + this._isRemote = !!browser && !!browser._remoteType; this._artifact = Promise.race([ new Promise(f => this._artifactCallback = f), page._closedOrCrashedPromise.then(() => null), diff --git a/src/client/waiter.ts b/src/client/waiter.ts index 80d9f8b8a6..72588120d3 100644 --- a/src/client/waiter.ts +++ b/src/client/waiter.ts @@ -23,6 +23,7 @@ import { ChannelOwner } from './channelOwner'; export class Waiter { private _dispose: (() => void)[]; private _failures: Promise[] = []; + private _immediateError?: Error; // TODO: can/should we move these logs into wrapApiCall? private _logs: string[] = []; private _channelOwner: ChannelOwner; @@ -59,6 +60,10 @@ export class Waiter { this._rejectOn(promise.then(() => { throw new TimeoutError(message); }), dispose); } + rejectImmediately(error: Error) { + this._immediateError = error; + } + dispose() { for (const dispose of this._dispose) dispose(); @@ -66,6 +71,8 @@ export class Waiter { async waitForPromise(promise: Promise, dispose?: () => void): Promise { try { + if (this._immediateError) + throw this._immediateError; const result = await Promise.race([promise, ...this._failures]); if (dispose) dispose(); diff --git a/src/dispatchers/playwrightDispatcher.ts b/src/dispatchers/playwrightDispatcher.ts index 92c8c9aebb..4913bdca0c 100644 --- a/src/dispatchers/playwrightDispatcher.ts +++ b/src/dispatchers/playwrightDispatcher.ts @@ -21,11 +21,10 @@ import { BrowserTypeDispatcher } from './browserTypeDispatcher'; import { Dispatcher, DispatcherScope } from './dispatcher'; import { ElectronDispatcher } from './electronDispatcher'; import { SelectorsDispatcher } from './selectorsDispatcher'; -import type { BrowserDispatcher } from './browserDispatcher'; import * as types from '../server/types'; export class PlaywrightDispatcher extends Dispatcher implements channels.PlaywrightChannel { - constructor(scope: DispatcherScope, playwright: Playwright, customSelectors?: SelectorsDispatcher, preLaunchedBrowser?: BrowserDispatcher) { + constructor(scope: DispatcherScope, playwright: Playwright, customSelectors?: channels.SelectorsChannel, preLaunchedBrowser?: channels.BrowserChannel) { const descriptors = require('../server/deviceDescriptors') as types.Devices; const deviceDescriptors = Object.entries(descriptors) .map(([name, descriptor]) => ({ name, descriptor })); diff --git a/src/remote/playwrightServer.ts b/src/remote/playwrightServer.ts index 57d35c6a15..669bbbd474 100644 --- a/src/remote/playwrightServer.ts +++ b/src/remote/playwrightServer.ts @@ -28,7 +28,7 @@ const debugLog = debug('pw:server'); export interface PlaywrightServerDelegate { path: string; allowMultipleClients: boolean; - onConnect(rootScope: DispatcherScope): () => any; + onConnect(rootScope: DispatcherScope, forceDisconnect: () => void): () => any; onClose: () => any; } @@ -94,9 +94,10 @@ export class PlaywrightServer { connection.dispatch(JSON.parse(Buffer.from(message).toString())); }); + const forceDisconnect = () => socket.close(); const scope = connection.rootDispatcher(); - const onDisconnect = this._delegate.onConnect(scope); - const disconnect = () => { + const onDisconnect = this._delegate.onConnect(scope, forceDisconnect); + const disconnected = () => { this._clientsCount--; // Avoid sending any more messages over closed socket. connection.onmessage = () => {}; @@ -104,11 +105,11 @@ export class PlaywrightServer { }; socket.on('close', () => { debugLog('Client closed'); - disconnect(); + disconnected(); }); socket.on('error', error => { debugLog('Client error ' + error); - disconnect(); + disconnected(); }); }); @@ -122,6 +123,7 @@ export class PlaywrightServer { // First disconnect all remaining clients. await new Promise(f => this._wsServer!.close(f)); await new Promise(f => this._wsServer!.options.server!.close(f)); + this._wsServer = undefined; await this._delegate.onClose(); } } diff --git a/tests/browsertype-connect.spec.ts b/tests/browsertype-connect.spec.ts index b73a675743..15d8e5ad43 100644 --- a/tests/browsertype-connect.spec.ts +++ b/tests/browsertype-connect.spec.ts @@ -79,7 +79,10 @@ test('disconnected event should be emitted when browser is closed or server is c const remoteServer = await startRemoteServer(); const browser1 = await browserType.connect({ wsEndpoint: remoteServer.wsEndpoint() }); + await browser1.newPage(); + const browser2 = await browserType.connect({ wsEndpoint: remoteServer.wsEndpoint() }); + await browser2.newPage(); let disconnected1 = 0; let disconnected2 = 0; @@ -141,6 +144,19 @@ test('should throw when used after isConnected returns false', async ({browserTy expect(error.message).toContain('has been closed'); }); +test('should throw when calling waitForNavigation after disconnect', async ({browserType, startRemoteServer}) => { + const remoteServer = await startRemoteServer(); + const browser = await browserType.connect({ wsEndpoint: remoteServer.wsEndpoint() }); + const page = await browser.newPage(); + await Promise.all([ + remoteServer.close(), + new Promise(f => browser.once('disconnected', f)), + ]); + expect(browser.isConnected()).toBe(false); + const error = await page.waitForNavigation().catch(e => e); + expect(error.message).toContain('Navigation failed because page was closed'); +}); + test('should reject navigation when browser closes', async ({browserType, startRemoteServer, server}) => { const remoteServer = await startRemoteServer(); server.setRoute('/one-style.css', () => {}); @@ -150,7 +166,7 @@ test('should reject navigation when browser closes', async ({browserType, startR await server.waitForRequest('/one-style.css'); await browser.close(); const error = await navigationPromise; - expect(error.message).toContain('Navigation failed because page was closed!'); + expect(error.message).toContain('has been closed'); }); test('should reject waitForSelector when browser closes', async ({browserType, startRemoteServer, server}) => { @@ -165,7 +181,7 @@ test('should reject waitForSelector when browser closes', async ({browserType, s await browser.close(); const error = await watchdog; - expect(error.message).toContain('Protocol error'); + expect(error.message).toContain('has been closed'); }); test('should emit close events on pages and contexts', async ({browserType, startRemoteServer}) => { @@ -360,3 +376,45 @@ test('should work with cluster', async ({browserType, startRemoteServer}) => { const page = await browser.newPage(); expect(await page.evaluate('1 + 2')).toBe(3); }); + +test('should properly disconnect when connection closes from the client side', async ({browserType, startRemoteServer, server}) => { + server.setRoute('/one-style.css', () => {}); + const remoteServer = await startRemoteServer(); + const browser = await browserType.connect({ wsEndpoint: remoteServer.wsEndpoint() }); + const page = await browser.newPage(); + const navigationPromise = page.goto(server.PREFIX + '/one-style.html', {timeout: 60000}).catch(e => e); + const waitForNavigationPromise = page.waitForNavigation().catch(e => e); + + const disconnectedPromise = new Promise(f => browser.once('disconnected', f)); + // This closes the websocket. + (browser as any)._connection.close(); + await disconnectedPromise; + expect(browser.isConnected()).toBe(false); + + expect((await navigationPromise).message).toContain('has been closed'); + expect((await waitForNavigationPromise).message).toContain('Navigation failed because page was closed'); + expect((await page.goto(server.EMPTY_PAGE).catch(e => e)).message).toContain('has been closed'); + expect((await page.waitForNavigation().catch(e => e)).message).toContain('Navigation failed because page was closed'); +}); + +test('should properly disconnect when connection closes from the server side', async ({browserType, startRemoteServer, server, platform}) => { + test.skip(platform === 'win32', 'Cannot send signals'); + + server.setRoute('/one-style.css', () => {}); + const remoteServer = await startRemoteServer({ disconnectOnSIGHUP: true }); + const browser = await browserType.connect({ wsEndpoint: remoteServer.wsEndpoint() }); + const page = await browser.newPage(); + const navigationPromise = page.goto(server.PREFIX + '/one-style.html', {timeout: 60000}).catch(e => e); + const waitForNavigationPromise = page.waitForNavigation().catch(e => e); + + const disconnectedPromise = new Promise(f => browser.once('disconnected', f)); + // This closes the websocket server. + process.kill(remoteServer.child().pid, 'SIGHUP'); + await disconnectedPromise; + expect(browser.isConnected()).toBe(false); + + expect((await navigationPromise).message).toContain('has been closed'); + expect((await waitForNavigationPromise).message).toContain('Navigation failed because page was closed'); + expect((await page.goto(server.EMPTY_PAGE).catch(e => e)).message).toContain('has been closed'); + expect((await page.waitForNavigation().catch(e => e)).message).toContain('Navigation failed because page was closed'); +}); diff --git a/tests/config/remote-server-impl.js b/tests/config/remote-server-impl.js index f21175e5aa..70a5ffaf72 100644 --- a/tests/config/remote-server-impl.js +++ b/tests/config/remote-server-impl.js @@ -1,7 +1,7 @@ const cluster = require('cluster'); async function start() { - const { playwrightPath, browserTypeName, launchOptions, stallOnClose } = JSON.parse(process.argv[2]); + const { playwrightPath, browserTypeName, launchOptions, stallOnClose, disconnectOnSIGHUP } = JSON.parse(process.argv[2]); if (stallOnClose) { launchOptions.__testHookGracefullyClose = () => { console.log(`(stalled=>true)`); @@ -11,7 +11,12 @@ async function start() { const playwright = require(require('path').join(playwrightPath, 'index')); + if (disconnectOnSIGHUP) + launchOptions.handleSIGHUP = false; const browserServer = await playwright[browserTypeName].launchServer(launchOptions); + if (disconnectOnSIGHUP) + process.on('SIGHUP', () => browserServer._disconnectForTest()); + browserServer.on('close', (exitCode, signal) => { console.log(`(exitCode=>${exitCode})`); console.log(`(signal=>${signal})`); @@ -20,6 +25,9 @@ async function start() { console.log(`(wsEndpoint=>${browserServer.wsEndpoint()})`); } +process.on('uncaughtException', error => console.log(error)); +process.on('unhandledRejection', reason => console.log(reason)); + if (cluster.isWorker || !JSON.parse(process.argv[2]).inCluster) { start(); } else { diff --git a/tests/config/remoteServer.ts b/tests/config/remoteServer.ts index 9db4ff17d3..1ec8bde955 100644 --- a/tests/config/remoteServer.ts +++ b/tests/config/remoteServer.ts @@ -22,6 +22,7 @@ const playwrightPath = path.join(__dirname, '..', '..'); export type RemoteServerOptions = { stallOnClose?: boolean; + disconnectOnSIGHUP?: boolean; inCluster?: boolean; url?: string; }; diff --git a/tests/page/page-wait-for-url.spec.ts b/tests/page/page-wait-for-url.spec.ts index a0409a6e52..fa9305a6e8 100644 --- a/tests/page/page-wait-for-url.spec.ts +++ b/tests/page/page-wait-for-url.spec.ts @@ -24,9 +24,9 @@ it('should work', async ({page, server}) => { }); it('should respect timeout', async ({page, server}) => { - const promise = page.waitForURL('**/frame.html', { timeout: 2500 }); + const promise = page.waitForURL('**/frame.html', { timeout: 2500 }).catch(e => e); await page.goto(server.EMPTY_PAGE); - const error = await promise.catch(e => e); + const error = await promise; expect(error.message).toContain('page.waitForNavigation: Timeout 2500ms exceeded.'); });