From 19606a3b79b881528877204bcdce46416bc171c9 Mon Sep 17 00:00:00 2001 From: Andrey Lushnikov Date: Wed, 10 Apr 2019 00:42:42 -0400 Subject: [PATCH] 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 | 32 +++++++++++++++------ lib/LifecycleWatcher.js | 3 +- lib/NetworkManager.js | 26 ++++++++++++++++-- lib/Page.js | 61 ++++++++++++----------------------------- test/page.spec.js | 12 ++++++++ 5 files changed, 78 insertions(+), 56 deletions(-) diff --git a/lib/FrameManager.js b/lib/FrameManager.js index cfca89a8894..5f42e4f4d33 100644 --- a/lib/FrameManager.js +++ b/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} */ 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 */ diff --git a/lib/LifecycleWatcher.js b/lib/LifecycleWatcher.js index 8d183ca171c..3f1f86dbd66 100644 --- a/lib/LifecycleWatcher.js +++ b/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 => { diff --git a/lib/NetworkManager.js b/lib/NetworkManager.js index 93382dad7ae..f5d45046299 100644 --- a/lib/NetworkManager.js +++ b/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} */ 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} */ 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 */ diff --git a/lib/Page.js b/lib/Page.js index 2f9510aa84b..d619fe403f9 100644 --- a/lib/Page.js +++ b/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} */ static async create(client, target, ignoreHTTPSErrors, defaultViewport, screenshotTaskQueue) { - - await client.send('Page.enable'); - const {frameTree} = await client.send('Page.getFrameTree'); - const page = new Page(client, target, frameTree, ignoreHTTPSErrors, screenshotTaskQueue); + const page = new Page(client, target, 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._networkManager.setFrameManager(this._frameManager); + this._frameManager = new FrameManager(client, this, ignoreHTTPSErrors, this._timeoutSettings); this._emulationManager = new EmulationManager(client); this._tracing = new Tracing(client); /** @type {!Map} */ 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)); - this._networkManager.on(Events.NetworkManager.Response, event => this.emit(Events.Page.Response, event)); - this._networkManager.on(Events.NetworkManager.RequestFailed, event => this.emit(Events.Page.RequestFailed, event)); - this._networkManager.on(Events.NetworkManager.RequestFinished, event => this.emit(Events.Page.RequestFinished, event)); + const networkManager = this._frameManager.networkManager(); + networkManager.on(Events.NetworkManager.Request, event => this.emit(Events.Page.Request, event)); + networkManager.on(Events.NetworkManager.Response, event => this.emit(Events.Page.Response, 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} @@ -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} 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); } /** diff --git a/test/page.spec.js b/test/page.spec.js index 074ef73db13..58f68420a0f 100644 --- a/test/page.spec.js +++ b/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