Introduce sync InitialWebUINavigationURLLoader [chromium/src : main]
Rakina Zata Amni (Gerrit)
Rakina Zata Amni added 2 comments![]( "Open in Gerrit")
Hi Alex, would you be able to take a look? I'm a bit unsure if this is the nicest way and it's small enough, so I would like to get your opinion if possible, but if you're fully booked by the hackathon that's fine too -- just let me know if it's better to get other people to review this instead. Thanks in advance!
Also I've been trying to write a browser test which has been pretty annoying so far due to having to mock the ResourceBundle loading in the renderer, but I'll make sure to have tests either here or in a separate CL before landing (I confirmed manually that it works and is synchronous)
class InitialWebUINavigationURLLoader : public NavigationURLLoader {This looks similar to `CachedNavigationURLLoader` and I was thinking of just combining them, but there are some WebUI specific calls in the impl (although pretty simple) and also the cached one can be async due to BFCache (so it can't just be called a `SyncNavigationURLLoader`). So I decided to just make it a separate class, but let me know if there are better options here.
Related details
- Alex Moshchuk
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rakina Zata Amni (Gerrit)
Rakina Zata Amni added 1 comment![]( "Open in Gerrit")
Adding Fergal to review since Alex is still OOO, PTAL :)
Related details
- Alex Moshchuk
- Fergal Daly
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fergal Daly (Gerrit)
Fergal Daly added 3 comments![]( "Open in Gerrit")
Nothing looks wrong and I guess everything only impacts us excep for that one `< READY_TO_COMMIT`.
state_ < NavigationState::READY_TO_COMMIT &&This seems kinda random, what happens if you don't have this? Should there be a test?
class InitialWebUINavigationURLLoader : public NavigationURLLoader {Fergal DalyThis looks similar to `CachedNavigationURLLoader` and I was thinking of just combining them, but there are some WebUI specific calls in the impl (although pretty simple) and also the cached one can be async due to BFCache (so it can't just be called a `SyncNavigationURLLoader`). So I decided to just make it a separate class, but let me know if there are better options here.
Perhaps if https://crbug.com/40188852 was fixed it would make sense but I'm not suggesting you fix that first.
Related details
- Alex Moshchuk
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rakina Zata Amni (Gerrit)
Rakina Zata Amni added 1 comment![]( "Open in Gerrit")
state_ < NavigationState::READY_TO_COMMIT &&This seems kinda random, what happens if you don't have this? Should there be a test?
Clarified in the comment. We don't need to do this if we've committed synchronously (and initial WebUI navs won't need to create speculative RFHs anyways), and calling `GetAssociatedRFHType()` would cause a crash if we call it when we're already at `READY_TO_COMMIT`. We already hit that crash in our test without this, which is why I added this.
Related details
- Alex Moshchuk
- Fergal Daly
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fergal Daly (Gerrit)
Fergal Daly added 2 comments![]( "Open in Gerrit")
I'm OK with your submitting this since it seems like we would only be breaking ourselves. I leave it up to you whether to wait for Alex.
state_ < NavigationState::READY_TO_COMMIT &&Rakina Zata AmniThis seems kinda random, what happens if you don't have this? Should there be a test?
Clarified in the comment. We don't need to do this if we've committed synchronously (and initial WebUI navs won't need to create speculative RFHs anyways), and calling `GetAssociatedRFHType()` would cause a crash if we call it when we're already at `READY_TO_COMMIT`. We already hit that crash in our test without this, which is why I added this.
- Alex Moshchuk
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fergal Daly (Gerrit)
Fergal Daly voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Alex Moshchuk
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Moshchuk (Gerrit)
Alex Moshchuk voted and added 4 comments![]( "Open in Gerrit")
Votes added by Alex Moshchuk
| Code-Review | +1 |
4 comments
LGTM, thanks! (Sorry it took me a while to get to it, with CSA hackathon and being out sick most of the week.)
state_ < NavigationState::READY_TO_COMMIT &&Curious to understand why this wasn't needed before this change, but is needed now? It seems that previously this was kind of handled on lines 5703-5706 below, which should also work for the new loader type? (Though I'm surprised it's behind a disabled-by-default feature param, kCreateSpeculativeRFHFilterRestore - do you know why?)
// differnt from what happens in production (which will use a resource ID),Please fix this WARNING reported by Spellchecker: "differnt" is a possible misspelling of "different".
To bypass Spellchecker, ad...
"differnt" is a possible misspelling of "different".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
CHECK(source);It seems that we might crash here for non-existent/unhandled WebUI URLs? Would the URL always be specified by something in the browser process, or could it come from a WebUI renderer? (If the latter might be possible, it's probably not desirable to crash.)
Related details
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rakina Zata Amni (Gerrit)
Rakina Zata Amni voted and added 5 comments![]( "Open in Gerrit")
Votes added by Rakina Zata Amni
| Commit-Queue | +2 |
Hi Alex, would you be able to take a look? I'm a bit unsure if this is the nicest way and it's small enough, so I would like to get your opinion if possible, but if you're fully booked by the hackathon that's fine too -- just let me know if it's better to get other people to review this instead. Thanks in advance!
Also I've been trying to write a browser test which has been pretty annoying so far due to having to mock the ResourceBundle loading in the renderer, but I'll make sure to have tests either here or in a separate CL before landing (I confirmed manually that it works and is synchronous)
Done
Thanks for all the reviews!!
Curious to understand why this wasn't needed before this change, but is needed now? It seems that previously this was kind of handled on lines 5703-5706 below, which should also work for the new loader type? (Though I'm surprised it's behind a disabled-by-default feature param, kCreateSpeculativeRFHFilterRestore - do you know why?)
Hmm, actually I think for prerender, we would have gone to DidCommit synchronously as well and deleted the NavigationRequest and returning earlier around line 5682. For BFCache, it's actually still asynchronous due to the task posted from `CachedNavigationURLLoader`, and so it hasn't reached `READY_TO_COMMIT` yet. The associated RFH type should be CURRENT due to [this code](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_manager.cc;l=1537-1538;drc=b62302554a75f11c55a399d210aa21b2eb441301).
So maybe we don't need the explicit restore filter.. cc @g...@google.com, did we actually see cases for restore navs hitting this path? It seems like it was added in crrev.com/c/5737130 without a repro test.
// differnt from what happens in production (which will use a resource ID),Please fix this WARNING reported by Spellchecker: "differnt" is a possible misspelling of "different".
To bypass Spellchecker, ad...
"differnt" is a possible misspelling of "different".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
Done
It seems that we might crash here for non-existent/unhandled WebUI URLs? Would the URL always be specified by something in the browser process, or could it come from a WebUI renderer? (If the latter might be possible, it's probably not desirable to crash.)
Yeah I think this is ok as a CHECK: the initial WebUI URL is a `chrome://` URL, which I think already has its own restrictions for renderer-initiated navigations right? We also have the stronger requirements for initial WebUI (must be the first navigation, etc). I can add some more invariant checks to check if all initial WebUI navigations are browser-initiated too around there. (Resolving this to land the CL for faster merging, but feel free to reopen for follow-up work).
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
11 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: content/browser/webui/initial_webui_browsertest.cc
Insertions: 1, Deletions: 1.
The diff is too large to show. Please review the diff.
```
Change information
Introduce sync InitialWebUINavigationURLLoader
Initial WebUI navigations should go from NavigationRequest creation
to CommitNavigation synchronously, to avoid having to wait for the
large browser startup task to finish before being able to render the
browser UI. One last asynchronous step of the initial WebUI navigation
is its NavigationURLLoader, which runs the reading of the body
resource asynchronously and also potentially throttled by
URLLoaderThrottles.
This CL makes initial WebUI navigations to use a new, sync
InitialWebUINavigationURLLoader instead, which immediately triggers
OnResponseStarted from Start(), moving the navigation to the
commit stage synchronously. This is possible because we don't need
to run the body loading on the browser side (thanks to WebUI
in-renderer resource loading) and there's no legitimate reason
for throttling initial WebUI navigations.
See doc: https://docs.google.com/document/d/1apOKbMw6YMAaOckaSiZ2N4AwJ3IemZS16SUh22-jLJc/edit?tab=t.0#heading=h.gi765z8iuoet
- M content/browser/BUILD.gn
- M content/browser/loader/cached_navigation_url_loader.cc
- M content/browser/loader/navigation_url_loader.cc
- M content/browser/loader/navigation_url_loader.h
- M content/browser/renderer_host/navigation_request.cc
- M content/browser/renderer_host/render_frame_host_impl.cc
- M content/browser/webui/initial_webui_browsertest.cc
- A content/browser/webui/initial_webui_navigation_url_loader.cc
- A content/browser/webui/initial_webui_navigation_url_loader.h
Code-Review: +1 by Alex Moshchuk, +1 by Fergal Daly
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |