[ContentIndex] Handle corrupted data in DB operations.

Bug: 973844
Change-Id: I5e71e72cb91a3e87c0f6dc7f21d45bc686c4fd77
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1782564Reviewed-by: Richard Knoll <knollr@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Commit-Queue: Rayan Kanso <rayankans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#693096}

content/browser/content_index/content_index_database.cc

@@ -4,10 +4,12 @@
 
 #include "content/browser/content_index/content_index_database.h"
 
+#include <set>
 #include <string>
 
 #include "base/barrier_closure.h"
 #include "base/optional.h"
+#include "base/stl_util.h"
 #include "base/task/post_task.h"
 #include "base/time/time.h"
 #include "content/browser/background_fetch/storage/image_helpers.h"
@@ -334,10 +336,12 @@ void ContentIndexDatabase::GetDescriptionsOnCoreThread(
   service_worker_context_->GetRegistrationUserDataByKeyPrefix(
       service_worker_registration_id, kEntryPrefix,
       base::BindOnce(&ContentIndexDatabase::DidGetDescriptions,
-                     weak_ptr_factory_core_.GetWeakPtr(), std::move(callback)));
+                     weak_ptr_factory_core_.GetWeakPtr(),
+                     service_worker_registration_id, std::move(callback)));
 }
 
 void ContentIndexDatabase::DidGetDescriptions(
+    int64_t service_worker_registration_id,
     blink::mojom::ContentIndexService::GetDescriptionsCallback callback,
     const std::vector<std::string>& data,
     blink::ServiceWorkerStatusCode status) {
@@ -356,10 +360,10 @@ void ContentIndexDatabase::DidGetDescriptions(
   std::vector<blink::mojom::ContentDescriptionPtr> descriptions;
   descriptions.reserve(data.size());
 
-  // TODO(crbug.com/973844): Clear the storage if there is data corruption.
   for (const auto& serialized_entry : data) {
     proto::ContentEntry entry;
     if (!entry.ParseFromString(serialized_entry)) {
+      ClearServiceWorkerDataOnCorruption(service_worker_registration_id);
       std::move(callback).Run(blink::mojom::ContentIndexError::STORAGE_ERROR,
                               /* descriptions= */ {});
       return;
@@ -367,9 +371,14 @@ void ContentIndexDatabase::DidGetDescriptions(
 
     auto description = DescriptionFromProto(entry.description());
     if (!description) {
-      std::move(callback).Run(blink::mojom::ContentIndexError::STORAGE_ERROR,
-                              /* descriptions= */ {});
-      return;
+      // Clear entry data.
+      service_worker_context_->ClearRegistrationUserData(
+          service_worker_registration_id,
+          {EntryKey(entry.description().id()),
+           IconsKey(entry.description().id())},
+          base::BindOnce(&content_index::RecordDatabaseOperationStatus,
+                         "ClearCorruptedData"));
+      continue;
     }
 
     descriptions.push_back(std::move(description));
@@ -410,10 +419,12 @@ void ContentIndexDatabase::GetIconsOnCoreThread(
   service_worker_context_->GetRegistrationUserData(
       service_worker_registration_id, {IconsKey(description_id)},
       base::BindOnce(&ContentIndexDatabase::DidGetSerializedIcons,
-                     weak_ptr_factory_core_.GetWeakPtr(), std::move(callback)));
+                     weak_ptr_factory_core_.GetWeakPtr(),
+                     service_worker_registration_id, std::move(callback)));
 }
 
 void ContentIndexDatabase::DidGetSerializedIcons(
+    int64_t service_worker_registration_id,
     ContentIndexContext::GetIconsCallback callback,
     const std::vector<std::string>& data,
     blink::ServiceWorkerStatusCode status) {
@@ -429,7 +440,7 @@ void ContentIndexDatabase::DidGetSerializedIcons(
   DCHECK_EQ(data.size(), 1u);
   proto::SerializedIcons serialized_icons;
   if (!serialized_icons.ParseFromString(data.front())) {
-    // TODO(crbug.com/973844): Clear the storage if there is data corruption.
+    ClearServiceWorkerDataOnCorruption(service_worker_registration_id);
     std::move(callback).Run({});
     return;
   }
@@ -521,19 +532,28 @@ void ContentIndexDatabase::DidGetEntries(
 
   std::vector<ContentIndexEntry> entries;
   entries.reserve(user_data.size());
+  std::set<int64_t> corrupted_sw_ids;
 
   for (const auto& ud : user_data) {
     auto entry = EntryFromSerializedProto(ud.first, ud.second);
-    // TODO(crbug.com/973844): Clear the storage if there is data corruption.
     if (!entry) {
-      std::move(callback).Run(blink::mojom::ContentIndexError::STORAGE_ERROR,
-                              /* entries= */ {});
-      return;
+      corrupted_sw_ids.insert(ud.first);
+      continue;
     }
 
     entries.emplace_back(std::move(*entry));
   }
 
+  if (!corrupted_sw_ids.empty()) {
+    // Remove soon-to-be-deleted entries.
+    base::EraseIf(entries, [&corrupted_sw_ids](const auto& entry) {
+      return corrupted_sw_ids.count(entry.service_worker_registration_id);
+    });
+
+    for (int64_t service_worker_registration_id : corrupted_sw_ids)
+      ClearServiceWorkerDataOnCorruption(service_worker_registration_id);
+  }
+
   std::move(callback).Run(blink::mojom::ContentIndexError::NONE,
                           std::move(entries));
 }
@@ -590,6 +610,16 @@ void ContentIndexDatabase::DidGetEntry(
       EntryFromSerializedProto(service_worker_registration_id, data.front()));
 }
 
+void ContentIndexDatabase::ClearServiceWorkerDataOnCorruption(
+    int64_t service_worker_registration_id) {
+  DCHECK_CURRENTLY_ON(ServiceWorkerContext::GetCoreThreadId());
+
+  service_worker_context_->ClearRegistrationUserDataByKeyPrefixes(
+      service_worker_registration_id, {kEntryPrefix, kIconPrefix},
+      base::BindOnce(&content_index::RecordDatabaseOperationStatus,
+                     "ClearCorruptedData"));
+}
+
 void ContentIndexDatabase::DeleteItem(int64_t service_worker_registration_id,
                                       const url::Origin& origin,
                                       const std::string& description_id) {

content/browser/content_index/content_index_database.h

@@ -127,12 +127,14 @@ class CONTENT_EXPORT ContentIndexDatabase {
 
   // GetDescriptions Callbacks.
   void DidGetDescriptions(
+      int64_t service_worker_registration_id,
       blink::mojom::ContentIndexService::GetDescriptionsCallback callback,
       const std::vector<std::string>& data,
       blink::ServiceWorkerStatusCode status);
 
   // GetIcons Callbacks.
-  void DidGetSerializedIcons(ContentIndexContext::GetIconsCallback callback,
+  void DidGetSerializedIcons(int64_t service_worker_registration_id,
+                             ContentIndexContext::GetIconsCallback callback,
                              const std::vector<std::string>& data,
                              blink::ServiceWorkerStatusCode status);
   void DidDeserializeIcons(ContentIndexContext::GetIconsCallback callback,
@@ -167,6 +169,10 @@ class CONTENT_EXPORT ContentIndexDatabase {
   void DidDispatchEvent(const url::Origin& origin,
                         blink::ServiceWorkerStatusCode service_worker_status);
 
+  // Clears all the content index related data in a service worker.
+  void ClearServiceWorkerDataOnCorruption(
+      int64_t service_worker_registration_id);
+
   // Callbacks on the UI thread to notify |provider_| of updates.
   void NotifyProviderContentAdded(std::vector<ContentIndexEntry> entries);
   void NotifyProviderContentDeleted(int64_t service_worker_registration_id,

content/public/browser/content_index_provider.cc

@@ -18,6 +18,14 @@ ContentIndexEntry::ContentIndexEntry(
 
 ContentIndexEntry::ContentIndexEntry(ContentIndexEntry&& other) = default;
 
+ContentIndexEntry& ContentIndexEntry::operator=(ContentIndexEntry&& other) {
+  service_worker_registration_id = other.service_worker_registration_id;
+  description = std::move(other.description);
+  launch_url = std::move(other.launch_url);
+  registration_time = other.registration_time;
+  return *this;
+}
+
 ContentIndexEntry::~ContentIndexEntry() = default;
 
 ContentIndexProvider::ContentIndexProvider() = default;

content/public/browser/content_index_provider.h

@@ -29,6 +29,7 @@ struct CONTENT_EXPORT ContentIndexEntry {
                     const GURL& launch_url,
                     base::Time registration_time);
   ContentIndexEntry(ContentIndexEntry&& other);
+  ContentIndexEntry& operator=(ContentIndexEntry&& other);
   ~ContentIndexEntry();
 
   // Part of the key for an entry since different service workers can use the

tools/metrics/histograms/histograms.xml

@@ -161368,6 +161368,7 @@ regressions. -->
 
 <histogram_suffixes name="ContentIndexDatabaseTask" separator=".">
   <suffix name="Add" label="Register content"/>
+  <suffix name="ClearCorruptedData" label="Clear data in SW"/>
   <suffix name="Delete" label="Delete content"/>
   <suffix name="GetAllEntries" label="Get all entries"/>
   <suffix name="GetDescriptions" label="Get registered content"/>