Reland "Fix an issue that ResourceDownloader was never deleted on download completion"

This reverts commit 9faf0129.

Reason for revert: <INSERT REASONING HERE>

Original change's description:
> Revert "Fix an issue that ResourceDownloader was never deleted on download completion"
> 
> This reverts commit e0b12de0.
> 
> Reason for revert: The Cl probably introduced flakiness in multiple download related tests (https://crbug.com/864922)
> 
> Original change's description:
> > 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}
> 
> TBR=qinmin@chromium.org,shaktisahu@chromium.org,xingliu@chromium.org
> 
> Change-Id: I669a69fc135547508b44ccc8150847cc5824b2ec
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: 864189
> Reviewed-on: https://chromium-review.googlesource.com/1141684
> Reviewed-by: Christian Dullweber <dullweber@chromium.org>
> Commit-Queue: Christian Dullweber <dullweber@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#575981}

TBR=qinmin@chromium.org,shaktisahu@chromium.org,dullweber@chromium.org,xingliu@chromium.org

Change-Id: I11012d0bf159f4944d69cc13881180db9ed98d7e
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 864189
Reviewed-on: https://chromium-review.googlesource.com/1142171Reviewed-by: Min Qin <qinmin@chromium.org>
Commit-Queue: Min Qin <qinmin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576190}

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