Fix Chrome not being killed when disconnecting while it's paused

- And tests not killing chrome
- And the launch test mocking the wrong thing

src/chromeDebugAdapter.ts

@@ -6,7 +6,7 @@ import * as os from 'os';
 import * as path from 'path';
 
 import {ChromeDebugAdapter as CoreDebugAdapter, logger, utils as coreUtils, ISourceMapPathOverrides, stoppedEvent} from 'vscode-chrome-debug-core';
-import {spawn, ChildProcess, fork, exec} from 'child_process';
+import {spawn, ChildProcess, fork, execSync} from 'child_process';
 import Crdp from 'chrome-remote-debug-protocol';
 import {DebugProtocol} from 'vscode-debugprotocol';
 
@@ -146,13 +146,19 @@ export class ChromeDebugAdapter extends CoreDebugAdapter {
     }
 
     public disconnect(): void {
-        if (this._chromeProc && !this._hasTerminated) {
+        const hadTerminated = this._hasTerminated;
+
+        // Disconnect before killing Chrome, because running "taskkill" when it's paused sometimes doesn't kill it
+        super.disconnect();
+
+        if (this._chromeProc && !hadTerminated) {
             // Only kill Chrome if the 'disconnect' originated from vscode. If we previously terminated
             // due to Chrome shutting down, or devtools taking over, don't kill Chrome.
             if (coreUtils.getPlatform() === coreUtils.Platform.Windows && this._chromePID) {
+                // Run synchronously because this process may be killed before exec() would run
                 const taskkillCmd = `taskkill /F /T /PID ${this._chromePID}`;
                 logger.log(`Killing Chrome process by pid: ${taskkillCmd}`);
-                exec(taskkillCmd, (err, stdout, stderr) => { });
+                execSync(taskkillCmd);
             } else {
                 logger.log('Killing Chrome process');
                 this._chromeProc.kill('SIGINT');
@@ -160,8 +166,6 @@ export class ChromeDebugAdapter extends CoreDebugAdapter {
         }
 
         this._chromeProc = null;
-
-        return super.disconnect();
     }
 
     /**
@@ -173,8 +177,7 @@ export class ChromeDebugAdapter extends CoreDebugAdapter {
 
     private spawnChrome(chromePath: string, chromeArgs: string[]): ChildProcess {
         if (coreUtils.getPlatform() === coreUtils.Platform.Windows) {
-            const spawnChromePath = path.join(__dirname, 'chromeSpawnHelper.js');
-            const chromeProc = fork(spawnChromePath, [chromePath, ...chromeArgs], { execArgv: [], silent: true });
+            const chromeProc = fork(getChromeSpawnHelperPath(), [chromePath, ...chromeArgs], { execArgv: [], silent: true });
             chromeProc.unref();
 
             chromeProc.on('message', data => {
@@ -237,3 +240,12 @@ export function resolveWebRootPattern(webRoot: string, sourceMapPathOverrides: I
 
     return resolvedOverrides;
 }
+
+function getChromeSpawnHelperPath(): string {
+    if (path.basename(__dirname) === 'src') {
+        // For tests
+        return path.join(__dirname, '../chromeSpawnHelper.js');
+    } else {
+        return path.join(__dirname, 'chromeSpawnHelper.js');
+    }
+}

test/chromeDebugAdapter.test.ts

@@ -68,33 +68,35 @@ suite('ChromeDebugAdapter', () => {
     });
 
     suite('launch()', () => {
-        let originalSpawn: any;
+        let originalFork: any;
         let originalStatSync: any;
 
         teardown(() => {
             // Hacky mock cleanup
-            require('child_process').spawn = originalSpawn;
+            require('child_process').spawn = originalFork;
             require('fs').statSync = originalStatSync;
         })
 
         test('launches with minimal correct args', () => {
-            let spawnCalled = false;
-            function spawn(chromePath: string, args: string[]): any {
+            let forkCalled = false;
+            function fork(chromeSpawnHelperPath: string, [chromePath, ...args]: string[]): any {
                 // Just assert that the chrome path is some string with 'chrome' in the path, and there are >0 args
+                assert(chromeSpawnHelperPath.indexOf('chromeSpawnHelper.js') >= 0);
                 assert(chromePath.toLowerCase().indexOf('chrome') >= 0);
                 assert(args.indexOf('--remote-debugging-port=9222') >= 0);
                 assert(args.indexOf('file:///c:/path%20with%20space/index.html') >= 0);
                 assert(args.indexOf('abc') >= 0);
                 assert(args.indexOf('def') >= 0);
-                spawnCalled = true;
+                forkCalled = true;
 
-                return { on: () => { }, unref: () => { } };
+                const stdio = { on: () => { } };
+                return { on: () => { }, unref: () => { }, stdout: stdio, stderr: stdio };
             }
 
-            // Mock spawn for chrome process, and 'fs' for finding chrome.exe.
+            // Mock fork for chrome process, and 'fs' for finding chrome.exe.
             // These are mocked as empty above - note that it's too late for mockery here.
-            originalSpawn = require('child_process').spawn;
-            require('child_process').spawn = spawn;
+            originalFork = require('child_process').fork;
+            require('child_process').fork = fork;
             originalStatSync = require('fs').statSync;
             require('fs').statSync = () => true;
 
@@ -108,7 +110,7 @@ suite('ChromeDebugAdapter', () => {
                 .returns(() => Promise.resolve<any>({ result: { type: 'string', value: '123' }}));
 
             return chromeDebugAdapter.launch({ file: 'c:\\path with space\\index.html', runtimeArgs: ['abc', 'def'] })
-                .then(() => assert(spawnCalled));
+                .then(() => assert(forkCalled));
         });
     });