feat(Browser): make browser.close() to always terminate remote browser

This patch: - changes `browser.close` to terminate browser. - introduces new `browser.disconnect` to disconnect from a browser without closing it This patch: fixes puppeteer#918, fixes puppeteer#989 BREAKING CHANGE: `browser.close()` will always close a browser, even if it was initialized with `puppeteer.connect`. To disconnect from a remote browser, use `browser.disconnect()` instead.
jhob · Oct 17, 2017 · 2b79514 · 2b79514
1 parent fbee98a
commit 2b79514
Show file tree

Hide file tree

Showing 4 changed files with 58 additions and 27 deletions.
diff --git a/docs/api.md b/docs/api.md
@@ -14,6 +14,7 @@
   * [puppeteer.launch([options])](#puppeteerlaunchoptions)
 - [class: Browser](#class-browser)
   * [browser.close()](#browserclose)
+  * [browser.disconnect()](#browserdisconnect)
   * [browser.newPage()](#browsernewpage)
   * [browser.version()](#browserversion)
   * [browser.wsEndpoint()](#browserwsendpoint)
@@ -259,10 +260,31 @@ puppeteer.launch().then(async browser => {
 });
 ```
 
+An example of disconnecting from and reconnecting to a [Browser]:
+```js
+const puppeteer = require('puppeteer');
+
+puppeteer.launch().then(async browser => {
+  // Store the endpoint to be able to reconnect to Chromium
+  const browserWSEndpoint = browser.wsEndpoint();
+  // Disconnect puppeteer from Chromium
+  browser.disconnect();
+
+  // Use the endpoint to reestablish a connection
+  const browser2 = await puppeteer.connect({browserWSEndpoint});
+  // Close Chromium
+  await browser2.close();
+});
+```
+
 #### browser.close()
 - returns: <[Promise]>
 
-Closes browser with all the pages (if any were opened). The browser object itself is considered to be disposed and can not be used anymore.
+Closes Chromium and all of its pages (if any were opened). The browser object itself is considered disposed and cannot be used anymore.
+
+#### browser.disconnect()
+
+Disconnects Puppeteer from the browser, but leaves the Chromium process running. After calling `disconnect`, the browser object is considered disposed and cannot be used anymore.
 
 #### browser.newPage()
 - returns: <[Promise]<[Page]>> Promise which resolves to a new [Page] object.

diff --git a/lib/Browser.js b/lib/Browser.js
@@ -58,8 +58,12 @@ class Browser extends EventEmitter {
   }
 
   async close() {
-    this._connection.dispose();
     await this._closeCallback.call(null);
+    this.disconnect();
+  }
+
+  disconnect() {
+    this._connection.dispose();
   }
 }
 

diff --git a/lib/Launcher.js b/lib/Launcher.js
@@ -110,8 +110,10 @@ class Launcher {
       chromeProcess.stderr.pipe(process.stderr);
     }
 
+    let chromeClosed = false;
     const waitForChromeToClose = new Promise((fulfill, reject) => {
       chromeProcess.once('close', () => {
+        chromeClosed = true;
         // Cleanup as processes exit.
         if (temporaryUserDataDir) {
           removeFolderAsync(temporaryUserDataDir)
@@ -126,11 +128,12 @@ class Launcher {
     const listeners = [ helper.addEventListener(process, 'exit', killChrome) ];
     if (options.handleSIGINT !== false)
       listeners.push(helper.addEventListener(process, 'SIGINT', killChrome));
-
+    /** @type {?Connection} */
+    let connection = null;
     try {
       const connectionDelay = options.slowMo || 0;
       const browserWSEndpoint = await waitForWSEndpoint(chromeProcess, options.timeout || 30 * 1000);
-      const connection = await Connection.create(browserWSEndpoint, connectionDelay);
+      connection = await Connection.create(browserWSEndpoint, connectionDelay);
       return new Browser(connection, options, killChrome);
     } catch (e) {
       killChrome();
@@ -142,26 +145,21 @@ class Launcher {
      */
     function killChrome() {
       helper.removeEventListeners(listeners);
-      if (chromeProcess.pid) {
-        if (temporaryUserDataDir) {
+      if (temporaryUserDataDir) {
+        if (chromeProcess.pid && !chromeProcess.killed && !chromeClosed) {
           // Force kill chrome.
           if (process.platform === 'win32')
             childProcess.execSync(`taskkill /pid ${chromeProcess.pid} /T /F`);
           else
             process.kill(-chromeProcess.pid, 'SIGKILL');
-        } else {
-          // Terminate chrome gracefully.
-          if (process.platform === 'win32')
-            childProcess.execSync(`taskkill /pid ${chromeProcess.pid}`);
-          else
-            process.kill(-chromeProcess.pid, 'SIGTERM');
         }
-      }
-      if (temporaryUserDataDir) {
         // Attempt to remove temporary profile directory to avoid littering.
         try {
           removeFolder.sync(temporaryUserDataDir);
         } catch (e) { }
+      } else if (connection) {
+        // Attempt to close chrome gracefully
+        connection.send('Browser.close');
       }
       return waitForChromeToClose;
     }
@@ -181,7 +179,7 @@ class Launcher {
    */
   static async connect(options = {}) {
     const connection = await Connection.create(options.browserWSEndpoint);
-    return new Browser(connection, options);
+    return new Browser(connection, options, () => connection.send('Browser.close'));
   }
 }
 

diff --git a/test/test.js b/test/test.js
@@ -38,7 +38,6 @@ const EMPTY_PAGE = PREFIX + '/empty.html';
 const HTTPS_PORT = 8908;
 const HTTPS_PREFIX = 'https://localhost:' + HTTPS_PORT;
 
-const windows = /^win/.test(process.platform);
 const headless = (process.env.HEADLESS || 'true').trim().toLowerCase() === 'true';
 const slowMo = parseInt((process.env.SLOW_MO || '0').trim(), 10);
 const executablePath = process.env.CHROME;
@@ -109,9 +108,7 @@ describe('Puppeteer', function() {
       await puppeteer.launch(options).catch(e => waitError = e);
       expect(waitError.message.startsWith('Failed to launch chrome! spawn random-invalid-path ENOENT')).toBe(true);
     }));
-    // Windows has issues running Chromium using a custom user data dir. It hangs when closing the browser.
-    // @see https://github.com/GoogleChrome/puppeteer/issues/918
-    (windows ? xit : it)('userDataDir option', SX(async function() {
+    it('userDataDir option', SX(async function() {
       const userDataDir = fs.mkdtempSync(path.join(__dirname, 'test-user-data-dir'));
       const options = Object.assign({userDataDir}, defaultBrowserOptions);
       const browser = await puppeteer.launch(options);
@@ -120,9 +117,7 @@ describe('Puppeteer', function() {
       expect(fs.readdirSync(userDataDir).length).toBeGreaterThan(0);
       rm(userDataDir);
     }));
-    // Windows has issues running Chromium using a custom user data dir. It hangs when closing the browser.
-    // @see https://github.com/GoogleChrome/puppeteer/issues/918
-    (windows ? xit : it)('userDataDir argument', SX(async function() {
+    it('userDataDir argument', SX(async function() {
       const userDataDir = fs.mkdtempSync(path.join(__dirname, 'test-user-data-dir'));
       const options = Object.assign({}, defaultBrowserOptions);
       options.args = [`--user-data-dir=${userDataDir}`].concat(options.args);
@@ -132,9 +127,7 @@ describe('Puppeteer', function() {
       expect(fs.readdirSync(userDataDir).length).toBeGreaterThan(0);
       rm(userDataDir);
     }));
-    // Headless has issues shutting down gracefully
-    // @see https://crbug.com/771830
-    (headless ? xit : it)('userDataDir option should restore state', SX(async function() {
+    it('userDataDir option should restore state', SX(async function() {
       const userDataDir = fs.mkdtempSync(path.join(__dirname, 'test-user-data-dir'));
       const options = Object.assign({userDataDir}, defaultBrowserOptions);
       const browser = await puppeteer.launch(options);
@@ -170,14 +163,28 @@ describe('Puppeteer', function() {
     }));
   });
   describe('Puppeteer.connect', function() {
-    it('should work', SX(async function() {
+    it('should be able to connect multiple times to the same browser', SX(async function() {
       const originalBrowser = await puppeteer.launch(defaultBrowserOptions);
       const browser = await puppeteer.connect({
         browserWSEndpoint: originalBrowser.wsEndpoint()
       });
       const page = await browser.newPage();
       expect(await page.evaluate(() => 7 * 8)).toBe(56);
-      originalBrowser.close();
+      browser.disconnect();
+
+      const secondPage = await originalBrowser.newPage();
+      expect(await secondPage.evaluate(() => 7 * 6)).toBe(42, 'original browser should still work');
+      await originalBrowser.close();
+    }));
+    it('should be able to reconnect to a disconnected browser', SX(async function() {
+      const originalBrowser = await puppeteer.launch(defaultBrowserOptions);
+      const browserWSEndpoint = originalBrowser.wsEndpoint();
+      originalBrowser.disconnect();
+
+      const browser = await puppeteer.connect({browserWSEndpoint});
+      const page = await browser.newPage();
+      expect(await page.evaluate(() => 7 * 8)).toBe(56);
+      await browser.close();
     }));
   });
   describe('Puppeteer.executablePath', function() {