-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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: reset streams on prerender only for affected browser versions #24317
fix: reset streams on prerender only for affected browser versions #24317
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
Builds ready [7f7b46b]
Page Load Metrics (547 ± 472 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #24317 +/- ##
===========================================
+ Coverage 67.31% 67.32% +0.01%
===========================================
Files 1276 1276
Lines 49731 49739 +8
Branches 12921 12925 +4
===========================================
+ Hits 33474 33482 +8
Misses 16257 16257 ☔ View full report in Codecov by Sentry. |
@@ -23,3 +23,24 @@ export const OUTDATED_BROWSER_VERSIONS = { | |||
// See https://en.wikipedia.org/wiki/History_of_the_Opera_web_browser |
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.
Probably we could treat Opera 99-(107?) as affected as well, but maybe not worth worrying about given the impact of the bug. We can adjust this later if we need to. Who knows what they're changing in their fork.
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.
LGTM!
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.
LGTM! Thanks for so much great digging here @jiexi!
Builds ready [08279ef]
Page Load Metrics (508 ± 537 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Description
The prerender regression in chromium has been resolved already. Resetting streams on prerendered pages no longer required for newer chromium versions and is actually the cause of unresponsive provider in many cases. This PR modifies the existing workaround to target only the affected chromium ranges from >=113 to <121.
My bisection of chromium for the fix landed on this range of commits: https://chromium.googlesource.com/chromium/src/+log/55b4344edfb41dda980d197743f25a2841d498a4..c13107c16780c195bd5ec003d9198d87cdcc59dd
@shanejonas and I have verified that the prerender issue has been resolved in the latest chrome stable. I have verified that the workaround is causing an issue in develop that is no longer seen in this branch that reverts it.
Related issues
See: #19727
Manual testing steps
Using chrome, of course.
Visit
chrome://settings/?search=preload
and ensure it is enabled and on "standard"https://voyager-snap.linea.build/
https://voyager-snap.linea.build/
to your clipboardhttps://voyager-snap.linea.build/
that may be openDo the same steps above, but using the changes in this branch. The page should never be stuck with "Loading" if you are on a chromium version from 102 to 112, or 121 onwards.
Screenshots/Recordings
Before
Screen.Recording.2024-04-29.at.11.34.02.AM.mov
After
Screen.Recording.2024-04-29.at.11.32.27.AM.mov
Browser version targeting proof
The following screenshots show the return value for
getIsBrowserPrerenderBroken()
on various chromium versions120:
119:
113:
112:
Pre-merge author checklist
Pre-merge reviewer checklist