From 28bad6909344b462cf1d7fcce3b74361de114527 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Wed, 8 Jan 2020 13:55:38 -0800 Subject: [PATCH] fix(server): tidy up BrowserServer.close methods (#426) --- src/chromium/crBrowser.ts | 3 +- src/firefox/ffBrowser.ts | 2 ++ src/server/crPlaywright.ts | 57 +++++++++++++++++------------------ src/server/ffPlaywright.ts | 47 +++++++++++++++-------------- src/server/processLauncher.ts | 23 +++++++++++--- src/server/wkPlaywright.ts | 46 ++++++++++++++-------------- src/webkit/wkBrowser.ts | 6 +++- src/webkit/wkConnection.ts | 15 +++------ src/webkit/wkPage.ts | 1 - test/firefox/browser.spec.js | 2 +- 10 files changed, 105 insertions(+), 97 deletions(-) diff --git a/src/chromium/crBrowser.ts b/src/chromium/crBrowser.ts index 831a50d455..02ca37020b 100644 --- a/src/chromium/crBrowser.ts +++ b/src/chromium/crBrowser.ts @@ -235,8 +235,9 @@ export class CRBrowser extends browser.Browser { } async close() { + const disconnected = new Promise(f => this._connection.once(ConnectionEvents.Disconnected, f)); await this._connection.rootSession.send('Browser.close'); - this.disconnect(); + await disconnected; } browserTarget(): CRTarget { diff --git a/src/firefox/ffBrowser.ts b/src/firefox/ffBrowser.ts index 2d76e40644..4d1cc66b3b 100644 --- a/src/firefox/ffBrowser.ts +++ b/src/firefox/ffBrowser.ts @@ -149,7 +149,9 @@ export class FFBrowser extends browser.Browser { async close() { helper.removeEventListeners(this._eventListeners); + const disconnected = new Promise(f => this._connection.once(ConnectionEvents.Disconnected, f)); await this._connection.send('Browser.close'); + await disconnected; } _createBrowserContext(browserContextId: string | null, options: BrowserContextOptions): BrowserContext { diff --git a/src/server/crPlaywright.ts b/src/server/crPlaywright.ts index c95f4df37d..2095e29b68 100644 --- a/src/server/crPlaywright.ts +++ b/src/server/crPlaywright.ts @@ -57,10 +57,12 @@ export type LaunchOptions = ChromeArgOptions & SlowMoOptions & { export class CRBrowserServer { private _process: ChildProcess; + private _gracefullyClose: () => Promise; private _connectOptions: CRConnectOptions; - constructor(process: ChildProcess, connectOptions: CRConnectOptions) { + constructor(process: ChildProcess, gracefullyClose: () => Promise, connectOptions: CRConnectOptions) { this._process = process; + this._gracefullyClose = gracefullyClose; this._connectOptions = connectOptions; } @@ -81,10 +83,7 @@ export class CRBrowserServer { } async close(): Promise { - const transport = await createTransport(this._connectOptions); - const connection = new CRConnection(transport); - await connection.rootSession.send('Browser.close'); - connection.dispose(); + await this._gracefullyClose(); } } @@ -151,7 +150,7 @@ export class CRPlaywright { const usePipe = chromeArguments.includes('--remote-debugging-pipe'); - const launchedProcess = await launchProcess({ + const { launchedProcess, gracefullyClose } = await launchProcess({ executablePath: chromeExecutable, args: chromeArguments, env, @@ -160,33 +159,31 @@ export class CRPlaywright { handleSIGHUP, dumpio, pipe: usePipe, - tempDir: temporaryUserDataDir - }, () => { - if (temporaryUserDataDir || !server) - return Promise.reject(); - return server.close(); + tempDir: temporaryUserDataDir, + attemptToGracefullyClose: async () => { + if (!connectOptions) + return Promise.reject(); + + // We try to gracefully close to prevent crash reporting and core dumps. + // Note that it's fine to reuse the pipe transport, since + // our connection is tolerant to unknown responses. + const transport = await createTransport(connectOptions); + const connection = new CRConnection(transport); + connection.rootSession.send('Browser.close'); + }, }); - let server: CRBrowserServer | undefined; - try { - let connectOptions: CRConnectOptions | undefined; - let browserWSEndpoint: string = ''; - if (!usePipe) { - const timeoutError = new TimeoutError(`Timed out after ${timeout} ms while trying to connect to Chrome! The only Chrome revision guaranteed to work is r${this._revision}`); - const match = await waitForLine(launchedProcess, launchedProcess.stderr, /^DevTools listening on (ws:\/\/.*)$/, timeout, timeoutError); - browserWSEndpoint = match[1]; - connectOptions = { browserWSEndpoint, slowMo }; - } else { - const transport = new PipeTransport(launchedProcess.stdio[3] as NodeJS.WritableStream, launchedProcess.stdio[4] as NodeJS.ReadableStream); - connectOptions = { slowMo, transport }; - } - server = new CRBrowserServer(launchedProcess, connectOptions); - return server; - } catch (e) { - if (server) - await server.close(); - throw e; + let connectOptions: CRConnectOptions | undefined; + if (!usePipe) { + const timeoutError = new TimeoutError(`Timed out after ${timeout} ms while trying to connect to Chrome! The only Chrome revision guaranteed to work is r${this._revision}`); + const match = await waitForLine(launchedProcess, launchedProcess.stderr, /^DevTools listening on (ws:\/\/.*)$/, timeout, timeoutError); + const browserWSEndpoint = match[1]; + connectOptions = { browserWSEndpoint, slowMo }; + } else { + const transport = new PipeTransport(launchedProcess.stdio[3] as NodeJS.WritableStream, launchedProcess.stdio[4] as NodeJS.ReadableStream); + connectOptions = { slowMo, transport }; } + return new CRBrowserServer(launchedProcess, gracefullyClose, connectOptions); } async connect(options: CRConnectOptions): Promise { diff --git a/src/server/ffPlaywright.ts b/src/server/ffPlaywright.ts index 613c10f38d..73a22353a8 100644 --- a/src/server/ffPlaywright.ts +++ b/src/server/ffPlaywright.ts @@ -33,10 +33,12 @@ import { assert } from '../helper'; export class FFBrowserServer { private _process: ChildProcess; + private _gracefullyClose: () => Promise; private _connectOptions: FFConnectOptions; - constructor(process: ChildProcess, connectOptions: FFConnectOptions) { + constructor(process: ChildProcess, gracefullyClose: () => Promise, connectOptions: FFConnectOptions) { this._process = process; + this._gracefullyClose = gracefullyClose; this._connectOptions = connectOptions; } @@ -57,10 +59,7 @@ export class FFBrowserServer { } async close(): Promise { - const transport = await createTransport(this._connectOptions); - const connection = new FFConnection(transport); - await connection.send('Browser.close'); - connection.dispose(); + await this._gracefullyClose(); } } @@ -123,7 +122,10 @@ export class FFPlaywright { throw new Error(missingText); firefoxExecutable = executablePath; } - const launchedProcess = await launchProcess({ + + let connectOptions: FFConnectOptions | undefined = undefined; + + const { launchedProcess, gracefullyClose } = await launchProcess({ executablePath: firefoxExecutable, args: firefoxArguments, env: os.platform() === 'linux' ? { @@ -136,25 +138,24 @@ export class FFPlaywright { handleSIGHUP, dumpio, pipe: false, - tempDir: temporaryProfileDir - }, () => { - if (temporaryProfileDir || !server) - return Promise.reject(); - server.close(); + tempDir: temporaryProfileDir, + attemptToGracefullyClose: async () => { + if (!connectOptions) + return Promise.reject(); + // We try to gracefully close to prevent crash reporting and core dumps. + // Note that we don't support pipe yet, so there is no issue + // with reusing the same connection - we can always create a new one. + const transport = await createTransport(connectOptions); + const connection = new FFConnection(transport); + connection.send('Browser.close'); + }, }); - let server: FFBrowserServer | undefined; - try { - const timeoutError = new TimeoutError(`Timed out after ${timeout} ms while trying to connect to Firefox!`); - const match = await waitForLine(launchedProcess, launchedProcess.stdout, /^Juggler listening on (ws:\/\/.*)$/, timeout, timeoutError); - const url = match[1]; - server = new FFBrowserServer(launchedProcess, { browserWSEndpoint: url, slowMo }); - return server; - } catch (e) { - if (server) - await server.close(); - throw e; - } + const timeoutError = new TimeoutError(`Timed out after ${timeout} ms while trying to connect to Firefox!`); + const match = await waitForLine(launchedProcess, launchedProcess.stdout, /^Juggler listening on (ws:\/\/.*)$/, timeout, timeoutError); + const url = match[1]; + connectOptions = { browserWSEndpoint: url, slowMo }; + return new FFBrowserServer(launchedProcess, gracefullyClose, connectOptions); } async connect(options: FFConnectOptions): Promise { diff --git a/src/server/processLauncher.ts b/src/server/processLauncher.ts index f4d6c68200..9ea382578e 100644 --- a/src/server/processLauncher.ts +++ b/src/server/processLauncher.ts @@ -36,9 +36,14 @@ export type LaunchProcessOptions = { dumpio?: boolean, pipe?: boolean, tempDir?: string, + + // Note: attemptToGracefullyClose should reject if it does not close the browser. + attemptToGracefullyClose: () => Promise, }; -export async function launchProcess(options: LaunchProcessOptions, attemptToGracefullyClose: () => Promise): Promise { +type LaunchResult = { launchedProcess: childProcess.ChildProcess, gracefullyClose: () => Promise }; + +export async function launchProcess(options: LaunchProcessOptions): Promise { let stdio: ('ignore' | 'pipe')[] = ['pipe', 'pipe', 'pipe']; if (options.pipe) { if (options.dumpio) @@ -61,7 +66,7 @@ export async function launchProcess(options: LaunchProcessOptions, attemptToGrac if (!spawnedProcess.pid) { let reject: (e: Error) => void; - const result = new Promise((f, r) => reject = r); + const result = new Promise((f, r) => reject = r); spawnedProcess.once('error', error => { reject(new Error('Failed to launch browser: ' + error)); }); @@ -96,12 +101,20 @@ export async function launchProcess(options: LaunchProcessOptions, attemptToGrac listeners.push(helper.addEventListener(process, 'SIGTERM', gracefullyClose)); if (options.handleSIGHUP) listeners.push(helper.addEventListener(process, 'SIGHUP', gracefullyClose)); - return spawnedProcess; + let gracefullyClosing = false; + return { launchedProcess: spawnedProcess, gracefullyClose }; async function gracefullyClose(): Promise { - helper.removeEventListeners(listeners); - attemptToGracefullyClose().catch(() => killProcess()); + // We keep listeners until we are done, to handle 'exit' and 'SIGINT' while + // asynchronously closing to prevent zombie processes. This might introduce + // reentrancy to this function. + if (gracefullyClosing) + return; + gracefullyClosing = true; + options.attemptToGracefullyClose().catch(() => killProcess()); + // TODO: forcefully kill the process after some timeout. await waitForProcessToClose; + helper.removeEventListeners(listeners); } // This method has to be sync to be used as 'exit' event handler. diff --git a/src/server/wkPlaywright.ts b/src/server/wkPlaywright.ts index bf4a277821..8eece46e6e 100644 --- a/src/server/wkPlaywright.ts +++ b/src/server/wkPlaywright.ts @@ -19,9 +19,8 @@ import { BrowserFetcher, BrowserFetcherOptions, OnProgressCallback, BrowserFetch import { DeviceDescriptors } from '../deviceDescriptors'; import * as Errors from '../errors'; import * as types from '../types'; -import { WKBrowser } from '../webkit/wkBrowser'; -import { WKConnectOptions, createTransport } from '../webkit/wkBrowser'; -import { WKConnection } from '../webkit/wkConnection'; +import { WKBrowser, createTransport } from '../webkit/wkBrowser'; +import { WKConnectOptions } from '../webkit/wkBrowser'; import { execSync, ChildProcess } from 'child_process'; import { PipeTransport } from './pipeTransport'; import { launchProcess } from './processLauncher'; @@ -29,6 +28,7 @@ import * as path from 'path'; import * as util from 'util'; import * as os from 'os'; import { assert } from '../helper'; +import { kBrowserCloseMessageId } from '../webkit/wkConnection'; export type LaunchOptions = { ignoreDefaultArgs?: boolean, @@ -46,10 +46,12 @@ export type LaunchOptions = { export class WKBrowserServer { private _process: ChildProcess; + private _gracefullyClose: () => Promise; private _connectOptions: WKConnectOptions; - constructor(process: ChildProcess, connectOptions: WKConnectOptions) { + constructor(process: ChildProcess, gracefullyClose: () => Promise, connectOptions: WKConnectOptions) { this._process = process; + this._gracefullyClose = gracefullyClose; this._connectOptions = connectOptions; } @@ -66,10 +68,7 @@ export class WKBrowserServer { } async close(): Promise { - const transport = await createTransport(this._connectOptions); - const connection = WKConnection.from(transport); - await connection.send('Browser.close'); - connection.dispose(); + await this._gracefullyClose(); } } @@ -125,7 +124,9 @@ export class WKPlaywright { if (process.platform === 'darwin' && options.headless !== false) webkitArguments.push('--headless'); - const launchedProcess = await launchProcess({ + let connectOptions: WKConnectOptions | undefined = undefined; + + const { launchedProcess, gracefullyClose } = await launchProcess({ executablePath: webkitExecutable, args: webkitArguments, env, @@ -134,23 +135,20 @@ export class WKPlaywright { handleSIGHUP, dumpio, pipe: true, - tempDir: null - }, () => { - if (!server) - return Promise.reject(); - server.close(); + tempDir: null, + attemptToGracefullyClose: async () => { + if (!connectOptions) + return Promise.reject(); + // We try to gracefully close to prevent crash reporting and core dumps. + const transport = await createTransport(connectOptions); + const message = JSON.stringify({method: 'Browser.close', params: {}, id: kBrowserCloseMessageId}); + transport.send(message); + }, }); - let server: WKBrowserServer | undefined; - try { - const transport = new PipeTransport(launchedProcess.stdio[3] as NodeJS.WritableStream, launchedProcess.stdio[4] as NodeJS.ReadableStream); - server = new WKBrowserServer(launchedProcess, { transport, slowMo }); - return server; - } catch (e) { - if (server) - await server.close(); - throw e; - } + const transport = new PipeTransport(launchedProcess.stdio[3] as NodeJS.WritableStream, launchedProcess.stdio[4] as NodeJS.ReadableStream); + connectOptions = { transport, slowMo }; + return new WKBrowserServer(launchedProcess, gracefullyClose, connectOptions); } executablePath(): string { diff --git a/src/webkit/wkBrowser.ts b/src/webkit/wkBrowser.ts index bd92fe4728..cbe430cddf 100644 --- a/src/webkit/wkBrowser.ts +++ b/src/webkit/wkBrowser.ts @@ -22,6 +22,7 @@ import * as network from '../network'; import { Page } from '../page'; import { ConnectionTransport, SlowMoTransport } from '../transport'; import * as types from '../types'; +import { Events } from '../events'; import { Protocol } from './protocol'; import { WKConnection, WKConnectionEvents, WKPageProxySession } from './wkConnection'; import { WKPageProxy } from './wkPageProxy'; @@ -51,7 +52,8 @@ export class WKBrowser extends browser.Browser { constructor(transport: ConnectionTransport) { super(); - this._connection = WKConnection.from(transport); + this._connection = new WKConnection(transport); + this._connection.on(WKConnectionEvents.Disconnected, () => this.emit(Events.Browser.Disconnected)); this._defaultContext = this._createBrowserContext(undefined, {}); @@ -127,7 +129,9 @@ export class WKBrowser extends browser.Browser { async close() { helper.removeEventListeners(this._eventListeners); + const disconnected = new Promise(f => this._connection.once(WKConnectionEvents.Disconnected, f)); await this._connection.send('Browser.close'); + await disconnected; } _createBrowserContext(browserContextId: string | undefined, options: BrowserContextOptions): BrowserContext { diff --git a/src/webkit/wkConnection.ts b/src/webkit/wkConnection.ts index 078cdce77c..1a8f6b5dea 100644 --- a/src/webkit/wkConnection.ts +++ b/src/webkit/wkConnection.ts @@ -24,6 +24,7 @@ const debugProtocol = platform.debug('playwright:protocol'); const debugWrappedMessage = platform.debug('wrapped'); export const WKConnectionEvents = { + Disconnected: Symbol('Disconnected'), PageProxyCreated: Symbol('ConnectionEvents.PageProxyCreated'), PageProxyDestroyed: Symbol('Connection.PageProxyDestroyed') }; @@ -34,7 +35,7 @@ export const WKPageProxySessionEvents = { DidCommitProvisionalTarget: Symbol('PageProxyEvents.DidCommitProvisionalTarget'), }; -const kConnectionSymbol = Symbol(); +export const kBrowserCloseMessageId = -9999; export class WKConnection extends platform.EventEmitter { private _lastId = 0; @@ -44,15 +45,6 @@ export class WKConnection extends platform.EventEmitter { private _closed = false; - static from(transport: ConnectionTransport): WKConnection { - let connection = (transport as any)[kConnectionSymbol]; - if (!connection) { - connection = new WKConnection(transport); - (transport as any)[kConnectionSymbol] = connection; - } - return connection; - } - constructor(transport: ConnectionTransport) { super(); this._transport = transport; @@ -96,7 +88,7 @@ export class WKConnection extends platform.EventEmitter { callback.reject(createProtocolError(callback.error, callback.method, object)); else callback.resolve(object.result); - } else { + } else if (object.id !== kBrowserCloseMessageId) { assert(this._closed, 'Received response for unknown callback: ' + object.id); } } else { @@ -135,6 +127,7 @@ export class WKConnection extends platform.EventEmitter { for (const pageProxySession of this._pageProxySessions.values()) pageProxySession.dispose(); this._pageProxySessions.clear(); + this.emit(WKConnectionEvents.Disconnected); } dispose() { diff --git a/src/webkit/wkPage.ts b/src/webkit/wkPage.ts index 80f9406018..30d6921e3a 100644 --- a/src/webkit/wkPage.ts +++ b/src/webkit/wkPage.ts @@ -27,7 +27,6 @@ import { WKWorkers } from './wkWorkers'; import { Page, PageDelegate, Coverage } from '../page'; import { Protocol } from './protocol'; import * as dialog from '../dialog'; -import { WKBrowser } from './wkBrowser'; import { BrowserContext } from '../browserContext'; import { RawMouseImpl, RawKeyboardImpl } from './wkInput'; import * as types from '../types'; diff --git a/test/firefox/browser.spec.js b/test/firefox/browser.spec.js index 183b1847cc..75e982e71c 100644 --- a/test/firefox/browser.spec.js +++ b/test/firefox/browser.spec.js @@ -34,7 +34,7 @@ module.exports.describe = function({testRunner, defaultBrowserOptions, playwrigh let output = ''; res.stdout.on('data', data => { output += data; - if (output.indexOf('\n')) + if (output.indexOf('\n') !== -1) wsEndPointCallback(output.substring(0, output.indexOf('\n'))); }); const browser = await playwright.connect({ browserWSEndpoint: await wsEndPointPromise });