feat: make browserServer.kill() wait for the process to exit (#2375)

This ensures we cleaned everything up.

Author: dgozman

docs/api.md

@@ -3941,8 +3941,9 @@ Emitted when the browser server closes.
 Closes the browser gracefully and makes sure the process is terminated.
 
 #### browserServer.kill()
+- returns: <[Promise]>
 
-Kills the browser process.
+Kills the browser process and waits for the process to exit.
 
 #### browserServer.process()
 - returns: <[ChildProcess]> Spawned browser application process.

src/server/browserServer.ts

@@ -50,13 +50,15 @@ export class WebSocketWrapper {
 
 export class BrowserServer extends EventEmitter {
   private _process: ChildProcess;
-  private _gracefullyClose: (() => Promise<void>);
+  private _gracefullyClose: () => Promise<void>;
+  private _kill: () => Promise<void>;
   _webSocketWrapper: WebSocketWrapper | null = null;
 
-  constructor(process: ChildProcess, gracefullyClose: () => Promise<void>) {
+  constructor(process: ChildProcess, gracefullyClose: () => Promise<void>, kill: () => Promise<void>) {
     super();
     this._process = process;
     this._gracefullyClose = gracefullyClose;
+    this._kill = kill;
   }
 
   process(): ChildProcess {
@@ -67,8 +69,8 @@ export class BrowserServer extends EventEmitter {
     return this._webSocketWrapper ? this._webSocketWrapper.wsEndpoint : '';
   }
 
-  kill() {
-    helper.killProcess(this._process);
+  async kill(): Promise<void> {
+    await this._kill();
   }
 
   async close(): Promise<void> {
@@ -84,7 +86,7 @@ export class BrowserServer extends EventEmitter {
     try {
       await helper.waitWithDeadline(this.close(), '', deadline, ''); // The error message is ignored.
     } catch (ignored) {
-      this.kill();
+      await this.kill(); // Make sure to await actual process exit.
     }
   }
 }

src/server/browserType.ts

@@ -230,7 +230,7 @@ export abstract class BrowserTypeBase implements BrowserType {
     // "Cannot access 'browserServer' before initialization" if something went wrong.
     let transport: ConnectionTransport | undefined = undefined;
     let browserServer: BrowserServer | undefined = undefined;
-    const { launchedProcess, gracefullyClose } = await launchProcess({
+    const { launchedProcess, gracefullyClose, kill } = await launchProcess({
       executablePath: executable,
       args: browserArguments,
       env: this._amendEnvironment(env, userDataDir, executable, browserArguments),
@@ -248,7 +248,7 @@ export abstract class BrowserTypeBase implements BrowserType {
         // our connection ignores kBrowserCloseMessageId.
         this._attemptToGracefullyCloseBrowser(transport!);
       },
-      onkill: (exitCode, signal) => {
+      onExit: (exitCode, signal) => {
         if (browserServer)
           browserServer.emit(Events.BrowserServer.Close, exitCode, signal);
       },
@@ -269,7 +269,7 @@ export abstract class BrowserTypeBase implements BrowserType {
       helper.killProcess(launchedProcess);
       throw e;
     }
-    browserServer = new BrowserServer(launchedProcess, gracefullyClose);
+    browserServer = new BrowserServer(launchedProcess, gracefullyClose, kill);
     return { browserServer, downloadsPath, transport };
   }
 

src/server/electron.ts

@@ -173,7 +173,7 @@ export class Electron  {
 
     const logger = new RootLogger(options.logger);
     const electronArguments = ['--inspect=0', '--remote-debugging-port=0', '--require', path.join(__dirname, 'electronLoader.js'), ...args];
-    const { launchedProcess, gracefullyClose } = await launchProcess({
+    const { launchedProcess, gracefullyClose, kill } = await launchProcess({
       executablePath,
       args: electronArguments,
       env,
@@ -185,7 +185,7 @@ export class Electron  {
       cwd: options.cwd,
       tempDirectories: [],
       attemptToGracefullyClose: () => app!.close(),
-      onkill: (exitCode, signal) => {
+      onExit: (exitCode, signal) => {
         if (app)
           app.emit(ElectronEvents.ElectronApplication.Close, exitCode, signal);
       },
@@ -198,7 +198,7 @@ export class Electron  {
 
     const chromeMatch = await waitForLine(launchedProcess, launchedProcess.stderr, /^DevTools listening on (ws:\/\/.*)$/, helper.timeUntilDeadline(deadline), timeoutError);
     const chromeTransport = await WebSocketTransport.connect(chromeMatch[1], logger, deadline);
-    const browserServer = new BrowserServer(launchedProcess, gracefullyClose);
+    const browserServer = new BrowserServer(launchedProcess, gracefullyClose, kill);
     const browser = await CRBrowser.connect(chromeTransport, { headful: true, logger, persistent: { viewport: null }, ownedServer: browserServer });
     app = new ElectronApplication(logger, browser, nodeConnection);
     await app._init();

src/server/processLauncher.ts

@@ -56,13 +56,14 @@ export type LaunchProcessOptions = {
 
   // Note: attemptToGracefullyClose should reject if it does not close the browser.
   attemptToGracefullyClose: () => Promise<any>,
-  onkill: (exitCode: number | null, signal: string | null) => void,
+  onExit: (exitCode: number | null, signal: string | null) => void,
   logger: RootLogger,
 };
 
 type LaunchResult = {
   launchedProcess: childProcess.ChildProcess,
   gracefullyClose: () => Promise<void>,
+  kill: () => Promise<void>,
 };
 
 export async function launchProcess(options: LaunchProcessOptions): Promise<LaunchResult> {
@@ -110,14 +111,14 @@ export async function launchProcess(options: LaunchProcessOptions): Promise<Laun
 
   let processClosed = false;
   let fulfillClose = () => {};
-  const waitForClose = new Promise(f => fulfillClose = f);
+  const waitForClose = new Promise<void>(f => fulfillClose = f);
   let fulfillCleanup = () => {};
-  const waitForCleanup = new Promise(f => fulfillCleanup = f);
+  const waitForCleanup = new Promise<void>(f => fulfillCleanup = f);
   spawnedProcess.once('exit', (exitCode, signal) => {
     logger._log(browserLog, `<process did exit ${exitCode}, ${signal}>`);
     processClosed = true;
     helper.removeEventListeners(listeners);
-    options.onkill(exitCode, signal);
+    options.onExit(exitCode, signal);
     fulfillClose();
     // Cleanup as process exits.
     cleanup().then(fulfillCleanup);
@@ -175,7 +176,12 @@ export async function launchProcess(options: LaunchProcessOptions): Promise<Laun
     } catch (e) { }
   }
 
-  return { launchedProcess: spawnedProcess, gracefullyClose };
+  function killAndWait() {
+    killProcess();
+    return waitForCleanup;
+  }
+
+  return { launchedProcess: spawnedProcess, gracefullyClose, kill: killAndWait };
 }
 
 export function waitForLine(process: childProcess.ChildProcess, inputStream: stream.Readable, regex: RegExp, timeout: number, timeoutError: TimeoutError): Promise<RegExpMatchArray> {

test/launcher.spec.js

@@ -223,8 +223,8 @@ describe('Browser.close', function() {
   });
 });
 
-describe('browserType.launch |webSocket| option', function() {
-  it('should support the webSocket option', async({browserType, defaultBrowserOptions}) => {
+describe('browserType.launchServer', function() {
+  it('should work', async({browserType, defaultBrowserOptions}) => {
     const browserServer = await browserType.launchServer(defaultBrowserOptions);
     const browser = await browserType.connect({ wsEndpoint: browserServer.wsEndpoint() });
     const browserContext = await browser.newContext();
@@ -237,7 +237,7 @@ describe('browserType.launch |webSocket| option', function() {
     await browserServer._checkLeaks();
     await browserServer.close();
   });
-  it('should fire "disconnected" when closing with webSocket', async({browserType, defaultBrowserOptions}) => {
+  it('should fire "disconnected" when closing the server', async({browserType, defaultBrowserOptions}) => {
     const browserServer = await browserType.launchServer(defaultBrowserOptions);
     const browser = await browserType.connect({ wsEndpoint: browserServer.wsEndpoint() });
     const disconnectedEventPromise = new Promise(resolve => browser.once('disconnected', resolve));
@@ -248,6 +248,19 @@ describe('browserType.launch |webSocket| option', function() {
       closedPromise,
     ]);
   });
+  it('should fire "close" event during kill', async({browserType, defaultBrowserOptions}) => {
+    const order = [];
+    const browserServer = await browserType.launchServer(defaultBrowserOptions);
+    const closedPromise = new Promise(f => browserServer.on('close', () => {
+      order.push('closed');
+      f();
+    }));
+    await Promise.all([
+      browserServer.kill().then(() => order.push('killed')),
+      closedPromise,
+    ]);
+    expect(order).toEqual(['closed', 'killed']);
+  });
 });
 
 describe('browserType.connect', function() {