[fix] Terminate chrome gracefully on Windows

[aslushnikov]

This patch starts using taskkill program on windows to gracefully
terminate chrome.

Fixes #839.

[paulirish]

I think you want /T /F on the end of this.

/f : Specifies that process(es) be forcefully terminated. This parameter is ignored for remote processes; all remote processes are forcefully terminated.

/t : Specifies to terminate all child processes along with the parent process, commonly known as a tree kill.

That's what we have in chrome-launcher, at least.

[paulirish]

@vsemozhetbyt does this address the dialog? now reading the issue thread it didn't seem like the /t /f combo was attempted.

[vsemozhetbyt]

I have tried taskkill without these key and that was OK. I do not know if /f is needed since WM_CLOSE do the thing, I do not know either is /t is needed since we use >nul here because:

Chrome generates many processes. The errors are caused when an process does not acknowledge the WM_CLOSE message that TASKKILL sends. Only processes with a message loop will be able to receive the message, therefore, the processes that do not have a message loop will generate that error. Most likely, these processes are the chrome extensions and plugins.

If I get this right, these keys are used in case of WM_CLOSE do not succeed, so maybe they will not do any harm?

[vsemozhetbyt]

I have tried this:

childProcess.execSync(`taskkill /pid ${chromeProcess.pid} /T /F >nul`);

and this DO NOT PREVENT the dialog. So it seems the /F has a privilege and force-closes the browser not-gracefully (without WM_CLOSE or without waiting for all the final clean-up).

[vsemozhetbyt]

As for /t it seems we send WM_CLOSE to the parent here, and all the subprocesses are also terminated (I've checked with process explorer), so there is no need for this key?

[paulirish]

An interesting thing here is that making this change will make killChrome take longer to complete on windows. Many users don't have a need for graceful shutdown and may prefer a forced quit.

However I understand the usecase if you're re-using a user-data-dir. @vsemozhetbyt Have you tried --restore-last-session=false ?

[vsemozhetbyt]

Trying this:

'use strict';

const puppeteer = require('puppeteer');

(async function main() {
  try {
    const browser = await puppeteer.launch({
      headless: false,
      args: ['--restore-last-session=false'],
      userDataDir: `${__dirname}/profile-dir`,
    });
    const page = await browser.newPage();
    await page.goto('https://www.nytimes.com/');

    await page.waitFor(1000);

    await browser.close();
  } catch (err) {
    console.error(err);
  }
})();

I still get the dialog for the second and next runs.

Maybe the graceful exit should be opt-in (or can be opt-out)?

[paulirish]

I still get the dialog for the second and next runs.

ah bummer. worth a shot.

@vsemozhetbyt can you measure the time it takes to kill the process these two ways? i'd load up the browser with a few heavy tabs.

@aslushnikov if you dont care either way, feel free to ignore me and merge :)

[vsemozhetbyt]

@paulirish Let me know if I should measure in other way:

'use strict';

const puppeteer = require('puppeteer');

(async function main() {
  try {
    const browser = await puppeteer.launch({
      headless: false,
      userDataDir: `${__dirname}/profile-dir`,
    });
    const page1 = await browser.newPage();
    await page1.goto('https://www.nytimes.com/');
    const page2 = await browser.newPage();
    await page2.goto('http://www.bbc.com/');
    const page3 = await browser.newPage();
    await page3.goto('http://www.dw.com/');

    await page1.waitFor(1000);

    console.time('test');
    await browser.close();
    console.timeEnd('test');
  } catch (err) {
    console.error(err);
  }
})();

Without this fix, 3 lanches:

166.242ms, 121.525ms, 153.104ms

With this fix, 3 lanches:

1386.207ms, 1179.338ms, 1126.386ms

[vsemozhetbyt]

FWIW, I have tried this:

childProcess.execFileSync('taskkill', ['/pid', `${chromeProcess.pid}`]);

as execFileSync does not use shell, but the gain is too small:

1231.945ms, 1062.428ms, 1099.981ms

BTW, I do not get any errors without >nul (with either execFileSync or execSync), so maybe this redirection is not so necessary,

[aslushnikov]

@paulirish we slow down only for the custom userDataDir argument, this doesn't affect the general case with a temp profile. The slow down itself makes sense to me: we ask chrome to finish its business before closing, and it takes some time in doing so.

@vsemozhetbyt thanks, I removed the redirect.