Use in-renderer resource loading for initial WebUI navigation body [chromium/src : main]
Rakina Zata Amni (Gerrit)
Rakina Zata Amni added 2 comments![]( "Open in Gerrit")
// is only for direct callers for `GetResource()`.This doesn't actually get hit in production since the body HTML should be in the ResourceBundle instead, but I'm adding this to make my test work. Somehow I couldn't find a way to override the `ResourceBundle` in the test in a browser test with a real renderer process (ContenClient overriding in the test only affects the browser process side).
Related details
- Keren Zhu
- Ming-Ying Chung
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. |
Ming-Ying Chung (Gerrit)
Ming-Ying Chung added 3 comments![]( "Open in Gerrit")
CHECK(requested_range->ComputeBounds(output_size));Is it intentional to crash when range header exist? It was `CallOnError()`.
}what happen when send_pipe_to_client is false? we don't handle OnReceiveResponse?
/*send_pipe_to_client=*/false, network::mojom::URLResponseHead::New(),Do we need to populate any other headers in addition to content length?
Related details
- Keren Zhu
- 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 3 comments![]( "Open in Gerrit")
CHECK(requested_range->ComputeBounds(output_size));Is it intentional to crash when range header exist? It was `CallOnError()`.
Yeah I was assuming that since the error case from `SendData()` returns early won't hit this, but apparently this fails in some unit test.. I didn't get the time to investigate this today but will take a look soon.
what happen when send_pipe_to_client is false? we don't handle OnReceiveResponse?
Yeah, this serves two purpose: We need the pipe to set the actual `response_body` in `RenderFrameImpl::CommitNavigation`, and [NavigationBodyLoader::OnReceiveResponse](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/loader/fetch/url_loader/navigation_body_loader.cc;l=394;drc=8872eb3933fce45ec87506192ac1f14ec74b6307) (which is what the call would trigger) doesn't actually expect to be called, since normally `OnReceiveResponse()` is called on the browser process during navigation. I'll add a comment here soon to clarify.
/*send_pipe_to_client=*/false, network::mojom::URLResponseHead::New(),Do we need to populate any other headers in addition to content length?
Similar to the `OnReceiveResponse()` call, this isn't actually needed by the `NavigationBodyLoader`, it only needs the `OnComplete()` call that has the body length. There is a call to [ResourceLoadInfoNotifierWrapper()](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/loader/fetch/url_loader/navigation_body_loader.cc;l=444;drc=397107e399d4b0f4a6e801e7a888d7d71778529c) with the response head but I think that also doesn't need the content length. However I think it's a bit subtle and there might be future usage that needs the actual body length in the header.. So maybe this function needs to return the modified header as well. I'll update in the next patch.
Related details
- Keren Zhu
- Ming-Ying Chung
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. |
Keren Zhu (Gerrit)
Keren Zhu added 2 comments![]( "Open in Gerrit")
bool send_pipe_to_client,This parameter is probably too confusing. If there are 2 different ways of using the result pipe, let the caller handles it.
Can we refactor this to always return the pipe? i.e. make it `GetPipe()`.
std::move(pipe_consumer_handle), std::nullopt);As the bot has caught it, this is probably a use-after-move? The moved `pipe_consumer_handle` is returned at the end of the function.
Related details
- Ming-Ying Chung
- 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 5 comments![]( "Open in Gerrit")
This parameter is probably too confusing. If there are 2 different ways of using the result pipe, let the caller handles it.
Can we refactor this to always return the pipe? i.e. make it `GetPipe()`.
That's true, actually I had that at the start but wanted to remove some repetition. But looking at it now it doesn't seem worth the confusion, changing it back and removing some params makes it look so much nicer, thanks!
Rakina Zata AmniIs it intentional to crash when range header exist? It was `CallOnError()`.
Yeah I was assuming that since the error case from `SendData()` returns early won't hit this, but apparently this fails in some unit test.. I didn't get the time to investigate this today but will take a look soon.
Oh actually didn't realize that this function doesn't want to be called twice. I think we can just remove the CHECK.
As the bot has caught it, this is probably a use-after-move? The moved `pipe_consumer_handle` is returned at the end of the function.
This is I think OK since the case where the `pipe_consumer_handle` is used by the caller is mutually exclusive with when we send it using `OnReceiveResponse()`. But it is error-prone, so I've removed it together with the change above.
Rakina Zata Amniwhat happen when send_pipe_to_client is false? we don't handle OnReceiveResponse?
Yeah, this serves two purpose: We need the pipe to set the actual `response_body` in `RenderFrameImpl::CommitNavigation`, and [NavigationBodyLoader::OnReceiveResponse](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/loader/fetch/url_loader/navigation_body_loader.cc;l=394;drc=8872eb3933fce45ec87506192ac1f14ec74b6307) (which is what the call would trigger) doesn't actually expect to be called, since normally `OnReceiveResponse()` is called on the browser process during navigation. I'll add a comment here soon to clarify.
This confusing part is now removed :)
/*send_pipe_to_client=*/false, network::mojom::URLResponseHead::New(),Rakina Zata AmniDo we need to populate any other headers in addition to content length?
Similar to the `OnReceiveResponse()` call, this isn't actually needed by the `NavigationBodyLoader`, it only needs the `OnComplete()` call that has the body length. There is a call to [ResourceLoadInfoNotifierWrapper()](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/loader/fetch/url_loader/navigation_body_loader.cc;l=444;drc=397107e399d4b0f4a6e801e7a888d7d71778529c) with the response head but I think that also doesn't need the content length. However I think it's a bit subtle and there might be future usage that needs the actual body length in the header.. So maybe this function needs to return the modified header as well. I'll update in the next patch.
Ah I think I get what your question is now, there are headers that need to be set for WebUI navigations. That is actually added in the next CL: https://chromium-review.googlesource.com/c/chromium/src/+/7236606/8/content/browser/webui/initial_webui_navigation_url_loader.cc#69
(Also the header handling here has been simplified and we do update the content length in the header now!)
Related details
- Keren Zhu
- Ming-Ying Chung
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. |
Keren Zhu (Gerrit)
Keren Zhu voted and added 3 comments![]( "Open in Gerrit")
Votes added by Keren Zhu
| Code-Review | +1 |
3 comments
// Send the given bytes to the client, or returns it, depending on whether
// `send_pipe_to_client` is true (calls OnReceiveResponse with the pipeThis comment needs update.
auto pipe_and_size = GetPipe(bytes, requested_range);optional: decompose the pair into 2 variables
```suggestion
auto [pipe, size] = GetPipe(bytes, requested_range);
```
Related details
- Ming-Ying Chung
- 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. |
Ming-Ying Chung (Gerrit)
Ming-Ying Chung voted and added 3 comments![]( "Open in Gerrit")
Votes added by Ming-Ying Chung
| Code-Review | +1 |
3 comments
// Initial WebUI navigations loads the response body locally within the
// renderer instead of reading from the pipe from the browser. Set up the
// pipes here.
mojo::PendingRemote<network::mojom::URLLoaderClient> client_remote;
mojo::PendingRemote<network::mojom::URLLoader> loader_remote;
std::ignore = loader_remote.InitWithNewPipeAndPassReceiver();I think it's worth making these more clear that why loader_remote is dropped (does it work without receiver?) but client_remote is still needed.
/*send_pipe_to_client=*/false, network::mojom::URLResponseHead::New(),Rakina Zata AmniDo we need to populate any other headers in addition to content length?
Rakina Zata AmniSimilar to the `OnReceiveResponse()` call, this isn't actually needed by the `NavigationBodyLoader`, it only needs the `OnComplete()` call that has the body length. There is a call to [ResourceLoadInfoNotifierWrapper()](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/loader/fetch/url_loader/navigation_body_loader.cc;l=444;drc=397107e399d4b0f4a6e801e7a888d7d71778529c) with the response head but I think that also doesn't need the content length. However I think it's a bit subtle and there might be future usage that needs the actual body length in the header.. So maybe this function needs to return the modified header as well. I'll update in the next patch.
Ah I think I get what your question is now, there are headers that need to be set for WebUI navigations. That is actually added in the next CL: https://chromium-review.googlesource.com/c/chromium/src/+/7236606/8/content/browser/webui/initial_webui_navigation_url_loader.cc#69
(Also the header handling here has been simplified and we do update the content length in the header now!)
Yes that's what mean. Thanks.
// Initial WebUI navigations loads the response body locally within the
// renderer instead of reading from the pipe from the browser. Set up the
// pipes here.
mojo::PendingRemote<network::mojom::URLLoaderClient> client_remote;
mojo::PendingRemote<network::mojom::URLLoader> loader_remote;
std::ignore = loader_remote.InitWithNewPipeAndPassReceiver();
url_loader_client_endpoints = network::mojom::URLLoaderClientEndpoints::New(
std::move(loader_remote),
client_remote.InitWithNewPipeAndPassReceiver());
// Read the response body locally within the renderer, and make the
// `response_body` pipe point to the result.
CHECK(subresource_loader_factories);
CHECK(subresource_loader_factories->local_resource_loader_config());
CHECK(subresource_loader_factories->local_resource_loader_config()
->sources.contains(commit_params->origin_to_commit));
const blink::mojom::LocalResourceSourcePtr& source =
subresource_loader_factories->local_resource_loader_config()
->sources[commit_params->origin_to_commit];
const std::map<std::string, std::string> replacement_strings(
source->replacement_strings.begin(), source->replacement_strings.end());
size_t output_size;
std::tie(response_body, output_size) = webui::GetPipe(
LocalResourceURLLoaderFactory::GetResource(
common_params->url, source, replacement_strings, "text/html"),
/*requested_range=*/std::nullopt);
// Update the content length, and send the completion signal with the
// correct length. Note that we don't need to call `OnReceiveResponse()`
// here, since `NavigationBodyLoader` doesn't need it, and just reads from
// the `response_body` pipe directly.
response_head->content_length = output_size;
mojo::Remote<network::mojom::URLLoaderClient> client(
std::move(client_remote));
network::URLLoaderCompletionStatus status(net::OK);
status.encoded_data_length = output_size;
status.encoded_body_length = output_size;
status.decoded_body_length = output_size;
client->OnComplete(status);optional but maybe we cn extract this out into a helper.
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 added 5 comments![]( "Open in Gerrit")
// Send the given bytes to the client, or returns it, depending on whether
// `send_pipe_to_client` is true (calls OnReceiveResponse with the pipeRakina Zata AmniThis comment needs update.
Done
optional: decompose the pair into 2 variables
```suggestion
auto [pipe, size] = GetPipe(bytes, requested_range);
```
Done
// is only for direct callers for `GetResource()`.This doesn't actually get hit in production since the body HTML should be in the ResourceBundle instead, but I'm adding this to make my test work. Somehow I couldn't find a way to override the `ResourceBundle` in the test in a browser test with a real renderer process (ContenClient overriding in the test only affects the browser process side).
Done
// Initial WebUI navigations loads the response body locally within the
// renderer instead of reading from the pipe from the browser. Set up the
// pipes here.
mojo::PendingRemote<network::mojom::URLLoaderClient> client_remote;
mojo::PendingRemote<network::mojom::URLLoader> loader_remote;
std::ignore = loader_remote.InitWithNewPipeAndPassReceiver();I think it's worth making these more clear that why loader_remote is dropped (does it work without receiver?) but client_remote is still needed.
Added comment, and it's actually possible to just use a `NullRemote` so I switched to that
// Initial WebUI navigations loads the response body locally within theYeah that's a good idea, done
Related details
Code-Coverage
Code-Owners
Code-Review
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 1 comment![]( "Open in Gerrit")
Votes added by Rakina Zata Amni
| Commit-Queue | +2 |
Thanks for the reviews!
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
20 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: content/renderer/render_frame_impl.cc
Insertions: 46, Deletions: 37.
The diff is too large to show. Please review the diff.
```
```
The name of the file: content/common/web_ui_loading_util.h
Insertions: 2, Deletions: 4.
The diff is too large to show. Please review the diff.
```
```
The name of the file: content/common/web_ui_loading_util.cc
Insertions: 6, Deletions: 7.
The diff is too large to show. Please review the diff.
```
Change information
Use in-renderer resource loading for initial WebUI navigation body
The HTML body for initial WebUIs already exist in the ResourceBundle
accessible from within the renderer process. This means when
navigating to initial WebUIs, we can skip the async step on the
browser-side WebUIURLLoader to load the body, and do that from
within the renderer when committing the navigation instead. This
helps remove an asynchronous step on the browser side that prevents
the initial WebUI navigation to go from start -> commit
synchronously.
- M content/common/web_ui_loading_util.cc
- M content/common/web_ui_loading_util.h
- M content/renderer/local_resource_url_loader_factory.cc
- M content/renderer/local_resource_url_loader_factory.h
- M content/renderer/render_frame_impl.cc
Code-Review: +1 by Keren Zhu, +1 by Ming-Ying Chung
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |