Fix leak when ServiceWorkerNavigationLoader forwards request to SW

Previously SWNavigationLoader::Cancel() didn't clear |binding_| and it didn't
delete itself. This CL is to close the binding explicitly and fixes the leak.

Bug: 859415
Change-Id: I99a79f2f0cc21802bdd36d507c4c8692955cc3d9
Reviewed-on: https://chromium-review.googlesource.com/1122065
Commit-Queue: Makoto Shimazu <shimazu@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572153}

content/browser/service_worker/service_worker_navigation_loader.cc

@@ -106,6 +106,7 @@ void ServiceWorkerNavigationLoader::FallbackToNetwork() {
       "ServiceWorker", "ServiceWorkerNavigationLoader::FallbackToNetwork", this,
       TRACE_EVENT_FLAG_FLOW_IN | TRACE_EVENT_FLAG_FLOW_OUT);
   response_type_ = ResponseType::FALLBACK_TO_NETWORK;
+  status_ = Status::kCompleted;
   // This could be called multiple times in some cases because we simply
   // call this synchronously here and don't wait for a separate async
   // StartRequest cue like what URLRequestJob case does.
@@ -126,18 +127,6 @@ bool ServiceWorkerNavigationLoader::ShouldFallbackToNetwork() {
   return response_type_ == ResponseType::FALLBACK_TO_NETWORK;
 }
 
-void ServiceWorkerNavigationLoader::Cancel() {
-  status_ = Status::kCancelled;
-  weak_factory_.InvalidateWeakPtrs();
-  fetch_dispatcher_.reset();
-  stream_waiter_.reset();
-
-  url_loader_client_->OnComplete(
-      network::URLLoaderCompletionStatus(net::ERR_ABORTED));
-  url_loader_client_.reset();
-  DeleteIfNeeded();
-}
-
 bool ServiceWorkerNavigationLoader::WasCanceled() const {
   return status_ == Status::kCancelled;
 }
@@ -147,6 +136,11 @@ void ServiceWorkerNavigationLoader::DetachedFromRequest() {
   DeleteIfNeeded();
 }
 
+base::WeakPtr<ServiceWorkerNavigationLoader>
+ServiceWorkerNavigationLoader::AsWeakPtr() {
+  return weak_factory_.GetWeakPtr();
+}
+
 void ServiceWorkerNavigationLoader::StartRequest() {
   DCHECK_EQ(ResponseType::FORWARD_TO_SERVICE_WORKER, response_type_);
   DCHECK_EQ(Status::kNotStarted, status_);
@@ -319,8 +313,9 @@ void ServiceWorkerNavigationLoader::StartResponse(
   DCHECK(!binding_.is_bound());
   DCHECK(!url_loader_client_.is_bound());
   binding_.Bind(std::move(request));
-  binding_.set_connection_error_handler(base::BindOnce(
-      &ServiceWorkerNavigationLoader::Cancel, base::Unretained(this)));
+  binding_.set_connection_error_handler(
+      base::BindOnce(&ServiceWorkerNavigationLoader::OnConnectionClosed,
+                     base::Unretained(this)));
   url_loader_client_ = std::move(client);
 
   ServiceWorkerLoaderHelpers::SaveResponseInfo(response, &response_head_);
@@ -446,6 +441,23 @@ void ServiceWorkerNavigationLoader::OnBlobReadingComplete(int net_error) {
   body_as_blob_.reset();
 }
 
+void ServiceWorkerNavigationLoader::OnConnectionClosed() {
+  weak_factory_.InvalidateWeakPtrs();
+  fetch_dispatcher_.reset();
+  stream_waiter_.reset();
+  binding_.Close();
+
+  // Cancel the request if this loader hasn't yet responded to it.
+  if (status_ != Status::kCompleted && status_ != Status::kCancelled) {
+    status_ = Status::kCancelled;
+    url_loader_client_->OnComplete(
+        network::URLLoaderCompletionStatus(net::ERR_ABORTED));
+  }
+  url_loader_client_.reset();
+
+  DeleteIfNeeded();
+}
+
 void ServiceWorkerNavigationLoader::DeleteIfNeeded() {
   if (!binding_.is_bound() && detached_from_request_)
     delete this;

content/browser/service_worker/service_worker_navigation_loader.h

@@ -87,7 +87,6 @@ class CONTENT_EXPORT ServiceWorkerNavigationLoader
   void FallbackToNetwork();
   void ForwardToServiceWorker();
   bool ShouldFallbackToNetwork();
-  void Cancel();
   bool WasCanceled() const;
 
   // The navigation request that was holding this job is
@@ -97,6 +96,8 @@ class CONTENT_EXPORT ServiceWorkerNavigationLoader
   // endpoint is held by the client.
   void DetachedFromRequest();
 
+  base::WeakPtr<ServiceWorkerNavigationLoader> AsWeakPtr();
+
  private:
   class StreamWaiter;
 
@@ -151,6 +152,7 @@ class CONTENT_EXPORT ServiceWorkerNavigationLoader
 
   void OnBlobReadingComplete(int net_error);
 
+  void OnConnectionClosed();
   void DeleteIfNeeded();
 
   ResponseType response_type_ = ResponseType::NOT_DETERMINED;

content/browser/service_worker/service_worker_navigation_loader_unittest.cc

@@ -843,7 +843,8 @@ TEST_F(ServiceWorkerNavigationLoaderTest, StreamResponseAndCancel) {
   EXPECT_TRUE(data_pipe.producer_handle.is_valid());
   EXPECT_FALSE(loader_->WasCanceled());
   EXPECT_TRUE(version_->HasWorkInBrowser());
-  loader_->Cancel();
+  loader_ptr_.reset();
+  base::RunLoop().RunUntilIdle();
   EXPECT_TRUE(loader_->WasCanceled());
   EXPECT_FALSE(version_->HasWorkInBrowser());
 
@@ -1008,5 +1009,56 @@ TEST_F(ServiceWorkerNavigationLoaderTest, Redirect) {
                                       blink::SERVICE_WORKER_OK, 1);
 }
 
+TEST_F(ServiceWorkerNavigationLoaderTest, LifetimeAfterForwardToServiceWorker) {
+  LoaderResult result = StartRequest(CreateRequest());
+  EXPECT_EQ(LoaderResult::kHandledRequest, result);
+  base::WeakPtr<ServiceWorkerNavigationLoader> loader = loader_->AsWeakPtr();
+  ASSERT_TRUE(loader);
+
+  client_.RunUntilComplete();
+  EXPECT_TRUE(loader);
+
+  // Even after calling DetachedFromRequest(), |loader_| should be alive until
+  // the Mojo connection to the loader is disconnected.
+  loader_.release()->DetachedFromRequest();
+  EXPECT_TRUE(loader);
+
+  // When the interface pointer to |loader_| is disconnected, its weak pointers
+  // (|loader|) are invalidated.
+  loader_ptr_.reset();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(loader);
+
+  // |loader_| is deleted here. LSan test will alert if it leaks.
+}
+
+TEST_F(ServiceWorkerNavigationLoaderTest, LifetimeAfterFallbackToNetwork) {
+  network::ResourceRequest request;
+  request.url = GURL("https://www.example.com/");
+  request.method = "GET";
+  request.fetch_request_mode = network::mojom::FetchRequestMode::kNavigate;
+  request.fetch_credentials_mode =
+      network::mojom::FetchCredentialsMode::kInclude;
+  request.fetch_redirect_mode = network::mojom::FetchRedirectMode::kManual;
+
+  SingleRequestURLLoaderFactory::RequestHandler handler;
+  auto loader = std::make_unique<ServiceWorkerNavigationLoader>(
+      base::BindOnce(&ReceiveRequestHandler, &handler), this, request,
+      base::WrapRefCounted<URLLoaderFactoryGetter>(
+          helper_->context()->loader_factory_getter()));
+  base::WeakPtr<ServiceWorkerNavigationLoader> loader_weakptr =
+      loader->AsWeakPtr();
+  // Ask the loader to fallback to network. In production code,
+  // ServiceWorkerControlleeRequestHandler calls FallbackToNetwork() to do this.
+  loader->FallbackToNetwork();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(handler);
+  EXPECT_TRUE(loader_weakptr);
+
+  // DetachedFromRequest() deletes |loader_|.
+  loader.release()->DetachedFromRequest();
+  EXPECT_FALSE(loader_weakptr);
+}
+
 }  // namespace service_worker_navigation_loader_unittest
 }  // namespace content