fix: support PlzNavigate in puppeteer.

This patch migrates puppeteer to support PlzNavigate chromium project. As a consequence of this patch, we no longer wait for both requestWillBeSent and requestIntercepted events to happen. This should resolve a ton of request interception bugs that "hanged" the loading. Fixes #877.
puppeteer · Nov 1, 2017 · f3a513d · f3a513d
1 parent 9f071bf
commit f3a513d
Show file tree

Hide file tree

Showing 5 changed files with 54 additions and 66 deletions.
diff --git a/docs/api.md b/docs/api.md
@@ -1006,9 +1006,6 @@ puppeteer.launch().then(async browser => {
 });
 ```
 
-> **NOTE** Request interception doesn't work with data URLs. Calling `abort`,
-> `continue` or `respond` on requests for data URLs is a noop.
-
 > **NOTE** Enabling request interception disables page caching.
 
 #### page.setUserAgent(userAgent)
@@ -1981,6 +1978,8 @@ page.on('request', request => {
 });
 ```
 
+> **NOTE** Mocking responses for dataURL requests is not supported.
+> Calling `request.respond` for a dataURL request is a noop.
 
 #### request.response()
 - returns: <[Response]> A matching [Response] object, or `null` if the response has not been received yet.

diff --git a/lib/Launcher.js b/lib/Launcher.js
@@ -34,8 +34,6 @@ const CHROME_PROFILE_PATH = path.join(os.tmpdir(), 'puppeteer_dev_profile-');
 const DEFAULT_ARGS = [
   '--disable-background-networking',
   '--disable-background-timer-throttling',
-  // TODO(aslushnikov): this flag should be removed. @see https://github.com/GoogleChrome/puppeteer/issues/877
-  '--disable-browser-side-navigation',
   '--disable-client-side-phishing-detection',
   '--disable-default-apps',
   '--disable-extensions',

diff --git a/lib/NetworkManager.js b/lib/NetworkManager.js
@@ -16,7 +16,6 @@
 const EventEmitter = require('events');
 const {helper, debugError} = require('./helper');
 const Multimap = require('./Multimap');
-const url = require('url');
 
 class NetworkManager extends EventEmitter {
   /**
@@ -43,7 +42,7 @@ class NetworkManager extends EventEmitter {
     /** @type {!Multimap} */
     this._requestHashToRequestIds = new Multimap();
     /** @type {!Multimap} */
-    this._requestHashToInterceptions = new Multimap();
+    this._requestHashToInterceptionIds = new Multimap();
 
     this._client.on('Network.requestWillBeSent', this._onRequestWillBeSent.bind(this));
     this._client.on('Network.requestIntercepted', this._onRequestIntercepted.bind(this));
@@ -127,9 +126,6 @@ class NetworkManager extends EventEmitter {
    * @param {!Object} event
    */
   _onRequestIntercepted(event) {
-    // Strip out url hash to be consistent with requestWillBeSent. @see crbug.com/755456
-    event.request.url = removeURLHash(event.request.url);
-
     if (event.authChallenge) {
       let response = 'Default';
       if (this._attemptedAuthentications.has(event.interceptionId)) {
@@ -159,8 +155,14 @@ class NetworkManager extends EventEmitter {
       return;
     }
     const requestHash = generateRequestHash(event.request);
-    this._requestHashToInterceptions.set(requestHash, event);
-    this._maybeResolveInterception(requestHash);
+    const requestId = this._requestHashToRequestIds.firstValue(requestHash);
+    if (requestId) {
+      this._requestHashToRequestIds.delete(requestHash, requestId);
+      this._handleRequestStart(requestId, event.interceptionId, event.request.url, event.resourceType, event.request);
+    } else {
+      this._requestHashToInterceptionIds.set(requestHash, event.interceptionId);
+      this._handleRequestStart(null, event.interceptionId, event.request.url, event.resourceType, event.request);
+    }
   }
 
   /**
@@ -179,30 +181,39 @@ class NetworkManager extends EventEmitter {
   }
 
   /**
-   * @param {string} requestId
-   * @param {string} interceptionId
+   * @param {?string} requestId
+   * @param {?string} interceptionId
    * @param {string} url
    * @param {string} resourceType
    * @param {!Object} requestPayload
    */
   _handleRequestStart(requestId, interceptionId, url, resourceType, requestPayload) {
     const request = new Request(this._client, requestId, interceptionId, this._userRequestInterceptionEnabled, url, resourceType, requestPayload);
-    this._requestIdToRequest.set(requestId, request);
-    this._interceptionIdToRequest.set(interceptionId, request);
+    if (requestId)
+      this._requestIdToRequest.set(requestId, request);
+    if (interceptionId)
+      this._interceptionIdToRequest.set(interceptionId, request);
     this.emit(NetworkManager.Events.Request, request);
   }
 
   /**
    * @param {!Object} event
    */
   _onRequestWillBeSent(event) {
-    if (this._protocolRequestInterceptionEnabled && !event.request.url.startsWith('data:')) {
+    if (this._protocolRequestInterceptionEnabled) {
       // All redirects are handled in requestIntercepted.
       if (event.redirectResponse)
         return;
       const requestHash = generateRequestHash(event.request);
-      this._requestHashToRequestIds.set(requestHash, event.requestId);
-      this._maybeResolveInterception(requestHash);
+      const interceptionId = this._requestHashToInterceptionIds.firstValue(requestHash);
+      const request = interceptionId ? this._interceptionIdToRequest.get(interceptionId) : null;
+      if (request) {
+        request._requestId = event.requestId;
+        this._requestIdToRequest.set(event.requestId, request);
+        this._requestHashToInterceptionIds.delete(requestHash, interceptionId);
+      } else {
+        this._requestHashToRequestIds.set(requestHash, event.requestId);
+      }
       return;
     }
     if (event.redirectResponse) {
@@ -212,19 +223,6 @@ class NetworkManager extends EventEmitter {
     this._handleRequestStart(event.requestId, null, event.request.url, event.type, event.request);
   }
 
-  /**
-   * @param {string} requestHash
-   */
-  _maybeResolveInterception(requestHash) {
-    const requestId = this._requestHashToRequestIds.firstValue(requestHash);
-    const interception = this._requestHashToInterceptions.firstValue(requestHash);
-    if (!requestId || !interception)
-      return;
-    this._requestHashToRequestIds.delete(requestHash, requestId);
-    this._requestHashToInterceptions.delete(requestHash, interception);
-    this._handleRequestStart(requestId, interception.interceptionId, interception.request.url, interception.resourceType, interception.request);
-  }
-
   /**
    * @param {!Object} event
    */
@@ -275,7 +273,7 @@ class NetworkManager extends EventEmitter {
 class Request {
   /**
    * @param {!Puppeteer.Session} client
-   * @param {string} requestId
+   * @param {?string} requestId
    * @param {string} interceptionId
    * @param {boolean} allowInterception
    * @param {string} url
@@ -325,9 +323,6 @@ class Request {
    * @param {!Object=} overrides
    */
   async continue(overrides = {}) {
-    // DataURL's are not interceptable. In this case, do nothing.
-    if (this.url.startsWith('data:'))
-      return;
     console.assert(this._allowInterception, 'Request Interception is not enabled!');
     console.assert(!this._interceptionHandled, 'Request is already handled!');
     this._interceptionHandled = true;
@@ -348,7 +343,7 @@ class Request {
    * @param {!{status: number, headers: Object, contentType: string, body: (string|Buffer)}} response
    */
   async respond(response) {
-    // DataURL's are not interceptable. In this case, do nothing.
+    // Mocking responses for dataURL requests is not currently supported.
     if (this.url.startsWith('data:'))
       return;
     console.assert(this._allowInterception, 'Request Interception is not enabled!');
@@ -396,9 +391,6 @@ class Request {
    * @param {string=} errorCode
    */
   async abort(errorCode = 'failed') {
-    // DataURL's are not interceptable. In this case, do nothing.
-    if (this.url.startsWith('data:'))
-      return;
     const errorReason = errorReasons[errorCode];
     console.assert(errorReason, 'Unknown error code: ' + errorCode);
     console.assert(this._allowInterception, 'Request Interception is not enabled!');
@@ -511,26 +503,19 @@ function generateRequestHash(request) {
     postData: request.postData,
     headers: {},
   };
-  const headers = Object.keys(request.headers);
-  headers.sort();
-  for (const header of headers) {
-    if (header === 'Accept' || header === 'Referer' || header === 'X-DevTools-Emulate-Network-Conditions-Client-Id')
-      continue;
-    hash.headers[header] = request.headers[header];
+
+  if (!normalizedURL.startsWith('data:')) {
+    const headers = Object.keys(request.headers);
+    headers.sort();
+    for (const header of headers) {
+      if (header === 'Accept' || header === 'Referer' || header === 'X-DevTools-Emulate-Network-Conditions-Client-Id')
+        continue;
+      hash.headers[header] = request.headers[header];
+    }
   }
   return JSON.stringify(hash);
 }
 
-/**
- * @param {string} urlString
- * @return {string}
- */
-function removeURLHash(urlString) {
-  const urlObject = url.parse(urlString);
-  urlObject.hash = '';
-  return url.format(urlObject);
-}
-
 NetworkManager.Events = {
   Request: 'request',
   Response: 'response',

diff --git a/lib/Page.js b/lib/Page.js
@@ -461,17 +461,14 @@ class Page extends EventEmitter {
     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'];
-    try {
-      // Await for the command to throw exception in case of illegal arguments.
-      await this._client.send('Page.navigate', {url, referrer});
-    } catch (e) {
-      watcher.cancel();
-      helper.removeEventListeners([listener]);
-      throw e;
-    }
-    const error = await navigationPromise;
+    const error = await Promise.race([
+      this._client.send('Page.navigate', {url, referrer})
+          .then(() => navigationPromise)
+          .catch(error => error),
+      navigationPromise
+    ]);
+    watcher.cancel();
     helper.removeEventListeners([listener]);
     if (error)
       throw error;

diff --git a/test/test.js b/test/test.js
@@ -1294,6 +1294,15 @@ describe('Page', function() {
       expect(requests.length).toBe(1);
       expect(requests[0].url).toBe(dataURL);
     }));
+    it('should abort data URLs', SX(async function() {
+      await page.setRequestInterception(true);
+      page.on('request', request => {
+        request.abort();
+      });
+      let error = null;
+      await page.goto('data:text/html,No way!').catch(err => error = err);
+      expect(error.message).toContain('Failed to navigate');
+    }));
     it('should navigate to URL with hash and and fire requests without hash', SX(async function() {
       await page.setRequestInterception(true);
       const requests = [];