fix: cache disabling should stick when toggling request interception (#4260)

This patch:
- refactors `NetworkManager`/`FrameManager` so that they enable all the
relevant domains themselves. This is a preparation for OOPIF support and
migration onto fetch domain.
- moves `networkManager` ownership into `FrameManager`. This way it's clear who owns what.
- stops enabling Security domain: it saves quite some traffic over
websocket since it no longer sends annoying "SecurityStateChanged" events.
Instead, use `Security.setIgnoreCertificateErrors` method.
- consolidates network cache state in network manager. This even fixes a
bug with caching and request interception interop.

lib/FrameManager.js

@@ -20,22 +20,23 @@ const {Events} = require('./Events');
 const {ExecutionContext, EVALUATION_SCRIPT_URL} = require('./ExecutionContext');
 const {LifecycleWatcher} = require('./LifecycleWatcher');
 const {DOMWorld} = require('./DOMWorld');
+const {NetworkManager} = require('./NetworkManager');
 
 const UTILITY_WORLD_NAME = '__puppeteer_utility_world__';
 
 class FrameManager extends EventEmitter {
   /**
    * @param {!Puppeteer.CDPSession} client
-   * @param {!Protocol.Page.FrameTree} frameTree
    * @param {!Puppeteer.Page} page
-   * @param {!Puppeteer.NetworkManager} networkManager
+   * @param {boolean} ignoreHTTPSErrors
    * @param {!Puppeteer.TimeoutSettings} timeoutSettings
    */
-  constructor(client, frameTree, page, networkManager, timeoutSettings) {
+  constructor(client, page, ignoreHTTPSErrors, timeoutSettings) {
     super();
     this._client = client;
     this._page = page;
-    this._networkManager = networkManager;
+    this._networkManager = new NetworkManager(client, ignoreHTTPSErrors);
+    this._networkManager.setFrameManager(this);
     this._timeoutSettings = timeoutSettings;
     /** @type {!Map<string, !Frame>} */
     this._frames = new Map();
@@ -53,7 +54,26 @@ class FrameManager extends EventEmitter {
     this._client.on('Runtime.executionContextDestroyed', event => this._onExecutionContextDestroyed(event.executionContextId));
     this._client.on('Runtime.executionContextsCleared', event => this._onExecutionContextsCleared());
     this._client.on('Page.lifecycleEvent', event => this._onLifecycleEvent(event));
+  }
+
+  async initialize() {
+    const [,{frameTree}] = await Promise.all([
+      this._client.send('Page.enable'),
+      this._client.send('Page.getFrameTree'),
+    ]);
     this._handleFrameTree(frameTree);
+    await Promise.all([
+      this._client.send('Page.setLifecycleEventsEnabled', { enabled: true }),
+      this._client.send('Runtime.enable', {}).then(() => this._ensureIsolatedWorld(UTILITY_WORLD_NAME)),
+      this._networkManager.initialize(),
+    ]);
+  }
+
+  /**
+   * @return {!NetworkManager}
+   */
+  networkManager() {
+    return this._networkManager;
   }
 
   /**
@@ -241,10 +261,6 @@ class FrameManager extends EventEmitter {
     this.emit(Events.FrameManager.FrameNavigated, frame);
   }
 
-  async ensureSecondaryDOMWorld() {
-    await this._ensureIsolatedWorld(UTILITY_WORLD_NAME);
-  }
-
   /**
    * @param {string} name
    */

lib/LifecycleWatcher.js

@@ -37,7 +37,6 @@ class LifecycleWatcher {
     });
 
     this._frameManager = frameManager;
-    this._networkManager = frameManager._networkManager;
     this._frame = frame;
     this._initialLoaderId = frame._loaderId;
     this._timeout = timeout;
@@ -48,7 +47,7 @@ class LifecycleWatcher {
       helper.addEventListener(this._frameManager, Events.FrameManager.LifecycleEvent, this._checkLifecycleComplete.bind(this)),
       helper.addEventListener(this._frameManager, Events.FrameManager.FrameNavigatedWithinDocument, this._navigatedWithinDocument.bind(this)),
       helper.addEventListener(this._frameManager, Events.FrameManager.FrameDetached, this._onFrameDetached.bind(this)),
-      helper.addEventListener(this._networkManager, Events.NetworkManager.Request, this._onRequest.bind(this)),
+      helper.addEventListener(this._frameManager.networkManager(), Events.NetworkManager.Request, this._onRequest.bind(this)),
     ];
 
     this._sameDocumentNavigationPromise = new Promise(fulfill => {

lib/NetworkManager.js

@@ -21,9 +21,10 @@ class NetworkManager extends EventEmitter {
   /**
    * @param {!Puppeteer.CDPSession} client
    */
-  constructor(client) {
+  constructor(client, ignoreHTTPSErrors) {
     super();
     this._client = client;
+    this._ignoreHTTPSErrors = ignoreHTTPSErrors;
     this._frameManager = null;
     /** @type {!Map<string, !Request>} */
     this._requestIdToRequest = new Map();
@@ -40,6 +41,7 @@ class NetworkManager extends EventEmitter {
     this._attemptedAuthentications = new Set();
     this._userRequestInterceptionEnabled = false;
     this._protocolRequestInterceptionEnabled = false;
+    this._userCacheDisabled = false;
     /** @type {!Map<string, string>} */
     this._requestIdToInterceptionId = new Map();
 
@@ -51,6 +53,12 @@ class NetworkManager extends EventEmitter {
     this._client.on('Network.loadingFailed', this._onLoadingFailed.bind(this));
   }
 
+  async initialize() {
+    await this._client.send('Network.enable');
+    if (this._ignoreHTTPSErrors)
+      await this._client.send('Security.setIgnoreCertificateErrors', {ignore: true});
+  }
+
   /**
    * @param {!Puppeteer.FrameManager} frameManager
    */
@@ -109,6 +117,14 @@ class NetworkManager extends EventEmitter {
     await this._client.send('Network.setUserAgentOverride', { userAgent });
   }
 
+  /**
+   * @param {boolean} enabled
+   */
+  async setCacheEnabled(enabled) {
+    this._userCacheDisabled = !enabled;
+    await this._updateProtocolCacheDisabled();
+  }
+
   /**
    * @param {boolean} value
    */
@@ -124,11 +140,17 @@ class NetworkManager extends EventEmitter {
     this._protocolRequestInterceptionEnabled = enabled;
     const patterns = enabled ? [{urlPattern: '*'}] : [];
     await Promise.all([
-      this._client.send('Network.setCacheDisabled', {cacheDisabled: enabled}),
+      this._updateProtocolCacheDisabled(),
       this._client.send('Network.setRequestInterception', {patterns})
     ]);
   }
 
+  async _updateProtocolCacheDisabled() {
+    await this._client.send('Network.setCacheDisabled', {
+      cacheDisabled: this._userCacheDisabled || this._protocolRequestInterceptionEnabled
+    });
+  }
+
   /**
    * @param {!Protocol.Network.requestWillBeSentPayload} event
    */

lib/Page.js

@@ -19,7 +19,6 @@ const EventEmitter = require('events');
 const mime = require('mime');
 const {Events} = require('./Events');
 const {Connection} = require('./Connection');
-const {NetworkManager} = require('./NetworkManager');
 const {Dialog} = require('./Dialog');
 const {EmulationManager} = require('./EmulationManager');
 const {FrameManager} = require('./FrameManager');
@@ -43,36 +42,25 @@ class Page extends EventEmitter {
    * @return {!Promise<!Page>}
    */
   static async create(client, target, ignoreHTTPSErrors, defaultViewport, screenshotTaskQueue) {
-
+    const page = new Page(client, target, ignoreHTTPSErrors, screenshotTaskQueue);
-    await client.send('Page.enable');
-    const {frameTree} = await client.send('Page.getFrameTree');
-    const page = new Page(client, target, frameTree, ignoreHTTPSErrors, screenshotTaskQueue);
     await Promise.all([
+      page._frameManager.initialize(),
       client.send('Target.setAutoAttach', {autoAttach: true, waitForDebuggerOnStart: false, flatten: true}),
-      client.send('Page.setLifecycleEventsEnabled', { enabled: true }),
-      client.send('Network.enable', {}),
-      client.send('Runtime.enable', {}).then(() => page._frameManager.ensureSecondaryDOMWorld()),
-      client.send('Security.enable', {}),
       client.send('Performance.enable', {}),
       client.send('Log.enable', {}),
     ]);
-    if (ignoreHTTPSErrors)
-      await client.send('Security.setOverrideCertificateErrors', {override: true});
-    // Initialize default page size.
     if (defaultViewport)
       await page.setViewport(defaultViewport);
-
     return page;
   }
 
   /**
    * @param {!Puppeteer.CDPSession} client
    * @param {!Puppeteer.Target} target
-   * @param {!Protocol.Page.FrameTree} frameTree
    * @param {boolean} ignoreHTTPSErrors
    * @param {!Puppeteer.TaskQueue} screenshotTaskQueue
    */
-  constructor(client, target, frameTree, ignoreHTTPSErrors, screenshotTaskQueue) {
+  constructor(client, target, ignoreHTTPSErrors, screenshotTaskQueue) {
     super();
     this._closed = false;
     this._client = client;
@@ -82,15 +70,12 @@ class Page extends EventEmitter {
     this._timeoutSettings = new TimeoutSettings();
     this._touchscreen = new Touchscreen(client, this._keyboard);
     this._accessibility = new Accessibility(client);
-    this._networkManager = new NetworkManager(client);
     /** @type {!FrameManager} */
-    this._frameManager = new FrameManager(client, frameTree, this, this._networkManager, this._timeoutSettings);
+    this._frameManager = new FrameManager(client, this, ignoreHTTPSErrors, this._timeoutSettings);
-    this._networkManager.setFrameManager(this._frameManager);
     this._emulationManager = new EmulationManager(client);
     this._tracing = new Tracing(client);
     /** @type {!Map<string, Function>} */
     this._pageBindings = new Map();
-    this._ignoreHTTPSErrors = ignoreHTTPSErrors;
     this._coverage = new Coverage(client);
     this._javascriptEnabled = true;
     /** @type {?Puppeteer.Viewport} */
@@ -125,10 +110,11 @@ class Page extends EventEmitter {
     this._frameManager.on(Events.FrameManager.FrameDetached, event => this.emit(Events.Page.FrameDetached, event));
     this._frameManager.on(Events.FrameManager.FrameNavigated, event => this.emit(Events.Page.FrameNavigated, event));
 
-    this._networkManager.on(Events.NetworkManager.Request, event => this.emit(Events.Page.Request, event));
+    const networkManager = this._frameManager.networkManager();
-    this._networkManager.on(Events.NetworkManager.Response, event => this.emit(Events.Page.Response, event));
+    networkManager.on(Events.NetworkManager.Request, event => this.emit(Events.Page.Request, event));
-    this._networkManager.on(Events.NetworkManager.RequestFailed, event => this.emit(Events.Page.RequestFailed, event));
+    networkManager.on(Events.NetworkManager.Response, event => this.emit(Events.Page.Response, event));
-    this._networkManager.on(Events.NetworkManager.RequestFinished, event => this.emit(Events.Page.RequestFinished, event));
+    networkManager.on(Events.NetworkManager.RequestFailed, event => this.emit(Events.Page.RequestFailed, event));
+    networkManager.on(Events.NetworkManager.RequestFinished, event => this.emit(Events.Page.RequestFinished, event));
 
     client.on('Page.domContentEventFired', event => this.emit(Events.Page.DOMContentLoaded));
     client.on('Page.loadEventFired', event => this.emit(Events.Page.Load));
@@ -136,7 +122,6 @@ class Page extends EventEmitter {
     client.on('Runtime.bindingCalled', event => this._onBindingCalled(event));
     client.on('Page.javascriptDialogOpening', event => this._onDialog(event));
     client.on('Runtime.exceptionThrown', exception => this._handleException(exception.exceptionDetails));
-    client.on('Security.certificateError', event => this._onCertificateError(event));
     client.on('Inspector.targetCrashed', event => this._onTargetCrashed());
     client.on('Performance.metrics', event => this._emitMetrics(event));
     client.on('Log.entryAdded', event => this._onLogEntryAdded(event));
@@ -256,14 +241,14 @@ class Page extends EventEmitter {
    * @param {boolean} value
    */
   async setRequestInterception(value) {
-    return this._networkManager.setRequestInterception(value);
+    return this._frameManager.networkManager().setRequestInterception(value);
   }
 
   /**
    * @param {boolean} enabled
    */
   setOfflineMode(enabled) {
-    return this._networkManager.setOfflineMode(enabled);
+    return this._frameManager.networkManager().setOfflineMode(enabled);
   }
 
   /**
@@ -280,18 +265,6 @@ class Page extends EventEmitter {
     this._timeoutSettings.setDefaultTimeout(timeout);
   }
 
-  /**
-   * @param {!Protocol.Security.certificateErrorPayload} event
-   */
-  _onCertificateError(event) {
-    if (!this._ignoreHTTPSErrors)
-      return;
-    this._client.send('Security.handleCertificateError', {
-      eventId: event.eventId,
-      action: 'continue'
-    }).catch(debugError);
-  }
-
   /**
    * @param {string} selector
    * @return {!Promise<?Puppeteer.ElementHandle>}
@@ -449,21 +422,21 @@ class Page extends EventEmitter {
    * @param {?{username: string, password: string}} credentials
    */
   async authenticate(credentials) {
-    return this._networkManager.authenticate(credentials);
+    return this._frameManager.networkManager().authenticate(credentials);
   }
 
   /**
    * @param {!Object<string, string>} headers
    */
   async setExtraHTTPHeaders(headers) {
-    return this._networkManager.setExtraHTTPHeaders(headers);
+    return this._frameManager.networkManager().setExtraHTTPHeaders(headers);
   }
 
   /**
    * @param {string} userAgent
    */
   async setUserAgent(userAgent) {
-    return this._networkManager.setUserAgent(userAgent);
+    return this._frameManager.networkManager().setUserAgent(userAgent);
   }
 
   /**
@@ -685,7 +658,7 @@ class Page extends EventEmitter {
     const {
       timeout = this._timeoutSettings.timeout(),
     } = options;
-    return helper.waitForEvent(this._networkManager, Events.NetworkManager.Request, request => {
+    return helper.waitForEvent(this._frameManager.networkManager(), Events.NetworkManager.Request, request => {
       if (helper.isString(urlOrPredicate))
         return (urlOrPredicate === request.url());
       if (typeof urlOrPredicate === 'function')
@@ -703,7 +676,7 @@ class Page extends EventEmitter {
     const {
       timeout = this._timeoutSettings.timeout(),
     } = options;
-    return helper.waitForEvent(this._networkManager, Events.NetworkManager.Response, response => {
+    return helper.waitForEvent(this._frameManager.networkManager(), Events.NetworkManager.Response, response => {
       if (helper.isString(urlOrPredicate))
         return (urlOrPredicate === response.url());
       if (typeof urlOrPredicate === 'function')
@@ -822,7 +795,7 @@ class Page extends EventEmitter {
    * @param {boolean} enabled
    */
   async setCacheEnabled(enabled = true) {
-    await this._client.send('Network.setCacheDisabled', {cacheDisabled: !enabled});
+    await this._frameManager.networkManager().setCacheEnabled(enabled);
   }
 
   /**

test/page.spec.js

@@ -994,6 +994,18 @@ module.exports.addTests = function({testRunner, expect, headless, Errors, Device
       ]);
       expect(nonCachedRequest.headers['if-modified-since']).toBe(undefined);
     });
+    it('should stay disabled when toggling request interception on/off', async({page, server}) => {
+      await page.setCacheEnabled(false);
+      await page.setRequestInterception(true);
+      await page.setRequestInterception(false);
+
+      await page.goto(server.PREFIX + '/cached/one-style.html');
+      const [nonCachedRequest] = await Promise.all([
+        server.waitForRequest('/cached/one-style.html'),
+        page.reload(),
+      ]);
+      expect(nonCachedRequest.headers['if-modified-since']).toBe(undefined);
+    });
   });
 
   // Printing to pdf is currently only supported in headless