fix(requestinterception): fix font loading issue #7060

Androbin · 2021-04-06T05:18:18Z

See #6996 (comment) and #6996 (comment) for context

starrify · 2021-04-06T11:51:33Z

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
In the issue report here I stated the very conditions that I think could trigger this issue, together with several minimal test cases I created.

Androbin · 2021-04-06T11:56:06Z

I have tested every combination of { without the fix, with the fix } and { https://github.com, minimal example included in PR }

src/common/NetworkManager.ts

Androbin · 2021-04-07T17:57:17Z

Turns out that for redirect requests, we receive multiple calls to to _onRequestWillBeSent with the same requestId and the current changeset might lead to _onRequestPaused calling _onRequest with a previous _onRequestWillBeSent event. I'm already working on a solution.

Androbin · 2021-04-08T01:18:02Z

src/common/NetworkManager.ts

+        requestId
+      );
+
+      this._requestIdToRequestWillBeSentEvent.set(requestId, event);


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.

src/common/NetworkManager.ts

OrKoN · 2021-04-08T11:08:47Z

Jan, could you please take a look as well?

OrKoN · 2021-04-13T08:08:32Z

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.

jschfflr · 2021-04-13T08:11:35Z

Yes, sure!
This patch looks good to me too from a technical side. But I'm not sure if this is the best way to fix the problem as it seems like this might be better fixed on the chromium side to get a more coherent flow of events.
Do you know about some documentation that specify in which order we can expect the events to arrive?

Androbin · 2021-04-14T15:01:51Z

The documentation for Fetch.requestPaused suggests that it's supposed to fire after Network.requestWillBeSent.
Experiments show that with the cache disabled, this holds true, but enabled, the order of those events is reversed.
And in the case of fonts, Fetch.requestPaused fires both before AND after Network.requestWillBeSent.
The current code anticipates the order to be reversed but it does not anticipate multiple Fetch.requestPaused events.

Androbin · 2021-04-14T15:12:38Z

There are three commits in this PR: testing, refactoring, fixing
The fixing commit consists of storing the requestWillBeSent event unconditionally and a fix for redirects.
For redirect requests, Chromium sends multiple requestWillBeSent events with the same requestId.
That means if we just check the requestId, we might now pair requestPaused with the wrong one.
A redirect might change either the url, the method or both, so we compare the values for both events.

Androbin · 2021-04-14T15:17:10Z

src/common/NetworkManager.ts

      if (requestPausedEvent) {
        const interceptionId = requestPausedEvent.requestId;
        this._onRequest(event, interceptionId);
        this._requestIdToRequestPausedEvent.delete(requestId);
-      } else {
-        this._requestIdToRequestWillBeSentEvent.set(requestId, event);


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

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

Androbin · 2021-04-14T15:23:22Z

The real issue is that the user interception code is called twice (once per requestPaused event).
This PR does not attempt to work around that, that's why we still need a fix to land in Chromium.

OrKoN · 2021-04-26T06:23:54Z

src/common/NetworkManager.ts

+   * There are four possible orders of events:
+   *  A. `_onRequestWillBeSent`
+   *  B. `_onRequestWillBeSent`, `_onRequestPaused`
+   *  C. `_onRequestPaused`, `_onRequestWillBeSent`


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

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)

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

jschfflr

This change looks good to me from a technical standpoint.
But could you add some documentation around the expected order of events for redirects?

…redirects

Androbin · 2021-04-27T19:04:04Z

@OrKoN Here's a remark from upstream: https://bugs.chromium.org/p/chromium/issues/detail?id=1196004#c10

There's no guaranteed order between Fetch.requestPaused and Network.requestWillBeSent, these are inherently racy because they come from different processes.

OrKoN · 2021-04-29T06:31:55Z

@jschfflr Have you looked into this Firefox test failure already?

jschfflr · 2021-04-29T08:30:30Z

@OrKoN Yes, I'm investigating in #7182

google-cla bot added the cla: yes label Apr 6, 2021

mathiasbynens requested a review from OrKoN April 6, 2021 05:53

OrKoN reviewed Apr 7, 2021

View reviewed changes

src/common/NetworkManager.ts Show resolved Hide resolved

OrKoN reviewed Apr 7, 2021

View reviewed changes

src/common/NetworkManager.ts Show resolved Hide resolved

Androbin added 2 commits April 8, 2021 03:09

test(requestinterception): add failing test for fonts issue

49a150c

refactor(requestinterception): refactor NetworkManager.ts

71d4c4a

Androbin commented Apr 8, 2021

View reviewed changes

fix(requestinterception): fix fonts loading issue

5f183e6

OrKoN reviewed Apr 8, 2021

View reviewed changes

src/common/NetworkManager.ts Show resolved Hide resolved

OrKoN requested a review from jschfflr April 8, 2021 11:08

Androbin commented Apr 14, 2021

View reviewed changes

OrKoN reviewed Apr 26, 2021

View reviewed changes

jschfflr approved these changes Apr 27, 2021

View reviewed changes

Androbin added 2 commits April 27, 2021 20:58

docs(requestinterception): document the expected order of events for …

4ada9b4

…redirects

Merge branch 'main' into requestinterception

82cf65b

jschfflr added 2 commits May 3, 2021 10:17

Disable flaky test on firefox (#7182)

129c3bb

Merge branch 'main' into requestinterception

1bafe5e

mathiasbynens force-pushed the main branch from 129c3bb to a293b96 Compare May 3, 2021 08:48

Merge branch 'main' into requestinterception

adea595

OrKoN approved these changes May 6, 2021

View reviewed changes

OrKoN merged commit c9978d2 into puppeteer:main May 6, 2021

Androbin deleted the requestinterception branch May 7, 2021 18:06

This was referenced May 7, 2021

feat(requestinterception): don't disable cache by default #7215

Closed

feat(requestinterception): remove cacheSafe flag #7217

Merged

m90 mentioned this pull request Jun 14, 2021

Update watchify package to v4.0.0 mantoni/mochify.js#226

Merged

Androbin mentioned this pull request Jul 4, 2021

fix(requestinterception): fix font loading issue hardkoded/puppeteer-sharp#1765

Merged

Androbin mentioned this pull request Aug 7, 2021

Cooperative request intercepts #6735

Merged

3 tasks

Androbin mentioned this pull request Nov 23, 2021

Request interception hanging forever #7475

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(requestinterception): fix font loading issue #7060

fix(requestinterception): fix font loading issue #7060

Androbin commented Apr 6, 2021 •

edited

starrify commented Apr 6, 2021

Androbin commented Apr 6, 2021

Androbin commented Apr 7, 2021

Androbin Apr 8, 2021

OrKoN commented Apr 8, 2021

OrKoN commented Apr 13, 2021

jschfflr commented Apr 13, 2021

Androbin commented Apr 14, 2021 •

edited

Androbin commented Apr 14, 2021

Androbin Apr 14, 2021 •

edited

Androbin Apr 14, 2021

Androbin commented Apr 14, 2021

OrKoN Apr 26, 2021

Androbin Apr 26, 2021 •

edited

Androbin Apr 26, 2021 •

edited

jschfflr left a comment

Androbin commented Apr 27, 2021

OrKoN commented Apr 29, 2021

jschfflr commented Apr 29, 2021

fix(requestinterception): fix font loading issue #7060

fix(requestinterception): fix font loading issue #7060

Conversation

Androbin commented Apr 6, 2021 • edited

starrify commented Apr 6, 2021

Androbin commented Apr 6, 2021

Androbin commented Apr 7, 2021

Androbin Apr 8, 2021

Choose a reason for hiding this comment

OrKoN commented Apr 8, 2021

OrKoN commented Apr 13, 2021

jschfflr commented Apr 13, 2021

Androbin commented Apr 14, 2021 • edited

Androbin commented Apr 14, 2021

Androbin Apr 14, 2021 • edited

Choose a reason for hiding this comment

Androbin Apr 14, 2021

Choose a reason for hiding this comment

Androbin commented Apr 14, 2021

OrKoN Apr 26, 2021

Choose a reason for hiding this comment

Androbin Apr 26, 2021 • edited

Choose a reason for hiding this comment

Androbin Apr 26, 2021 • edited

Choose a reason for hiding this comment

jschfflr left a comment

Choose a reason for hiding this comment

Androbin commented Apr 27, 2021

OrKoN commented Apr 29, 2021

jschfflr commented Apr 29, 2021

Androbin commented Apr 6, 2021 •

edited

Androbin commented Apr 14, 2021 •

edited

Androbin Apr 14, 2021 •

edited

Androbin Apr 26, 2021 •

edited

Androbin Apr 26, 2021 •

edited