Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(requestinterception): fix font loading issue #7060

Merged
merged 8 commits into from May 6, 2021
48 changes: 41 additions & 7 deletions src/common/NetworkManager.ts
Expand Up @@ -70,6 +70,25 @@ export class NetworkManager extends EventEmitter {
_ignoreHTTPSErrors: boolean;
_frameManager: FrameManager;

/*
* There are four possible orders of events:
* A. `_onRequestWillBeSent`
* B. `_onRequestWillBeSent`, `_onRequestPaused`
* C. `_onRequestPaused`, `_onRequestWillBeSent`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when does the C sequence happen? So A is when there is no interception and B is the normal interception flow?

Copy link
Contributor Author

@Androbin Androbin Apr 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is correct. The difference between B and C is undocumented.
Seems to depend on whether the cache is enabled and whether the cache is hit or missed: #7038 (comment)

Copy link
Contributor Author

@Androbin Androbin Apr 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This module has always been explicitly anticipating both B and C but it previously failed for D.
D is trivially fixed by preserving the entry in _requestIdToRequestWillBeSentEvent until _onLoadingFinished (and adding a guard clause for redirect requests as they have the same requestId): 5f183e6#r613342144

* D. `_onRequestPaused`, `_onRequestWillBeSent`, `_onRequestPaused`
* (see crbug.com/1196004)
*
* For `_onRequest` we need the event from `_onRequestWillBeSent` and
* optionally the `interceptionId` from `_onRequestPaused`.
*
* If request interception is disabled, call `_onRequest` once per call to
* `_onRequestWillBeSent`.
* If request interception is enabled, call `_onRequest` once per call to
* `_onRequestPaused` (once per `interceptionId`).
*
* Events are stored to allow for subsequent events to call `_onRequest`.
* Note that redirect requests have the same `requestId` (!).
*/
_requestIdToRequestWillBeSentEvent = new Map<
string,
Protocol.Network.RequestWillBeSentEvent
Expand Down Expand Up @@ -252,12 +271,12 @@ export class NetworkManager extends EventEmitter {
requestId
);

this._requestIdToRequestWillBeSentEvent.set(requestId, event);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory, the condition for this line could be stricter:
!requestPausedEvent || (!this._cacheDisabled() && event.type === 'Font')
But we don't know for sure if that would cover all cases.


if (requestPausedEvent) {
const interceptionId = requestPausedEvent.requestId;
this._onRequest(event, interceptionId);
this._requestIdToRequestPausedEvent.delete(requestId);
Androbin marked this conversation as resolved.
Show resolved Hide resolved
} else {
this._requestIdToRequestWillBeSentEvent.set(requestId, event);
Copy link
Contributor Author

@Androbin Androbin Apr 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jschfflr Making this line run unconditionally is the only "workaround" introduced, the rest is just refactoring.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And the value is cleared anyway. I really don't see why we wouldn't do this regardless of Chomium changes.

}

return;
Expand Down Expand Up @@ -308,10 +327,20 @@ export class NetworkManager extends EventEmitter {
return;
}

const requestWillBeSentEvent = this._requestIdToRequestWillBeSentEvent.get(
let requestWillBeSentEvent = this._requestIdToRequestWillBeSentEvent.get(
requestId
);

// redirect requests have the same `requestId`,
if (
requestWillBeSentEvent &&
(requestWillBeSentEvent.request.url !== event.request.url ||
requestWillBeSentEvent.request.method !== event.request.method)
) {
this._requestIdToRequestWillBeSentEvent.delete(requestId);
requestWillBeSentEvent = null;
}

if (requestWillBeSentEvent) {
this._onRequest(requestWillBeSentEvent, interceptionId);
this._requestIdToRequestWillBeSentEvent.delete(requestId);
Expand Down Expand Up @@ -367,7 +396,7 @@ export class NetworkManager extends EventEmitter {
response._resolveBody(
new Error('Response body is unavailable for redirect responses')
);
this._forgetRequest(request);
this._forgetRequest(request, false);
this.emit(NetworkManagerEmittedEvents.Response, response);
this.emit(NetworkManagerEmittedEvents.RequestFinished, request);
}
Expand All @@ -381,12 +410,17 @@ export class NetworkManager extends EventEmitter {
this.emit(NetworkManagerEmittedEvents.Response, response);
}

_forgetRequest(request: HTTPRequest): void {
_forgetRequest(request: HTTPRequest, events: boolean): void {
const requestId = request._requestId;
const interceptionId = request._interceptionId;

this._requestIdToRequest.delete(requestId);
this._attemptedAuthentications.delete(interceptionId);

if (events) {
this._requestIdToRequestWillBeSentEvent.delete(requestId);
this._requestIdToRequestPausedEvent.delete(requestId);
}
}

_onLoadingFinished(event: Protocol.Network.LoadingFinishedEvent): void {
Expand All @@ -398,7 +432,7 @@ export class NetworkManager extends EventEmitter {
// Under certain conditions we never get the Network.responseReceived
// event from protocol. @see https://crbug.com/883475
if (request.response()) request.response()._resolveBody(null);
this._forgetRequest(request);
this._forgetRequest(request, true);
this.emit(NetworkManagerEmittedEvents.RequestFinished, request);
}

Expand All @@ -410,7 +444,7 @@ export class NetworkManager extends EventEmitter {
request._failureText = event.errorText;
const response = request.response();
if (response) response._resolveBody(null);
this._forgetRequest(request);
this._forgetRequest(request, true);
this.emit(NetworkManagerEmittedEvents.RequestFailed, request);
}
}