Fix an issue that ResourceDownloader was never deleted on download completion

This CL calls the InProgressDownloadManager to delete ResourceDownloader
after response is completed.
It also fixes the issue when download is cancelled.

BUG=864189

Change-Id: I184d5faaace90f49874e45b4126a0a9821fdced7
Reviewed-on: https://chromium-review.googlesource.com/1138676
Commit-Queue: Min Qin <qinmin@chromium.org>
Reviewed-by: Xing Liu <xingliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#575882}

components/download/internal/common/BUILD.gn

@@ -48,6 +48,7 @@ source_set("internal") {
     "save_package_download_job.h",
     "stream_handle_input_stream.cc",
     "url_download_handler_factory.cc",
+    "url_download_request_handle.cc",
   ]
 
   public_deps = [

components/download/internal/common/download_response_handler.cc

@@ -202,8 +202,10 @@ void DownloadResponseHandler::OnComplete(
         ConvertInterruptReasonToMojoNetworkRequestStatus(reason));
   }
 
-  if (started_)
+  if (started_) {
+    delegate_->OnResponseCompleted();
     return;
+  }
 
   // OnComplete() called without OnReceiveResponse(). This should only
   // happen when the request was aborted.
@@ -211,6 +213,7 @@ void DownloadResponseHandler::OnComplete(
   create_info_->result = reason;
 
   OnResponseStarted(mojom::DownloadStreamHandlePtr());
+  delegate_->OnResponseCompleted();
 }
 
 void DownloadResponseHandler::OnResponseStarted(

components/download/internal/common/resource_downloader.cc

@@ -8,6 +8,7 @@
 
 #include "components/download/public/common/download_url_loader_factory_getter.h"
 #include "components/download/public/common/stream_handle_input_stream.h"
+#include "components/download/public/common/url_download_request_handle.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 
 namespace network {
@@ -198,6 +199,8 @@ void ResourceDownloader::InterceptResponse(
 void ResourceDownloader::OnResponseStarted(
     std::unique_ptr<DownloadCreateInfo> download_create_info,
     mojom::DownloadStreamHandlePtr stream_handle) {
+  download_create_info->request_handle.reset(new UrlDownloadRequestHandle(
+      weak_ptr_factory_.GetWeakPtr(), base::SequencedTaskRunnerHandle::Get()));
   download_create_info->is_new_download = is_new_download_;
   download_create_info->guid = guid_;
   download_create_info->site_url = site_url_;
@@ -219,4 +222,19 @@ void ResourceDownloader::OnReceiveRedirect() {
   url_loader_->FollowRedirect(base::nullopt, base::nullopt);
 }
 
+void ResourceDownloader::OnResponseCompleted() {
+  Destroy();
+}
+
+void ResourceDownloader::CancelRequest() {
+  Destroy();
+}
+
+void ResourceDownloader::Destroy() {
+  delegate_task_runner_->PostTask(
+      FROM_HERE,
+      base::BindOnce(&UrlDownloadHandler::Delegate::OnUrlDownloadStopped,
+                     delegate_, this));
+}
+
 }  // namespace download

components/download/internal/common/resource_downloader.h

@@ -73,6 +73,7 @@ class COMPONENTS_DOWNLOAD_EXPORT ResourceDownloader
       std::unique_ptr<download::DownloadCreateInfo> download_create_info,
       download::mojom::DownloadStreamHandlePtr stream_handle) override;
   void OnReceiveRedirect() override;
+  void OnResponseCompleted() override;
 
  private:
   // Helper method to start the network request.
@@ -87,6 +88,12 @@ class COMPONENTS_DOWNLOAD_EXPORT ResourceDownloader
       net::CertStatus cert_status,
       network::mojom::URLLoaderClientEndpointsPtr url_loader_client_endpoints);
 
+  // UrlDownloadHandler implementations.
+  void CancelRequest() override;
+
+  // Ask the |delegate_| to destroy this object.
+  void Destroy();
+
   base::WeakPtr<download::UrlDownloadHandler::Delegate> delegate_;
 
   // The ResourceRequest for this object.

components/download/internal/common/url_download_request_handle.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/download/public/common/url_download_request_handle.h"
+
+namespace download {
+
+UrlDownloadRequestHandle::UrlDownloadRequestHandle(
+    base::WeakPtr<UrlDownloadHandler> downloader,
+    scoped_refptr<base::SequencedTaskRunner> downloader_task_runner)
+    : downloader_(downloader),
+      downloader_task_runner_(downloader_task_runner) {}
+
+UrlDownloadRequestHandle::UrlDownloadRequestHandle(
+    UrlDownloadRequestHandle&& other)
+    : downloader_(std::move(other.downloader_)),
+      downloader_task_runner_(std::move(other.downloader_task_runner_)) {}
+
+UrlDownloadRequestHandle& UrlDownloadRequestHandle::operator=(
+    UrlDownloadRequestHandle&& other) {
+  downloader_ = std::move(other.downloader_);
+  downloader_task_runner_ = std::move(other.downloader_task_runner_);
+  return *this;
+}
+
+UrlDownloadRequestHandle::~UrlDownloadRequestHandle() = default;
+
+void UrlDownloadRequestHandle::PauseRequest() {
+  downloader_task_runner_->PostTask(
+      FROM_HERE,
+      base::BindOnce(&UrlDownloadHandler::PauseRequest, downloader_));
+}
+
+void UrlDownloadRequestHandle::ResumeRequest() {
+  downloader_task_runner_->PostTask(
+      FROM_HERE,
+      base::BindOnce(&UrlDownloadHandler::ResumeRequest, downloader_));
+}
+
+void UrlDownloadRequestHandle::CancelRequest(bool user_cancel) {
+  downloader_task_runner_->PostTask(
+      FROM_HERE,
+      base::BindOnce(&UrlDownloadHandler::CancelRequest, downloader_));
+}
+
+}  // namespace download

components/download/public/common/BUILD.gn

@@ -57,6 +57,7 @@ component("public") {
     "resume_mode.h",
     "stream_handle_input_stream.h",
     "url_download_handler_factory.h",
+    "url_download_request_handle.h",
   ]
 
   configs += [ ":components_download_implementation" ]

components/download/public/common/download_response_handler.h

@@ -26,13 +26,14 @@ namespace download {
 class COMPONENTS_DOWNLOAD_EXPORT DownloadResponseHandler
     : public network::mojom::URLLoaderClient {
  public:
-  // Class for handling the stream once response starts.
+  // Class for handling the stream response.
   class Delegate {
    public:
     virtual void OnResponseStarted(
         std::unique_ptr<DownloadCreateInfo> download_create_info,
         mojom::DownloadStreamHandlePtr stream_handle) = 0;
     virtual void OnReceiveRedirect() = 0;
+    virtual void OnResponseCompleted() = 0;
   };
 
   DownloadResponseHandler(

components/download/public/common/url_download_handler.h

@@ -40,6 +40,15 @@ class COMPONENTS_DOWNLOAD_EXPORT UrlDownloadHandler {
   UrlDownloadHandler() = default;
   virtual ~UrlDownloadHandler() = default;
 
+  // Called on the io thread to pause the url request.
+  virtual void PauseRequest() {}
+
+  // Called on the io thread to resume the url request.
+  virtual void ResumeRequest() {}
+
+  // Called on the io thread to cancel the url request.
+  virtual void CancelRequest() {}
+
   DISALLOW_COPY_AND_ASSIGN(UrlDownloadHandler);
 };
 

components/download/public/common/url_download_request_handle.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_DOWNLOAD_PUBLIC_COMMON_URL_DOWNLOAD_REQUEST_HANDLE_H_
+#define COMPONENTS_DOWNLOAD_PUBLIC_COMMON_URL_DOWNLOAD_REQUEST_HANDLE_H_
+
+#include "base/memory/weak_ptr.h"
+#include "components/download/public/common/download_export.h"
+#include "components/download/public/common/download_request_handle_interface.h"
+#include "components/download/public/common/url_download_handler.h"
+
+namespace download {
+
+// Implementation of the DownloadRequestHandleInterface to handle url download.
+class COMPONENTS_DOWNLOAD_EXPORT UrlDownloadRequestHandle
+    : public DownloadRequestHandleInterface {
+ public:
+  UrlDownloadRequestHandle(
+      base::WeakPtr<UrlDownloadHandler> downloader,
+      scoped_refptr<base::SequencedTaskRunner> downloader_task_runner);
+  UrlDownloadRequestHandle(UrlDownloadRequestHandle&& other);
+  UrlDownloadRequestHandle& operator=(UrlDownloadRequestHandle&& other);
+  ~UrlDownloadRequestHandle() override;
+
+  // DownloadRequestHandleInterface
+  void PauseRequest() override;
+  void ResumeRequest() override;
+  void CancelRequest(bool user_cancel) override;
+
+ private:
+  base::WeakPtr<UrlDownloadHandler> downloader_;
+  scoped_refptr<base::SequencedTaskRunner> downloader_task_runner_;
+
+  DISALLOW_COPY_AND_ASSIGN(UrlDownloadRequestHandle);
+};
+
+}  // namespace download
+
+#endif  // COMPONENTS_DOWNLOAD_PUBLIC_COMMON_URL_DOWNLOAD_REQUEST_HANDLE_H_

content/browser/download/url_downloader.cc

@@ -13,6 +13,7 @@
 #include "components/download/public/common/download_interrupt_reasons.h"
 #include "components/download/public/common/download_request_handle_interface.h"
 #include "components/download/public/common/download_url_parameters.h"
+#include "components/download/public/common/url_download_request_handle.h"
 #include "content/browser/byte_stream.h"
 #include "content/browser/download/byte_stream_input_stream.h"
 #include "content/public/browser/browser_thread.h"
@@ -25,43 +26,6 @@
 
 namespace content {
 
-class UrlDownloader::RequestHandle
-    : public download::DownloadRequestHandleInterface {
- public:
-  RequestHandle(base::WeakPtr<UrlDownloader> downloader,
-                scoped_refptr<base::SequencedTaskRunner> downloader_task_runner)
-      : downloader_(downloader),
-        downloader_task_runner_(downloader_task_runner) {}
-  RequestHandle(RequestHandle&& other)
-      : downloader_(std::move(other.downloader_)),
-        downloader_task_runner_(std::move(other.downloader_task_runner_)) {}
-  RequestHandle& operator=(RequestHandle&& other) {
-    downloader_ = std::move(other.downloader_);
-    downloader_task_runner_ = std::move(other.downloader_task_runner_);
-    return *this;
-  }
-
-  // DownloadRequestHandleInterface
-  void PauseRequest() override {
-    downloader_task_runner_->PostTask(
-        FROM_HERE, base::BindOnce(&UrlDownloader::PauseRequest, downloader_));
-  }
-  void ResumeRequest() override {
-    downloader_task_runner_->PostTask(
-        FROM_HERE, base::BindOnce(&UrlDownloader::ResumeRequest, downloader_));
-  }
-  void CancelRequest(bool user_cancel) override {
-    downloader_task_runner_->PostTask(
-        FROM_HERE, base::BindOnce(&UrlDownloader::CancelRequest, downloader_));
-  }
-
- private:
-  base::WeakPtr<UrlDownloader> downloader_;
-  scoped_refptr<base::SequencedTaskRunner> downloader_task_runner_;
-
-  DISALLOW_COPY_AND_ASSIGN(RequestHandle);
-};
-
 // static
 std::unique_ptr<UrlDownloader> UrlDownloader::BeginDownload(
     base::WeakPtr<download::UrlDownloadHandler::Delegate> delegate,
@@ -221,7 +185,7 @@ void UrlDownloader::OnStart(
     std::unique_ptr<download::DownloadCreateInfo> create_info,
     std::unique_ptr<ByteStreamReader> stream_reader,
     const download::DownloadUrlParameters::OnStartedCallback& callback) {
-  create_info->request_handle.reset(new RequestHandle(
+  create_info->request_handle.reset(new download::UrlDownloadRequestHandle(
       weak_ptr_factory_.GetWeakPtr(), base::SequencedTaskRunnerHandle::Get()));
 
   BrowserThread::PostTask(

content/browser/download/url_downloader.h

@@ -42,8 +42,6 @@ class UrlDownloader : public net::URLRequest::Delegate,
       bool is_parallel_request);
 
  private:
-  class RequestHandle;
-
   void Start();
 
   // URLRequest::Delegate:
@@ -64,9 +62,10 @@ class UrlDownloader : public net::URLRequest::Delegate,
       override;
   void OnReadyToRead() override;
 
-  void PauseRequest();
-  void ResumeRequest();
-  void CancelRequest();
+  // UrlDownloadHandler implementations.
+  void PauseRequest() override;
+  void ResumeRequest() override;
+  void CancelRequest() override;
 
   // Called when the UrlDownloader is done with the request. Posts a task to
   // remove itself from its download manager, which in turn would cause the