New issue
Advanced search Search tips

Issue 846334 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 792546



Sign in to add a comment

CORB: Need to make sure that prefetch/caching works

Project Member Reported by lukasza@chromium.org, May 24 2018

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
 
Note that in https://crbug.com/c/809261#c9 there was an attempt to add test coverage via Web Platform Tests (WPT).  I think I'll give up on this direction and try to add a browser test instead.
WIP CL @ https://crrev.com/c/1070601
Project Member

Comment 3 by bugdroid1@chromium.org, 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

Cc: lukasza@chromium.org jam@chromium.org
Owner: ----
Status: Available (was: Assigned)
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).
Components: Internals>Plugins>Pepper
Components: -Internals>Plugins>Pepper
Ooops, wrong bug...
Cc: -lukasza@chromium.org
Owner: lukasza@chromium.org
Status: Started (was: Available)
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.
Cc: kinuko@chromium.org
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.
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).
Project Member

Comment 10 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)
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