fix(server): tidy up BrowserServer.close methods (#426)

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 {

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 {

src/server/crPlaywright.ts

@@ -57,10 +57,12 @@ export type LaunchOptions = ChromeArgOptions & SlowMoOptions & {
 
 export class CRBrowserServer {
   private _process: ChildProcess;
+  private _gracefullyClose: () => Promise<void>;
   private _connectOptions: CRConnectOptions;
 
-  constructor(process: ChildProcess, connectOptions: CRConnectOptions) {
+  constructor(process: ChildProcess, gracefullyClose: () => Promise<void>, connectOptions: CRConnectOptions) {
     this._process = process;
+    this._gracefullyClose = gracefullyClose;
     this._connectOptions = connectOptions;
   }
 
@@ -81,10 +83,7 @@ export class CRBrowserServer {
   }
 
   async close(): Promise<void> {
-    const transport = await createTransport(this._connectOptions);
+    await this._gracefullyClose();
-    const connection = new CRConnection(transport);
-    await connection.rootSession.send('Browser.close');
-    connection.dispose();
   }
 }
 
@@ -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
+      tempDir: temporaryUserDataDir,
-    }, () => {
+      attemptToGracefullyClose: async () => {
-      if (temporaryUserDataDir || !server)
+        if (!connectOptions)
-        return Promise.reject();
+          return Promise.reject();
-      return server.close();
+
+        // 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;
+    let connectOptions: CRConnectOptions | undefined;
-    try {
+    if (!usePipe) {
-      let connectOptions: CRConnectOptions | undefined;
+      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}`);
-      let browserWSEndpoint: string = '';
+      const match = await waitForLine(launchedProcess, launchedProcess.stderr, /^DevTools listening on (ws:\/\/.*)$/, timeout, timeoutError);
-      if (!usePipe) {
+      const browserWSEndpoint = match[1];
-        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}`);
+      connectOptions = { browserWSEndpoint, slowMo };
-        const match = await waitForLine(launchedProcess, launchedProcess.stderr, /^DevTools listening on (ws:\/\/.*)$/, timeout, timeoutError);
+    } else {
-        browserWSEndpoint = match[1];
+      const transport = new PipeTransport(launchedProcess.stdio[3] as NodeJS.WritableStream, launchedProcess.stdio[4] as NodeJS.ReadableStream);
-        connectOptions = { browserWSEndpoint, slowMo };
+      connectOptions = { slowMo, transport };
-      } 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;
     }
+    return new CRBrowserServer(launchedProcess, gracefullyClose, connectOptions);
   }
 
   async connect(options: CRConnectOptions): Promise<CRBrowser> {

src/server/ffPlaywright.ts

@@ -33,10 +33,12 @@ import { assert } from '../helper';
 
 export class FFBrowserServer {
   private _process: ChildProcess;
+  private _gracefullyClose: () => Promise<void>;
   private _connectOptions: FFConnectOptions;
 
-  constructor(process: ChildProcess, connectOptions: FFConnectOptions) {
+  constructor(process: ChildProcess, gracefullyClose: () => Promise<void>, connectOptions: FFConnectOptions) {
     this._process = process;
+    this._gracefullyClose = gracefullyClose;
     this._connectOptions = connectOptions;
   }
 
@@ -57,10 +59,7 @@ export class FFBrowserServer {
   }
 
   async close(): Promise<void> {
-    const transport = await createTransport(this._connectOptions);
+    await this._gracefullyClose();
-    const connection = new FFConnection(transport);
-    await connection.send('Browser.close');
-    connection.dispose();
   }
 }
 
@@ -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
+      tempDir: temporaryProfileDir,
-    }, () => {
+      attemptToGracefullyClose: async () => {
-      if (temporaryProfileDir || !server)
+        if (!connectOptions)
-        return Promise.reject();
+          return Promise.reject();
-      server.close();
+        // 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;
+    const timeoutError = new TimeoutError(`Timed out after ${timeout} ms while trying to connect to Firefox!`);
-    try {
+    const match = await waitForLine(launchedProcess, launchedProcess.stdout, /^Juggler listening on (ws:\/\/.*)$/, timeout, timeoutError);
-      const timeoutError = new TimeoutError(`Timed out after ${timeout} ms while trying to connect to Firefox!`);
+    const url = match[1];
-      const match = await waitForLine(launchedProcess, launchedProcess.stdout, /^Juggler listening on (ws:\/\/.*)$/, timeout, timeoutError);
+    connectOptions = { browserWSEndpoint: url, slowMo };
-      const url = match[1];
+    return new FFBrowserServer(launchedProcess, gracefullyClose, connectOptions);
-      server = new FFBrowserServer(launchedProcess, { browserWSEndpoint: url, slowMo });
-      return server;
-    } catch (e) {
-      if (server)
-        await server.close();
-      throw e;
-    }
   }
 
   async connect(options: FFConnectOptions): Promise<FFBrowser> {

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<any>,
 };
 
-export async function launchProcess(options: LaunchProcessOptions, attemptToGracefullyClose: () => Promise<any>): Promise<childProcess.ChildProcess> {
+type LaunchResult = { launchedProcess: childProcess.ChildProcess, gracefullyClose: () => Promise<void> };
+
+export async function launchProcess(options: LaunchProcessOptions): Promise<LaunchResult> {
   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<childProcess.ChildProcess>((f, r) => reject = r);
+    const result = new Promise<LaunchResult>((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<void> {
-    helper.removeEventListeners(listeners);
+    // We keep listeners until we are done, to handle 'exit' and 'SIGINT' while
-    attemptToGracefullyClose().catch(() => killProcess());
+    // 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.

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 { WKBrowser, createTransport } from '../webkit/wkBrowser';
-import { WKConnectOptions, createTransport } from '../webkit/wkBrowser';
+import { WKConnectOptions } from '../webkit/wkBrowser';
-import { WKConnection } from '../webkit/wkConnection';
 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<void>;
   private _connectOptions: WKConnectOptions;
 
-  constructor(process: ChildProcess, connectOptions: WKConnectOptions) {
+  constructor(process: ChildProcess, gracefullyClose: () => Promise<void>, connectOptions: WKConnectOptions) {
     this._process = process;
+    this._gracefullyClose = gracefullyClose;
     this._connectOptions = connectOptions;
   }
 
@@ -66,10 +68,7 @@ export class WKBrowserServer {
   }
 
   async close(): Promise<void> {
-    const transport = await createTransport(this._connectOptions);
+    await this._gracefullyClose();
-    const connection = WKConnection.from(transport);
-    await connection.send('Browser.close');
-    connection.dispose();
   }
 }
 
@@ -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
+      tempDir: null,
-    }, () => {
+      attemptToGracefullyClose: async () => {
-      if (!server)
+        if (!connectOptions)
-        return Promise.reject();
+          return Promise.reject();
-      server.close();
+        // 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;
+    const transport = new PipeTransport(launchedProcess.stdio[3] as NodeJS.WritableStream, launchedProcess.stdio[4] as NodeJS.ReadableStream);
-    try {
+    connectOptions = { transport, slowMo };
-      const transport = new PipeTransport(launchedProcess.stdio[3] as NodeJS.WritableStream, launchedProcess.stdio[4] as NodeJS.ReadableStream);
+    return new WKBrowserServer(launchedProcess, gracefullyClose, connectOptions);
-      server = new WKBrowserServer(launchedProcess, { transport, slowMo });
-      return server;
-    } catch (e) {
-      if (server)
-        await server.close();
-      throw e;
-    }
   }
 
   executablePath(): string {

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 {

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() {

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';

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 });