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

This is a reland of f4b58578

The fix is to add an early return of null `handle_` in
MaybeCreateSubresourceLoaderParams() which is a weak ptr.
The comment for `handle_` explains that for workers it can
be destroyed before the interceptor, because
{Dedicated,Shared}WorkerHost can be destroyed during loading.
Also change WorkerScriptLoader to early return when the host is
already destroyed, to avoid unexpected behavior like going to
network instead of the service worker when it should.

Original change's description:
> service worker: Simplify main resource "handler" & "interceptor" (3/3)
>
> Bug: 1138155
> Change-Id: Iafb12ae9de19e68e637e7fb990bb8439ec4df848
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2602307
> Reviewed-by: Asami Doi <asamidoi@chromium.org>
> Commit-Queue: Matt Falkenhagen <falken@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#842305}

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

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));
 }
@@ -212,6 +206,9 @@ void ServiceWorkerMainResourceLoaderInterceptor::MaybeCreateLoader(
 base::Optional<SubresourceLoaderParams>
 ServiceWorkerMainResourceLoaderInterceptor::
     MaybeCreateSubresourceLoaderParams() {
+  if (!handle_) {
+    return base::nullopt;
+  }
   base::WeakPtr<ServiceWorkerContainerHost> container_host =
       handle_->container_host();
 

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);
 };
 

content/browser/worker_host/worker_script_loader.cc

@@ -118,6 +118,12 @@ void WorkerScriptLoader::MaybeStartLoader(
   DCHECK(!completed_);
   DCHECK(interceptor);
 
+  if (!service_worker_handle_) {
+    // The DedicatedWorkerHost or SharedWorkerHost is already destroyed.
+    Abort();
+    return;
+  }
+
   // Create SubresourceLoaderParams for intercepting subresource requests and
   // populating the "controller" field in ServiceWorkerContainer. This can be
   // null if the interceptor is not interested in this request.