ServiceWorker: Abort service worker interception when ServiceWorkerNavigationHandle is destroyed

ServiceWorkerNavigationHandle can be destroyed while service worker interception
is happining for workers. To avoid crashes, this CL adds guards in
ServiceWorkerNavigationLoaderInterceptor and WorkerScriptLoader.

For navigation, it's guaranteed that the handle outlives the interceptor, so
it's safe. See https://bugs.chromium.org/p/chromium/issues/detail?id=988247#c3

Bug: 988247
Change-Id: Ib72005c4832895eb82f8e28160d5b879ad19794c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1722933
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#682085}

content/browser/loader/navigation_url_loader_impl.cc

@@ -498,8 +498,8 @@ class NavigationURLLoaderImpl::URLLoaderRequestController
     if (service_worker_navigation_handle_) {
       std::unique_ptr<NavigationLoaderInterceptor> service_worker_interceptor =
           ServiceWorkerRequestHandler::CreateForNavigationUI(
-              resource_request_->url, service_worker_navigation_handle_,
-              *request_info);
+              resource_request_->url,
+              service_worker_navigation_handle_->AsWeakPtr(), *request_info);
       // The interceptor may not be created in certain cases (e.g., the origin
       // is not secure).
       if (service_worker_interceptor)

content/browser/service_worker/service_worker_navigation_loader_interceptor.cc

@@ -163,10 +163,11 @@ void MaybeCreateLoaderOnIO(
 ServiceWorkerNavigationLoaderInterceptor::
     ServiceWorkerNavigationLoaderInterceptor(
         const ServiceWorkerNavigationLoaderInterceptorParams& params,
-        ServiceWorkerNavigationHandle* handle)
-    : handle_(handle), params_(params) {
+        base::WeakPtr<ServiceWorkerNavigationHandle> handle)
+    : handle_(std::move(handle)), params_(params) {
   DCHECK(NavigationURLLoaderImpl::IsNavigationLoaderOnUIEnabled());
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DCHECK(handle_);
 }
 
 ServiceWorkerNavigationLoaderInterceptor::
@@ -182,6 +183,7 @@ void ServiceWorkerNavigationLoaderInterceptor::MaybeCreateLoader(
     FallbackCallback fallback_callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(!resource_context);
+  DCHECK(handle_);
 
   bool initialize_provider_only = false;
   if (!handle_->context_wrapper()->HasRegistrationForOrigin(
@@ -217,6 +219,16 @@ void ServiceWorkerNavigationLoaderInterceptor::LoaderCallbackWrapper(
     SingleRequestURLLoaderFactory::RequestHandler handler_on_io) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
+  // For worker main script requests, |handle_| can be destroyed during
+  // interception. The initiator of this interceptor (i.e., WorkerScriptLoader)
+  // will handle the case.
+  // For navigation requests, this case should not happen because it's
+  // guaranteed that this interceptor is destroyed before |handle_|.
+  if (!handle_) {
+    std::move(loader_callback).Run({});
+    return;
+  }
+
   // |provider_info| is non-null if this is the first request before redirects,
   // which makes the provider host for this navigation.
   if (provider_info)

content/browser/service_worker/service_worker_navigation_loader_interceptor.h

@@ -50,7 +50,7 @@ class ServiceWorkerNavigationLoaderInterceptor final
  public:
   ServiceWorkerNavigationLoaderInterceptor(
       const ServiceWorkerNavigationLoaderInterceptorParams& params,
-      ServiceWorkerNavigationHandle* handle);
+      base::WeakPtr<ServiceWorkerNavigationHandle> handle);
   ~ServiceWorkerNavigationLoaderInterceptor() override;
 
   // NavigationLoaderInterceptor overrides:
@@ -88,8 +88,14 @@ class ServiceWorkerNavigationLoaderInterceptor final
       network::mojom::URLLoaderRequest request,
       network::mojom::URLLoaderClientPtr client);
 
-  // |handle_| owns |this|.
-  ServiceWorkerNavigationHandle* const handle_;
+  // For navigations, |handle_| outlives |this|. It's owned by
+  // NavigationHandleImpl which outlives NavigationURLLoaderImpl which owns
+  // |this|.
+  // For workers, |handle_| may be destroyed during interception. It's owned by
+  // DedicatedWorkerHost or SharedWorkerHost which may be destroyed before
+  // WorkerScriptLoader which owns |this|.
+  // TODO(falken): Arrange things so |handle_| outlives |this| for workers too.
+  const base::WeakPtr<ServiceWorkerNavigationHandle> handle_;
 
   const ServiceWorkerNavigationLoaderInterceptorParams params_;
 

content/browser/service_worker/service_worker_request_handler.cc

@@ -41,7 +41,7 @@ bool SchemeMaySupportRedirectingToHTTPS(const GURL& url) {
 std::unique_ptr<NavigationLoaderInterceptor>
 ServiceWorkerRequestHandler::CreateForNavigationUI(
     const GURL& url,
-    ServiceWorkerNavigationHandle* navigation_handle,
+    base::WeakPtr<ServiceWorkerNavigationHandle> navigation_handle,
     const NavigationRequestInfo& request_info) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
@@ -61,7 +61,7 @@ ServiceWorkerRequestHandler::CreateForNavigationUI(
   params.frame_tree_node_id = request_info.frame_tree_node_id;
 
   return std::make_unique<ServiceWorkerNavigationLoaderInterceptor>(
-      params, navigation_handle);
+      params, std::move(navigation_handle));
 }
 
 // static
@@ -109,7 +109,7 @@ std::unique_ptr<NavigationLoaderInterceptor>
 ServiceWorkerRequestHandler::CreateForWorkerUI(
     const network::ResourceRequest& resource_request,
     int process_id,
-    ServiceWorkerNavigationHandle* navigation_handle) {
+    base::WeakPtr<ServiceWorkerNavigationHandle> navigation_handle) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   auto resource_type =
@@ -131,7 +131,7 @@ ServiceWorkerRequestHandler::CreateForWorkerUI(
   params.process_id = process_id;
 
   return std::make_unique<ServiceWorkerNavigationLoaderInterceptor>(
-      params, navigation_handle);
+      params, std::move(navigation_handle));
 }
 
 // static

content/browser/service_worker/service_worker_request_handler.h

@@ -35,7 +35,7 @@ class CONTENT_EXPORT ServiceWorkerRequestHandler {
   // navigation cannot use service workers. Called on the UI thread.
   static std::unique_ptr<NavigationLoaderInterceptor> CreateForNavigationUI(
       const GURL& url,
-      ServiceWorkerNavigationHandle* navigation_handle,
+      base::WeakPtr<ServiceWorkerNavigationHandle> navigation_handle,
       const NavigationRequestInfo& request_info);
 
   // Returns a loader interceptor for a navigation. May return nullptr if the
@@ -52,7 +52,7 @@ class CONTENT_EXPORT ServiceWorkerRequestHandler {
   static std::unique_ptr<NavigationLoaderInterceptor> CreateForWorkerUI(
       const network::ResourceRequest& resource_request,
       int process_id,
-      ServiceWorkerNavigationHandle* navigation_handle);
+      base::WeakPtr<ServiceWorkerNavigationHandle> navigation_handle);
 
   // Returns a loader interceptor for a dedicated worker or shared worker. May
   // return nullptr if the worker cannot use service workers. Called on the UI

content/browser/worker_host/worker_script_loader.cc

@@ -59,7 +59,7 @@ WorkerScriptLoader::WorkerScriptLoader(
       return;
     }
     service_worker_interceptor = ServiceWorkerRequestHandler::CreateForWorkerUI(
-        resource_request_, process_id, service_worker_handle_.get());
+        resource_request_, process_id, service_worker_handle_);
   } else {
     if (!service_worker_handle_core_) {
       // The DedicatedWorkerHost or SharedWorkerHost is already destroyed.
@@ -102,6 +102,18 @@ void WorkerScriptLoader::Start() {
   DCHECK_CURRENTLY_ON(WorkerScriptFetchInitiator::GetLoaderThreadID());
   DCHECK(!completed_);
 
+  // The DedicatedWorkerHost or SharedWorkerHost is already destroyed.
+  if (NavigationURLLoaderImpl::IsNavigationLoaderOnUIEnabled() &&
+      !service_worker_handle_) {
+    Abort();
+    return;
+  }
+  if (!NavigationURLLoaderImpl::IsNavigationLoaderOnUIEnabled() &&
+      !service_worker_handle_core_) {
+    Abort();
+    return;
+  }
+
   BrowserContext* browser_context = nullptr;
   if (NavigationURLLoaderImpl::IsNavigationLoaderOnUIEnabled()) {
     browser_context = browser_context_getter_.Run();

third_party/blink/web_tests/TestExpectations

@@ -6423,7 +6423,6 @@ crbug.com/988232 virtual/controls-refresh/fast/forms/controls-new-ui/password/pa
 crbug.com/988232 virtual/controls-refresh/fast/forms/controls-new-ui/password/password-with-reveal-button.html [ Pass Failure ]
 crbug.com/988246 external/wpt/cookie-store/serviceworker_cookieStore_subscriptions_mismatch.tentative.https.html [ Skip ]
 crbug.com/988248 media/video-persistence.html [ Pass Failure ]
-crbug.com/988250 virtual/omt-worker-fetch/http/tests/workers/worker-invalid-context.html [ Crash Pass Failure Timeout ]
 crbug.com/988252 [ Linux Debug ] virtual/threaded/transitions/composited-with-hit-testing.html [ Pass Failure ]
 crbug.com/959129 virtual/threaded/http/tests/devtools/tracing/timeline-script-parse.js [ Pass Failure ]
 crbug.com/959129 http/tests/devtools/tracing/timeline-script-parse.js [ Pass Failure ]