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

This reverts commit f4b58578.

Reason for revert: caused layout test CRASH:
https://ci.chromium.org/ui/p/chromium/builders/ci/Linux%20Tests%20(dbg)(1)/94262/overview

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}

TBR=falken@chromium.org,mstensho@chromium.org,chromium-scoped@luci-project-accounts.iam.gserviceaccount.com,asamidoi@chromium.org

Change-Id: I5b4756817dd149a9f721c5aaeb2da54aadaf4308
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1138155
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2623572Reviewed-by: Yoichi Osato <yoichio@chromium.org>
Commit-Queue: Yoichi Osato <yoichio@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842375}

content/browser/service_worker/service_worker_controllee_request_handler.cc

@@ -118,17 +118,15 @@ void ServiceWorkerControlleeRequestHandler::MaybeCreateLoader(
     BrowserContext* browser_context,
     NavigationLoaderInterceptor::LoaderCallback loader_callback,
     NavigationLoaderInterceptor::FallbackCallback fallback_callback) {
-  if (!container_host_) {
+  // 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)) {
     // 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.
@@ -175,8 +173,14 @@ void ServiceWorkerControlleeRequestHandler::MaybeCreateLoader(
           weak_factory_.GetWeakPtr()));
 }
 
-void ServiceWorkerControlleeRequestHandler::InitializeContainerHost(
+bool 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,
@@ -188,6 +192,7 @@ void ServiceWorkerControlleeRequestHandler::InitializeContainerHost(
                                   ? tentative_resource_request.trusted_params
                                         ->isolation_info.top_frame_origin()
                                   : base::nullopt);
+  return true;
 }
 
 void ServiceWorkerControlleeRequestHandler::ContinueWithRegistration(
@@ -515,6 +520,16 @@ 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,15 +31,12 @@ class ServiceWorkerContextCore;
 class ServiceWorkerRegistration;
 class ServiceWorkerVersion;
 
-// 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.
+// Handles main resource requests for service worker clients (documents and
+// shared workers).
 //
-// 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.
+// TODO(crbug.com/1138155): Merge into
+// ServiceWorkerMainResourceLoaderInterceptor now that they are on the same
+// thread.
 class CONTENT_EXPORT ServiceWorkerControlleeRequestHandler final {
  public:
   // If |skip_service_worker| is true, service workers are bypassed for
@@ -52,14 +49,19 @@ class CONTENT_EXPORT ServiceWorkerControlleeRequestHandler final {
       ServiceWorkerAccessedCallback service_worker_accessed_callback);
   ~ServiceWorkerControlleeRequestHandler();
 
-  // This is called only once. On redirects, a new instance of this
-  // class is created.
+  // 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)
   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;
@@ -69,10 +71,6 @@ 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);
@@ -90,6 +88,10 @@ 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,6 +9,7 @@
 #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"
@@ -114,6 +115,15 @@ 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_;
   }
@@ -134,6 +144,8 @@ 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,6 +16,7 @@
 #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"
@@ -147,7 +148,7 @@ void ServiceWorkerMainResourceLoaderInterceptor::MaybeCreateLoader(
     // `container_info`.
     DCHECK(!handle_->container_host());
     base::WeakPtr<ServiceWorkerContainerHost> container_host;
-    bool inherit_controller_only = false;
+    bool inherit_container_host_only = false;
 
     if (request_destination_ == network::mojom::RequestDestination::kDocument ||
         request_destination_ == network::mojom::RequestDestination::kIframe) {
@@ -176,29 +177,34 @@ void ServiceWorkerMainResourceLoaderInterceptor::MaybeCreateLoader(
           tentative_resource_request.url.SchemeIsBlob()) {
         container_host->InheritControllerFrom(*parent_container_host,
                                               tentative_resource_request.url);
-        inherit_controller_only = true;
+        inherit_container_host_only = true;
       }
     }
     DCHECK(container_host);
     handle_->set_container_host(container_host);
 
-    // 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) {
+    // 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) {
       std::move(loader_callback).Run(/*handler=*/{});
       return;
     }
   }
 
-  // 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(
+  // Start the inner interceptor. It will invoke the loader callback
+  // or fallback callback.
+  handle_->interceptor()->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,7 +13,6 @@
 #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"
@@ -127,9 +126,6 @@ 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);
 };