[Background Fetch] Handle start-up resumption cases

Update the BackgroundFetchDownloadClient to handle the new
DownloadService contract. The Background Fetch scheduler will attempt to
start all active downloads on start-up. The ones that are actually resuming will
be ignored by the client.

Since the response metadata and the actual response are propagated in
different calls, the response metadata will be propagated twice to
account for restarts. This will be fixed in a follow-up CL where all of the information
associated with the response will be sent once to the Background Fetch scheduler.

Bug: 881314
Change-Id: I4d806c0eec2cb9c35a42da568613fb2d5ede856e
Reviewed-on: https://chromium-review.googlesource.com/1227995Reviewed-by: Mugdha Lakhani <nator@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Commit-Queue: Rayan Kanso <rayankans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593164}

chrome/browser/background_fetch/background_fetch_delegate_impl.cc

@@ -79,7 +79,6 @@ BackgroundFetchDelegateImpl::JobDetails::JobDetails(
           fetch_description->job_unique_id)),
       fetch_description(std::move(fetch_description)) {
   offline_item.is_off_the_record = is_off_the_record;
-  current_download_guids = std::move(this->fetch_description->current_guids);
   UpdateOfflineItem();
 }
 
@@ -213,9 +212,6 @@ void BackgroundFetchDelegateImpl::CreateDownloadJob(
     std::unique_ptr<content::BackgroundFetchDescription> fetch_description) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  // Initialize the download service in case it hasn't been yet.
-  GetDownloadService();
-
   std::string job_unique_id = fetch_description->job_unique_id;
   DCHECK(!job_details_map_.count(job_unique_id));
   auto emplace_result = job_details_map_.emplace(
@@ -224,14 +220,8 @@ void BackgroundFetchDelegateImpl::CreateDownloadJob(
                  profile_->IsOffTheRecord()));
 
   const JobDetails& details = emplace_result.first->second;
-  for (const auto& download_guid : details.current_download_guids) {
-    DCHECK(!download_job_unique_id_map_.count(download_guid));
-    download_job_unique_id_map_.emplace(download_guid, job_unique_id);
-  }
-
-  for (auto* observer : observers_) {
+  for (auto* observer : observers_)
     observer->OnItemsAdded({details.offline_item});
-  }
 }
 
 void BackgroundFetchDelegateImpl::DownloadUrl(
@@ -459,6 +449,9 @@ void BackgroundFetchDelegateImpl::OnDownloadReceived(
     case StartResult::ACCEPTED:
       // Nothing to do.
       break;
+    case StartResult::UNEXPECTED_GUID:
+      // The download started in a previous session. Nothing to do.
+      break;
     case StartResult::BACKOFF:
       // TODO(delphick): try again later?
       NOTREACHED();
@@ -468,10 +461,6 @@ void BackgroundFetchDelegateImpl::OnDownloadReceived(
       // DownloadClient.
       NOTREACHED();
       break;
-    case StartResult::UNEXPECTED_GUID:
-      // TODO(delphick): try again with a different GUID.
-      NOTREACHED();
-      break;
     case StartResult::CLIENT_CANCELLED:
       // TODO(delphick): do we need to do anything here, since we will have
       // cancelled it?
@@ -615,3 +604,31 @@ void BackgroundFetchDelegateImpl::AddObserver(Observer* observer) {
 void BackgroundFetchDelegateImpl::RemoveObserver(Observer* observer) {
   observers_.erase(observer);
 }
+
+bool BackgroundFetchDelegateImpl::IsGuidOutstanding(
+    const std::string& guid) const {
+  auto unique_id_it = download_job_unique_id_map_.find(guid);
+  if (unique_id_it == download_job_unique_id_map_.end())
+    return false;
+
+  auto job_details_it = job_details_map_.find(unique_id_it->second);
+  if (job_details_it == job_details_map_.end())
+    return false;
+
+  std::vector<std::string>& outstanding_guids =
+      job_details_it->second.fetch_description->outstanding_guids;
+  return std::find(outstanding_guids.begin(), outstanding_guids.end(), guid) !=
+         outstanding_guids.end();
+}
+
+std::set<std::string> BackgroundFetchDelegateImpl::TakeOutstandingGuids() {
+  std::set<std::string> outstanding_guids;
+  for (auto& job_id_details : job_details_map_) {
+    std::vector<std::string>& job_outstanding_guids =
+        job_id_details.second.fetch_description->outstanding_guids;
+    for (std::string& outstanding_guid : job_outstanding_guids)
+      outstanding_guids.insert(std::move(outstanding_guid));
+    job_outstanding_guids.clear();
+  }
+  return outstanding_guids;
+}

chrome/browser/background_fetch/background_fetch_delegate_impl.h

@@ -108,6 +108,15 @@ class BackgroundFetchDelegateImpl
   void AddObserver(Observer* observer) override;
   void RemoveObserver(Observer* observer) override;
 
+  // Whether the provided GUID is resuming from the perspective of Background
+  // Fetch.
+  bool IsGuidOutstanding(const std::string& guid) const;
+
+  // Returns the set of download GUIDs that have started but did not finish
+  // according to Background Fetch. Clears out all references to outstanding
+  // GUIDs.
+  std::set<std::string> TakeOutstandingGuids();
+
   base::WeakPtr<BackgroundFetchDelegateImpl> GetWeakPtr() {
     return weak_ptr_factory_.GetWeakPtr();
   }

chrome/browser/background_fetch/background_fetch_download_client.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/background_fetch/background_fetch_download_client.h"
 
 #include <memory>
+#include <set>
 #include <utility>
 
 #include "base/threading/sequenced_task_runner_handle.h"
@@ -26,17 +27,31 @@ BackgroundFetchDownloadClient::~BackgroundFetchDownloadClient() = default;
 void BackgroundFetchDownloadClient::OnServiceInitialized(
     bool state_lost,
     const std::vector<download::DownloadMetaData>& downloads) {
-  content::BackgroundFetchDelegate* delegate =
-      browser_context_->GetBackgroundFetchDelegate();
-
-  // TODO(crbug.com/766082): Support incognito mode in
-  // BackgroundFetchDelegateFactory, currently |delegate| will be nullptr in
-  // incognito mode.
-  if (!delegate)
-    return;
+  std::set<std::string> outstanding_guids =
+      GetDelegate()->TakeOutstandingGuids();
+  for (const auto& download : downloads) {
+    if (!outstanding_guids.count(download.guid)) {
+      // Background Fetch is not aware of this GUID, so it successfully
+      // completed but the information is still around.
+      continue;
+    }
+
+    if (download.completion_info) {
+      // The download finished but was not persisted.
+      OnDownloadSucceeded(download.guid, *download.completion_info);
+    }
+
+    // The download is resuming, and will call the appropriate functions.
+  }
 
-  delegate_ = static_cast<BackgroundFetchDelegateImpl*>(delegate)->GetWeakPtr();
-  DCHECK(delegate_);
+  // There is also the case that the Download Service is not aware of the GUID.
+  // i.e. there is a guid in |outstanding_guids| not in |downloads|.
+  // This can be due to:
+  // 1. The browser crashing before the download started.
+  // 2. The download failing before persisting the state.
+  // 3. The browser was forced to clean-up the the download.
+  // In either case the download should be allowed to restart, so there is
+  // nothing to do here.
 }
 
 void BackgroundFetchDownloadClient::OnServiceUnavailable() {}
@@ -46,11 +61,9 @@ BackgroundFetchDownloadClient::OnDownloadStarted(
     const std::string& guid,
     const std::vector<GURL>& url_chain,
     const scoped_refptr<const net::HttpResponseHeaders>& headers) {
-  if (delegate_) {
-    std::unique_ptr<content::BackgroundFetchResponse> response =
-        std::make_unique<content::BackgroundFetchResponse>(url_chain, headers);
-    delegate_->OnDownloadStarted(guid, std::move(response));
-  }
+  std::unique_ptr<content::BackgroundFetchResponse> response =
+      std::make_unique<content::BackgroundFetchResponse>(url_chain, headers);
+  GetDelegate()->OnDownloadStarted(guid, std::move(response));
 
   // TODO(delphick): validate the chain/headers before returning CONTINUE
   return download::Client::ShouldDownload::CONTINUE;
@@ -59,31 +72,34 @@ BackgroundFetchDownloadClient::OnDownloadStarted(
 void BackgroundFetchDownloadClient::OnDownloadUpdated(
     const std::string& guid,
     uint64_t bytes_downloaded) {
-  if (delegate_)
-    delegate_->OnDownloadUpdated(guid, bytes_downloaded);
+  GetDelegate()->OnDownloadUpdated(guid, bytes_downloaded);
 }
 
 void BackgroundFetchDownloadClient::OnDownloadFailed(
     const std::string& guid,
     const download::CompletionInfo& info,
     download::Client::FailureReason reason) {
-  if (delegate_)
-    delegate_->OnDownloadFailed(guid, reason);
+  GetDelegate()->OnDownloadFailed(guid, reason);
 }
 
 void BackgroundFetchDownloadClient::OnDownloadSucceeded(
     const std::string& guid,
     const download::CompletionInfo& info) {
-  if (delegate_)
-    delegate_->OnDownloadSucceeded(guid, info.path, info.blob_handle,
-                                   info.bytes_downloaded);
+  // Pass the response headers and url_chain to the client again, in case
+  // this fetch was restarted and the client has lost this info.
+  // TODO(crbug.com/884672): Move CORS checks to `OnDownloadStarted`,
+  // and pass all the completion info to the client once.
+  OnDownloadStarted(guid, info.url_chain, info.response_headers);
+  GetDelegate()->OnDownloadSucceeded(guid, info.path, info.blob_handle,
+                                     info.bytes_downloaded);
 }
 
 bool BackgroundFetchDownloadClient::CanServiceRemoveDownloadedFile(
     const std::string& guid,
     bool force_delete) {
-  // TODO(delphick): Return false if the background fetch hasn't finished yet
-  return true;
+  // If |force_delete| is true the file will be removed anyway.
+  // TODO(rayankans): Add UMA to see how often this happens.
+  return force_delete || GetDelegate()->IsGuidOutstanding(guid);
 }
 
 void BackgroundFetchDownloadClient::GetUploadData(
@@ -92,3 +108,15 @@ void BackgroundFetchDownloadClient::GetUploadData(
   base::SequencedTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::BindOnce(std::move(callback), nullptr));
 }
+
+BackgroundFetchDelegateImpl* BackgroundFetchDownloadClient::GetDelegate() {
+  if (delegate_)
+    return delegate_.get();
+
+  content::BackgroundFetchDelegate* delegate =
+      browser_context_->GetBackgroundFetchDelegate();
+
+  delegate_ = static_cast<BackgroundFetchDelegateImpl*>(delegate)->GetWeakPtr();
+  DCHECK(delegate_);
+  return delegate_.get();
+}

chrome/browser/background_fetch/background_fetch_download_client.h

@@ -26,6 +26,9 @@ class BackgroundFetchDownloadClient : public download::Client {
   ~BackgroundFetchDownloadClient() override;
 
  private:
+  // Lazily initializes and returns |delegate_| as a raw pointer.
+  BackgroundFetchDelegateImpl* GetDelegate();
+
   // download::Client implementation
   void OnServiceInitialized(
       bool state_lost,

content/browser/background_fetch/background_fetch_job_controller.cc

@@ -44,15 +44,13 @@ void BackgroundFetchJobController::InitializeRequestStatus(
   DCHECK_GT(total_downloads, 0);
   DCHECK_EQ(total_downloads_, 0);
 
+  outstanding_requests_ = active_fetch_requests;
   completed_downloads_ = completed_downloads;
   total_downloads_ = total_downloads;
 
   // TODO(nator): Update this when we support uploads.
   total_downloads_size_ = options_.download_total;
 
-  if (!active_fetch_requests.empty())
-    is_processing_a_request_ = true;
-
   std::vector<std::string> active_guids;
   active_guids.reserve(active_fetch_requests.size());
   for (const auto& request_info : active_fetch_requests)
@@ -83,9 +81,7 @@ void BackgroundFetchJobController::StartRequest(
   DCHECK_LT(completed_downloads_, total_downloads_);
   DCHECK(request_finished_callback);
   DCHECK(request);
-  DCHECK(!is_processing_a_request_);
 
-  is_processing_a_request_ = true;
   active_request_downloaded_bytes_ = 0;
   active_request_finished_callback_ = std::move(request_finished_callback);
 
@@ -93,15 +89,9 @@ void BackgroundFetchJobController::StartRequest(
                                 registration_id().origin(), request);
 }
 
-bool BackgroundFetchJobController::IsProcessingARequest() {
-  return is_processing_a_request_;
-}
-
-void BackgroundFetchJobController::Resume(
-    RequestFinishedCallback request_finished_callback) {
-  // At the moment, the Download Service immediately resumes downloading a
-  // request on startup. Ideally we should be able to control when this happens.
-  active_request_finished_callback_ = std::move(request_finished_callback);
+std::vector<scoped_refptr<BackgroundFetchRequestInfo>>
+BackgroundFetchJobController::TakeOutstandingRequests() {
+  return std::move(outstanding_requests_);
 }
 
 void BackgroundFetchJobController::DidStartRequest(
@@ -143,8 +133,6 @@ void BackgroundFetchJobController::DidCompleteRequest(
   complete_requests_downloaded_bytes_cache_ += request->GetFileSize();
   ++completed_downloads_;
 
-  is_processing_a_request_ = false;
-
   std::move(active_request_finished_callback_).Run(request);
 }
 

content/browser/background_fetch/background_fetch_job_controller.h

@@ -110,8 +110,8 @@ class CONTENT_EXPORT BackgroundFetchJobController final
   bool HasMoreRequests() override;
   void StartRequest(scoped_refptr<BackgroundFetchRequestInfo> request,
                     RequestFinishedCallback request_finished_callback) override;
-  bool IsProcessingARequest() override;
-  void Resume(RequestFinishedCallback request_finished_callback) override;
+  std::vector<scoped_refptr<BackgroundFetchRequestInfo>>
+  TakeOutstandingRequests() override;
   void Abort(
       blink::mojom::BackgroundFetchFailureReason reason_to_abort) override;
 
@@ -122,6 +122,10 @@ class CONTENT_EXPORT BackgroundFetchJobController final
   // Icon for the represented background fetch registration.
   SkBitmap icon_;
 
+  // The list of requests for this fetch that started in a previous session
+  // and did not finish.
+  std::vector<scoped_refptr<BackgroundFetchRequestInfo>> outstanding_requests_;
+
   // Number of bytes downloaded for the active request.
   uint64_t active_request_downloaded_bytes_ = 0;
 
@@ -148,9 +152,6 @@ class CONTENT_EXPORT BackgroundFetchJobController final
   // Number of the requests that have been completed so far.
   int completed_downloads_ = 0;
 
-  // Whether the DownloadService is processing a request.
-  bool is_processing_a_request_ = false;
-
   // The reason background fetch was aborted.
   blink::mojom::BackgroundFetchFailureReason reason_to_abort_ =
       blink::mojom::BackgroundFetchFailureReason::NONE;

content/browser/background_fetch/background_fetch_scheduler.cc

@@ -52,19 +52,26 @@ BackgroundFetchScheduler::~BackgroundFetchScheduler() = default;
 
 void BackgroundFetchScheduler::AddJobController(Controller* controller) {
   DCHECK(controller);
+  controller_queue_.push_back(controller);
 
-  if (controller->IsProcessingARequest()) {
-    // There is a resuming download from the previous session, no need to
-    // schedule.
+  std::vector<scoped_refptr<content::BackgroundFetchRequestInfo>>
+      outstanding_requests = controller->TakeOutstandingRequests();
+  // There are active downloads from the previous session.
+  if (!outstanding_requests.empty()) {
+    // The current assumption is that there can be at most one active fetch
+    // at any given time.
     DCHECK(!active_controller_);
+    DCHECK_EQ(outstanding_requests.size(), 1u);
+
     active_controller_ = controller;
-    active_controller_->Resume(
-        base::BindOnce(&BackgroundFetchScheduler::MarkRequestAsComplete,
-                       weak_ptr_factory_.GetWeakPtr()));
-    return;
+    for (auto& request_info : outstanding_requests) {
+      active_controller_->StartRequest(
+          std::move(request_info),
+          base::BindOnce(&BackgroundFetchScheduler::MarkRequestAsComplete,
+                         weak_ptr_factory_.GetWeakPtr()));
+    }
   }
 
-  controller_queue_.push_back(controller);
   if (!active_controller_)
     ScheduleDownload();
 }

content/browser/background_fetch/background_fetch_scheduler.h

@@ -46,12 +46,10 @@ class CONTENT_EXPORT BackgroundFetchScheduler {
     virtual void StartRequest(scoped_refptr<BackgroundFetchRequestInfo> request,
                               RequestFinishedCallback callback) = 0;
 
-    // Whether the controller has a request that is being serviced by
-    // DownloadService. This can span sessions.
-    virtual bool IsProcessingARequest() = 0;
-
-    // Resumes a fetch started in a previous session.
-    virtual void Resume(RequestFinishedCallback request_finished_callback) = 0;
+    // Returns a list of requests that started in a previous session and did not
+    // complete. Clears the list of outstanding GUIDs in the controller.
+    virtual std::vector<scoped_refptr<BackgroundFetchRequestInfo>>
+    TakeOutstandingRequests() = 0;
 
     void Finish(blink::mojom::BackgroundFetchFailureReason reason_to_abort);
 

content/browser/background_fetch/background_fetch_scheduler_unittest.cc

@@ -55,8 +55,11 @@ class FakeController : public BackgroundFetchScheduler::Controller {
         FROM_HERE, {BrowserThread::IO},
         base::BindOnce(std::move(callback), std::move(request)));
   }
-  bool IsProcessingARequest() override { return false; }
-  void Resume(RequestFinishedCallback callback) override { NOTREACHED(); }
+
+  std::vector<scoped_refptr<BackgroundFetchRequestInfo>>
+  TakeOutstandingRequests() override {
+    return {};
+  }
 
  private:
   int jobs_started_ = 0;

content/public/browser/background_fetch_description.cc

@@ -15,7 +15,7 @@ BackgroundFetchDescription::BackgroundFetchDescription(
     int total_parts,
     int completed_parts_size,
     int total_parts_size,
-    std::vector<std::string> current_guids)
+    std::vector<std::string> outstanding_guids)
     : job_unique_id(job_unique_id),
       title(title),
       origin(origin),
@@ -24,7 +24,7 @@ BackgroundFetchDescription::BackgroundFetchDescription(
       total_parts(total_parts),
       completed_parts_size(completed_parts_size),
       total_parts_size(total_parts_size),
-      current_guids(std::move(current_guids)) {}
+      outstanding_guids(std::move(outstanding_guids)) {}
 
 BackgroundFetchDescription::~BackgroundFetchDescription() = default;
 

content/public/browser/background_fetch_description.h

@@ -23,7 +23,7 @@ struct CONTENT_EXPORT BackgroundFetchDescription {
                              int total_parts,
                              int completed_parts_size,
                              int total_parts_size,
-                             std::vector<std::string> current_guids);
+                             std::vector<std::string> outstanding_guids);
   ~BackgroundFetchDescription();
 
   const std::string job_unique_id;
@@ -34,7 +34,7 @@ struct CONTENT_EXPORT BackgroundFetchDescription {
   int total_parts;
   int completed_parts_size;
   int total_parts_size;
-  std::vector<std::string> current_guids;
+  std::vector<std::string> outstanding_guids;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(BackgroundFetchDescription);