fix(browser): browser closing/disconnecting should abort navigations (#3245)

Fixes #2721.

lib/Browser.js

@@ -18,6 +18,7 @@ const { helper, assert } = require('./helper');
 const {Target} = require('./Target');
 const EventEmitter = require('events');
 const {TaskQueue} = require('./TaskQueue');
+const {Connection} = require('./Connection');
 
 class Browser extends EventEmitter {
   /**
@@ -45,9 +46,7 @@ class Browser extends EventEmitter {
 
     /** @type {Map<string, Target>} */
     this._targets = new Map();
-    this._connection.setClosedCallback(() => {
+    this._connection.on(Connection.Events.Disconnected, () => this.emit(Browser.Events.Disconnected));
-      this.emit(Browser.Events.Disconnected);
-    });
     this._connection.on('Target.targetCreated', this._targetCreated.bind(this));
     this._connection.on('Target.targetDestroyed', this._targetDestroyed.bind(this));
     this._connection.on('Target.targetInfoChanged', this._targetInfoChanged.bind(this));

lib/Connection.js

@@ -37,6 +37,19 @@ class Connection extends EventEmitter {
     this._transport.onclose = this._onClose.bind(this);
     /** @type {!Map<string, !CDPSession>}*/
     this._sessions = new Map();
+    this._closed = false;
+  }
+
+  /**
+   * @param {!CDPSession} session
+   * @return {!Connection}
+   */
+  static fromSession(session) {
+    let connection = session._connection;
+    // TODO(lushnikov): move to flatten protocol to avoid this.
+    while (connection instanceof CDPSession)
+      connection = connection._connection;
+    return connection;
   }
 
   /**
@@ -61,13 +74,6 @@ class Connection extends EventEmitter {
     });
   }
 
-  /**
-   * @param {function()} callback
-   */
-  setClosedCallback(callback) {
-    this._closeCallback = callback;
-  }
-
   /**
    * @param {string} message
    */
@@ -103,10 +109,9 @@ class Connection extends EventEmitter {
   }
 
   _onClose() {
-    if (this._closeCallback) {
+    if (this._closed)
-      this._closeCallback();
+      return;
-      this._closeCallback = null;
+    this._closed = true;
-    }
     this._transport.onmessage = null;
     this._transport.onclose = null;
     for (const callback of this._callbacks.values())
@@ -115,6 +120,7 @@ class Connection extends EventEmitter {
     for (const session of this._sessions.values())
       session._onClosed();
     this._sessions.clear();
+    this.emit(Connection.Events.Disconnected);
   }
 
   dispose() {
@@ -134,6 +140,10 @@ class Connection extends EventEmitter {
   }
 }
 
+Connection.Events = {
+  Disconnected: Symbol('Connection.Events.Disconnected'),
+};
+
 class CDPSession extends EventEmitter {
   /**
    * @param {!Connection|!CDPSession} connection

lib/NavigatorWatcher.js

@@ -17,15 +17,17 @@
 const {helper, assert} = require('./helper');
 const {FrameManager} = require('./FrameManager');
 const {TimeoutError} = require('./Errors');
+const {Connection} = require('./Connection');
 
 class NavigatorWatcher {
   /**
+   * @param {!Puppeteer.CDPSession} client
    * @param {!FrameManager} frameManager
    * @param {!Puppeteer.Frame} frame
    * @param {number} timeout
    * @param {!Object=} options
    */
-  constructor(frameManager, frame, timeout, options = {}) {
+  constructor(client, frameManager, frame, timeout, options = {}) {
     assert(options.networkIdleTimeout === undefined, 'ERROR: networkIdleTimeout option is no longer supported.');
     assert(options.networkIdleInflight === undefined, 'ERROR: networkIdleInflight option is no longer supported.');
     assert(options.waitUntil !== 'networkidle', 'ERROR: "networkidle" option is no longer supported. Use "networkidle2" instead');
@@ -46,6 +48,7 @@ class NavigatorWatcher {
     this._timeout = timeout;
     this._hasSameDocumentNavigation = false;
     this._eventListeners = [
+      helper.addEventListener(Connection.fromSession(client), Connection.Events.Disconnected, () => this._terminate(new Error('Navigation failed because browser has disconnected!'))),
       helper.addEventListener(this._frameManager, FrameManager.Events.LifecycleEvent, this._checkLifecycleComplete.bind(this)),
       helper.addEventListener(this._frameManager, FrameManager.Events.FrameNavigatedWithinDocument, this._navigatedWithinDocument.bind(this)),
       helper.addEventListener(this._frameManager, FrameManager.Events.FrameDetached, this._checkLifecycleComplete.bind(this))
@@ -60,6 +63,16 @@ class NavigatorWatcher {
     });
 
     this._timeoutPromise = this._createTimeoutPromise();
+    this._terminationPromise = new Promise(fulfill => {
+      this._terminationCallback = fulfill;
+    });
+  }
+
+  /**
+   * @param {!Error} error
+   */
+  _terminate(error) {
+    this._terminationCallback.call(null, error);
   }
 
   /**
@@ -79,8 +92,8 @@ class NavigatorWatcher {
   /**
    * @return {!Promise<?Error>}
    */
-  timeoutPromise() {
+  timeoutOrTerminationPromise() {
-    return this._timeoutPromise;
+    return Promise.race([this._timeoutPromise, this._terminationPromise]);
   }
 
   /**

lib/Page.js

@@ -590,15 +590,15 @@ class Page extends EventEmitter {
 
     const mainFrame = this._frameManager.mainFrame();
     const timeout = typeof options.timeout === 'number' ? options.timeout : this._defaultNavigationTimeout;
-    const watcher = new NavigatorWatcher(this._frameManager, mainFrame, timeout, options);
+    const watcher = new NavigatorWatcher(this._client, this._frameManager, mainFrame, timeout, options);
     let ensureNewDocumentNavigation = false;
     let error = await Promise.race([
       navigate(this._client, url, referrer),
-      watcher.timeoutPromise(),
+      watcher.timeoutOrTerminationPromise(),
     ]);
     if (!error) {
       error = await Promise.race([
-        watcher.timeoutPromise(),
+        watcher.timeoutOrTerminationPromise(),
         ensureNewDocumentNavigation ? watcher.newDocumentNavigationPromise() : watcher.sameDocumentNavigationPromise(),
       ]);
     }
@@ -645,12 +645,12 @@ class Page extends EventEmitter {
   async waitForNavigation(options = {}) {
     const mainFrame = this._frameManager.mainFrame();
     const timeout = typeof options.timeout === 'number' ? options.timeout : this._defaultNavigationTimeout;
-    const watcher = new NavigatorWatcher(this._frameManager, mainFrame, timeout, options);
+    const watcher = new NavigatorWatcher(this._client, this._frameManager, mainFrame, timeout, options);
 
     const responses = new Map();
     const listener = helper.addEventListener(this._networkManager, NetworkManager.Events.Response, response => responses.set(response.url(), response));
     const error = await Promise.race([
-      watcher.timeoutPromise(),
+      watcher.timeoutOrTerminationPromise(),
       watcher.sameDocumentNavigationPromise(),
       watcher.newDocumentNavigationPromise()
     ]);

lib/helper.js

@@ -133,9 +133,9 @@ class Helper {
 
   /**
    * @param {!NodeJS.EventEmitter} emitter
-   * @param {string} eventName
+   * @param {(string|symbol)} eventName
    * @param {function(?)} handler
-   * @return {{emitter: !NodeJS.EventEmitter, eventName: string, handler: function(?)}}
+   * @return {{emitter: !NodeJS.EventEmitter, eventName: (string|symbol), handler: function(?)}}
    */
   static addEventListener(emitter, eventName, handler) {
     emitter.on(eventName, handler);
@@ -143,7 +143,7 @@ class Helper {
   }
 
   /**
-   * @param {!Array<{emitter: !NodeJS.EventEmitter, eventName: string, handler: function(?)}>} listeners
+   * @param {!Array<{emitter: !NodeJS.EventEmitter, eventName: (string|symbol), handler: function(?)}>} listeners
    */
   static removeEventListeners(listeners) {
     for (const listener of listeners)

test/puppeteer.spec.js

@@ -60,6 +60,31 @@ module.exports.addTests = function({testRunner, expect, defaultBrowserOptions})
         await rmAsync(downloadsFolder);
       });
     });
+    describe('Browser.disconnect', function() {
+      it('should reject navigation when browser closes', async({server}) => {
+        server.setRoute('/one-style.css', () => {});
+        const browser = await puppeteer.launch(defaultBrowserOptions);
+        const remote = await puppeteer.connect({browserWSEndpoint: browser.wsEndpoint()});
+        const page = await remote.newPage();
+        const navigationPromise = page.goto(server.PREFIX + '/one-style.html', {timeout: 60000}).catch(e => e);
+        await server.waitForRequest('/one-style.css');
+        await remote.disconnect();
+        const error = await navigationPromise;
+        expect(error.message).toBe('Navigation failed because browser has disconnected!');
+        await browser.close();
+      });
+      it('should reject waitForSelector when browser closes', async({server}) => {
+        server.setRoute('/empty.html', () => {});
+        const browser = await puppeteer.launch(defaultBrowserOptions);
+        const remote = await puppeteer.connect({browserWSEndpoint: browser.wsEndpoint()});
+        const page = await remote.newPage();
+        const watchdog = page.waitForSelector('div', {timeout: 60000}).catch(e => e);
+        await remote.disconnect();
+        const error = await watchdog;
+        expect(error.message).toBe('Protocol error (Runtime.callFunctionOn): Session closed. Most likely the page has been closed.');
+        await browser.close();
+      });
+    });
     describe('Puppeteer.launch', function() {
       it('should reject all promises when browser is closed', async() => {
         const browser = await puppeteer.launch(defaultBrowserOptions);