revise taskkill procedure on windows

.github/workflows/lh-smoke.yml

@@ -0,0 +1,47 @@
+name: smoke
+
+on:
+  push:
+    branches: [master]
+  pull_request: # run on all PRs, not just PRs to a particular branch
+
+jobs:
+  # Only run smoke tests for windows against stable chrome.
+  lh-smoke-windows:
+    strategy:
+      matrix:
+        smoke-test-shard: [1, 2]
+      # e.g. if set 1 fails, continue with set 2 anyway
+      fail-fast: false
+    runs-on: windows-latest
+    name: Windows smoke ${{ matrix.smoke-test-shard }}/2
+
+    steps:
+    - name: git clone
+      uses: actions/checkout@v2
+
+    - name: Use Node.js 14.x
+      uses: actions/setup-node@v1
+      with:
+        node-version: 14.x
+
+    # chrome-launcher
+    # This'll add lighthouse AND install chrome-launcher's deps
+    - run: yarn add --frozen-lockfile --network-timeout 1000000 -D https://github.com/GoogleChrome/lighthouse.git#master
+    - run: yarn build
+
+    # lighthouse 
+    - run: yarn --cwd node_modules/lighthouse/ install --frozen-lockfile --network-timeout 1000000
+    - run: yarn reset-link
+    - run: yarn --cwd node_modules/lighthouse/ build-report
+
+    - name: Run smoke tests
+      # Windows bots are slow, so only run enough tests to verify matching behavior.
+      run: yarn --cwd node_modules/lighthouse/ smoke --debug -j=2 --retries=0 --shard=${{ matrix.smoke-test-shard }}/2 dbw oopif offline lantern metrics
+
+    - name: Upload failures
+      if: failure()
+      uses: actions/upload-artifact@v1
+      with:
+        name: Smokehouse (windows)
+        path: node_modules/lighthouse/.tmp/smokehouse-ci-failures/

package.json

@@ -11,7 +11,8 @@
     "test-formatting": "test/check-formatting.sh",
     "format": "scripts/format.sh",
     "type-check": "tsc --allowJs --checkJs --noEmit --target es2019 *.js",
-    "prepublishOnly": "npm run build && npm run test"
+    "prepublishOnly": "npm run build && npm run test",
+    "reset-link": "(yarn unlink || true) && yarn link && yarn --cwd node_modules/lighthouse/ link chrome-launcher"
   },
   "bin": {
     "print-chrome-path": "bin/print-chrome-path.js"

src/chrome-launcher.ts

@@ -13,9 +13,8 @@ import {getRandomPort} from './random-port';
 import {DEFAULT_FLAGS} from './flags';
 import {makeTmpDir, defaults, delay, getPlatform, toWin32Path, InvalidUserDataDirectoryError, UnsupportedPlatformError, ChromeNotInstalledError} from './utils';
 import {ChildProcess} from 'child_process';
+import {spawn, spawnSync} from 'child_process';
 const log = require('lighthouse-logger');
-const spawn = childProcess.spawn;
-const execSync = childProcess.execSync;
 const isWsl = getPlatform() === 'wsl';
 const isWindows = getPlatform() === 'win32';
 const _SIGINT = 'SIGINT';
@@ -81,7 +80,7 @@ async function launch(opts: Options = {}): Promise<LaunchedChrome> {
     return instance.kill();
   };
 
-  return {pid: instance.pid!, port: instance.port!, kill, process: instance.chrome!};
+  return {pid: instance.pid!, port: instance.port!, kill, process: instance.chromeProc!};
 }
 
 /** Returns Chrome installation path that chrome-launcher will launch by default. */
@@ -126,7 +125,7 @@ class Launcher {
   private useDefaultProfile: boolean;
   private envVars: {[key: string]: string|undefined};
 
-  chrome?: childProcess.ChildProcess;
+  chromeProc?: childProcess.ChildProcess;
   userDataDir?: string;
   port?: number;
   pid?: number;
@@ -175,6 +174,8 @@ class Launcher {
       flags.push(`--user-data-dir=${isWsl ? toWin32Path(this.userDataDir) : this.userDataDir}`);
     }
 
+    if (process.env.HEADLESS) flags.push('--headless');
+
     flags.push(...this.chromeFlags);
     flags.push(this.startingUrl);
 
@@ -276,9 +277,9 @@ class Launcher {
 
   private async spawnProcess(execPath: string) {
     const spawnPromise = (async () => {
-      if (this.chrome) {
-        log.log('ChromeLauncher', `Chrome already running with pid ${this.chrome.pid}.`);
-        return this.chrome.pid;
+      if (this.chromeProc) {
+        log.log('ChromeLauncher', `Chrome already running with pid ${this.chromeProc.pid}.`);
+        return this.chromeProc.pid;
       }
 
 
@@ -292,17 +293,22 @@ class Launcher {
 
       log.verbose(
           'ChromeLauncher', `Launching with command:\n"${execPath}" ${this.flags.join(' ')}`);
-      const chrome = this.spawn(
-          execPath, this.flags,
-          {detached: true, stdio: ['ignore', this.outFile, this.errFile], env: this.envVars});
-      this.chrome = chrome;
+      this.chromeProc = this.spawn(execPath, this.flags, {
+        // On non-windows platforms, `detached: true` makes child process a leader of a new
+        // process group, making it possible to kill child process tree with `.kill(-pid)` command.
+        // @see https://nodejs.org/api/child_process.html#child_process_options_detached
+        detached: process.platform !== 'win32',
+        stdio: ['ignore', this.outFile, this.errFile],
+        env: this.envVars
+      });
 
-      if (chrome.pid) {
-        this.fs.writeFileSync(this.pidFile, chrome.pid.toString());
+      if (this.chromeProc.pid) {
+        this.fs.writeFileSync(this.pidFile, this.chromeProc.pid.toString());
       }
 
-      log.verbose('ChromeLauncher', `Chrome running with pid ${chrome.pid} on port ${this.port}.`);
-      return chrome.pid;
+      log.verbose(
+          'ChromeLauncher', `Chrome running with pid ${this.chromeProc.pid} on port ${this.port}.`);
+      return this.chromeProc.pid;
     })();
 
     const pid = await spawnPromise;
@@ -373,28 +379,30 @@ class Launcher {
   }
 
   kill() {
-    return new Promise<void>((resolve, reject) => {
-      if (this.chrome) {
-        this.chrome.on('close', () => {
-          delete this.chrome;
+    return new Promise<void>((resolve) => {
+      if (this.chromeProc) {
+        this.chromeProc.on('close', () => {
+          delete this.chromeProc;
           this.destroyTmp().then(resolve);
         });
 
-        log.log('ChromeLauncher', `Killing Chrome instance ${this.chrome.pid}`);
+        log.log('ChromeLauncher', `Killing Chrome instance ${this.chromeProc.pid}`);
         try {
           if (isWindows) {
-            // While pipe is the default, stderr also gets printed to process.stderr
-            // if you don't explicitly set `stdio`
-            execSync(`taskkill /pid ${this.chrome.pid} /T /F`, {stdio: 'pipe'});
+            // https://github.com/GoogleChrome/chrome-launcher/issues/266
+            const taskkillProc = spawnSync(
+                `taskkill /pid ${this.chromeProc.pid} /T /F`, {shell: true, encoding: 'utf-8'});
+
+            const {stderr} = taskkillProc;
+            if (stderr) log.error('ChromeLauncher', `taskkill stderr`, stderr);
           } else {
-            if (this.chrome.pid) {
-              process.kill(-this.chrome.pid);
+            if (this.chromeProc.pid) {
+              process.kill(-this.chromeProc.pid, 'SIGKILL');
             }
           }
         } catch (err) {
           const message = `Chrome could not be killed ${err.message}`;
           log.warn('ChromeLauncher', message);
-          reject(new Error(message));
         }
       } else {
         // fail silently as we did not start chrome