fix(Navigation): correctly wait for navigation in Page.goto (#1355)

This patch: - starts persisting lifecycle state for every frame - migrates NavigationWatcher to rely on these lifecycle events - refactors Page.goto to properly return navigation errors Fixes #1218.
2024-06-14 14:02:48 +00:00 · 2017-11-10 15:33:14 -08:00 · 2017-11-10 15:33:14 -08:00 · 44d1e834a4
commit 44d1e834a4
parent 9b907b9ada
4 changed files with 128 additions and 73 deletions
--- a/lib/FrameManager.js
+++ b/lib/FrameManager.js
@ -41,10 +41,22 @@ class FrameManager extends EventEmitter {
    this._client.on('Page.frameNavigated', event => this._onFrameNavigated(event.frame));
    this._client.on('Page.frameDetached', event => this._onFrameDetached(event.frameId));
    this._client.on('Runtime.executionContextCreated', event => this._onExecutionContextCreated(event.context));
+    this._client.on('Page.lifecycleEvent', event => this._onLifecycleEvent(event));

    this._handleFrameTree(frameTree);
  }

+  /**
+   * @param {!Object} event
+   */
+  _onLifecycleEvent(event) {
+    const frame = this._frames.get(event.frameId);
+    if (!frame)
+      return;
+    frame._onLifecycleEvent(event.loaderId, event.name);
+    this.emit(FrameManager.Events.LifecycleEvent, frame);
+  }
+
  /**
   * @param {{frame: Object, childFrames: ?Array}} frameTree
   */
@ -171,20 +183,14 @@ class FrameManager extends EventEmitter {
    this._frames.delete(frame._id);
    this.emit(FrameManager.Events.FrameDetached, frame);
  }
-
-  /**
-   * @return {?string}
-   */
-  unreachableMainFrameURL() {
-    return this._mainFrame._unreachableURL || null;
-  }
 }

 /** @enum {string} */
 FrameManager.Events = {
  FrameAttached: 'frameattached',
  FrameNavigated: 'framenavigated',
-  FrameDetached: 'framedetached'
+  FrameDetached: 'framedetached',
+  LifecycleEvent: 'lifecycleevent'
 };

 /**
@ -205,6 +211,9 @@ class Frame {
    this._context = null;
    /** @type {!Set<!WaitTask>} */
    this._waitTasks = new Set();
+    this._loaderId = '';
+    /** @type {!Set<string>} */
+    this._lifecycleEvents = new Set();

    /** @type {!Set<!Frame>} */
    this._childFrames = new Set();
@ -523,7 +532,18 @@ class Frame {
  _navigated(framePayload) {
    this._name = framePayload.name;
    this._url = framePayload.url;
-    this._unreachableURL = framePayload.unreachableUrl;
+  }
+
+  /**
+   * @param {string} loaderId
+   * @param {string} name
+   */
+  _onLifecycleEvent(loaderId, name) {
+    if (name === 'init') {
+      this._loaderId = loaderId;
+      this._lifecycleEvents.clear();
+    }
+    this._lifecycleEvents.add(name);
  }

  _detach() {
--- a/lib/NavigatorWatcher.js
+++ b/lib/NavigatorWatcher.js
@ -15,66 +15,87 @@
 */

 const {helper} = require('./helper');
+const {FrameManager} = require('./FrameManager');

 class NavigatorWatcher {
  /**
-   * @param {!Puppeteer.Session} client
-   * @param {string} frameId
+   * @param {!FrameManager} frameManager
+   * @param {!Puppeteer.Frame} frame
   * @param {!Object=} options
   */
-  constructor(client, frameId, options = {}) {
+  constructor(frameManager, frame, 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._timeout = typeof options.timeout === 'number' ? options.timeout : 30000;
    let waitUntil = ['load'];
    if (Array.isArray(options.waitUntil))
      waitUntil = options.waitUntil.slice();
    else if (typeof options.waitUntil === 'string')
      waitUntil = [options.waitUntil];
-    for (const value of waitUntil) {
-      const isAllowedValue = value === 'networkidle0' || value === 'networkidle2' || value === 'load' || value === 'domcontentloaded';
-      console.assert(isAllowedValue, 'Unknown value for options.waitUntil: ' + value);
-    }
-    this._pendingEvents = new Set(waitUntil);
+    this._expectedLifecycle = waitUntil.map(value => {
+      const protocolEvent = puppeteerToProtocolLifecycle[value];
+      console.assert(protocolEvent, 'Unknown value for options.waitUntil: ' + value);
+      return protocolEvent;
+    });
+
+    this._frameManager = frameManager;
+    this._frame = frame;
+    this._initialLoaderId = frame._loaderId;
+    this._timeout = typeof options.timeout === 'number' ? options.timeout : 30000;
+    this._eventListeners = [
+      helper.addEventListener(this._frameManager, FrameManager.Events.LifecycleEvent, this._checkLifecycleComplete.bind(this))
+    ];
+
+    const lifecycleCompletePromise = new Promise(fulfill => {
+      this._lifecycleCompleteCallback = fulfill;
+    });
+    this._navigationPromise = Promise.race([
+      this._createTimeoutPromise(),
+      lifecycleCompletePromise
+    ]).then(error => {
+      this._cleanup();
+      return error;
+    });
+  }
+
+  /**
+   * @return {?Promise<?Error>}
+   */
+  _createTimeoutPromise() {
+    if (!this._timeout)
+      return null;
+    const errorMessage = 'Navigation Timeout Exceeded: ' + this._timeout + 'ms exceeded';
+    return new Promise(fulfill => this._maximumTimer = setTimeout(fulfill, this._timeout))
+        .then(() => new Error(errorMessage));
  }

  /**
   * @return {!Promise<?Error>}
   */
-  async waitForNavigation() {
-    this._eventListeners = [];
-
-    const navigationPromises = [];
-    if (this._timeout) {
-      const watchdog = new Promise(fulfill => this._maximumTimer = setTimeout(fulfill, this._timeout))
-          .then(() => 'Navigation Timeout Exceeded: ' + this._timeout + 'ms exceeded');
-      navigationPromises.push(watchdog);
-    }
-
-    this._eventListeners.push(helper.addEventListener(this._client, 'Page.lifecycleEvent', this._onLifecycleEvent.bind(this)));
-    const pendingEventsFired = new Promise(fulfill => this._pendingEventsCallback = fulfill);
-    navigationPromises.push(pendingEventsFired);
-
-    const error = await Promise.race(navigationPromises);
-    this._cleanup();
-    return error ? new Error(error) : null;
+  async navigationPromise() {
+    return this._navigationPromise;
  }

-  /**
-   * @param {!{frameId: string, name: string}} event
-   */
-  _onLifecycleEvent(event) {
-    if (event.frameId !== this._frameId)
+  _checkLifecycleComplete() {
+    // We expect navigation to commit.
+    if (this._frame._loaderId === this._initialLoaderId)
      return;
-    const pptrName = protocolLifecycleToPuppeteer[event.name];
-    if (!pptrName)
+    if (!checkLifecycle(this._frame, this._expectedLifecycle))
      return;
-    this._pendingEvents.delete(pptrName);
-    if (this._pendingEvents.size === 0)
-      this._pendingEventsCallback();
+    this._lifecycleCompleteCallback();
+
+    /**
+     * @param {!Puppeteer.Frame} frame
+     * @param {!Array<string>} expectedLifecycle
+     * @return {boolean}
+     */
+    function checkLifecycle(frame, expectedLifecycle) {
+      for (const event of expectedLifecycle) {
+        if (!frame._lifecycleEvents.has(event))
+          return false;
+      }
+      return true;
+    }
  }

  cancel() {
@ -83,15 +104,16 @@ class NavigatorWatcher {

  _cleanup() {
    helper.removeEventListeners(this._eventListeners);
+    this._lifecycleCompleteCallback(new Error('Navigation failed'));
    clearTimeout(this._maximumTimer);
  }
 }

-const protocolLifecycleToPuppeteer = {
+const puppeteerToProtocolLifecycle = {
  'load': 'load',
-  'DOMContentLoaded': 'domcontentloaded',
-  'networkIdle': 'networkidle0',
-  'networkAlmostIdle': 'networkidle2',
+  'domcontentloaded': 'DOMContentLoaded',
+  'networkidle0': 'networkIdle',
+  'networkidle2': 'networkAlmostIdle',
 };

 module.exports = NavigatorWatcher;
--- a/lib/Page.js
+++ b/lib/Page.js
@ -453,30 +453,43 @@ class Page extends EventEmitter {
   * @return {!Promise<?Response>}
   */
  async goto(url, options) {
-    const mainFrame = this._frameManager.mainFrame();
-    const watcher = new NavigatorWatcher(this._client, mainFrame._id, options);
-    const requests = new Map();
-    const listener = helper.addEventListener(this._networkManager, NetworkManager.Events.Request, request => requests.set(request.url, request));
-    const navigationPromise = watcher.waitForNavigation();
    const referrer = this._networkManager.extraHTTPHeaders()['referer'];
-    const error = await Promise.race([
-      this._client.send('Page.navigate', {url, referrer})
-          .then(() => navigationPromise)
-          .catch(error => error),
-      navigationPromise
+
+    const requests = new Map();
+    const eventListeners = [
+      helper.addEventListener(this._networkManager, NetworkManager.Events.Request, request => requests.set(request.url, request))
+    ];
+
+    const mainFrame = this._frameManager.mainFrame();
+    const watcher = new NavigatorWatcher(this._frameManager, mainFrame, options);
+    const navigationPromise = watcher.navigationPromise();
+    let error = await Promise.race([
+      navigate(this._client, url, referrer),
+      navigationPromise,
    ]);
+    if (!error)
+      error = await navigationPromise;
    watcher.cancel();
-    helper.removeEventListeners([listener]);
+    helper.removeEventListeners(eventListeners);
    if (error)
      throw error;
-    const unreachableURL = this._frameManager.unreachableMainFrameURL();
-    if (unreachableURL) {
-      const request = requests.get(unreachableURL) || null;
-      const failure = request ? request.failure() : null;
-      throw new Error('Failed to navigate: ' + url + (failure ? ' ' + failure.errorText : ''));
-    }
    const request = requests.get(this.mainFrame().url());
    return request ? request.response() : null;
+
+    /**
+     * @param {!Puppeteer.Session} client
+     * @param {string} url
+     * @param {string} referrer
+     * @return {!Promise<?Error>}
+     */
+    async function navigate(client, url, referrer) {
+      try {
+        const response = await client.send('Page.navigate', {url, referrer});
+        return response.errorText ? new Error(response.errorText) : null;
+      } catch (error) {
+        return error;
+      }
+    }
  }

  /**
@ -497,11 +510,11 @@ class Page extends EventEmitter {
   */
  async waitForNavigation(options) {
    const mainFrame = this._frameManager.mainFrame();
-    const watcher = new NavigatorWatcher(this._client, mainFrame._id, options);
+    const watcher = new NavigatorWatcher(this._frameManager, mainFrame, options);

    const responses = new Map();
    const listener = helper.addEventListener(this._networkManager, NetworkManager.Events.Response, response => responses.set(response.url, response));
-    const error = await watcher.waitForNavigation();
+    const error = await watcher.navigationPromise();
    helper.removeEventListeners([listener]);
    if (error)
      throw error;
--- a/test/test.js
+++ b/test/test.js
@ -972,7 +972,7 @@ describe('Page', function() {
    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);
-      expect(error.message).toContain('Failed to navigate');
+      expect(error.message).toContain('net::ERR_CONNECTION_REFUSED');
    }));
    it('should fail when exceeding maximum navigation timeout', SX(async function() {
      let hasUnhandledRejection = false;
@ -1293,7 +1293,7 @@ describe('Page', function() {
      let error = null;
      await page.goto(EMPTY_PAGE).catch(e => error = e);
      expect(error).toBeTruthy();
-      expect(error.message).toContain('Failed to navigate');
+      expect(error.message).toContain('net::ERR_FAILED');
    }));
    it('should work with redirects', SX(async function() {
      await page.setRequestInterception(true);
@ -1371,7 +1371,7 @@ describe('Page', function() {
      });
      let error = null;
      await page.goto('data:text/html,No way!').catch(err => error = err);
-      expect(error.message).toContain('Failed to navigate');
+      expect(error.message).toContain('net::ERR_FAILED');
    }));
    it('should navigate to URL with hash and and fire requests without hash', SX(async function() {
      await page.setRequestInterception(true);