New issue
Advanced search Search tips

Issue 873061 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 27
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug

Blocked on:
issue 825717
issue 873575

Blocking:
issue 715640



Sign in to add a comment

Variation ids aren't added to navigation preload requests.

Project Member Reported by falken@chromium.org, Aug 10

Issue description

Field study ids aren't added to navigation preload requests.

This is confusing server-side metrics.
 
Cc: kinuko@chromium.org
Kinuko: Do you think this is a type of problem that happens by using a raw network factory instead of ThrottlingURLLoader? 

It looks like GoogleURLLoaderThrottle normally would add the variations header for navigations.
It looks like x-client-data is present for nav preload in the non-s13nsw case, though I'm not sure where it gets injected. The non-s13nsw case uses:
  auto url_loader_factory = std::make_unique<URLLoaderFactoryImpl>(
      ResourceRequesterInfo::CreateForNavigationPreload(requester_info));
  url_loader_factory->CreateLoaderAndStart(

And the s13nsw case uses:
   url_loader_factory_getter->GetNetworkFactory()->CreateLoaderAndStart

Oh there is a beautiful comment here:
https://cs.chromium.org/chromium/src/components/variations/variations_http_header_provider.cc?l=24&rcl=3bbeb5c8e30902453ab121eaede2649007b41299

Maybe we can make nav preload use a simple url loader, or else make it go through throttles.
Blockedon: 825717
Blocking: 715640
Owner: falken@chromium.org
Status: Started (was: Available)
I'm planning to  fix this as part of  issue 873575 .
Project Member

Comment 7 by bugdroid1@chromium.org, Aug 17

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b7da35746e044414f0ecda8fad9b92c88bd26f21

commit b7da35746e044414f0ecda8fad9b92c88bd26f21
Author: Matt Falkenhagen <falken@chromium.org>
Date: Fri Aug 17 01:14:19 2018

service worker: Filter embedder-injected headers at the renderer instead of browser.

The legacy/default path was filtering X-Client-Data at
ServiceWorkerURLRequestJob.

The S13nSW/NetworkService path did not need to filter this yet, because
it gets injected by GoogleURLLoaderThrottle in that path, and throttling
happens after SW interception. However the plan is to move throttling in
front of SW interception, so the SW would start seeing the headers. This
is needed so the navigation preload request can have the header too, and
also looks right for correctness as some throttles also mutate the
request in ways that the service worker should observe; and also would
prevent wasteful double-interception when the request URL changes.

Since in S13nSW/NetworkService the header can also be injected by the
renderer and sent directly to the service worker, filter the headers
just before the event is dispatched.

Bug:  873061 ,  873575 
Change-Id: I6a2011a2d1e6d1d6c08b1fc254b2cd7ae013d9f3
Reviewed-on: https://chromium-review.googlesource.com/1177418
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583916}
[modify] https://crrev.com/b7da35746e044414f0ecda8fad9b92c88bd26f21/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc
[modify] https://crrev.com/b7da35746e044414f0ecda8fad9b92c88bd26f21/chrome/renderer/chrome_content_renderer_client.cc
[modify] https://crrev.com/b7da35746e044414f0ecda8fad9b92c88bd26f21/chrome/renderer/chrome_content_renderer_client.h
[modify] https://crrev.com/b7da35746e044414f0ecda8fad9b92c88bd26f21/components/variations/net/variations_http_headers.cc
[modify] https://crrev.com/b7da35746e044414f0ecda8fad9b92c88bd26f21/components/variations/net/variations_http_headers.h
[modify] https://crrev.com/b7da35746e044414f0ecda8fad9b92c88bd26f21/content/browser/service_worker/service_worker_context_core.cc
[modify] https://crrev.com/b7da35746e044414f0ecda8fad9b92c88bd26f21/content/browser/service_worker/service_worker_context_wrapper.cc
[modify] https://crrev.com/b7da35746e044414f0ecda8fad9b92c88bd26f21/content/browser/service_worker/service_worker_url_request_job.cc
[modify] https://crrev.com/b7da35746e044414f0ecda8fad9b92c88bd26f21/content/public/browser/service_worker_context.h
[modify] https://crrev.com/b7da35746e044414f0ecda8fad9b92c88bd26f21/content/public/renderer/content_renderer_client.cc
[modify] https://crrev.com/b7da35746e044414f0ecda8fad9b92c88bd26f21/content/public/renderer/content_renderer_client.h
[modify] https://crrev.com/b7da35746e044414f0ecda8fad9b92c88bd26f21/content/renderer/service_worker/service_worker_context_client.cc
[modify] https://crrev.com/b7da35746e044414f0ecda8fad9b92c88bd26f21/content/renderer/service_worker/service_worker_context_client_unittest.cc
[modify] https://crrev.com/b7da35746e044414f0ecda8fad9b92c88bd26f21/content/renderer/service_worker/service_worker_type_util.h

Project Member

Comment 8 by bugdroid1@chromium.org, Aug 22

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4376512e75b378efdacc9ad1d7af6a8871f92ccf

commit 4376512e75b378efdacc9ad1d7af6a8871f92ccf
Author: Matt Falkenhagen <falken@chromium.org>
Date: Wed Aug 22 19:37:22 2018

service worker: Apply navigation throttles before the fetch event.

First some background (true both before and after this CL).
NavigationURLLoaderImpl roughly operates like this:
1. Ask interceptors whether they want to handle a request until
   one says yes (call MaybeCreateLoader).
2. Apply URLLoaderThrottles to the request, possibly rewriting it.
3. Have the interceptor handle the request (invoke its RequestHandler).

But before this CL, the interceptor could not observe the rewritten
request. Therefore, this CL:
1. Adds |resource_request| to RequestHandler signature.
2. Renames the request passed to MaybeCreateLoader to
   |tentative_resource_request| to make more clear that it is not
   necessarily the final request.

Of course, in the current code, interceptors are treating
|tentative_resource_request| as the final request, so this CL adds TODOs
to those interceptors.

The motivation of this CL is service worker interception. Before this
CL, service worker interception would dispatch the fetch event and get a
response before calling the LoaderCallback indicating whether the
service worker wants to handle the request. This was convenient for
network fallback in case the service worker fetch event did not provide
a response.

This CL changes service worker interception to operate after throttling.
This has several motivations:
1. The service worker fetch event may provide an inappropriate response
   for the request if it sees the pre-throttled request rather than the
   post-throttled request.
2. If throttling changes the URL, it would result in a simulated
   redirect, so the service worker interception would have been
   wasteful.
3. This also fixes the missing variation header for navigation preload
   requests, since the throttles will add the header before the fetch
   event.

Change-Id: If13ff956e46de91abb43f3919f869dc5e1eba3e6
Bug:  873061 ,  873575 , 876531, 876527
Reviewed-on: https://chromium-review.googlesource.com/1174204
Reviewed-by: Victor Costan <pwnall@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Carlos Knippschild <carlosk@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585201}
[modify] https://crrev.com/4376512e75b378efdacc9ad1d7af6a8871f92ccf/chrome/browser/offline_pages/offline_page_request_handler_unittest.cc
[modify] https://crrev.com/4376512e75b378efdacc9ad1d7af6a8871f92ccf/chrome/browser/offline_pages/offline_page_url_loader.cc
[modify] https://crrev.com/4376512e75b378efdacc9ad1d7af6a8871f92ccf/chrome/browser/offline_pages/offline_page_url_loader.h
[modify] https://crrev.com/4376512e75b378efdacc9ad1d7af6a8871f92ccf/chrome/browser/offline_pages/offline_page_url_loader_request_interceptor.cc
[modify] https://crrev.com/4376512e75b378efdacc9ad1d7af6a8871f92ccf/chrome/browser/offline_pages/offline_page_url_loader_request_interceptor.h
[modify] https://crrev.com/4376512e75b378efdacc9ad1d7af6a8871f92ccf/content/browser/appcache/appcache_request_handler.cc
[modify] https://crrev.com/4376512e75b378efdacc9ad1d7af6a8871f92ccf/content/browser/appcache/appcache_request_handler.h
[modify] https://crrev.com/4376512e75b378efdacc9ad1d7af6a8871f92ccf/content/browser/appcache/appcache_subresource_url_factory.cc
[modify] https://crrev.com/4376512e75b378efdacc9ad1d7af6a8871f92ccf/content/browser/appcache/appcache_url_loader_job.cc
[modify] https://crrev.com/4376512e75b378efdacc9ad1d7af6a8871f92ccf/content/browser/appcache/appcache_url_loader_job.h
[modify] https://crrev.com/4376512e75b378efdacc9ad1d7af6a8871f92ccf/content/browser/loader/navigation_loader_interceptor.h
[modify] https://crrev.com/4376512e75b378efdacc9ad1d7af6a8871f92ccf/content/browser/loader/navigation_url_loader_impl.cc
[modify] https://crrev.com/4376512e75b378efdacc9ad1d7af6a8871f92ccf/content/browser/loader/navigation_url_loader_impl_unittest.cc
[modify] https://crrev.com/4376512e75b378efdacc9ad1d7af6a8871f92ccf/content/browser/service_worker/service_worker_browsertest.cc
[modify] https://crrev.com/4376512e75b378efdacc9ad1d7af6a8871f92ccf/content/browser/service_worker/service_worker_controllee_request_handler.cc
[modify] https://crrev.com/4376512e75b378efdacc9ad1d7af6a8871f92ccf/content/browser/service_worker/service_worker_controllee_request_handler.h
[modify] https://crrev.com/4376512e75b378efdacc9ad1d7af6a8871f92ccf/content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc
[modify] https://crrev.com/4376512e75b378efdacc9ad1d7af6a8871f92ccf/content/browser/service_worker/service_worker_navigation_loader.cc
[modify] https://crrev.com/4376512e75b378efdacc9ad1d7af6a8871f92ccf/content/browser/service_worker/service_worker_navigation_loader.h
[modify] https://crrev.com/4376512e75b378efdacc9ad1d7af6a8871f92ccf/content/browser/service_worker/service_worker_navigation_loader_unittest.cc
[modify] https://crrev.com/4376512e75b378efdacc9ad1d7af6a8871f92ccf/content/browser/service_worker/service_worker_provider_host.cc
[modify] https://crrev.com/4376512e75b378efdacc9ad1d7af6a8871f92ccf/content/browser/service_worker/service_worker_request_handler.cc
[modify] https://crrev.com/4376512e75b378efdacc9ad1d7af6a8871f92ccf/content/browser/service_worker/service_worker_request_handler.h
[modify] https://crrev.com/4376512e75b378efdacc9ad1d7af6a8871f92ccf/content/browser/service_worker/service_worker_request_handler_unittest.cc
[modify] https://crrev.com/4376512e75b378efdacc9ad1d7af6a8871f92ccf/content/browser/shared_worker/shared_worker_script_loader.cc
[modify] https://crrev.com/4376512e75b378efdacc9ad1d7af6a8871f92ccf/content/browser/shared_worker/shared_worker_script_loader.h
[modify] https://crrev.com/4376512e75b378efdacc9ad1d7af6a8871f92ccf/content/browser/web_package/signed_exchange_request_handler.cc
[modify] https://crrev.com/4376512e75b378efdacc9ad1d7af6a8871f92ccf/content/browser/web_package/signed_exchange_request_handler.h
[modify] https://crrev.com/4376512e75b378efdacc9ad1d7af6a8871f92ccf/content/common/single_request_url_loader_factory.cc
[modify] https://crrev.com/4376512e75b378efdacc9ad1d7af6a8871f92ccf/content/common/single_request_url_loader_factory.h
[modify] https://crrev.com/4376512e75b378efdacc9ad1d7af6a8871f92ccf/content/public/browser/url_loader_request_interceptor.h
[modify] https://crrev.com/4376512e75b378efdacc9ad1d7af6a8871f92ccf/content/public/common/url_loader_throttle.h

Status: Fixed (was: Started)
Summary: Variation ids aren't added to navigation preload requests. (was: Field study ids aren't added to navigation preload requests.)
Fixed in 70.0.3531.0

Comment 10 by falken@google.com, Jan 18 (5 days ago)

Blockedon: 873575

Sign in to add a comment