service worker: Simplify main resource "handler" & "interceptor" (3/3)

Bug: 1138155
Change-Id: Iafb12ae9de19e68e637e7fb990bb8439ec4df848
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2602307Reviewed-by: Asami Doi <asamidoi@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842305}

content/browser/service_worker/service_worker_controllee_request_handler.cc

@@ -118,15 +118,17 @@ void ServiceWorkerControlleeRequestHandler::MaybeCreateLoader(
     BrowserContext* browser_context,
     NavigationLoaderInterceptor::LoaderCallback loader_callback,
     NavigationLoaderInterceptor::FallbackCallback fallback_callback) {
-  // InitializeContainerHost() will update the host. This is important to do
-  // before falling back to network below, so service worker APIs still work
-  // even if the service worker is bypassed for request interception.
-  if (!InitializeContainerHost(tentative_resource_request)) {
+  if (!container_host_) {
     // We can't do anything other than to fall back to network.
     std::move(loader_callback).Run({});
     return;
   }
 
+  // Update the host. This is important to do before falling back to network
+  // below, so service worker APIs still work even if the service worker is
+  // bypassed for request interception.
+  InitializeContainerHost(tentative_resource_request);
+
   // Fall back to network if we were instructed to bypass the service worker for
   // request interception, or if the context is gone so we have to bypass
   // anyway.
@@ -173,14 +175,8 @@ void ServiceWorkerControlleeRequestHandler::MaybeCreateLoader(
           weak_factory_.GetWeakPtr()));
 }
 
-bool ServiceWorkerControlleeRequestHandler::InitializeContainerHost(
+void ServiceWorkerControlleeRequestHandler::InitializeContainerHost(
     const network::ResourceRequest& tentative_resource_request) {
-  ClearJob();
-
-  if (!container_host_) {
-    return false;
-  }
-
   // Update the container host with this request, clearing old controller state
   // if this is a redirect.
   container_host_->SetControllerRegistration(nullptr,
@@ -192,7 +188,6 @@ bool ServiceWorkerControlleeRequestHandler::InitializeContainerHost(
                                   ? tentative_resource_request.trusted_params
                                         ->isolation_info.top_frame_origin()
                                   : base::nullopt);
-  return true;
 }
 
 void ServiceWorkerControlleeRequestHandler::ContinueWithRegistration(
@@ -520,16 +515,6 @@ void ServiceWorkerControlleeRequestHandler::OnUpdatedVersionStatusChanged(
       weak_factory_.GetWeakPtr(), std::move(registration), version));
 }
 
-void ServiceWorkerControlleeRequestHandler::ClearJob() {
-  // Invalidate weak pointers to cancel RegisterStatusChangeCallback().
-  // Otherwise we may end up calling ForwardToServiceWorer()
-  // or FallbackToNetwork() twice on the same |loader()|.
-  // TODO(bashi): Consider not to reuse this handler when restarting the
-  // request after S13nServiceWorker is shipped.
-  weak_factory_.InvalidateWeakPtrs();
-  loader_wrapper_.reset();
-}
-
 void ServiceWorkerControlleeRequestHandler::CompleteWithoutLoader() {
   std::move(loader_callback_).Run({});
 }

content/browser/service_worker/service_worker_controllee_request_handler.h

@@ -31,12 +31,15 @@ class ServiceWorkerContextCore;
 class ServiceWorkerRegistration;
 class ServiceWorkerVersion;
 
-// Handles main resource requests for service worker clients (documents and
-// shared workers).
+// Handles a main resource request for service worker clients (documents and
+// shared workers). This manages state for a single request and does not
+// live across redirects. ServiceWorkerMainResourceLoaderInterceptor creates
+// one instance of this class for each request/redirect.
 //
-// TODO(crbug.com/1138155): Merge into
-// ServiceWorkerMainResourceLoaderInterceptor now that they are on the same
-// thread.
+// This class associates the ServiceWorkerContainerHost undergoing navigation
+// with a controller service worker, after looking up the registration and
+// activating the service worker if needed.  Once ready, it creates
+// ServiceWorkerMainResourceLoader to perform the resource load.
 class CONTENT_EXPORT ServiceWorkerControlleeRequestHandler final {
  public:
   // If |skip_service_worker| is true, service workers are bypassed for
@@ -49,19 +52,14 @@ class CONTENT_EXPORT ServiceWorkerControlleeRequestHandler final {
       ServiceWorkerAccessedCallback service_worker_accessed_callback);
   ~ServiceWorkerControlleeRequestHandler();
 
-  // This could get called multiple times during the lifetime in redirect
-  // cases. (In fallback-to-network cases we basically forward the request
-  // to the request to the next request handler)
+  // This is called only once. On redirects, a new instance of this
+  // class is created.
   void MaybeCreateLoader(
       const network::ResourceRequest& tentative_request,
       BrowserContext* browser_context,
       NavigationLoaderInterceptor::LoaderCallback loader_callback,
       NavigationLoaderInterceptor::FallbackCallback fallback_callback);
 
-  // Does all initialization of |container_host_| for a request.
-  bool InitializeContainerHost(
-      const network::ResourceRequest& tentative_request);
-
   // Exposed for testing.
   ServiceWorkerMainResourceLoader* loader() {
     return loader_wrapper_ ? loader_wrapper_->get() : nullptr;
@@ -71,6 +69,10 @@ class CONTENT_EXPORT ServiceWorkerControlleeRequestHandler final {
   FRIEND_TEST_ALL_PREFIXES(ServiceWorkerControlleeRequestHandlerTest,
                            ActivateWaitingVersion);
 
+  // Does all initialization of |container_host_| for a request.
+  void InitializeContainerHost(
+      const network::ResourceRequest& tentative_request);
+
   void ContinueWithRegistration(
       blink::ServiceWorkerStatusCode status,
       scoped_refptr<ServiceWorkerRegistration> registration);
@@ -88,10 +90,6 @@ class CONTENT_EXPORT ServiceWorkerControlleeRequestHandler final {
       scoped_refptr<ServiceWorkerRegistration> registration,
       scoped_refptr<ServiceWorkerVersion> version);
 
-  // Sets |job_| to nullptr, and clears all extra response info associated with
-  // that job, except for timing information.
-  void ClearJob();
-
   void CompleteWithoutLoader();
 
   // Schedules a service worker update to occur shortly after the page and its

content/browser/service_worker/service_worker_main_resource_handle.h

@@ -9,7 +9,6 @@
 #include "base/memory/weak_ptr.h"
 #include "content/browser/service_worker/service_worker_accessed_callback.h"
 #include "content/browser/service_worker/service_worker_container_host.h"
-#include "content/browser/service_worker/service_worker_controllee_request_handler.h"
 #include "content/common/content_export.h"
 #include "services/metrics/public/cpp/ukm_source_id.h"
 #include "services/network/public/mojom/network_context.mojom.h"
@@ -115,15 +114,6 @@ class CONTENT_EXPORT ServiceWorkerMainResourceHandle {
     return parent_container_host_;
   }
 
-  void set_interceptor(
-      std::unique_ptr<ServiceWorkerControlleeRequestHandler> interceptor) {
-    interceptor_ = std::move(interceptor);
-  }
-
-  ServiceWorkerControlleeRequestHandler* interceptor() {
-    return interceptor_.get();
-  }
-
   const ServiceWorkerAccessedCallback& service_worker_accessed_callback() {
     return service_worker_accessed_callback_;
   }
@@ -144,8 +134,6 @@ class CONTENT_EXPORT ServiceWorkerMainResourceHandle {
   // Only used for workers with a blob URL.
   base::WeakPtr<ServiceWorkerContainerHost> parent_container_host_;
 
-  std::unique_ptr<ServiceWorkerControlleeRequestHandler> interceptor_;
-
   ServiceWorkerAccessedCallback service_worker_accessed_callback_;
 
   scoped_refptr<ServiceWorkerContextWrapper> context_wrapper_;

content/browser/service_worker/service_worker_main_resource_loader_interceptor.cc

@@ -16,7 +16,6 @@
 #include "content/browser/service_worker/service_worker_container_host.h"
 #include "content/browser/service_worker/service_worker_context_core.h"
 #include "content/browser/service_worker/service_worker_context_wrapper.h"
-#include "content/browser/service_worker/service_worker_controllee_request_handler.h"
 #include "content/browser/service_worker/service_worker_main_resource_handle.h"
 #include "content/browser/service_worker/service_worker_object_host.h"
 #include "content/public/common/content_client.h"
@@ -148,7 +147,7 @@ void ServiceWorkerMainResourceLoaderInterceptor::MaybeCreateLoader(
     // `container_info`.
     DCHECK(!handle_->container_host());
     base::WeakPtr<ServiceWorkerContainerHost> container_host;
-    bool inherit_container_host_only = false;
+    bool inherit_controller_only = false;
 
     if (request_destination_ == network::mojom::RequestDestination::kDocument ||
         request_destination_ == network::mojom::RequestDestination::kIframe) {
@@ -177,34 +176,29 @@ void ServiceWorkerMainResourceLoaderInterceptor::MaybeCreateLoader(
           tentative_resource_request.url.SchemeIsBlob()) {
         container_host->InheritControllerFrom(*parent_container_host,
                                               tentative_resource_request.url);
-        inherit_container_host_only = true;
+        inherit_controller_only = true;
       }
     }
     DCHECK(container_host);
     handle_->set_container_host(container_host);
 
-    // Also make the inner interceptor.
-    DCHECK(!handle_->interceptor());
-    handle_->set_interceptor(
-        std::make_unique<ServiceWorkerControlleeRequestHandler>(
-            context_core->AsWeakPtr(), container_host, request_destination_,
-            skip_service_worker_, handle_->service_worker_accessed_callback()));
-
-    // For the blob worker case, we only inherit the controller and do not
-    // let it intercept the requests. Blob URLs are not eligible to go through
-    // service worker interception. So just call the loader callback now.
-    // We don't use the interceptor but have to set it because we need
-    // ControllerServiceWorkerInfoPtr and ServiceWorkerObjectHost from the
-    // subresource loader params which is created by the interceptor.
-    if (inherit_container_host_only) {
+    // For the blob worker case, we only inherit the controller and do not let
+    // it intercept the main resource request. Blob URLs are not eligible to
+    // go through service worker interception. So just call the loader
+    // callback now.
+    if (inherit_controller_only) {
       std::move(loader_callback).Run(/*handler=*/{});
       return;
     }
   }
 
-  // Start the inner interceptor. It will invoke the loader callback
-  // or fallback callback.
-  handle_->interceptor()->MaybeCreateLoader(
+  // Create and start the handler for this request. It will invoke the loader
+  // callback or fallback callback.
+  request_handler_ = std::make_unique<ServiceWorkerControlleeRequestHandler>(
+      context_core->AsWeakPtr(), handle_->container_host(),
+      request_destination_, skip_service_worker_,
+      handle_->service_worker_accessed_callback());
+  request_handler_->MaybeCreateLoader(
       tentative_resource_request, browser_context, std::move(loader_callback),
       std::move(fallback_callback));
 }

content/browser/service_worker/service_worker_main_resource_loader_interceptor.h

@@ -13,6 +13,7 @@
 #include "content/browser/loader/navigation_loader_interceptor.h"
 #include "content/browser/loader/single_request_url_loader_factory.h"
 #include "content/browser/navigation_subresource_loader_params.h"
+#include "content/browser/service_worker/service_worker_controllee_request_handler.h"
 #include "content/common/content_export.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/service_worker_client_info.h"
@@ -126,6 +127,9 @@ class CONTENT_EXPORT ServiceWorkerMainResourceLoaderInterceptor final
   const int process_id_;
   const base::Optional<DedicatedOrSharedWorkerToken> worker_token_;
 
+  // Handles a single request. Set to a new instance on redirects.
+  std::unique_ptr<ServiceWorkerControlleeRequestHandler> request_handler_;
+
   DISALLOW_COPY_AND_ASSIGN(ServiceWorkerMainResourceLoaderInterceptor);
 };