From 03ab1c1b9c755cd56e78763fb3050078eab3cc23 Mon Sep 17 00:00:00 2001
From: Tim van der Lippe <TimvdLippe@users.noreply.github.com>
Date: Mon, 15 Jun 2020 14:02:00 +0100
Subject: [PATCH] fix: improve Ctrl + C support (#6011)

Fix child process killing when the parent process SIGINTs.

If you `ctrl + c` the Puppeteer parent process, we would sometimes not properly handle killing of the child processes. This would then leave child processes behind, with running Chromium instances. This in turn could block Puppeteer from launching again and results in
cryptic errors.

Instead of using the generic `process.kill` with the process id (which for some reason is negative the pid, which I don't get), we can kill the child process directly by calling `proc.kill`.

Fixes #5729.
Fixes #4796.
Fixes #4963.
Fixes #4333.
Fixes #1825.
---
 mocha-config/base.js          |  2 ++
 src/launcher/BrowserRunner.ts | 36 +++++++++++++++++++++++------------
 test/mocha-utils.js           |  2 +-
 3 files changed, 27 insertions(+), 13 deletions(-)

diff --git a/mocha-config/base.js b/mocha-config/base.js
index 6b46c08a712a2..dc0b04d73ffdb 100644
--- a/mocha-config/base.js
+++ b/mocha-config/base.js
@@ -1,4 +1,6 @@
 module.exports = {
   reporter: 'dot',
+  // Allow `console.log`s to show up during test execution
+  logLevel: 'debug',
   exit: !!process.env.CI,
 };
diff --git a/src/launcher/BrowserRunner.ts b/src/launcher/BrowserRunner.ts
index b6785af8ca9c7..29629a5b57ff6 100644
--- a/src/launcher/BrowserRunner.ts
+++ b/src/launcher/BrowserRunner.ts
@@ -28,6 +28,10 @@ import { TimeoutError } from '../Errors';
 
 const removeFolderAsync = helper.promisify(removeFolder);
 const debugLauncher = debug('puppeteer:launcher');
+const PROCESS_ERROR_EXPLANATION = `Puppeteer was unable to kill the process which ran the browser binary.
+This means that, on future Puppeteer launches, Puppeteer might not be able to launch the browser.
+Please check your open processes and ensure that the browser processes that Puppeteer launched have been killed.
+If you think this is a bug, please report it on the Puppeteer issue tracker.`;
 
 export class BrowserRunner {
   private _executablePath: string;
@@ -51,7 +55,7 @@ export class BrowserRunner {
     this._tempDirectory = tempDirectory;
   }
 
-  start(options: LaunchOptions = {}): void {
+  start(options: LaunchOptions): void {
     const {
       handleSIGINT,
       handleSIGTERM,
@@ -121,7 +125,6 @@ export class BrowserRunner {
 
   close(): Promise<void> {
     if (this._closed) return Promise.resolve();
-    helper.removeEventListeners(this._listeners);
     if (this._tempDirectory) {
       this.kill();
     } else if (this.connection) {
@@ -131,24 +134,33 @@ export class BrowserRunner {
         this.kill();
       });
     }
+    // Cleanup this listener last, as that makes sure the full callback runs. If we
+    // perform this earlier, then the previous function calls would not happen.
+    helper.removeEventListeners(this._listeners);
     return this._processClosing;
   }
 
   kill(): void {
-    helper.removeEventListeners(this._listeners);
-    if (this.proc && this.proc.pid && !this.proc.killed && !this._closed) {
-      try {
-        if (process.platform === 'win32')
-          childProcess.execSync(`taskkill /pid ${this.proc.pid} /T /F`);
-        else process.kill(-this.proc.pid, 'SIGKILL');
-      } catch (error) {
-        // the process might have already stopped
-      }
-    }
     // Attempt to remove temporary profile directory to avoid littering.
     try {
       removeFolder.sync(this._tempDirectory);
     } catch (error) {}
+
+    // If the process failed to launch (for example if the browser executable path
+    // is invalid), then the process does not get a pid assigned. A call to
+    // `proc.kill` would error, as the `pid` to-be-killed can not be found.
+    if (this.proc && this.proc.pid && !this.proc.killed) {
+      try {
+        this.proc.kill('SIGKILL');
+      } catch (error) {
+        throw new Error(
+          `${PROCESS_ERROR_EXPLANATION}\nError cause: ${error.stack}`
+        );
+      }
+    }
+    // Cleanup this listener last, as that makes sure the full callback runs. If we
+    // perform this earlier, then the previous function calls would not happen.
+    helper.removeEventListeners(this._listeners);
   }
 
   async setupConnection(options: {
diff --git a/test/mocha-utils.js b/test/mocha-utils.js
index 80ab4768b69dc..469545e2207a7 100644
--- a/test/mocha-utils.js
+++ b/test/mocha-utils.js
@@ -71,7 +71,7 @@ try {
 
 const defaultBrowserOptions = Object.assign(
   {
-    handleSIGINT: false,
+    handleSIGINT: true,
     executablePath: process.env.BINARY,
     slowMo: false,
     headless: isHeadless,