New issue
Advanced search Search tips

Issue 876527 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Task



Sign in to add a comment

Make offline page interceptor aware of throttles.

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

Issue description

Related to  issue 873575  and https://chromium-review.googlesource.com/c/chromium/src/+/1174204.

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).

The offline page interceptor is currently using the request from step (1) and not using the request from step (3). It's probably correct to use the request from step (3) instead.
 

Comment 1 Deleted

Project Member

Comment 2 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

Owner: jianli@chromium.org
Status: Assigned (was: Untriaged)
Jian, could you please take a look to make sure this is right change? Thanks!

Sign in to add a comment