Resubmit cr/1261477

I reproduced the failure locally, and then re-ran with the changes 100
time successfully

TBR=peter@chromium.org

Change-Id: Ieb60211fcd9c6beb1aec2b152f298335c4d9178b
Reviewed-on: https://chromium-review.googlesource.com/c/1264640Reviewed-by: Mugdha Lakhani <nator@chromium.org>
Commit-Queue: Rayan Kanso <rayankans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597106}

content/browser/background_fetch/background_fetch_context.cc

@@ -404,6 +404,14 @@ void BackgroundFetchContext::Abort(
     blink::mojom::BackgroundFetchService::AbortCallback callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
+  auto controllers_iter = job_controllers_.find(registration_id.unique_id());
+  if (controllers_iter == job_controllers_.end()) {
+    std::move(callback).Run(blink::mojom::BackgroundFetchError::INVALID_ID);
+    return;
+  }
+
+  controllers_iter->second->Abort(FailureReason::CANCELLED_BY_DEVELOPER);
+
   DidFinishJob(std::move(callback), registration_id,
                FailureReason::CANCELLED_BY_DEVELOPER);
 }
@@ -411,21 +419,21 @@ void BackgroundFetchContext::Abort(
 void BackgroundFetchContext::DidFinishJob(
     base::OnceCallback<void(blink::mojom::BackgroundFetchError)> callback,
     const BackgroundFetchRegistrationId& registration_id,
-    FailureReason reason_to_abort) {
+    FailureReason failure_reason) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-  // If |aborted| is true, this will also propagate the event to any active
-  // JobController for the registration, to terminate in-progress requests.
+  // If the registration was aborted, this will also propagate the event to any
+  // active JobController, to terminate in-progress requests.
   data_manager_->MarkRegistrationForDeletion(
       registration_id,
       base::BindOnce(&BackgroundFetchContext::DidMarkForDeletion,
                      weak_factory_.GetWeakPtr(), registration_id,
-                     reason_to_abort, std::move(callback)));
+                     failure_reason, std::move(callback)));
 }
 
 void BackgroundFetchContext::DidMarkForDeletion(
     const BackgroundFetchRegistrationId& registration_id,
-    FailureReason reason_to_abort,
+    FailureReason failure_reason,
     base::OnceCallback<void(blink::mojom::BackgroundFetchError)> callback,
     blink::mojom::BackgroundFetchError error) {
   DCHECK(callback);
@@ -434,51 +442,30 @@ void BackgroundFetchContext::DidMarkForDeletion(
   // It's normal to get INVALID_ID errors here - it means the registration was
   // already inactive (marked for deletion). This happens when an abort (from
   // developer or from user) races with the download completing/failing, or even
-  // when two aborts race. TODO(johnme): Log STORAGE_ERRORs to UMA though.
+  // when two aborts race.
   if (error != blink::mojom::BackgroundFetchError::NONE)
     return;
 
-  auto controllers_iter = job_controllers_.find(registration_id.unique_id());
-
-  if (reason_to_abort == FailureReason::CANCELLED_BY_DEVELOPER) {
-    DCHECK(controllers_iter != job_controllers_.end());
-    controllers_iter->second->Abort(reason_to_abort);
+  if (failure_reason == FailureReason::NONE) {
+    // As far as we know the fetch was successful, go over the entries in the
+    // cache and make sure all the responses are there and successful.
+    data_manager_->GetSettledFetchesForRegistration(
+        registration_id, std::make_unique<BackgroundFetchRequestMatchParams>(),
+        base::BindOnce(&BackgroundFetchContext::DidGetSettledFetches,
+                       weak_factory_.GetWeakPtr(), registration_id));
+    return;
   }
+
+  // The fetch failed, dispatch an appropriate event.
+  auto controllers_iter = job_controllers_.find(registration_id.unique_id());
+  DCHECK(controllers_iter != job_controllers_.end());
   auto registration = controllers_iter->second->NewRegistration(
       blink::mojom::BackgroundFetchResult::FAILURE);
-
-  switch (reason_to_abort) {
-    case FailureReason::CANCELLED_BY_DEVELOPER:
-    case FailureReason::CANCELLED_FROM_UI:
-      CleanupRegistration(registration_id, {},
-                          blink::mojom::BackgroundFetchResult::FAILURE);
-      event_dispatcher_.DispatchBackgroundFetchAbortEvent(
-          registration_id, std::move(registration), base::DoNothing());
-      return;
-    case FailureReason::TOTAL_DOWNLOAD_SIZE_EXCEEDED:
-    case FailureReason::SERVICE_WORKER_UNAVAILABLE:
-    case FailureReason::QUOTA_EXCEEDED:
-    case FailureReason::BAD_STATUS:
-    case FailureReason::FETCH_ERROR:
-    case FailureReason::NONE:
-      // This will send a BackgroundFetchFetched or BackgroundFetchFail event.
-      // We still need this to figure out which event to send.
-      // TODO(crbug.com/699957, crbug.com/874092): Add a method to only return
-      // the information needed to dispatch these events, instead of settled
-      // fetches.
-      data_manager_->GetSettledFetchesForRegistration(
-          registration_id,
-          std::make_unique<BackgroundFetchRequestMatchParams>(),
-          base::BindOnce(&BackgroundFetchContext::DidGetSettledFetches,
-                         weak_factory_.GetWeakPtr(), registration_id,
-                         std::move(registration)));
-      return;
-  }
+  DispatchCompletionEvent(registration_id, std::move(registration));
 }
 
 void BackgroundFetchContext::DidGetSettledFetches(
     const BackgroundFetchRegistrationId& registration_id,
-    std::unique_ptr<BackgroundFetchRegistration> registration,
     blink::mojom::BackgroundFetchError error,
     FailureReason failure_reason,
     std::vector<BackgroundFetchSettledFetch> settled_fetches,
@@ -489,55 +476,53 @@ void BackgroundFetchContext::DidGetSettledFetches(
          failure_reason == FailureReason::SERVICE_WORKER_UNAVAILABLE ||
          failure_reason == FailureReason::BAD_STATUS);
 
-  if (error != blink::mojom::BackgroundFetchError::NONE) {
-    CleanupRegistration(registration_id, {} /* fetches */,
-                        blink::mojom::BackgroundFetchResult::FAILURE,
-                        true /* preserve_info_to_dispatch_click_event */);
-    return;
-  }
+  auto controllers_iter = job_controllers_.find(registration_id.unique_id());
+  DCHECK(controllers_iter != job_controllers_.end());
+  failure_reason = controllers_iter->second->MergeFailureReason(failure_reason);
 
-  DCHECK(job_controllers_.count(registration_id.unique_id()));
+  blink::mojom::BackgroundFetchResult result =
+      failure_reason == FailureReason::NONE
+          ? blink::mojom::BackgroundFetchResult::SUCCESS
+          : blink::mojom::BackgroundFetchResult::FAILURE;
 
-  // The `backgroundfetchsuccess` event will be invoked when all requests in the
-  // registration have completed successfully. In all other cases, the
-  // `backgroundfetchfail` event will be invoked instead.
-  if (registration->failure_reason == FailureReason::NONE &&
-      failure_reason != FailureReason::NONE) {
-    registration->failure_reason = failure_reason;
-  }
+  auto registration = controllers_iter->second->NewRegistration(result);
+  DispatchCompletionEvent(registration_id, std::move(registration));
+}
+
+void BackgroundFetchContext::DispatchCompletionEvent(
+    const BackgroundFetchRegistrationId& registration_id,
+    std::unique_ptr<BackgroundFetchRegistration> registration) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   switch (registration->failure_reason) {
     case FailureReason::NONE:
-      registration->result = blink::mojom::BackgroundFetchResult::SUCCESS;
       event_dispatcher_.DispatchBackgroundFetchSuccessEvent(
           registration_id, std::move(registration),
           base::BindOnce(
               &BackgroundFetchContext::CleanupRegistration,
               weak_factory_.GetWeakPtr(), registration_id,
-              // The blob uuid is sent as part of |settled_fetches|. Bind
-              // |blob_data_handles| to the callback to keep them alive
-              // until the waitUntil event is resolved.
-              std::move(blob_data_handles),
               blink::mojom::BackgroundFetchResult::SUCCESS,
               true /* preserve_info_to_dispatch_click_event */));
       return;
     case FailureReason::CANCELLED_FROM_UI:
     case FailureReason::CANCELLED_BY_DEVELOPER:
+      event_dispatcher_.DispatchBackgroundFetchAbortEvent(
+          registration_id, std::move(registration),
+          base::BindOnce(&BackgroundFetchContext::CleanupRegistration,
+                         weak_factory_.GetWeakPtr(), registration_id,
+                         blink::mojom::BackgroundFetchResult::FAILURE,
+                         false /* preserve_info_to_dispatch_click_event */));
+      return;
     case FailureReason::BAD_STATUS:
     case FailureReason::FETCH_ERROR:
     case FailureReason::SERVICE_WORKER_UNAVAILABLE:
     case FailureReason::QUOTA_EXCEEDED:
     case FailureReason::TOTAL_DOWNLOAD_SIZE_EXCEEDED:
-      registration->result = blink::mojom::BackgroundFetchResult::FAILURE;
       event_dispatcher_.DispatchBackgroundFetchFailEvent(
           registration_id, std::move(registration),
           base::BindOnce(
               &BackgroundFetchContext::CleanupRegistration,
               weak_factory_.GetWeakPtr(), registration_id,
-              // The blob uuid is sent as part of |settled_fetches|. Bind
-              // |blob_data_handles| to the callback to keep them alive
-              // until the waitUntil event is resolved.
-              std::move(blob_data_handles),
               blink::mojom::BackgroundFetchResult::FAILURE,
               true /* preserve_info_to_dispatch_click_event */));
       return;
@@ -546,7 +531,6 @@ void BackgroundFetchContext::DidGetSettledFetches(
 
 void BackgroundFetchContext::CleanupRegistration(
     const BackgroundFetchRegistrationId& registration_id,
-    const std::vector<std::unique_ptr<storage::BlobDataHandle>>& blob_handles,
     blink::mojom::BackgroundFetchResult background_fetch_result,
     bool preserve_info_to_dispatch_click_event) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);

content/browser/background_fetch/background_fetch_context.h

@@ -201,12 +201,12 @@ class CONTENT_EXPORT BackgroundFetchContext
   void DidFinishJob(
       base::OnceCallback<void(blink::mojom::BackgroundFetchError)> callback,
       const BackgroundFetchRegistrationId& registration_id,
-      blink::mojom::BackgroundFetchFailureReason reason_to_abort);
+      blink::mojom::BackgroundFetchFailureReason failure_reason);
 
   // Called when the data manager finishes marking a registration as deleted.
   void DidMarkForDeletion(
       const BackgroundFetchRegistrationId& registration_id,
-      blink::mojom::BackgroundFetchFailureReason reason_to_abort,
+      blink::mojom::BackgroundFetchFailureReason failure_reason,
       base::OnceCallback<void(blink::mojom::BackgroundFetchError)> callback,
       blink::mojom::BackgroundFetchError error);
 
@@ -214,7 +214,6 @@ class CONTENT_EXPORT BackgroundFetchContext
   // retrieved from storage, and the Service Worker event can be invoked.
   void DidGetSettledFetches(
       const BackgroundFetchRegistrationId& registration_id,
-      std::unique_ptr<BackgroundFetchRegistration> registration,
       blink::mojom::BackgroundFetchError error,
       blink::mojom::BackgroundFetchFailureReason failure_reason,
       std::vector<BackgroundFetchSettledFetch> settled_fetches,
@@ -230,6 +229,11 @@ class CONTENT_EXPORT BackgroundFetchContext
       std::vector<BackgroundFetchSettledFetch> settled_fetches,
       std::vector<std::unique_ptr<storage::BlobDataHandle>> blob_data_handles);
 
+  // Dispatches an appropriate event (success, fail, abort).
+  void DispatchCompletionEvent(
+      const BackgroundFetchRegistrationId& registration_id,
+      std::unique_ptr<BackgroundFetchRegistration> registration);
+
   // Called when the notification UI for the background fetch job associated
   // with |unique_id| is activated.
   void DispatchClickEvent(const std::string& unique_id);
@@ -241,16 +245,14 @@ class CONTENT_EXPORT BackgroundFetchContext
           initialization_data);
 
   // Called when all processing for the |registration_id| has been finished and
-  // the job is ready to be deleted. |blob_handles| are unused, but some callers
-  // use it to keep blobs alive for the right duration.
-  // |partial cleanup|, when set, preserves  the registration ID, and the result
-  // of Fetch when it completed, in |completed_fetches_|. This is not done when
-  // fetch is aborted or cancelled. We use this information to propagate
-  // BackgroundFetchClicked event to the developer, when the user taps the UI.
+  // the job is ready to be deleted.
+  // |preserve_info_to_dispatch_click_event|, when set, preserves the
+  // registration ID, and the result of the Fetch when it completed, in
+  // |completed_fetches_|. This is not done when fetch is aborted or cancelled.
+  // We use this information to propagate BackgroundFetchClicked event to the
+  // developer, when the user taps the UI.
   void CleanupRegistration(
       const BackgroundFetchRegistrationId& registration_id,
-      const std::vector<std::unique_ptr<storage::BlobDataHandle>>&
-          blob_data_handles,
       blink::mojom::BackgroundFetchResult background_fetch_result,
       bool preserve_info_to_dispatch_click_event = false);
 

content/browser/background_fetch/background_fetch_job_controller.cc

@@ -13,6 +13,8 @@
 
 namespace content {
 
+using blink::mojom::BackgroundFetchFailureReason;
+
 BackgroundFetchJobController::BackgroundFetchJobController(
     BackgroundFetchDelegateProxy* delegate_proxy,
     BackgroundFetchScheduler* scheduler,
@@ -197,7 +199,7 @@ BackgroundFetchJobController::NewRegistration(
   return std::make_unique<BackgroundFetchRegistration>(
       registration_id().developer_id(), registration_id().unique_id(),
       0 /* upload_total */, 0 /* uploaded */, total_downloads_size_,
-      complete_requests_downloaded_bytes_cache_, result, reason_to_abort_);
+      complete_requests_downloaded_bytes_cache_, result, failure_reason_);
 }
 
 uint64_t BackgroundFetchJobController::GetInProgressDownloadedBytes() {
@@ -205,8 +207,8 @@ uint64_t BackgroundFetchJobController::GetInProgressDownloadedBytes() {
 }
 
 void BackgroundFetchJobController::Abort(
-    blink::mojom::BackgroundFetchFailureReason reason_to_abort) {
-  reason_to_abort_ = reason_to_abort;
+    BackgroundFetchFailureReason failure_reason) {
+  failure_reason_ = failure_reason;
 
   // Stop propagating any in-flight events to the scheduler.
   active_request_finished_callback_.Reset();
@@ -214,7 +216,14 @@ void BackgroundFetchJobController::Abort(
   // Cancel any in-flight downloads and UI through the BGFetchDelegate.
   delegate_proxy_->Abort(registration_id().unique_id());
 
-  Finish(reason_to_abort);
+  Finish(failure_reason_);
+}
+
+BackgroundFetchFailureReason BackgroundFetchJobController::MergeFailureReason(
+    BackgroundFetchFailureReason failure_reason) {
+  if (failure_reason_ == BackgroundFetchFailureReason::NONE)
+    failure_reason_ = failure_reason;
+  return failure_reason_;
 }
 
 }  // namespace content

content/browser/background_fetch/background_fetch_job_controller.h

@@ -94,6 +94,11 @@ class CONTENT_EXPORT BackgroundFetchJobController final
   // Returns the number of requests that comprise the whole job.
   int total_downloads() const { return total_downloads_; }
 
+  // If |failure_reason_| is none, overwrites it with |failure_reason|, and
+  // returns the new value.
+  blink::mojom::BackgroundFetchFailureReason MergeFailureReason(
+      blink::mojom::BackgroundFetchFailureReason failure_reason);
+
   base::WeakPtr<BackgroundFetchJobController> GetWeakPtr() {
     return weak_ptr_factory_.GetWeakPtr();
   }
@@ -114,7 +119,7 @@ class CONTENT_EXPORT BackgroundFetchJobController final
   std::vector<scoped_refptr<BackgroundFetchRequestInfo>>
   TakeOutstandingRequests() override;
   void Abort(
-      blink::mojom::BackgroundFetchFailureReason reason_to_abort) override;
+      blink::mojom::BackgroundFetchFailureReason failure_reason) override;
 
  private:
   // Performs mixed content checks on the |request| for Background Fetch.
@@ -167,7 +172,7 @@ class CONTENT_EXPORT BackgroundFetchJobController final
   int completed_downloads_ = 0;
 
   // The reason background fetch was aborted.
-  blink::mojom::BackgroundFetchFailureReason reason_to_abort_ =
+  blink::mojom::BackgroundFetchFailureReason failure_reason_ =
       blink::mojom::BackgroundFetchFailureReason::NONE;
 
   base::WeakPtrFactory<BackgroundFetchJobController> weak_ptr_factory_;

third_party/WebKit/LayoutTests/external/wpt/background-fetch/abort.https.window.js

+// META: script=/service-workers/service-worker/resources/test-helpers.sub.js
+// META: script=resources/utils.js
+'use strict';
+
+// Covers basic functionality provided by BackgroundFetchManager.abort().
+// https://wicg.github.io/background-fetch/#background-fetch-registration-abort
+
+backgroundFetchTest(async (test, backgroundFetch) => {
+  const registration = await backgroundFetch.fetch(
+      uniqueId(),
+      ['resources/feature-name.txt', '/serviceworker/resources/slow-response.php']);
+
+  assert_true(await registration.abort());
+  assert_false(await registration.abort());
+
+}, 'Aborting the same registration twice fails');
+
+backgroundFetchTest(async (test, backgroundFetch) => {
+  const registration = await backgroundFetch.fetch(
+      uniqueId(),
+      ['resources/feature-name.txt', '/serviceworker/resources/slow-response.php']);
+  const resultPromise = getMessageFromServiceWorker();
+
+  await new Promise(resolve => {
+    registration.onprogress = async (e) => {
+      // The size of the first file.
+      if (e.target.downloaded < 16)
+        return;
+
+      // At this point the first file is downloaded.
+
+      assert_true(await registration.abort());
+
+      const {type, eventRegistration, results} = await resultPromise;
+
+      assert_equals(eventRegistration.result, 'failure');
+      assert_equals(eventRegistration.failureReason, 'aborted');
+
+      assert_equals(type, 'backgroundfetchabort');
+      assert_equals(results.length, 1);
+
+      assert_true(results[0].url.includes('resources/feature-name.txt'));
+      assert_equals(results[0].status, 200);
+      assert_equals(results[0].text, 'Background Fetch');
+
+      resolve();
+    };
+  });
+
+}, 'Calling BackgroundFetchRegistration.abort sets the correct fields and responses are still available');
\ No newline at end of file

third_party/WebKit/LayoutTests/external/wpt/background-fetch/service_workers/sw.js

@@ -27,3 +27,4 @@ function handleBackgroundFetchUpdateEvent(event) {
 
 self.addEventListener('backgroundfetchsuccess', handleBackgroundFetchUpdateEvent);
 self.addEventListener('backgroundfetchfail', handleBackgroundFetchUpdateEvent);
+self.addEventListener('backgroundfetchabort', handleBackgroundFetchUpdateEvent);