feat(chromium)!: roll to Chromium 97.0.4692.0 (r938248)

[OrKoN]

Issues: #7458

[mathiasbynens]

For my own understanding, is this a TODO you expect to wrap up before landing this patch, or a follow-up? (In case of the latter, let's file a tracking bug?)

[OrKoN]

I copied it from Joey's PR #7640 so I plan to add him as a reviewer here.

[josepharhar]

I think I tried to do this but ran into an issue. I'll try again and update with more context

[josepharhar]

Ok, I just pushed a commit to my PR which takes care of this.

[OrKoN]

@josepharhar I am trying to roll Chromium now that your CLs have landed. I have copied the fixes from your PR #7640 but there is still one test failing (https://github.com/puppeteer/puppeteer/runs/4176540107?check_suite_focus=true) Do you think it could be related to your CLs too? Also, please review the changes.

[OrKoN]

It looks like the status of the response after the redirect is sometimes wrong (302 instead of 200) https://github.com/puppeteer/puppeteer/runs/4177611057?check_suite_focus=true

[josepharhar]

Sorry I didn’t look at this today, hopefully I’ll have a chance tomorrow

[josepharhar]

I checked out this PR on my glinux machine, and I wasn't able to reproduce the failure when running the tests.
I wonder if its flaky or if I'm doing something wrong... can you reproduce the failure locally?

Wouldn't it make sense to do this roll and let me merge my PR separately? You could mark any network tests as failing if needed.

[OrKoN]

I checked out this PR on my glinux machine, and I wasn't able to reproduce the failure when running the tests.
I wonder if its flaky or if I'm doing something wrong... can you reproduce the failure locally?

No, I cannot reproduce locally as well :( I am going to try to reproduce it again next week.

Wouldn't it make sense to do this roll and let me merge my PR separately? You could mark any network tests as failing if needed.

Usually we fix everything before or with the roll PR. I think it does not make much sense to roll with the broken functionality. Feel free to make changes to this branch if you need to! Or you can take over other changes to your PR and include the roll there.

[OrKoN]

I wonder if its flaky

I believe it's flaky. Sometimes there are completely different network test failing but all seem to be failing on assertions for the network response attributes.

[josepharhar]

I think that page.goto isn't waiting for responseReceviedExtraInfo, I'm working on a fix and I'll push it to this PR soon

[josepharhar]

I don't have permission to push to this branch, but I added commits to my PR to try to try to wait for ExtraInfo events. Unfortunately the tests still fail, and I don't know if my changes actually made it better or not because I can't reproduce the failure locally and I can't even push a commit that uses it.only because the github actions build will fail before it tries to run the tests.
It might be that my changes don't account for redirects. I can try to look into this more another day.

[OrKoN]

@josepharhar is the eslint complaining about .only? usually you can push /* eslint-disable RuleNameItComplainsAbout */ to suppress it

[OrKoN]

I have merged the changes from the other branch here and cleaned up some debugging stuff. Let's see if it passes now.

[OrKoN]

@josepharhar I was able to reproduce the flake with the CDP log on the bot. Here is the relevant part https://gist.github.com/OrKoN/369323d11b1106c525f23a4ad9ec5a5b So the test fails because the response code is 302 instead of 200. I can see the extra info events with 302 and 200 status codes so I suspect that we might be calling _emitResponseEvent with the wrong extraInfo event.

[OrKoN]

@josepharhar I came to a conclusion that there is an extra extraInfo event sent for empty.html URL sometimes that cause the wrong HTTP status for the entire redirection chain. I have captured that in a reproducible new (true) unit test. Could you please take a look?

[OrKoN]

@josepharhar in particular, I have added this check here: is my understanding correct that there should be no unprocessed extraInfo events if we emit _emitResponseEvent?

[josepharhar]

For each redirect, chromium should emit one requestWillBeSentExtraInfo and one responseReceivedExtraInfo.
It should look like this for a request which redirects twice:

• first request:
  • requestWillBeSent
  • requestWillBeSentExtraInfo
  • responseReceivedExtraInfo
• second request:
  • requestWillBeSent
  • requestWillBeSentExtraInfo
  • responseReceivedExtraInfo
• third final request:
  • requestWillBeSent
  • requestWillBeSentExtraInfo
  • responseReceivedExtraInfo
  • responseReceived

Is the issue that the order of events as I listed is wrong? Or is the issue that there are more or less ExtraInfo events than I listed?
If there are more or less ExtraInfo events, then it's a chromium problem. If they are just in a different order in the test, then it's a puppeteer problem.

[josepharhar]

I'm reading the log you shared to try to answer that question

[OrKoN]

@josepharhar thanks a lot for fixing it! I am not very familiar with the backend implementation for these events but it'd be great to expand the unit tests if you know what are the possible/supported (or also unsupported) orders in which events could arrive.

[josepharhar]

Sounds good, I can add more coverage to NetworkManager.spec.ts. I don't think it needs to block this PR.

[OrKoN]

Thanks, I think that we don't need to block on that too. I have just removed some awaits that I have added previously to see if they are redundant with the fixes. For the unit tests, I think we need to make some changes in types and TS configuration so that we could define test cases without defining the entire event payloads that are not used by the network manager directly.

[josepharhar]

We could just move the NetworkManager.spec.ts changes in this PR to a new one, would that resolve it?

[OrKoN]

Let's keep the test for coverage and refactor later. I think this is ready to land: do you want me to land your PR or is it okay to land this one?