[Background Fetch] Add UMA collection for remaining Database Tasks.

Bug: 780025
Change-Id: Ia8bb5d2667c4b9132cccbea5cfe5f6ce53e919a2
Reviewed-on: https://chromium-review.googlesource.com/1170829Reviewed-by: Mark Pearson <mpearson@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Commit-Queue: Rayan Kanso <rayankans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582868}

content/browser/background_fetch/background_fetch_data_manager_unittest.cc

@@ -25,6 +25,7 @@
 #include "content/browser/background_fetch/background_fetch_test_data_manager.h"
 #include "content/browser/background_fetch/storage/database_helpers.h"
 #include "content/browser/background_fetch/storage/image_helpers.h"
+#include "content/browser/cache_storage/cache_storage_cache_handle.h"
 #include "content/browser/cache_storage/cache_storage_manager.h"
 #include "content/browser/service_worker/service_worker_context_wrapper.h"
 #include "content/public/browser/background_fetch_response.h"
@@ -426,6 +427,39 @@ class BackgroundFetchDataManagerTest
     return result;
   }
 
+  void DeleteFromCache(const ServiceWorkerFetchRequest& request) {
+    CacheStorageCacheHandle handle;
+    {
+      base::RunLoop run_loop;
+      background_fetch_data_manager_->cache_manager()->OpenCache(
+          origin(), CacheStorageOwner::kBackgroundFetch,
+          kExampleUniqueId /* cache_name */,
+          base::BindOnce(&BackgroundFetchDataManagerTest::DidOpenCache,
+                         base::Unretained(this), run_loop.QuitClosure(),
+                         &handle));
+      run_loop.Run();
+    }
+
+    DCHECK(handle.value());
+
+    {
+      base::RunLoop run_loop;
+      std::vector<blink::mojom::BatchOperationPtr> operation_ptr_vec;
+      operation_ptr_vec.push_back(blink::mojom::BatchOperation::New());
+      operation_ptr_vec[0]->operation_type =
+          blink::mojom::OperationType::kDelete;
+      operation_ptr_vec[0]->request = request;
+
+      handle.value()->BatchOperation(
+          std::move(operation_ptr_vec),
+          base::BindOnce(&BackgroundFetchDataManagerTest::DidDeleteFromCache,
+                         base::Unretained(this), run_loop.QuitClosure()),
+          base::DoNothing());
+
+      run_loop.Run();
+    }
+  }
+
   // Returns the title and the icon.
   std::pair<std::string, SkBitmap> GetUIOptions(
       int64_t service_worker_registration_id) {
@@ -614,6 +648,22 @@ class BackgroundFetchDataManagerTest
     std::move(quit_closure).Run();
   }
 
+  void DidOpenCache(base::OnceClosure quit_closure,
+                    CacheStorageCacheHandle* out_handle,
+                    CacheStorageCacheHandle handle,
+                    blink::mojom::CacheStorageError error) {
+    DCHECK(out_handle);
+    DCHECK_EQ(error, blink::mojom::CacheStorageError::kSuccess);
+    *out_handle = std::move(handle);
+    std::move(quit_closure).Run();
+  }
+
+  void DidDeleteFromCache(base::OnceClosure quit_closure,
+                          blink::mojom::CacheStorageError error) {
+    DCHECK_EQ(error, blink::mojom::CacheStorageError::kSuccess);
+    std::move(quit_closure).Run();
+  }
+
   std::unique_ptr<BackgroundFetchTestDataManager>
       background_fetch_data_manager_;
 };
@@ -1237,7 +1287,8 @@ TEST_F(BackgroundFetchDataManagerTest, GetSettledFetchesForRegistration) {
   int64_t sw_id = RegisterServiceWorker();
   ASSERT_NE(blink::mojom::kInvalidServiceWorkerRegistrationId, sw_id);
 
-  std::vector<ServiceWorkerFetchRequest> requests(2u);
+  std::vector<ServiceWorkerFetchRequest> requests =
+      CreateValidRequests(origin(), 2u /* num_requests */);
   BackgroundFetchOptions options;
   blink::mojom::BackgroundFetchError error;
   BackgroundFetchRegistrationId registration_id(
@@ -1690,6 +1741,41 @@ TEST_F(BackgroundFetchDataManagerTest, StorageErrorsReported) {
         "BackgroundFetch.Storage.CreateMetadataTask",
         1 /* kServiceWorkerStorageError */, 1);
   }
+
+  scoped_refptr<BackgroundFetchRequestInfo> request_info;
+  PopNextRequest(registration_id, &request_info);
+  ASSERT_TRUE(request_info);
+  AnnotateRequestInfoWithFakeDownloadManagerData(request_info.get(),
+                                                 true /* success */);
+  MarkRequestAsComplete(registration_id, request_info.get());
+
+  bool succeeded = false;
+  std::vector<BackgroundFetchSettledFetch> settled_fetches;
+
+  {
+    GetSettledFetchesForRegistration(registration_id,
+                                     base::nullopt /* request_to_match */,
+                                     &error, &succeeded, &settled_fetches);
+
+    ASSERT_EQ(error, blink::mojom::BackgroundFetchError::NONE);
+  }
+
+  // Delete an expected entry to get a CachStorageError.
+  EXPECT_TRUE(MatchCache(requests[0]));
+  DeleteFromCache(requests[0]);
+  ASSERT_FALSE(MatchCache(requests[0]));
+
+  {
+    base::HistogramTester histogram_tester;
+    GetSettledFetchesForRegistration(registration_id,
+                                     base::nullopt /* request_to_match */,
+                                     &error, &succeeded, &settled_fetches);
+
+    ASSERT_EQ(error, blink::mojom::BackgroundFetchError::STORAGE_ERROR);
+    histogram_tester.ExpectBucketCount(
+        "BackgroundFetch.Storage.GetSettledFetchesTask",
+        2 /* kCacheStorageError */, 1);
+  }
 }
 
 }  // namespace content

content/browser/background_fetch/storage/database_task.cc

@@ -93,6 +93,10 @@ void DatabaseTask::ReportStorageError() {
                                 storage_error_);
 }
 
+bool DatabaseTask::HasStorageError() {
+  return storage_error_ != BackgroundFetchStorageError::kNone;
+}
+
 std::string DatabaseTask::HistogramName() const {
   NOTREACHED() << "HistogramName needs to be provided.";
   return "GeneralDatabaseTask";

content/browser/background_fetch/storage/database_task.h

@@ -103,6 +103,7 @@ class DatabaseTask : public DatabaseTaskHost {
   void SetStorageError(BackgroundFetchStorageError error);
   void SetStorageErrorAndFinish(BackgroundFetchStorageError error);
   void ReportStorageError();
+  bool HasStorageError();
 
  private:
   // Each task must override this function and perform the following steps:

content/browser/background_fetch/storage/delete_registration_task.cc

@@ -57,12 +57,9 @@ DeleteRegistrationTask::~DeleteRegistrationTask() = default;
 
 void DeleteRegistrationTask::Start() {
   base::RepeatingClosure barrier_closure = base::BarrierClosure(
-      2u, base::BindOnce(
-              [](base::WeakPtr<DeleteRegistrationTask> task) {
-                if (task)
-                  task->FinishWithError(task->error_);
-              },
-              weak_factory_.GetWeakPtr()));
+      2u, base::BindOnce(&DeleteRegistrationTask::FinishWithError,
+                         weak_factory_.GetWeakPtr(),
+                         blink::mojom::BackgroundFetchError::NONE));
 
 #if DCHECK_IS_ON()
   // Get the registration |developer_id| to check it was deactivated.
@@ -97,7 +94,7 @@ void DeleteRegistrationTask::DidGetRegistration(
           base::BindOnce(&DCheckRegistrationNotActive, unique_id_));
     } else {
       // Service worker database has been corrupted. Abandon all fetches.
-      error_ = blink::mojom::BackgroundFetchError::STORAGE_ERROR;
+      SetStorageError(BackgroundFetchStorageError::kServiceWorkerStorageError);
       AbandonFetches(service_worker_registration_id_);
       std::move(done_closure).Run();
     }
@@ -125,7 +122,7 @@ void DeleteRegistrationTask::DidDeleteRegistration(
     case DatabaseStatus::kNotFound:
       break;
     case DatabaseStatus::kFailed:
-      error_ = blink::mojom::BackgroundFetchError::STORAGE_ERROR;
+      SetStorageError(BackgroundFetchStorageError::kServiceWorkerStorageError);
       break;
   }
   std::move(done_closure).Run();
@@ -136,17 +133,24 @@ void DeleteRegistrationTask::DidDeleteCache(
     blink::mojom::CacheStorageError error) {
   if (error != blink::mojom::CacheStorageError::kSuccess &&
       error != blink::mojom::CacheStorageError::kErrorNotFound) {
-    error_ = blink::mojom::BackgroundFetchError::STORAGE_ERROR;
+    SetStorageError(BackgroundFetchStorageError::kCacheStorageError);
   }
   std::move(done_closure).Run();
 }
 
 void DeleteRegistrationTask::FinishWithError(
     blink::mojom::BackgroundFetchError error) {
+  if (HasStorageError())
+    error = blink::mojom::BackgroundFetchError::STORAGE_ERROR;
+  ReportStorageError();
   std::move(callback_).Run(error);
   Finished();  // Destroys |this|.
 }
 
+std::string DeleteRegistrationTask::HistogramName() const {
+  return "DeleteRegistrationTask";
+}
+
 }  // namespace background_fetch
 
 }  // namespace content

content/browser/background_fetch/storage/delete_registration_task.h

@@ -43,15 +43,13 @@ class DeleteRegistrationTask : public background_fetch::DatabaseTask {
 
   void FinishWithError(blink::mojom::BackgroundFetchError error) override;
 
+  std::string HistogramName() const override;
+
   int64_t service_worker_registration_id_;
   url::Origin origin_;
   std::string unique_id_;
   HandleBackgroundFetchErrorCallback callback_;
 
-  // The error to report once all async work is completed.
-  blink::mojom::BackgroundFetchError error_ =
-      blink::mojom::BackgroundFetchError::NONE;
-
   base::WeakPtrFactory<DeleteRegistrationTask> weak_factory_;  // Keep as last.
 
   DISALLOW_COPY_AND_ASSIGN(DeleteRegistrationTask);

content/browser/background_fetch/storage/get_settled_fetches_task.cc

@@ -50,8 +50,7 @@ void GetSettledFetchesTask::DidOpenCache(
     CacheStorageCacheHandle handle,
     blink::mojom::CacheStorageError error) {
   if (error != blink::mojom::CacheStorageError::kSuccess) {
-    // TODO(crbug.com/780025): Log failures to UMA.
-    error_ = blink::mojom::BackgroundFetchError::STORAGE_ERROR;
+    SetStorageError(BackgroundFetchStorageError::kCacheStorageError);
   } else {
     DCHECK(handle.value());
     handle_ = std::move(handle);
@@ -66,9 +65,8 @@ void GetSettledFetchesTask::DidGetCompletedRequests(
   switch (ToDatabaseStatus(status)) {
     case DatabaseStatus::kOk:
       break;
-    // TODO(crbug.com/780025): Log failures to UMA.
     case DatabaseStatus::kFailed:
-      error_ = blink::mojom::BackgroundFetchError::STORAGE_ERROR;
+      SetStorageError(BackgroundFetchStorageError::kServiceWorkerStorageError);
       break;
     case DatabaseStatus::kNotFound:
       background_fetch_succeeded_ = false;
@@ -82,7 +80,7 @@ void GetSettledFetchesTask::DidGetCompletedRequests(
     if (!completed_requests_.back().ParseFromString(
             serialized_completed_request)) {
       // Service worker database has been corrupted. Abandon fetches.
-      error_ = blink::mojom::BackgroundFetchError::STORAGE_ERROR;
+      SetStorageError(BackgroundFetchStorageError::kServiceWorkerStorageError);
       background_fetch_succeeded_ = false;
       AbandonFetches(registration_id_.service_worker_registration_id());
       break;
@@ -94,6 +92,11 @@ void GetSettledFetchesTask::DidGetCompletedRequests(
 }
 
 void GetSettledFetchesTask::GetResponses() {
+  // Handle potential errors.
+  if (HasStorageError()) {
+    FinishWithError(blink::mojom::BackgroundFetchError::STORAGE_ERROR);
+    return;
+  }
   if (error_ != blink::mojom::BackgroundFetchError::NONE) {
     FinishWithError(error_);
     return;
@@ -166,8 +169,19 @@ void GetSettledFetchesTask::DidMatchRequest(
     base::OnceClosure callback,
     blink::mojom::CacheStorageError error,
     blink::mojom::FetchAPIResponsePtr cache_response) {
-  if (error != blink::mojom::CacheStorageError::kSuccess) {
-    DCHECK(settled_fetch);
+  DCHECK(settled_fetch);
+
+  // Handle error cases.
+  if (error == blink::mojom::CacheStorageError::kErrorNotFound) {
+    // If we are matching everything then we expect to find all responses
+    // in the cache.
+    if (!match_params_->FilterByRequest())
+      SetStorageError(BackgroundFetchStorageError::kCacheStorageError);
+  } else if (error != blink::mojom::CacheStorageError::kSuccess) {
+    SetStorageError(BackgroundFetchStorageError::kCacheStorageError);
+  }
+
+  if (!cache_response) {
     FillUncachedResponse(settled_fetch, std::move(callback));
     return;
   }
@@ -192,12 +206,19 @@ void GetSettledFetchesTask::FillUncachedResponse(
 
 void GetSettledFetchesTask::FinishWithError(
     blink::mojom::BackgroundFetchError error) {
+  if (HasStorageError())
+    error = blink::mojom::BackgroundFetchError::STORAGE_ERROR;
+  ReportStorageError();
   std::move(settled_fetches_callback_)
       .Run(error, background_fetch_succeeded_, std::move(settled_fetches_),
            {} /* blob_data_handles */);
   Finished();  // Destroys |this|.
 }
 
+std::string GetSettledFetchesTask::HistogramName() const {
+  return "GetSettledFetchesTask";
+};
+
 }  // namespace background_fetch
 
 }  // namespace content

content/browser/background_fetch/storage/get_settled_fetches_task.h

@@ -65,6 +65,8 @@ class GetSettledFetchesTask : public DatabaseTask {
 
   void FinishWithError(blink::mojom::BackgroundFetchError error) override;
 
+  std::string HistogramName() const override;
+
   BackgroundFetchRegistrationId registration_id_;
   std::unique_ptr<BackgroundFetchRequestMatchParams> match_params_;
   SettledFetchesCallback settled_fetches_callback_;

tools/metrics/histograms/histograms.xml

@@ -117348,10 +117348,13 @@ uploading your change for review.
 <histogram_suffixes name="BackgroundFetchDatabaseStorageErrors" separator=".">
   <suffix name="CleanupTask" label="CleanupTask"/>
   <suffix name="CreateMetadataTask" label="CreateMetadata DatabaseTask"/>
+  <suffix name="DeleteRegistrationTask"
+      label="DeleteRegistration DatabaseTask"/>
   <suffix name="GetDeveloperIdsTask" label="GetDeveloperIds DatabaseTask"/>
   <suffix name="GetInitializationDataTask"
       label="GetInitializationData DatabaseTask"/>
   <suffix name="GetRegistrationTask" label="GetRegistration DatabaseTask"/>
+  <suffix name="GetSettledFetchesTask" label="GetSettledFetches DatabaseTask"/>
   <suffix name="MarkRegistrationForDeletionTask"
       label="MarkRegistrationForDeletion DatabaseTask"/>
   <suffix name="MarkRequestCompleteTask"