Mark Background Fetch scheduler controllers as aborted when needed

There's a race condition where acknowledgement of having marked a
request as completed comes in after the Background Fetch has been
aborted, due to the way the BackgroundFetchDataManager executes its
tasks. We need to handle that appropriately in the scheduler.

Change-Id: Idb4483433aae4abb57e56be68362c4e0333402a7
Reviewed-on: https://chromium-review.googlesource.com/1152969
Commit-Queue: Peter Beverloo <peter@chromium.org>
Reviewed-by: Rayan Kanso <rayankans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578700}

content/browser/background_fetch/background_fetch_job_controller.h

@@ -47,6 +47,7 @@ class CONTENT_EXPORT BackgroundFetchJobController final
       base::RepeatingCallback<void(const std::string& /* unique_id */,
                                    uint64_t /* download_total */,
                                    uint64_t /* downloaded */)>;
+
   BackgroundFetchJobController(
       BackgroundFetchDelegateProxy* delegate_proxy,
       const BackgroundFetchRegistrationId& registration_id,

content/browser/background_fetch/background_fetch_scheduler.cc

@@ -24,6 +24,10 @@ void BackgroundFetchScheduler::Controller::Finish(
   DCHECK(reason_to_abort != BackgroundFetchReasonToAbort::NONE ||
          !HasMoreRequests());
 
+  if (reason_to_abort != BackgroundFetchReasonToAbort::NONE)
+    aborted_ = true;
+
+  DCHECK(finished_callback_);
   std::move(finished_callback_).Run(registration_id_, reason_to_abort);
 }
 
@@ -75,6 +79,7 @@ void BackgroundFetchScheduler::DidPopNextRequest(
     BackgroundFetchScheduler::Controller* controller,
     scoped_refptr<BackgroundFetchRequestInfo> request_info) {
   DCHECK(controller);
+
   lock_scheduler_ = false;  // Can schedule downloads again.
 
   // Storage error, fetch might have been aborted.
@@ -83,6 +88,12 @@ void BackgroundFetchScheduler::DidPopNextRequest(
     return;
   }
 
+  // Database tasks issued by the DataManager cannot be recalled, which means
+  // that it's possible to have a race condition where a request will have been
+  // retrieved for a controller that's otherwise been aborted.
+  if (controller->aborted())
+    return;
+
   download_controller_map_[request_info->download_guid()] = controller;
   controller->StartRequest(request_info);
 
@@ -105,6 +116,12 @@ void BackgroundFetchScheduler::MarkRequestAsComplete(
 
 void BackgroundFetchScheduler::DidMarkRequestAsComplete(
     BackgroundFetchScheduler::Controller* controller) {
+  // Database tasks issued by the DataManager cannot be recalled, which means
+  // that it's possible to have a race condition where a request will have been
+  // marked as complete for a controller that's otherwise been aborted.
+  if (controller->aborted())
+    return;
+
   if (controller->HasMoreRequests())
     controller_queue_.push_back(controller);
   else

content/browser/background_fetch/background_fetch_scheduler.h

@@ -51,6 +51,8 @@ class CONTENT_EXPORT BackgroundFetchScheduler
       return registration_id_;
     }
 
+    bool aborted() const { return aborted_; }
+
    protected:
     Controller(const BackgroundFetchRegistrationId& registration_id,
                FinishedCallback finished_callback);
@@ -58,6 +60,7 @@ class CONTENT_EXPORT BackgroundFetchScheduler
    private:
     BackgroundFetchRegistrationId registration_id_;
     FinishedCallback finished_callback_;
+    bool aborted_ = false;
   };
 
   using NextRequestCallback =