-
Notifications
You must be signed in to change notification settings - Fork 9.1k
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: Handle multiple/duplicate Fetch.requestPaused events #7802
Conversation
Requesting reviews from: @jschfflr @Androbin @starrify @marvinhagemeister @dekelev @hemanthreddyk |
@benallfree Looks like your fix resolved the request stalling issue in my case. With caching enabled I no longer see the timeout issue, but I see a lot of unhandled rejections
puppeteer/src/common/NetworkManager.ts Lines 525 to 529 in 641588a
|
@hemanthreddyk Thank you for testing, that helps a lot!
For the |
@benallfree Sry for the confusion here, I wasn't actually using any such extra packages like pdf-generator, I just replaced the name of our internal package with some random name (my bad I used pdf-generator). it is just listening to 'unhandledRejection' event and logging the error. Actual error is being originated from here I think this has nothing to do with your PR, it is happening even in main branch. I could see a same error even when I load google.
|
@hemanthreddyk It seems to be the |
@josepharhar This touches your work in #7764 and #7760 and appears to expose an incorrect extraInfo exception. Can you PTAL? |
@benallfree I think the extraInfo exception is this issue https://bugs.chromium.org/p/chromium/issues/detail?id=1274813 It might still turn out that Puppeteer is doing smth wrong but it's also like that it's a Chromium bug. |
@OrKoN Def a CDP bug. Network.responseReceived says about the
I think the issue is that CDP only flags But in all reality that field can be ignored if we already have an extraInfo event. I think the bug in Puppeteer is that it doesn't expect |
I don't believe that chromium has a race condition, but I also haven't spent the time to look into the chromium bug I filed yet. |
Yep I'm just guessing too. Now I'm curious though :) |
Looks good to me. Which of the related issues will be resolved by this PR? |
Issues this PR should fix:
Issues mentioned just for context: |
Thanks for the details and the contribution! I will try to find time for the other PR next week. |
What kind of change does this PR introduce?
bugfix
Did you add tests for your changes?
TBD
If relevant, did you update the documentation?
N/A
Summary
CDP will sometimes fire multiple
Fetch.requestPaused
events for a singleNetwork.requestWillBeSent
event. Puppeteer incorrectly assumes a 1:1 relationship between these events which causes request interception to hang because the secondFetch.requestPaused
event is nevercontinue()
'd.Does this PR introduce a breaking change?
No, but it's really hard to test this.
Other information
Related issues:
https://github.com/puppeteer/puppeteer/pull/7060/files#diff-3280fbae5b6189c9b87a76ea21a5da3e10140467e29532e0b7b72608036f5070R290
#7475 (comment)
#7475
#7288
#6696
#6735
#7225
https://bugs.chromium.org/p/chromium/issues/detail?id=1196004
Here is a test case:
test.js
To run: