BackgroundFetch aborts now cancel active Downloads.

Pass through the Abort message to the DownloadService for any
outstanding downloads matching the given job_unique_id.

Update abort unit tests to verify that the download successful messages
do not get delivered.

Change-Id: I600a3f102047939d7189dc6d30e9ca88eedf4c04
Bug: 758562
Reviewed-on: https://chromium-review.googlesource.com/723321Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: John Mellor <johnme@chromium.org>
Commit-Queue: Dan Elphick <delphick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510090}

chrome/browser/background_fetch/background_fetch_delegate_impl.cc

@@ -140,6 +140,23 @@ void BackgroundFetchDelegateImpl::DownloadUrl(
   download_service_->StartDownload(params);
 }
 
+void BackgroundFetchDelegateImpl::Abort(const std::string& job_unique_id) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  auto job_details_iter = job_details_map_.find(job_unique_id);
+  if (job_details_iter == job_details_map_.end())
+    return;
+
+  JobDetails& job_details = job_details_iter->second;
+
+  for (const auto& download_guid : job_details.current_download_guids) {
+    download_service_->CancelDownload(download_guid);
+    download_job_unique_id_map_.erase(download_guid);
+  }
+
+  job_details_map_.erase(job_details_iter);
+}
+
 void BackgroundFetchDelegateImpl::OnDownloadStarted(
     const std::string& download_guid,
     std::unique_ptr<content::BackgroundFetchResponse> response) {
@@ -166,8 +183,16 @@ void BackgroundFetchDelegateImpl::OnDownloadFailed(
   using FailureReason = content::BackgroundFetchResult::FailureReason;
   FailureReason failure_reason;
 
-  const std::string& job_unique_id = download_job_unique_id_map_[download_guid];
+  auto download_job_unique_id_iter =
+      download_job_unique_id_map_.find(download_guid);
+  // Cancelled downloads will already have been deleted so just return.
+  if (download_job_unique_id_iter == download_job_unique_id_map_.end())
+    return;
+
+  const std::string& job_unique_id = download_job_unique_id_iter->second;
   JobDetails& job_details = job_details_map_.find(job_unique_id)->second;
+  ++job_details.completed_parts;
+  job_details.UpdateOfflineItem();
 
   switch (reason) {
     case download::Client::FailureReason::NETWORK:

chrome/browser/background_fetch/background_fetch_delegate_impl.h

@@ -59,6 +59,7 @@ class BackgroundFetchDelegateImpl
                    const GURL& url,
                    const net::NetworkTrafficAnnotationTag& traffic_annotation,
                    const net::HttpRequestHeaders& headers) override;
+  void Abort(const std::string& job_unique_id) override;
 
   void OnDownloadStarted(const std::string& guid,
                          std::unique_ptr<content::BackgroundFetchResponse>);

content/browser/background_fetch/background_fetch_delegate_proxy.cc

@@ -117,6 +117,13 @@ class BackgroundFetchDelegateProxy::Core
                            fetch_request.url, traffic_annotation, headers);
   }
 
+  void Abort(const std::string& job_unique_id) {
+    DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+    if (delegate_)
+      delegate_->Abort(job_unique_id);
+  }
+
   // BackgroundFetchDelegate::Client implementation:
   void OnDownloadUpdated(const std::string& guid,
                          uint64_t bytes_downloaded) override;
@@ -234,10 +241,12 @@ void BackgroundFetchDelegateProxy::UpdateUI(const std::string& title) {
   // TODO(delphick): Update the user interface with |title|.
 }
 
-void BackgroundFetchDelegateProxy::Abort() {
+void BackgroundFetchDelegateProxy::Abort(const std::string& job_unique_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-  // TODO(delphick): Abort all in-progress downloads.
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::BindOnce(&Core::Abort, ui_core_ptr_, job_unique_id));
 }
 
 void BackgroundFetchDelegateProxy::DidStartRequest(

content/browser/background_fetch/background_fetch_delegate_proxy.h

@@ -85,10 +85,10 @@ class CONTENT_EXPORT BackgroundFetchDelegateProxy {
   // thread).
   void UpdateUI(const std::string& title);
 
-  // Immediately aborts this Background Fetch by request of the developer.
-  // Should only be called from the BackgroundFetchJobController (on the IO
-  // thread).
-  void Abort();
+  // Immediately aborts the job identified by |job_unique_id| by request of the
+  // developer. Should only be called from the BackgroundFetchJobController (on
+  // the IO thread).
+  void Abort(const std::string& job_unique_id);
 
  private:
   class Core;

content/browser/background_fetch/background_fetch_delegate_proxy_unittest.cc

@@ -4,6 +4,7 @@
 
 #include "content/browser/background_fetch/background_fetch_delegate_proxy.h"
 
+#include <set>
 #include <vector>
 
 #include "base/memory/weak_ptr.h"
@@ -38,26 +39,43 @@ class FakeBackgroundFetchDelegate : public BackgroundFetchDelegate {
     if (!client())
       return;
 
+    download_guid_to_job_id_map_[guid] = job_unique_id;
+
     auto response = std::make_unique<BackgroundFetchResponse>(
         std::vector<GURL>({url}),
         base::MakeRefCounted<net::HttpResponseHeaders>("200 OK"));
 
     client()->OnDownloadStarted(guid, std::move(response));
     if (complete_downloads_) {
-      auto result = std::make_unique<BackgroundFetchResult>(
-          base::Time::Now(), base::FilePath(), 10u);
       BrowserThread::PostTask(
           BrowserThread::IO, FROM_HERE,
-          base::BindOnce(&BackgroundFetchDelegate::Client::OnDownloadComplete,
-                         client(), guid, std::move(result)));
+          base::BindOnce(&FakeBackgroundFetchDelegate::CompleteDownload,
+                         base::Unretained(this), guid));
     }
   }
+  void Abort(const std::string& job_unique_id) override {
+    aborted_jobs_.insert(job_unique_id);
+  }
 
   void set_complete_downloads(bool complete_downloads) {
     complete_downloads_ = complete_downloads;
   }
 
  private:
+  void CompleteDownload(const std::string& guid) {
+    if (!client())
+      return;
+
+    if (aborted_jobs_.count(download_guid_to_job_id_map_[guid]))
+      return;
+
+    client()->OnDownloadComplete(
+        guid, std::make_unique<BackgroundFetchResult>(base::Time::Now(),
+                                                      base::FilePath(), 10u));
+  }
+
+  std::set<std::string> aborted_jobs_;
+  std::map<std::string, std::string> download_guid_to_job_id_map_;
   bool complete_downloads_ = true;
 };
 
@@ -138,4 +156,34 @@ TEST_F(BackgroundFetchDelegateProxyTest, StartRequest_NotCompleted) {
   EXPECT_FALSE(controller.request_completed_);
 }
 
+TEST_F(BackgroundFetchDelegateProxyTest, Abort) {
+  FakeController controller;
+  FakeController controller2;
+  ServiceWorkerFetchRequest fetch_request;
+  ServiceWorkerFetchRequest fetch_request2;
+  auto request = base::MakeRefCounted<BackgroundFetchRequestInfo>(
+      0 /* request_index */, fetch_request);
+  auto request2 = base::MakeRefCounted<BackgroundFetchRequestInfo>(
+      1 /* request_index */, fetch_request2);
+
+  EXPECT_FALSE(controller.request_started_);
+  EXPECT_FALSE(controller.request_completed_);
+  EXPECT_FALSE(controller2.request_started_);
+  EXPECT_FALSE(controller2.request_completed_);
+
+  delegate_proxy_.StartRequest("jobid1",
+                               controller.weak_ptr_factory_.GetWeakPtr(),
+                               url::Origin(), request);
+  delegate_proxy_.StartRequest("jobid2",
+                               controller2.weak_ptr_factory_.GetWeakPtr(),
+                               url::Origin(), request2);
+  delegate_proxy_.Abort("jobid1");
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_TRUE(controller.request_started_);
+  EXPECT_FALSE(controller.request_completed_) << "Aborted job completed";
+  EXPECT_TRUE(controller2.request_started_);
+  EXPECT_TRUE(controller2.request_completed_) << "Normal job did not complete";
+}
+
 }  // namespace content

content/browser/background_fetch/background_fetch_job_controller.cc

@@ -97,12 +97,7 @@ void BackgroundFetchJobController::DidCompleteRequest(
     const scoped_refptr<BackgroundFetchRequestInfo>& request,
     const std::string& download_guid) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  DCHECK(state_ == State::FETCHING || state_ == State::ABORTED);
-
-  // TODO(delphick): When ABORT is implemented correctly we should hopefully
-  // never get here and the DCHECK above should only allow FETCHING.
-  if (state_ == State::ABORTED)
-    return;
+  DCHECK(state_ == State::FETCHING);
 
   // This request is no longer in-progress, so the DataManager will take over
   // responsibility for storing its downloaded bytes, though still need a cache.
@@ -162,7 +157,7 @@ void BackgroundFetchJobController::Abort() {
       return;  // Ignore attempt to abort after completion/abort.
   }
 
-  delegate_proxy_->Abort();
+  delegate_proxy_->Abort(registration_id_.unique_id());
 
   state_ = State::ABORTED;
   // Inform the owner of the controller about the job having aborted.

content/browser/background_fetch/background_fetch_job_controller_unittest.cc

@@ -242,18 +242,14 @@ TEST_F(BackgroundFetchJobControllerTest, AbortJob) {
             BackgroundFetchJobController::State::INITIALIZED);
 
   // Start the first few requests, and abort them immediately after.
-  {
-    base::RunLoop run_loop;
-    job_completed_closure_ = run_loop.QuitClosure();
-
-    controller->Start();
-    EXPECT_EQ(controller->state(),
-              BackgroundFetchJobController::State::FETCHING);
+  controller->Start();
+  EXPECT_EQ(controller->state(), BackgroundFetchJobController::State::FETCHING);
 
-    controller->Abort();
+  controller->Abort();
 
-    run_loop.Run();
-  }
+  // Run until idle to ensure that spurious download successful tasks are not
+  // executed.
+  base::RunLoop().RunUntilIdle();
 
   // TODO(peter): Verify that the issued download items have had their state
   // updated to be cancelled as well.

content/browser/background_fetch/mock_background_fetch_delegate.cc

@@ -74,6 +74,8 @@ void MockBackgroundFetchDelegate::DownloadUrl(
   // use the DownloadService, we should signal StartResult::UNEXPECTED_GUID.
   DCHECK(seen_guids_.find(guid) == seen_guids_.end());
 
+  download_guid_to_job_id_map_[guid] = job_unique_id;
+
   auto url_iter = url_responses_.find(url);
   if (url_iter == url_responses_.end()) {
     // Since no response was provided, do not respond. This allows testing
@@ -88,21 +90,21 @@ void MockBackgroundFetchDelegate::DownloadUrl(
       std::make_unique<BackgroundFetchResponse>(std::vector<GURL>({url}),
                                                 test_response->headers);
 
-  base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE,
+  PostAbortCheckingTask(
+      job_unique_id,
       base::BindOnce(&BackgroundFetchDelegate::Client::OnDownloadStarted,
                      client(), guid, std::move(response)));
 
   if (test_response->data.size()) {
     // Report progress at 50% complete.
-    base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE,
+    PostAbortCheckingTask(
+        job_unique_id,
         base::BindOnce(&BackgroundFetchDelegate::Client::OnDownloadUpdated,
                        client(), guid, test_response->data.size() / 2));
 
     // Report progress at 100% complete.
-    base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE,
+    PostAbortCheckingTask(
+        job_unique_id,
         base::BindOnce(&BackgroundFetchDelegate::Client::OnDownloadUpdated,
                        client(), guid, test_response->data.size()));
   }
@@ -121,16 +123,16 @@ void MockBackgroundFetchDelegate::DownloadUrl(
              base::WriteFile(response_path, test_response->data.c_str(),
                              test_response->data.size()));
 
-    base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE,
+    PostAbortCheckingTask(
+        job_unique_id,
         base::BindOnce(
             &BackgroundFetchDelegate::Client::OnDownloadComplete, client(),
             guid,
             std::make_unique<BackgroundFetchResult>(
                 base::Time::Now(), response_path, test_response->data.size())));
   } else {
-    base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE,
+    PostAbortCheckingTask(
+        job_unique_id,
         base::BindOnce(&BackgroundFetchDelegate::Client::OnDownloadComplete,
                        client(), guid,
                        std::make_unique<BackgroundFetchResult>(
@@ -141,6 +143,10 @@ void MockBackgroundFetchDelegate::DownloadUrl(
   seen_guids_.insert(guid);
 }
 
+void MockBackgroundFetchDelegate::Abort(const std::string& job_unique_id) {
+  aborted_jobs_.insert(job_unique_id);
+}
+
 void MockBackgroundFetchDelegate::RegisterResponse(
     const GURL& url,
     std::unique_ptr<TestResponse> response) {
@@ -148,4 +154,21 @@ void MockBackgroundFetchDelegate::RegisterResponse(
   url_responses_[url] = std::move(response);
 }
 
+void MockBackgroundFetchDelegate::PostAbortCheckingTask(
+    const std::string& job_unique_id,
+    base::OnceCallback<void()> callback) {
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE,
+      base::BindOnce(&MockBackgroundFetchDelegate::RunAbortCheckingTask,
+                     base::Unretained(this), job_unique_id,
+                     std::move(callback)));
+}
+
+void MockBackgroundFetchDelegate::RunAbortCheckingTask(
+    const std::string& job_unique_id,
+    base::OnceCallback<void()> callback) {
+  if (!aborted_jobs_.count(job_unique_id))
+    std::move(callback).Run();
+}
+
 }  // namespace content

content/browser/background_fetch/mock_background_fetch_delegate.h

@@ -76,11 +76,22 @@ class MockBackgroundFetchDelegate : public BackgroundFetchDelegate {
                    const GURL& url,
                    const net::NetworkTrafficAnnotationTag& traffic_annotation,
                    const net::HttpRequestHeaders& headers) override;
+  void Abort(const std::string& job_unique_id) override;
 
   void RegisterResponse(const GURL& url,
                         std::unique_ptr<TestResponse> response);
 
  private:
+  // Posts (to the default task runner) a callback that is only run if the job
+  // indicated by |job_unique_id| has not been aborted.
+  void PostAbortCheckingTask(const std::string& job_unique_id,
+                             base::OnceCallback<void()> callback);
+
+  // Immediately runs the callback if the job indicated by |job_unique_id| has
+  // not been aborted.
+  void RunAbortCheckingTask(const std::string& job_unique_id,
+                            base::OnceCallback<void()> callback);
+
   // Single-use responses registered for specific URLs.
   std::map<const GURL, std::unique_ptr<TestResponse>> url_responses_;
 
@@ -90,6 +101,12 @@ class MockBackgroundFetchDelegate : public BackgroundFetchDelegate {
   // Temporary directory in which successfully downloaded files will be stored.
   base::ScopedTempDir temp_directory_;
 
+  // Set of unique job ids that have been aborted.
+  std::set<std::string> aborted_jobs_;
+
+  // Map from download GUIDs to unique job ids.
+  std::map<std::string, std::string> download_guid_to_job_id_map_;
+
   DISALLOW_COPY_AND_ASSIGN(MockBackgroundFetchDelegate);
 };
 

content/public/browser/background_fetch_delegate.h

@@ -91,6 +91,9 @@ class CONTENT_EXPORT BackgroundFetchDelegate {
       const net::NetworkTrafficAnnotationTag& traffic_annotation,
       const net::HttpRequestHeaders& headers) = 0;
 
+  // Aborts any downloads associated with |job_unique_id|.
+  virtual void Abort(const std::string& job_unique_id) = 0;
+
   // Set the client that the delegate should communicate changes to.
   void SetDelegateClient(base::WeakPtr<Client> client) { client_ = client; }