Reland: 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.
The new CL fixes an issue that StreamHandleInputStream::OnStreamCompleted() could get called twice

TBR=xingliu@chromium.org
BUG=864189

Change-Id: Ia46040ceee88fd9978911c26ee32765a1ac4a099
Reviewed-on: https://chromium-review.googlesource.com/1142462Reviewed-by: Min Qin <qinmin@chromium.org>
Commit-Queue: Min Qin <qinmin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576401}

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

@@ -210,8 +210,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.
@@ -219,6 +221,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/stream_handle_input_stream.cc

@@ -100,6 +100,11 @@ DownloadInterruptReason StreamHandleInputStream::GetCompletionStatus() {
 
 void StreamHandleInputStream::OnStreamCompleted(
     mojom::NetworkRequestStatus status) {
+  // This method could get called again when the URLLoader is being destroyed.
+  // However, if the response is already completed, don't set the
+  // |completion_status_| again.
+  if (is_response_completed_)
+    return;
   // This can be called before or after data pipe is completely drained.
   completion_status_ = ConvertMojoNetworkRequestStatusToInterruptReason(status);
   is_response_completed_ = true;

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