CORB: Need to make sure that prefetch/caching works |
|||||||
Issue description
CrossSiteDocumentResourceHandler tries to ensure that prefetching/caching still works for responses blocked by Cross-Origin Read Blocking (CORB):
const ResourceRequestInfoImpl* info = GetRequestInfo();
if (info && info->detachable_handler()) {
// Ensure that prefetch, etc, continue to cache the response, without
// sending it to the renderer.
info->detachable_handler()->Detach();
} else {
// If it's not detachable, cancel the rest of the request.
controller->Cancel();
}
What's missing is
1. A regresion test for prefetch/caching
2. Replicating prefetch/caching behavior in CORB/NetworkService integration
,
May 24 2018
WIP CL @ https://crrev.com/c/1070601
,
May 31 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a0f4deaef14c4f91fc92f56325eb2270d97c5f65 commit a0f4deaef14c4f91fc92f56325eb2270d97c5f65 Author: Lukasz Anforowicz <lukasza@chromium.org> Date: Thu May 31 18:31:49 2018 Test that CORB doesn't break prefetching / caching. Bug: 846334 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo Change-Id: Iaf93935bc728584cb1dd4e58c6437b02d61505ee Reviewed-on: https://chromium-review.googlesource.com/1070601 Reviewed-by: Charlie Reis <creis@chromium.org> Reviewed-by: John Abd-El-Malek <jam@chromium.org> Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org> Cr-Commit-Position: refs/heads/master@{#563328} [modify] https://crrev.com/a0f4deaef14c4f91fc92f56325eb2270d97c5f65/content/browser/loader/cross_site_document_blocking_browsertest.cc [modify] https://crrev.com/a0f4deaef14c4f91fc92f56325eb2270d97c5f65/services/network/url_loader.cc [modify] https://crrev.com/a0f4deaef14c4f91fc92f56325eb2270d97c5f65/testing/buildbot/filters/mojo.fyi.network_content_browsertests.filter
,
Jun 12 2018
I need help please with figuring out: 1) how URLLoader::BlockResponseForCorb can "detach" the downstream consumer of the response (returning empty body + stripped headers for the renderer) while still keep finishing processing the network response (and populating the network cache). 2) figuring out if "detaching" (both in the NetworkService world and in the old ResourceHandler world) can be done without injecting an error. See DetachableResourceHandler::Detach which sets the error to net::ERR_ABORTED which results in a probably-undesirable, web-observable CORB side-effect of firing link.onerror in the prefetch case (see CrossSiteDocumentBlockingTest.PrefetchIsNotImpacted and how it listens to onerror events in javascript).
,
Jun 14 2018
,
Jun 14 2018
Ooops, wrong bug...
,
Jun 20 2018
WIP CL @ https://crrev.com/c/1108746 I am at a point where I wrote the code that I think should make everything work, but tests are still failing... I need to understand why the final fetch is failing - I suspect that the network cache was not populated when processing the CORB-blocked response.
,
Jun 21 2018
TIL that (I think) prefetch requests are handled in the following way:
- Renderer -> Browser:
- asks for mojom::PrefetchURLLoaderService
- gets mojom::URLLoaderFactory via mojom::PrefetchURLLoaderService::GetFactory
This mojom::URLLoaderFactory is bound-to/implemented by content::PrefetchURLLoaderService,
which lives in the browser process and wraps a real URLLoaderFactory that lives in the
network service
- Renderer -> Browser:
- starts a prefetch request by calling mojom::URLLoaderFactory::CreateLoaderAndStart
which goes to content::PrefetchURLLoaderService::CreateLoaderAndStart
- Browser -> Network Service:
- content::PrefetchURLLoader::PrefetchURLLoader forwards the request to the
network service
I am not yet sure if CORB for prefetch requests should:
1. Operate in the network service:
- this probably would duplicate draining logic across network::URLLoader and content::PrefetchURLLoader
- it is unclear to me if blocking at the network service layer would prevent any disk caches from working
2. Operate in the content::PrefetchURLLoader
- this might be desirable for avoiding resource type dependency in services/network
- this probably would duplicate sniffing / delaying mojom::URLLoaderClient::OnReceiveResponse call
(required for network::CrossOriginReadBlocking::ResponseAnalyzer)
- OTOH, AFAICT PrefetchURLLoader::network_loader_factory_ comes from RFHI::::CloneSubresourceFactories
(see also RFHI::ConnectToPrefetchURLLoaderService). This means that prefetch responses are cancelled
before they get a chance to be cached.
,
Jun 21 2018
I guess I should expand my last point. RFHI::::CloneSubresourceFactories returns the URLLoaderFactory associated with a frame - such factory would have is_corb_enabled=true. Therefore CORB running in network service would cancel the request as soon as it determines that it needs to be blocked (at least this is what would happen in the CORB implementation at ToT today).
,
Jun 26 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/41f16fe15100d43df033a012190e8172bf1eae25 commit 41f16fe15100d43df033a012190e8172bf1eae25 Author: Lukasz Anforowicz <lukasza@chromium.org> Date: Tue Jun 26 16:53:28 2018 Support prefetching in NetworkService version of CORB. Bug: 846334 Cq-Include-Trybots: luci.chromium.try:linux_mojo Change-Id: I63ed2ebb60528b004e3f2ffa605c3f492bf15efd Reviewed-on: https://chromium-review.googlesource.com/1108746 Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Matt Menke <mmenke@chromium.org> Reviewed-by: John Abd-El-Malek <jam@chromium.org> Cr-Commit-Position: refs/heads/master@{#570430} [modify] https://crrev.com/41f16fe15100d43df033a012190e8172bf1eae25/components/mirroring/service/session.cc [modify] https://crrev.com/41f16fe15100d43df033a012190e8172bf1eae25/content/browser/loader/cross_site_document_blocking_browsertest.cc [modify] https://crrev.com/41f16fe15100d43df033a012190e8172bf1eae25/content/public/browser/site_isolation_policy.cc [modify] https://crrev.com/41f16fe15100d43df033a012190e8172bf1eae25/services/network/BUILD.gn [add] https://crrev.com/41f16fe15100d43df033a012190e8172bf1eae25/services/network/empty_url_loader_client.cc [add] https://crrev.com/41f16fe15100d43df033a012190e8172bf1eae25/services/network/empty_url_loader_client.h [modify] https://crrev.com/41f16fe15100d43df033a012190e8172bf1eae25/services/network/public/mojom/network_context.mojom [modify] https://crrev.com/41f16fe15100d43df033a012190e8172bf1eae25/services/network/url_loader.cc [modify] https://crrev.com/41f16fe15100d43df033a012190e8172bf1eae25/services/network/url_loader.h [modify] https://crrev.com/41f16fe15100d43df033a012190e8172bf1eae25/testing/buildbot/filters/mojo.fyi.network_content_browsertests.filter
,
Jun 26 2018
Marking this as fixed. There is still some follow-up work (to get rid of content::ResourceType in the NetworkService), but this is separate from the current bug. |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by lukasza@chromium.org
, May 24 2018