Add better network idle definition (#38)

This patch:
- Changes network idle promise to wait for 2 or fewer network requests for at least idleTime (defaults to 5s) before resolving.
- Adds timer cleanup to failure navigation case.
- Adds handling of webSocketClosed.
- Ignores unrecognized requestIds to avoid negative inflight requests.

References #10

lib/Navigator.js

@@ -14,19 +14,31 @@
  * limitations under the License.
  */
 
+const VALID_WAIT_CONDITIONS = ['load', 'networkidle'];
+
 class Navigator {
   /**
    * @param {!Connection} client
    * @param {!Object=} options
    */
-  constructor(client, options) {
+  constructor(client, options = {}) {
     this._client = client;
+    this._minTime = typeof options.minTime === 'number' ? options.minTime : 0;
+    this._maxTime = typeof options.maxTime === 'number' ? options.maxTime : 30000;
+    this._idleTime = typeof options.networkIdleTimeout === 'number' ? options.networkIdleTimeout : 1000;
+    this._idleInflight = typeof options.networkIdleInflight === 'number' ? options.networkIdleInflight : 2;
+    this._waitFor = typeof options.waitFor === 'string' ? options.waitFor : 'load';
+    this._inflightRequests = 0;
+
+    console.assert(VALID_WAIT_CONDITIONS.includes(this._waitFor));
+
+    if (this._waitFor === 'networkidle') {
       client.on('Network.requestWillBeSent', event => this._onRequestWillBeSent(event));
       client.on('Network.loadingFinished', event => this._onLoadingFinished(event));
       client.on('Network.loadingFailed', event => this._onLoadingFailed(event));
-    this._minTime = options && options['minTime'] ? options['minTime'] : 0;
-    this._maxTime = options && options['maxTime'] ? options['maxTime'] : 30000;
-    this._inflightRequests = 0;
+      client.on('Network.webSocketCreated', event => this._onWebSocketCreated(event));
+      client.on('Network.webSocketClosed', event => this._onWebSocketClosed(event));
+    }
   }
 
   /**
@@ -34,29 +46,64 @@ class Navigator {
    * @param {string=} referrer
    */
   async navigate(url, referrer) {
+    this._requestIds = new Set();
     this._navigationStartTime = Date.now();
-    this._watchdogTimer = setTimeout(this._completeNavigation.bind(this, true), this._maxTime);
-    this._minimumTimer = setTimeout(this._completeNavigation.bind(this, false), this._minTime);
-    let onload = new Promise(fulfill => this._client.once('Page.loadEventFired', fulfill));
-    let networkIdle = new Promise(fulfill => this._navigationLoadCallback = fulfill);
-    let interstitialPromise = new Promise(fulfill => this._client.once('Security.certificateError', fulfill)).then(() => false);
+    this._idleReached = false;
+
+    let navigationComplete;
+    let navigationFailure = new Promise(fulfill => this._client.once('Security.certificateError', fulfill)).then(() => false);
+
+    switch (this._waitFor) {
+      case 'load':
+        navigationComplete = new Promise(fulfill => this._client.once('Page.loadEventFired', fulfill));
+        break;
+      case 'networkidle':
+        navigationComplete = new Promise(fulfill => this._navigationLoadCallback = fulfill);
+        break;
+      default:
+        throw new Error(`Unrecognized wait condition: ${this._waitFor}`);
+    }
 
     this._inflightRequests = 0;
+
+    this._minimumTimer = setTimeout(this._completeNavigation.bind(this, false), this._minTime);
+    this._maximumTimer = setTimeout(this._completeNavigation.bind(this, true), this._maxTime);
+    this._idleTimer = setTimeout(this._onIdleReached.bind(this), this._idleTime);
+
     // Await for the command to throw exception in case of illegal arguments.
     try {
       await this._client.send('Page.navigate', {url, referrer});
     } catch (e) {
       return false;
     }
-    return await Promise.race([Promise.all([onload, networkIdle]).then(() => true), interstitialPromise]);
+
+    return await Promise.race([navigationComplete.then(() => true), navigationFailure]).then(retVal => {
+      clearTimeout(this._idleTimer);
+      clearTimeout(this._minimumTimer);
+      clearTimeout(this._maximumTimer);
+      return retVal;
+    });
   }
 
   /**
    * @param {!Object} event
    */
   _onRequestWillBeSent(event) {
-    if (!event.redirectResponse)
-      ++this._inflightRequests;
+    this._onLoadingStarted(event);
+  }
+
+  /**
+   * @param {!Object} event
+   */
+  _onWebSocketCreated(event) {
+    this._onLoadingStarted(event);
+  }
+
+  /**
+   * @param {!Object} event
+   */
+  _onWebSocketClosed(event) {
+    this._onLoadingCompleted(event);
   }
 
   /**
@@ -73,10 +120,33 @@ class Navigator {
     this._onLoadingCompleted(event);
   }
 
+  /**
+   * @param {!Object} event
+   */
+  _onLoadingStarted(event) {
+    this._requestIds.add(event.requestId);
+    if (!event.redirectResponse)
+      ++this._inflightRequests;
+    if (this._inflightRequests > this._idleInflight) {
+      clearTimeout(this._idleTimer);
+      this._idleTimer = null;
+    }
+  }
+
+  /**
+   * @param {!Object} event
+   */
   _onLoadingCompleted(event) {
-    --this._inflightRequests;
-    if (Date.now() - this._navigationStartTime < this._minTime)
+    if (!this._requestIds.has(event.requestId))
       return;
+
+    --this._inflightRequests;
+    if (this._inflightRequests <= this._idleInflight && !this._idleTimer)
+      this._idleTimer = setTimeout(this._onIdleReached.bind(this), this._idleTime);
+  }
+
+  _onIdleReached() {
+    this._idleReached = true;
     this._completeNavigation(false);
   }
 
@@ -86,9 +156,9 @@ class Navigator {
   _completeNavigation(force) {
     if (!this._navigationLoadCallback)
       return;
-    if (this._inflightRequests < 2 || force) {
-      clearTimeout(this._minimumTimer);
-      clearTimeout(this._watchdogTimer);
+
+    const elapsedTime = Date.now() - this._navigationStartTime;
+    if ((elapsedTime >= this._minTime && this._idleReached) || force) {
       this._navigationLoadCallback();
       this._navigationLoadCallback = null;
     }

test/StaticServer.js

@@ -19,6 +19,7 @@ let url = require('url');
 let fs = require('fs');
 let path = require('path');
 let mime = require('mime');
+let WebSocketServer = require('ws').Server;
 
 const fulfillSymbol = Symbol('fullfill callback');
 const rejectSymbol = Symbol('reject callback');
@@ -30,6 +31,8 @@ class StaticServer {
    */
   constructor(dirPath, port) {
     this._server = http.createServer(this._onRequest.bind(this));
+    this._wsServer = new WebSocketServer({server: this._server});
+    this._wsServer.on('connection', this._onWebSocketConnection.bind(this));
     this._server.listen(port);
     this._dirPath = dirPath;
 
@@ -99,6 +102,7 @@ class StaticServer {
     if (pathName === '/')
       pathName = '/index.html';
     pathName = path.join(this._dirPath, pathName.substring(1));
+
     fs.readFile(pathName, function(err, data) {
       if (err) {
         response.statusCode = 404;
@@ -109,6 +113,10 @@ class StaticServer {
       response.end(data);
     });
   }
+
+  _onWebSocketConnection(connection) {
+    connection.send('opened');
+  }
 }
 
 module.exports = StaticServer;

test/assets/networkidle.html

@@ -1,5 +1,19 @@
 <script>
-  fetch('fetch-request-a.js');
-  fetch('fetch-request-b.js');
-  fetch('fetch-request-c.js');
+  async function sleep(delay) {
+    return new Promise(resolve => setTimeout(resolve, delay));
+  }
+
+  async function main() {
+    const roundOne = Promise.all([
+      fetch('fetch-request-a.js'),
+      fetch('fetch-request-b.js'),
+      fetch('fetch-request-c.js'),
+    ]);
+
+    await roundOne;
+    await sleep(50);
+    await fetch('fetch-request-d.js');
+  }
+
+  main();
 </script>

test/assets/websocket.html

@@ -0,0 +1,28 @@
+<script>
+  function createClient() {
+    const client = new WebSocket(`ws://${location.host}`);
+    client.addEventListener('open', () => console.log('opened'));
+    client.addEventListener('close', () => console.log('closed'));
+    return client;
+  }
+
+  function createAndDestroyClientAfterDelay(delay = 50) {
+    const client = createClient();
+    return new Promise(resolve => {
+      setTimeout(() => {
+        client.close();
+        resolve();
+      }, delay);
+    });
+  }
+
+  async function main() {
+    await createAndDestroyClientAfterDelay();
+    await createAndDestroyClientAfterDelay();
+    await createAndDestroyClientAfterDelay();
+    await createAndDestroyClientAfterDelay();
+    await fetch('fetch-request.js');
+  }
+
+  main().then(() => console.log('done!'));
+</script>

test/test.js

@@ -115,19 +115,24 @@ describe('Puppeteer', function() {
     }));
     it('should wait for network idle to succeed navigation', SX(async function() {
       let responses = [];
-      // Hold on a bunch of requests without answering.
+      // Hold on to a bunch of requests without answering.
       staticServer.setRoute('/fetch-request-a.js', (req, res) => responses.push(res));
       staticServer.setRoute('/fetch-request-b.js', (req, res) => responses.push(res));
       staticServer.setRoute('/fetch-request-c.js', (req, res) => responses.push(res));
-      let fetchResourcesRequested = Promise.all([
+      staticServer.setRoute('/fetch-request-d.js', (req, res) => responses.push(res));
+      let initialFetchResourcesRequested = Promise.all([
         staticServer.waitForRequest('/fetch-request-a.js'),
         staticServer.waitForRequest('/fetch-request-b.js'),
         staticServer.waitForRequest('/fetch-request-c.js'),
       ]);
+      let secondFetchResourceRequested = staticServer.waitForRequest('/fetch-request-d.js');
+
       // Navigate to a page which loads immediately and then does a bunch of
       // requests via javascript's fetch method.
       let navigationPromise = page.navigate(STATIC_PREFIX + '/networkidle.html', {
-        minTime: 50 // Give page time to request more resources dynamically.
+        waitFor: 'networkidle',
+        networkIdleTimeout: 100,
+        networkIdleInflight: 0, // Only be idle when there are 0 inflight requests
       });
       // Track when the navigation gets completed.
       let navigationFinished = false;
@@ -137,13 +142,63 @@ describe('Puppeteer', function() {
       await new Promise(fulfill => page.once('load', fulfill));
       expect(navigationFinished).toBe(false);
 
-      // Wait for all three resources to be requested.
-      await fetchResourcesRequested;
+      // Wait for the initial three resources to be requested.
+      await initialFetchResourcesRequested;
 
       // Expect navigation still to be not finished.
       expect(navigationFinished).toBe(false);
 
-      // Respond to all requests.
+      // Respond to initial requests.
+      for (let response of responses) {
+        response.statusCode = 404;
+        response.end(`File not found`);
+      }
+
+      // Reset responses array
+      responses = [];
+
+      // Wait for the second round to be requested.
+      await secondFetchResourceRequested;
+      // Expect navigation still to be not finished.
+      expect(navigationFinished).toBe(false);
+
+      // Respond to requests.
+      for (let response of responses) {
+        response.statusCode = 404;
+        response.end(`File not found`);
+      }
+
+      let success = await navigationPromise;
+      // Expect navigation to succeed.
+      expect(success).toBe(true);
+    }));
+    it('should wait for websockets to succeed navigation', SX(async function() {
+      let responses = [];
+      // Hold on to the fetch request without answering.
+      staticServer.setRoute('/fetch-request.js', (req, res) => responses.push(res));
+      let fetchResourceRequested = staticServer.waitForRequest('/fetch-request.js');
+      // Navigate to a page which loads immediately and then opens a bunch of
+      // websocket connections and then a fetch request.
+      let navigationPromise = page.navigate(STATIC_PREFIX + '/websocket.html', {
+        waitFor: 'networkidle',
+        networkIdleTimeout: 100,
+        networkIdleInflight: 0, // Only be idle when there are 0 inflight requests/connections
+      });
+      // Track when the navigation gets completed.
+      let navigationFinished = false;
+      navigationPromise.then(() => navigationFinished = true);
+
+      // Wait for the page's 'load' event.
+      await new Promise(fulfill => page.once('load', fulfill));
+      expect(navigationFinished).toBe(false);
+
+      // Wait for the resource to be requested.
+      await fetchResourceRequested;
+
+      // Expect navigation still to be not finished.
+      expect(navigationFinished).toBe(false);
+
+      // Respond to the request.
       for (let response of responses) {
         response.statusCode = 404;
         response.end(`File not found`);