fix: cache disabling should stick when toggling request interception #4260
Conversation
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. - 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.
| client.send('Performance.enable', {}), | ||
| client.send('Log.enable', {}), | ||
| ]); | ||
| if (ignoreHTTPSErrors) | ||
| await client.send('Security.setOverrideCertificateErrors', {override: true}); | ||
| // Initialize default page size. |
There was a problem hiding this comment.
The comment got killed! I guess that’s ok
lib/Page.js
Outdated
| await Promise.all([ | ||
| page._networkManager.initialize(), |
There was a problem hiding this comment.
I have to think a lot about this. The frame manager can initialize before the network manager, and it has a network manager that it doesn’t know is uninitialized. I believe that it works, but it’s hard to reason about.
There was a problem hiding this comment.
The frame manager can initialize before the network manager, and it has a network manager that it doesn’t know is uninitialized. I believe that it works, but it’s hard to reason about.
The following observations should help with reasoning:
- all state that depends on events is encapsulated inside the related manager.
pageinstance is not exposed to a userland at this point - so all missed events is a natural race.
Also, there's no semantic change here: the initialization sequence maintains the same contracts as before. So if there's some confidence in the tip-of-tree code, it should be inheritable here.
| const [,{frameTree}] = await Promise.all([ | ||
| this._client.send('Page.enable'), | ||
| this._client.send('Page.getFrameTree'), | ||
| ]); | ||
| this._handleFrameTree(frameTree); |
There was a problem hiding this comment.
We can get frame events in between page Page.getFrameTree and handleFrameTree. What happens to them?
There was a problem hiding this comment.
We can get frame events in between page Page.getFrameTree and handleFrameTree. What happens to them?
This is the unfortunate consequence of the current protocol design. The race is already present in the tip of tree; this patch shortens the gap a bit.
| @@ -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)), | |||
| } | ||
|
|
||
| /** | ||
| * @param {!Object<string, string>} headers | ||
| */ | ||
| async setExtraHTTPHeaders(headers) { | ||
| return this._networkManager.setExtraHTTPHeaders(headers); |
There was a problem hiding this comment.
How do you feel about adding this._networkManager = this._frameMangaer.networkManager()?
There was a problem hiding this comment.
Didn't do it to stress the ownership
This patch:
NetworkManager/FrameManagerso that they enable all therelevant domains themselves. This is a preparation for OOPIF support and
migration onto fetch domain.
websocket since it no longer sends annoying "SecurityStateChanged" events.
Instead, use
Security.setIgnoreCertificateErrorsmethod.bug with caching and request interception interop.