refactor: migrate NavigatorWatcher to lifecycle events (#1141)

This patch:
- migrates navigation watcher to use protocol-issued lifecycle events.
- removes `networkIdleTimeout` and `networkIdleInflight` options for
  `page.goto` method
- adds a new `networkidle0` value to the waitUntil option of navigation
  methods

References #728.

BREAKING CHANGE:

As an implication of this new approach, the `networkIdleTimeout` and
`networkIdleInflight` options are no longer supported. Interested
clients should implement the behavior themselves using the `request` and
`response` events.

README.md

@@ -63,7 +63,7 @@ const puppeteer = require('puppeteer');
 (async () => {
   const browser = await puppeteer.launch();
   const page = await browser.newPage();
-  await page.goto('https://news.ycombinator.com', {waitUntil: 'networkidle'});
+  await page.goto('https://news.ycombinator.com', {waitUntil: 'networkidle2'});
   await page.pdf({path: 'hn.pdf', format: 'A4'});
 
   await browser.close();

docs/api.md

@@ -763,9 +763,8 @@ If there's no element matching `selector`, the method throws an error.
   - `timeout` <[number]> Maximum navigation time in milliseconds, defaults to 30 seconds, pass `0` to disable timeout.
   - `waitUntil` <[string]> When to consider a navigation finished, defaults to `load`. Can be either:
     - `load` - consider navigation to be finished when the `load` event is fired.
-    - `networkidle` - consider navigation to be finished when the network activity stays "idle" for at least `networkIdleTimeout` ms.
+    - `networkidle0` - consider navigation to be finished when there are no more then 0 network connections for at least `500` ms.
-  - `networkIdleInflight` <[number]> Maximum amount of inflight requests which are considered "idle". Takes effect only with `waitUntil: 'networkidle'` parameter.
+    - `networkidle2` - consider navigation to be finished when there are no more then 2 network connections for at least `500` ms.
-  - `networkIdleTimeout` <[number]> A timeout to wait before completing navigation. Takes effect only with `waitUntil: 'networkidle'` parameter.
 - returns: <[Promise]<[Response]>> Promise which resolves to the main resource response. In case of multiple redirects, the navigation will resolve with the response of the last redirect. If
 can not go back, resolves to null.
 
@@ -776,9 +775,8 @@ Navigate to the previous page in history.
   - `timeout` <[number]> Maximum navigation time in milliseconds, defaults to 30 seconds, pass `0` to disable timeout.
   - `waitUntil` <[string]> When to consider navigation succeeded, defaults to `load`. Can be either:
     - `load` - consider navigation to be finished when the `load` event is fired.
-    - `networkidle` - consider navigation to be finished when the network activity stays "idle" for at least `networkIdleTimeout` ms.
+    - `networkidle0` - consider navigation to be finished when there are no more then 0 network connections for at least `500` ms.
-  - `networkIdleInflight` <[number]> Maximum amount of inflight requests which are considered "idle". Takes effect only with `waitUntil: 'networkidle'` parameter.
+    - `networkidle2` - consider navigation to be finished when there are no more then 2 network connections for at least `500` ms.
-  - `networkIdleTimeout` <[number]> A timeout to wait before completing navigation. Takes effect only with `waitUntil: 'networkidle'` parameter.
 - returns: <[Promise]<[Response]>> Promise which resolves to the main resource response. In case of multiple redirects, the navigation will resolve with the response of the last redirect. If
 can not go back, resolves to null.
 
@@ -790,9 +788,8 @@ Navigate to the next page in history.
   - `timeout` <[number]> Maximum navigation time in milliseconds, defaults to 30 seconds, pass `0` to disable timeout.
   - `waitUntil` <[string]> When to consider navigation succeeded, defaults to `load`. Can be either:
     - `load` - consider navigation to be finished when the `load` event is fired.
-    - `networkidle` - consider navigation to be finished when the network activity stays "idle" for at least `networkIdleTimeout` ms.
+    - `networkidle0` - consider navigation to be finished when there are no more then 0 network connections for at least `500` ms.
-  - `networkIdleInflight` <[number]> Maximum amount of inflight requests which are considered "idle". Takes effect only with `waitUntil: 'networkidle'` parameter. Defaults to 2.
+    - `networkidle2` - consider navigation to be finished when there are no more then 2 network connections for at least `500` ms.
-  - `networkIdleTimeout` <[number]> A timeout to wait before completing navigation. Takes effect only with `waitUntil: 'networkidle'` parameter. Defaults to 1000 ms.
 - returns: <[Promise]<[Response]>> Promise which resolves to the main resource response. In case of multiple redirects, the navigation will resolve with the response of the last redirect.
 
 The `page.goto` will throw an error if:
@@ -905,9 +902,8 @@ Shortcut for [page.mainFrame().executionContext().queryObjects(prototypeHandle)]
   - `timeout` <[number]> Maximum navigation time in milliseconds, defaults to 30 seconds, pass `0` to disable timeout.
   - `waitUntil` <[string]> When to consider navigation succeeded, defaults to `load`. Can be either:
     - `load` - consider navigation to be finished when the `load` event is fired.
-    - `networkidle` - consider navigation to be finished when the network activity stays "idle" for at least `networkIdleTimeout` ms.
+    - `networkidle0` - consider navigation to be finished when there are no more then 0 network connections for at least `500` ms.
-  - `networkIdleInflight` <[number]> Maximum amount of inflight requests which are considered "idle". Takes effect only with `waitUntil: 'networkidle'` parameter.
+    - `networkidle2` - consider navigation to be finished when there are no more then 2 network connections for at least `500` ms.
-  - `networkIdleTimeout` <[number]> A timeout to wait before completing navigation. Takes effect only with `waitUntil: 'networkidle'` parameter.
 - returns: <[Promise]<[Response]>> Promise which resolves to the main resource response. In case of multiple redirects, the navigation will resolve with the response of the last redirect.
 
 #### page.screenshot([options])
@@ -1109,9 +1105,8 @@ Shortcut for [page.mainFrame().waitForFunction(pageFunction[, options[, ...args]
   - `timeout` <[number]> Maximum navigation time in milliseconds, defaults to 30 seconds, pass `0` to disable timeout.
   - `waitUntil` <[string]> When to consider navigation succeeded, defaults to `load`. Can be either:
     - `load` - consider navigation to be finished when the `load` event is fired.
-    - `networkidle` - consider navigation to be finished when the network activity stays "idle" for at least `networkIdleTimeout` ms.
+    - `networkidle0` - consider navigation to be finished when there are no more then 0 network connections for at least `500` ms.
-  - `networkIdleInflight` <[number]> Maximum amount of inflight requests which are considered "idle". Takes effect only with `waitUntil: 'networkidle'` parameter.
+    - `networkidle2` - consider navigation to be finished when there are no more then 2 network connections for at least `500` ms.
-  - `networkIdleTimeout` <[number]> A timeout to wait before completing navigation. Takes effect only with `waitUntil: 'networkidle'` parameter.
 - returns: <[Promise]<[Response]>> Promise which resolves to the main resource response. In case of multiple redirects, the navigation will resolve with the response of the last redirect.
 
 #### page.waitForSelector(selector[, options])

examples/custom-event.js

@@ -43,7 +43,7 @@ function listenFor(type) {
 
 await listenFor('app-ready'); // Listen for "app-ready" custom event on page load.
 
-await page.goto('https://www.chromestatus.com/features', {waitUntil: 'networkidle'});
+await page.goto('https://www.chromestatus.com/features', {waitUntil: 'networkidle2'});
 
 await browser.close();
 

examples/detect-sniff.js

@@ -38,7 +38,7 @@ function sniffDetector() {
 const browser = await puppeteer.launch();
 const page = await browser.newPage();
 await page.evaluateOnNewDocument(sniffDetector);
-await page.goto('https://www.google.com', {waitUntil: 'networkidle'});
+await page.goto('https://www.google.com', {waitUntil: 'networkidle2'});
 console.log('Sniffed: ' + (await page.evaluate(() => !!navigator.sniffed)));
 
 await browser.close();

examples/pdf.js

@@ -22,7 +22,7 @@ const puppeteer = require('puppeteer');
 
 const browser = await puppeteer.launch();
 const page = await browser.newPage();
-await page.goto('https://news.ycombinator.com', {waitUntil: 'networkidle'});
+await page.goto('https://news.ycombinator.com', {waitUntil: 'networkidle2'});
 // page.pdf() is currently supported only in headless mode.
 // @see https://bugs.chromium.org/p/chromium/issues/detail?id=753118
 await page.pdf({

examples/search.js

@@ -22,7 +22,7 @@ const puppeteer = require('puppeteer');
 
 const browser = await puppeteer.launch();
 const page = await browser.newPage();
-await page.goto('https://google.com', {waitUntil: 'networkidle'});
+await page.goto('https://google.com', {waitUntil: 'networkidle2'});
 // Type our query into the search bar
 await page.type('input[name=q]', 'puppeteer');
 

lib/NavigatorWatcher.js

@@ -19,25 +19,28 @@ const {helper} = require('./helper');
 class NavigatorWatcher {
   /**
    * @param {!Puppeteer.Session} client
+   * @param {string} frameId
    * @param {boolean} ignoreHTTPSErrors
    * @param {!Object=} options
    */
-  constructor(client, ignoreHTTPSErrors, options = {}) {
+  constructor(client, frameId, ignoreHTTPSErrors, options = {}) {
+    console.assert(options.networkIdleTimeout === undefined, 'ERROR: networkIdleTimeout option is no longer supported.');
+    console.assert(options.networkIdleInflight === undefined, 'ERROR: networkIdleInflight option is no longer supported.');
+    console.assert(options.waitUntil !== 'networkidle', 'ERROR: "networkidle" option is no longer supported. Use "networkidle2" instead');
     this._client = client;
+    this._frameId = frameId;
     this._ignoreHTTPSErrors = ignoreHTTPSErrors;
     this._timeout = typeof options['timeout'] === 'number' ? options['timeout'] : 30000;
-    this._idleTime = typeof options['networkIdleTimeout'] === 'number' ? options['networkIdleTimeout'] : 1000;
+    const waitUntil = typeof options['waitUntil'] === 'string' ? options['waitUntil'] : 'load';
-    this._idleInflight = typeof options['networkIdleInflight'] === 'number' ? options['networkIdleInflight'] : 2;
+    const isAllowedWaitUntil = waitUntil === 'networkidle0' || waitUntil === 'networkidle2' || waitUntil === 'load';
-    this._waitUntil = typeof options['waitUntil'] === 'string' ? options['waitUntil'] : 'load';
+    console.assert(isAllowedWaitUntil, 'Unknown value for options.waitUntil: ' + waitUntil);
-    console.assert(this._waitUntil === 'load' || this._waitUntil === 'networkidle', 'Unknown value for options.waitUntil: ' + this._waitUntil);
+    this._pendingEvents = new Set([waitUntil]);
   }
 
   /**
    * @return {!Promise<?Error>}
    */
   async waitForNavigation() {
-    this._requestIds = new Set();
-
     this._eventListeners = [];
 
     const navigationPromises = [];
@@ -54,58 +57,43 @@ class NavigatorWatcher {
       navigationPromises.push(certificateError);
     }
 
-    if (this._waitUntil === 'load') {
+    this._eventListeners.push(helper.addEventListener(this._client, 'Page.lifecycleEvent', this._onLifecycleEvent.bind(this)));
-      const loadEventFired = new Promise(fulfill => {
+    const pendingEventsFired = new Promise(fulfill => this._pendingEventsCallback = fulfill);
-        this._eventListeners.push(helper.addEventListener(this._client, 'Page.loadEventFired', fulfill));
+    navigationPromises.push(pendingEventsFired);
-      }).then(() => null);
-      navigationPromises.push(loadEventFired);
-    } else {
-      this._eventListeners.push(...[
-        helper.addEventListener(this._client, 'Network.requestWillBeSent', this._onLoadingStarted.bind(this)),
-        helper.addEventListener(this._client, 'Network.loadingFinished', this._onLoadingCompleted.bind(this)),
-        helper.addEventListener(this._client, 'Network.loadingFailed', this._onLoadingCompleted.bind(this)),
-        helper.addEventListener(this._client, 'Network.webSocketCreated', this._onLoadingStarted.bind(this)),
-        helper.addEventListener(this._client, 'Network.webSocketClosed', this._onLoadingCompleted.bind(this)),
-      ]);
-      const networkIdle = new Promise(fulfill => this._networkIdleCallback = fulfill).then(() => null);
-      navigationPromises.push(networkIdle);
-    }
 
     const error = await Promise.race(navigationPromises);
     this._cleanup();
     return error ? new Error(error) : null;
   }
 
+  /**
+   * @param {!{frameId: string, name: string}} event
+   */
+  _onLifecycleEvent(event) {
+    if (event.frameId !== this._frameId)
+      return;
+    const pptrName = protocolLifecycleToPuppeteer[event.name];
+    if (!pptrName)
+      return;
+    this._pendingEvents.delete(pptrName);
+    if (this._pendingEvents.size === 0)
+      this._pendingEventsCallback();
+  }
+
   cancel() {
     this._cleanup();
   }
 
-  /**
-   * @param {!Object} event
-   */
-  _onLoadingStarted(event) {
-    this._requestIds.add(event.requestId);
-    if (this._requestIds.size > this._idleInflight) {
-      clearTimeout(this._idleTimer);
-      this._idleTimer = null;
-    }
-  }
-
-  /**
-   * @param {!Object} event
-   */
-  _onLoadingCompleted(event) {
-    this._requestIds.delete(event.requestId);
-    if (this._requestIds.size <= this._idleInflight && !this._idleTimer)
-      this._idleTimer = setTimeout(this._networkIdleCallback, this._idleTime);
-  }
-
   _cleanup() {
     helper.removeEventListeners(this._eventListeners);
-
-    clearTimeout(this._idleTimer);
     clearTimeout(this._maximumTimer);
   }
 }
 
+const protocolLifecycleToPuppeteer = {
+  'load': 'load',
+  'networkIdle': 'networkidle0',
+  'networkAlmostIdle': 'networkidle2'
+};
+
 module.exports = NavigatorWatcher;

lib/Page.js

@@ -452,7 +452,8 @@ class Page extends EventEmitter {
    * @return {!Promise<?Response>}
    */
   async goto(url, options) {
-    const watcher = new NavigatorWatcher(this._client, this._ignoreHTTPSErrors, options);
+    const mainFrame = this._frameManager.mainFrame();
+    const watcher = new NavigatorWatcher(this._client, mainFrame._id, this._ignoreHTTPSErrors, options);
     const responses = new Map();
     const listener = helper.addEventListener(this._networkManager, NetworkManager.Events.Response, response => responses.set(response.url, response));
     const navigationPromise = watcher.waitForNavigation();
@@ -492,7 +493,8 @@ class Page extends EventEmitter {
    * @return {!Promise<!Response>}
    */
   async waitForNavigation(options) {
-    const watcher = new NavigatorWatcher(this._client, this._ignoreHTTPSErrors, options);
+    const mainFrame = this._frameManager.mainFrame();
+    const watcher = new NavigatorWatcher(this._client, mainFrame._id, this._ignoreHTTPSErrors, options);
 
     const responses = new Map();
     const listener = helper.addEventListener(this._networkManager, NetworkManager.Events.Response, response => responses.set(response.url, response));

test/test.js

@@ -884,6 +884,14 @@ describe('Page', function() {
       const response = await page.goto('about:blank');
       expect(response).toBe(null);
     }));
+    it('should navigate to empty page with networkidle0', SX(async function() {
+      const response = await page.goto(EMPTY_PAGE, {waitUntil: 'networkidle0'});
+      expect(response.status).toBe(200);
+    }));
+    it('should navigate to empty page with networkidle2', SX(async function() {
+      const response = await page.goto(EMPTY_PAGE, {waitUntil: 'networkidle2'});
+      expect(response.status).toBe(200);
+    }));
     it('should fail when navigating to bad url', SX(async function() {
       let error = null;
       await page.goto('asdfasdf').catch(e => error = e);
@@ -899,6 +907,11 @@ describe('Page', function() {
       await page.goto(HTTPS_PREFIX + '/empty.html').catch(e => error = e);
       expect(error.message).toContain('SSL Certificate error');
     }));
+    it('should throw if networkidle is passed as an option', SX(async function() {
+      let error = null;
+      await page.goto(EMPTY_PAGE, {waitUntil: 'networkidle'}).catch(err => error = err);
+      expect(error.message).toContain('"networkidle" option is no longer supported');
+    }));
     it('should fail when main resources failed to load', SX(async function() {
       let error = null;
       await page.goto('http://localhost:44123/non-existing-url').catch(e => error = e);
@@ -959,9 +972,7 @@ describe('Page', function() {
       // Navigate to a page which loads immediately and then does a bunch of
       // requests via javascript's fetch method.
       const navigationPromise = page.goto(PREFIX + '/networkidle.html', {
-        waitUntil: 'networkidle',
+        waitUntil: 'networkidle0',
-        networkIdleTimeout: 100,
-        networkIdleInflight: 0, // Only be idle when there are 0 inflight requests
       });
       // Track when the navigation gets completed.
       let navigationFinished = false;
@@ -1009,9 +1020,7 @@ describe('Page', function() {
       // Navigate to a page which loads immediately and then opens a bunch of
       // websocket connections and then a fetch request.
       const navigationPromise = page.goto(PREFIX + '/websocket.html', {
-        waitUntil: 'networkidle',
+        waitUntil: 'networkidle0',
-        networkIdleTimeout: 100,
-        networkIdleInflight: 0, // Only be idle when there are 0 inflight requests/connections
       });
       // Track when the navigation gets completed.
       let navigationFinished = false;