Skip other interceptors after SignedExchangeRequestHandler started handling the request

Currently when both service-worker-servicification and network-service are not
enabled, the internally redirected request doesn't go to the service worker.
But when service-worker-servicification or network-service is enabled, the
internally redirected request goes to the service worker.

We may support service worker integration, like FetchEvent's preloadResponse in
future. But we don't have plan to support service worker integration without
service-worker-servicification.

To make this behavior consistent, this CL makes
NavigationURLLoaderImpl::URLLoaderRequestController skip sending the redirected
request of signed exchange to the service worker.

Bug: 894755
Change-Id: I6f22819655169b4d3c30de0aa269911111c45586
Reviewed-on: https://chromium-review.googlesource.com/c/1278433Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599815}

content/browser/appcache/appcache_request_handler.cc

@@ -573,7 +573,8 @@ bool AppCacheRequestHandler::MaybeCreateLoaderForResponse(
     const network::ResourceResponseHead& response,
     network::mojom::URLLoaderPtr* loader,
     network::mojom::URLLoaderClientRequest* client_request,
-    ThrottlingURLLoader* url_loader) {
+    ThrottlingURLLoader* url_loader,
+    bool* skip_other_interceptors) {
   // The sync interface of this method is inherited from the
   // NavigationLoaderInterceptor class. The LoaderCallback created here is
   // invoked synchronously in fallback cases, and only when there really is

content/browser/appcache/appcache_request_handler.h

@@ -84,7 +84,8 @@ class CONTENT_EXPORT AppCacheRequestHandler
       const network::ResourceResponseHead& response,
       network::mojom::URLLoaderPtr* loader,
       network::mojom::URLLoaderClientRequest* client_request,
-      ThrottlingURLLoader* url_loader) override;
+      ThrottlingURLLoader* url_loader,
+      bool* skip_other_interceptors) override;
   base::Optional<SubresourceLoaderParams> MaybeCreateSubresourceLoaderParams()
       override;
 

content/browser/loader/navigation_loader_interceptor.cc

@@ -17,7 +17,8 @@ bool NavigationLoaderInterceptor::MaybeCreateLoaderForResponse(
     const network::ResourceResponseHead& response,
     network::mojom::URLLoaderPtr* loader,
     network::mojom::URLLoaderClientRequest* client_request,
-    ThrottlingURLLoader* url_loader) {
+    ThrottlingURLLoader* url_loader,
+    bool* skip_other_interceptors) {
   return false;
 }
 

content/browser/loader/navigation_loader_interceptor.h

@@ -93,11 +93,17 @@ class CONTENT_EXPORT NavigationLoaderInterceptor {
   // intercept the inflight loading if necessary.  Note that the |url_loader|
   // will be reset after this method is called, which will also drop the
   // URLLoader held by |url_loader_| if it is not unbound yet.
+  // |skip_other_interceptors| is set to true when this interceptor will
+  // exclusively handle the navigation even after redirections. TODO(horo): This
+  // flag was introduced to skip service worker after signed exchange redirect.
+  // Remove this flag when we support service worker and signed exchange
+  // integration. See crbug.com/894755#c1. Nullptr is not allowed.
   virtual bool MaybeCreateLoaderForResponse(
       const network::ResourceResponseHead& response,
       network::mojom::URLLoaderPtr* loader,
       network::mojom::URLLoaderClientRequest* client_request,
-      ThrottlingURLLoader* url_loader);
+      ThrottlingURLLoader* url_loader,
+      bool* skip_other_interceptors);
 };
 
 }  // namespace content

content/browser/loader/navigation_url_loader_impl.cc

@@ -1391,16 +1391,24 @@ class NavigationURLLoaderImpl::URLLoaderRequestController
     if (!default_loader_used_)
       return false;
 
-    for (auto& interceptor : interceptors_) {
+    for (size_t i = 0u; i < interceptors_.size(); ++i) {
+      NavigationLoaderInterceptor* interceptor = interceptors_[i].get();
       network::mojom::URLLoaderClientRequest response_client_request;
+      bool skip_other_interceptors = false;
       if (interceptor->MaybeCreateLoaderForResponse(
               response, &response_url_loader_, &response_client_request,
-              url_loader_.get())) {
+              url_loader_.get(), &skip_other_interceptors)) {
         if (response_loader_binding_.is_bound())
           response_loader_binding_.Close();
         response_loader_binding_.Bind(std::move(response_client_request));
         default_loader_used_ = false;
         url_loader_.reset();
+        if (skip_other_interceptors) {
+          std::vector<std::unique_ptr<NavigationLoaderInterceptor>>
+              new_interceptors;
+          new_interceptors.push_back(std::move(interceptors_[i]));
+          new_interceptors.swap(interceptors_);
+        }
         return true;
       }
     }

content/browser/loader/navigation_url_loader_impl_unittest.cc

@@ -101,7 +101,8 @@ class TestNavigationLoaderInterceptor : public NavigationLoaderInterceptor {
       const network::ResourceResponseHead& response,
       network::mojom::URLLoaderPtr* loader,
       network::mojom::URLLoaderClientRequest* client_request,
-      ThrottlingURLLoader* url_loader) override {
+      ThrottlingURLLoader* url_loader,
+      bool* skip_other_interceptors) override {
     return false;
   }
 

content/browser/service_worker/embedded_worker_instance.cc

@@ -7,9 +7,12 @@
 #include <utility>
 
 #include "base/bind_helpers.h"
+#include "base/callback.h"
 #include "base/feature_list.h"
 #include "base/macros.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/no_destructor.h"
+#include "base/optional.h"
 #include "base/task/post_task.h"
 #include "base/trace_event/trace_event.h"
 #include "content/browser/bad_message.h"
@@ -44,6 +47,14 @@ namespace content {
 
 namespace {
 
+base::Optional<EmbeddedWorkerInstance::CreateNetworkFactoryCallback>&
+GetNetworkFactoryCallbackForTest() {
+  static base::NoDestructor<
+      base::Optional<EmbeddedWorkerInstance::CreateNetworkFactoryCallback>>
+      callback;
+  return *callback;
+}
+
 // When a service worker version's failure count exceeds
 // |kMaxSameProcessFailureCount|, the embedded worker is forced to start in a
 // new process.
@@ -95,7 +106,16 @@ std::unique_ptr<URLLoaderFactoryBundleInfo> CreateFactoryBundle(
     bool use_non_network_factories) {
   auto factory_bundle = std::make_unique<URLLoaderFactoryBundleInfo>();
   network::mojom::URLLoaderFactoryPtrInfo default_factory_info;
-  rph->CreateURLLoaderFactory(origin, mojo::MakeRequest(&default_factory_info));
+  if (!GetNetworkFactoryCallbackForTest()) {
+    rph->CreateURLLoaderFactory(origin,
+                                mojo::MakeRequest(&default_factory_info));
+  } else {
+    network::mojom::URLLoaderFactoryPtr original_factory;
+    rph->CreateURLLoaderFactory(origin, mojo::MakeRequest(&original_factory));
+    GetNetworkFactoryCallbackForTest()->Run(
+        mojo::MakeRequest(&default_factory_info), rph->GetID(),
+        original_factory.PassInterface());
+  }
   factory_bundle->default_factory_info() = std::move(default_factory_info);
 
   if (use_non_network_factories) {
@@ -1069,4 +1089,10 @@ std::string EmbeddedWorkerInstance::StartingPhaseToString(StartingPhase phase) {
   return std::string();
 }
 
+// static
+void EmbeddedWorkerInstance::SetNetworkFactoryForTesting(
+    const CreateNetworkFactoryCallback& create_network_factory_callback) {
+  GetNetworkFactoryCallbackForTest() = create_network_factory_callback;
+}
+
 }  // namespace content

content/browser/service_worker/embedded_worker_instance.h

@@ -200,6 +200,16 @@ class CONTENT_EXPORT EmbeddedWorkerInstance
   static std::string StatusToString(EmbeddedWorkerStatus status);
   static std::string StartingPhaseToString(StartingPhase phase);
 
+  using CreateNetworkFactoryCallback = base::RepeatingCallback<void(
+      network::mojom::URLLoaderFactoryRequest request,
+      int process_id,
+      network::mojom::URLLoaderFactoryPtrInfo original_factory)>;
+  // Allows overriding the URLLoaderFactory creation for loading subresources
+  // from service workers (i.e., fetch()) and for loading non-installed service
+  // worker scripts.
+  static void SetNetworkFactoryForTesting(
+      const CreateNetworkFactoryCallback& url_loader_factory_callback);
+
   // Forces this instance into STOPPED status and releases any state about the
   // running worker. Called when connection with the renderer died or the
   // renderer is unresponsive.  Essentially, it throws away any information

content/browser/shared_worker/shared_worker_script_loader.cc

@@ -253,9 +253,13 @@ bool SharedWorkerScriptLoader::MaybeCreateLoaderForResponse(
     ThrottlingURLLoader* url_loader) {
   DCHECK(default_loader_used_);
   for (auto& interceptor : interceptors_) {
-    if (interceptor->MaybeCreateLoaderForResponse(response, response_url_loader,
-                                                  response_client_request,
-                                                  url_loader)) {
+    bool skip_other_interceptors = false;
+    if (interceptor->MaybeCreateLoaderForResponse(
+            response, response_url_loader, response_client_request, url_loader,
+            &skip_other_interceptors)) {
+      // Both ServiceWorkerRequestHandler and AppCacheRequestHandler don't set
+      // skip_other_interceptors.
+      DCHECK(!skip_other_interceptors);
       subresource_loader_params_ =
           interceptor->MaybeCreateSubresourceLoaderParams();
       return true;

content/browser/web_package/signed_exchange_request_handler.cc

@@ -83,7 +83,8 @@ bool SignedExchangeRequestHandler::MaybeCreateLoaderForResponse(
     const network::ResourceResponseHead& response,
     network::mojom::URLLoaderPtr* loader,
     network::mojom::URLLoaderClientRequest* client_request,
-    ThrottlingURLLoader* url_loader) {
+    ThrottlingURLLoader* url_loader,
+    bool* skip_other_interceptors) {
   DCHECK(!signed_exchange_loader_);
   if (!signed_exchange_utils::ShouldHandleAsSignedHTTPExchange(
           request_initiator_.GetURL(), response)) {
@@ -109,6 +110,8 @@ bool SignedExchangeRequestHandler::MaybeCreateLoaderForResponse(
       url_loader_factory_, url_loader_throttles_getter_,
       base::BindRepeating([](int id) { return id; }, frame_tree_node_id_),
       metric_recorder_);
+
+  *skip_other_interceptors = true;
   return true;
 }
 

content/browser/web_package/signed_exchange_request_handler.h

@@ -53,7 +53,8 @@ class SignedExchangeRequestHandler final : public NavigationLoaderInterceptor {
       const network::ResourceResponseHead& response,
       network::mojom::URLLoaderPtr* loader,
       network::mojom::URLLoaderClientRequest* client_request,
-      ThrottlingURLLoader* url_loader) override;
+      ThrottlingURLLoader* url_loader,
+      bool* skip_other_interceptors) override;
 
  private:
   void StartResponse(const network::ResourceRequest& resource_request,

content/public/test/url_loader_interceptor.cc

@@ -18,6 +18,7 @@
 #include "content/browser/loader/navigation_url_loader_impl.h"
 #include "content/browser/loader/resource_message_filter.h"
 #include "content/browser/loader/url_loader_factory_impl.h"
+#include "content/browser/service_worker/embedded_worker_instance.h"
 #include "content/browser/storage_partition_impl.h"
 #include "content/browser/url_loader_factory_getter.h"
 #include "content/public/browser/browser_task_traits.h"
@@ -227,6 +228,12 @@ URLLoaderInterceptor::URLLoaderInterceptor(const InterceptCallback& callback)
     RenderFrameHostImpl::SetNetworkFactoryForTesting(base::BindRepeating(
         &URLLoaderInterceptor::CreateURLLoaderFactoryForSubresources,
         base::Unretained(this)));
+    // Note: This URLLoaderFactory creation callback will be used not only for
+    // subresource loading from service workers (i.e., fetch()), but also for
+    // loading non-installed service worker scripts.
+    EmbeddedWorkerInstance::SetNetworkFactoryForTesting(base::BindRepeating(
+        &URLLoaderInterceptor::CreateURLLoaderFactoryForSubresources,
+        base::Unretained(this)));
   }
 
   StoragePartitionImpl::
@@ -253,6 +260,8 @@ URLLoaderInterceptor::~URLLoaderInterceptor() {
   if (base::FeatureList::IsEnabled(network::features::kNetworkService)) {
     RenderFrameHostImpl::SetNetworkFactoryForTesting(
         RenderFrameHostImpl::CreateNetworkFactoryCallback());
+    EmbeddedWorkerInstance::SetNetworkFactoryForTesting(
+        RenderFrameHostImpl::CreateNetworkFactoryCallback());
   }
 
   StoragePartitionImpl::

content/public/test/url_loader_interceptor.h

@@ -22,13 +22,18 @@ class URLLoaderFactoryGetter;
 // Helper class to intercept URLLoaderFactory calls for tests.
 // This intercepts:
 //   -frame requests (which start from the browser, with PlzNavigate)
-//   -subresource requests
+//   -subresource requests from pages and dedicad workers and shared workers.
 //     -at ResourceMessageFilter for non network-service code path
 //     -by sending renderer an intermediate URLLoaderFactory for network-service
-//      codepath, as that normally routes directly to the network process
+//      code path, as that normally routes directly to the network process
+//   -subresource requests from service workers and requests of non-installed
+//    service worker scripts
+//     -at ResourceMessageFilter for non network-service code path
+//     -at EmbeddedWorkerInstance for network-service code path.
+//   -requests by the browser
+//
 //   -http(s)://mock.failed.request/foo URLs internally, copying the behavior
 //    of net::URLRequestFailedJob
-//   -requests by the browser
 //
 // Prefer not to use this class. In order of ease of use & simplicity:
 //  -if you need to serve static data, use net::test::EmbeddedTestServer and

content/test/data/sxg/publisher-service-worker.html

+<script>
+async function register(script, scope) {
+  const registration = await navigator.serviceWorker.register(
+      script, {scope: scope})
+  await new Promise(resolve =>
+    registration.installing.addEventListener('statechange', resolve));
+}
+async function initialize() {
+  await register('publisher-service-worker.js', './');
+  document.title = "Done";
+}
+initialize();
+</script>

content/test/data/sxg/publisher-service-worker.js

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+self.addEventListener('fetch', (event) => {
+  event.respondWith(new Response(
+    '<title>Generated</title>',
+    {headers:[['content-type', 'text/html']]}));
+});

content/test/data/sxg/publisher-service-worker.js.mock-http-headers

+HTTP/1.1 200 OK
+Content-Type: application/javascript