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
Conversation
Thanks @Androbin for the updates! A quick question please: are you sure that the test case you added does trigger the very issue as earlier reported issue in #7038 ? Asking because I recall having the highly similar tests as well and the very test case just won't cause anything wrong. It shall be easy to confirm, though: Just keep the test case and drop the newly added fixes to see whether the issue is triggered. Or maybe you could check your fix towards https://github.com/ because this page stably reproduces the issue, at least for me. I have indeed got some further checks regarding this very issue and have it reported here: https://bugs.chromium.org/p/chromium/issues/detail?id=1196004 |
I have tested every combination of { without the fix, with the fix } and { https://github.com, minimal example included in PR } |
Turns out that for redirect requests, we receive multiple calls to to |
requestId | ||
); | ||
|
||
this._requestIdToRequestWillBeSentEvent.set(requestId, event); |
There was a problem hiding this comment.
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.
Jan, could you please take a look as well? |
In general, it looks good to me but let's wait for a review for Jan since he should know more about the request interception. |
Yes, sure! |
The documentation for |
There are three commits in this PR: testing, refactoring, fixing |
src/common/NetworkManager.ts
Outdated
if (requestPausedEvent) { | ||
const interceptionId = requestPausedEvent.requestId; | ||
this._onRequest(event, interceptionId); | ||
this._requestIdToRequestPausedEvent.delete(requestId); | ||
} else { | ||
this._requestIdToRequestWillBeSentEvent.set(requestId, event); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
The real issue is that the user interception code is called twice (once per |
* There are four possible orders of events: | ||
* A. `_onRequestWillBeSent` | ||
* B. `_onRequestWillBeSent`, `_onRequestPaused` | ||
* C. `_onRequestPaused`, `_onRequestWillBeSent` |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change looks good to me from a technical standpoint.
But could you add some documentation around the expected order of events for redirects?
@OrKoN Here's a remark from upstream: https://bugs.chromium.org/p/chromium/issues/detail?id=1196004#c10
|
@jschfflr Have you looked into this Firefox test failure already? |
Fixes #7038 @starrify
See #6996 (comment) and #6996 (comment) for context