service worker: Reset fallback callback when navigation goes away

crrev.com/c/1717885 was a workaround to fix use-after-free of
URLLoaderRequestController. This CL reverts the workaround.

This CL makes ServiceWorkerNavigationLoader reset
fallback callback when the controller is destructed. This
way we won't touch a stale controller.

Bug: 989404
Change-Id: Icf24e22b6fe5a3a6f29ccca95b21eb4233dab3b9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1730409
Commit-Queue: Kenichi Ishibashi <bashi@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#683407}

content/browser/loader/navigation_loader_interceptor.h

@@ -66,6 +66,9 @@ class CONTENT_EXPORT NavigationLoaderInterceptor {
   // indicates whether to discard the subresource loader params previously
   // returned by MaybeCreateSubresourceLoaderParams().
   //
+  // |callback| and |fallback_callback| must not be invoked after the
+  // destruction of this interceptor.
+  //
   // |browser_context| will only be non-null when this interceptor is running on
   // the UI thread, and |resource_context| will only be non-null when running on
   // the IO thread.

content/browser/loader/navigation_url_loader_impl.cc

@@ -698,18 +698,13 @@ class NavigationURLLoaderImpl::URLLoaderRequestController
         browser_context_to_use = browser_context_;
       else
         resource_context_to_use = resource_context_;
-      // TODO(crbug.com/984460): Don't use WeakPtrs for callbacks. These
-      // WeakPtrs are to work around a case where ServiceWorkerNavigationLoader
-      // outlives the interceptor. We may want to reset fallback callback in
-      // ServiceWorkerNavigationLoader when the URLLoaderRequestController is
-      // going away.
       next_interceptor->MaybeCreateLoader(
           *resource_request_, browser_context_to_use, resource_context_to_use,
           base::BindOnce(&URLLoaderRequestController::MaybeStartLoader,
-                         weak_factory_.GetWeakPtr(), next_interceptor),
+                         base::Unretained(this), next_interceptor),
           base::BindOnce(
               &URLLoaderRequestController::FallbackToNonInterceptedRequest,
-              weak_factory_.GetWeakPtr()));
+              base::Unretained(this)));
       return;
     }
 

content/browser/service_worker/service_worker_navigation_loader.cc

@@ -102,6 +102,9 @@ ServiceWorkerNavigationLoader::~ServiceWorkerNavigationLoader() {
 
 void ServiceWorkerNavigationLoader::DetachedFromRequest() {
   is_detached_ = true;
+  // Clear |fallback_callback_| since it's no longer safe to invoke it because
+  // the bound object has been destroyed.
+  fallback_callback_.Reset();
   DeleteIfNeeded();
 }
 
@@ -283,8 +286,10 @@ void ServiceWorkerNavigationLoader::DidDispatchFetchEvent(
     // page, but the risk is that the user will be stuck if there's a persistent
     // failure.
     provider_host_->NotifyControllerLost();
-    std::move(fallback_callback_)
-        .Run(true /* reset_subresource_loader_params */);
+    if (fallback_callback_) {
+      std::move(fallback_callback_)
+          .Run(true /* reset_subresource_loader_params */);
+    }
     return;
   }
 
@@ -294,8 +299,10 @@ void ServiceWorkerNavigationLoader::DidDispatchFetchEvent(
     RecordTimingMetrics(false);
     // TODO(falken): Propagate the timing info to the renderer somehow, or else
     // Navigation Timing etc APIs won't know about service worker.
-    std::move(fallback_callback_)
-        .Run(false /* reset_subresource_loader_params */);
+    if (fallback_callback_) {
+      std::move(fallback_callback_)
+          .Run(false /* reset_subresource_loader_params */);
+    }
     return;
   }