2 files changed, 86 insertions, 10 deletions

diff --git a/chromium/content/browser/service_worker/service_worker_database.cc b/chromium/content/browser/service_worker/service_worker_database.cc
index 4f39ee07144..22ea65c9ed8 100644
--- a/chromium/content/browser/service_worker/service_worker_database.cc
+++ b/chromium/content/browser/service_worker/service_worker_database.cc
@@ -397,6 +397,8 @@ ServiceWorkerDatabase::Status ServiceWorkerDatabase::GetRegistrationsForOrigin(
     return status;
 
   std::string prefix = CreateRegistrationKeyPrefix(origin);
+
+  // Read all registrations.
   {
     std::unique_ptr<leveldb::Iterator> itr(
         db_->NewIterator(leveldb::ReadOptions()));
@@ -421,21 +423,34 @@ ServiceWorkerDatabase::Status ServiceWorkerDatabase::GetRegistrationsForOrigin(
         break;
       }
       registrations->push_back(registration);
+    }
+  }
 
-      if (opt_resources_list) {
-        std::vector<ResourceRecord> resources;
-        status = ReadResourceRecords(registration, &resources);
-        if (status != STATUS_OK) {
-          registrations->clear();
-          opt_resources_list->clear();
-          break;
-        }
-        opt_resources_list->push_back(resources);
+  // Count reading all registrations as one "read operation" for UMA
+  // purposes.
+  HandleReadResult(FROM_HERE, status);
+  if (status != STATUS_OK)
+    return status;
+
+  // Read the resources if requested. This must be done after the loop with
+  // leveldb::Iterator above, because it calls ReadResouceRecords() which
+  // deletes |db_| on failure, and iterators must be destroyed before the
+  // database.
+  if (opt_resources_list) {
+    for (const auto& registration : *registrations) {
+      std::vector<ResourceRecord> resources;
+      // NOTE: ReadResourceRecords already calls HandleReadResult() on its own,
+      // so to avoid double-counting the UMA, don't call it again after this.
+      status = ReadResourceRecords(registration, &resources);
+      if (status != STATUS_OK) {
+        registrations->clear();
+        opt_resources_list->clear();
+        break;
       }
+      opt_resources_list->push_back(resources);
     }
   }
 
-  HandleReadResult(FROM_HERE, status);
   return status;
 }
 
diff --git a/chromium/content/browser/service_worker/service_worker_database_unittest.cc b/chromium/content/browser/service_worker/service_worker_database_unittest.cc
index 011e30bfbad..033225edb83 100644
--- a/chromium/content/browser/service_worker/service_worker_database_unittest.cc
+++ b/chromium/content/browser/service_worker/service_worker_database_unittest.cc
@@ -14,6 +14,7 @@
 #include "base/macros.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/test/metrics/histogram_tester.h"
 #include "content/browser/service_worker/service_worker_database.pb.h"
 #include "content/common/service_worker/service_worker_types.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -2040,4 +2041,64 @@ TEST(ServiceWorkerDatabaseTest, Corruption_NoMainResource) {
   EXPECT_TRUE(resources_out.empty());
 }
 
+// Tests that GetRegistrationsForOrigin() detects corruption without crashing.
+// It must delete the database after freeing the iterator it uses to read all
+// registrations. Regression test for https://crbug.com/909024.
+TEST(ServiceWorkerDatabaseTest, Corruption_GetRegistrationsForOrigin) {
+  std::unique_ptr<ServiceWorkerDatabase> database(CreateDatabaseInMemory());
+  ServiceWorkerDatabase::RegistrationData deleted_version;
+  std::vector<int64_t> newly_purgeable_resources;
+  std::vector<Resource> resources;
+  GURL origin("https://example.com");
+
+  // Write a normal registration.
+  RegistrationData data1;
+  data1.registration_id = 1;
+  data1.scope = URL(origin, "/foo");
+  data1.script = URL(origin, "/resource1");
+  data1.version_id = 1;
+  data1.resources_total_size_bytes = 2016;
+  resources = {CreateResource(1, URL(origin, "/resource1"), 2016)};
+  ASSERT_EQ(ServiceWorkerDatabase::STATUS_OK,
+            database->WriteRegistration(data1, resources, &deleted_version,
+                                        &newly_purgeable_resources));
+
+  // Write a corrupt registration.
+  RegistrationData data2;
+  data2.registration_id = 2;
+  data2.scope = URL(origin, "/foo");
+  data2.script = URL(origin, "/resource2");
+  data2.version_id = 2;
+  data2.resources_total_size_bytes = 2016;
+  // Simulate that "/resource2" wasn't correctly written in the database by
+  // not adding it.
+  resources = {CreateResource(3, URL(origin, "/resource3"), 2016)};
+  ASSERT_EQ(ServiceWorkerDatabase::STATUS_OK,
+            database->WriteRegistration(data2, resources, &deleted_version,
+                                        &newly_purgeable_resources));
+
+  // Call GetRegistrationsForOrigin(). It should detect corruption, and not
+  // crash.
+  base::HistogramTester histogram_tester;
+  std::vector<RegistrationData> registrations;
+  std::vector<std::vector<ServiceWorkerDatabase::ResourceRecord>>
+      resources_list;
+  EXPECT_EQ(ServiceWorkerDatabase::STATUS_ERROR_CORRUPTED,
+            database->GetRegistrationsForOrigin(origin, &registrations,
+                                                &resources_list));
+  EXPECT_TRUE(registrations.empty());
+  EXPECT_TRUE(resources_list.empty());
+
+  // There should be three "read" operations logged:
+  // 1. Reading all registration data.
+  // 2. Reading the resources of the first registration.
+  // 3. Reading the resources of the second registration. This one fails.
+  histogram_tester.ExpectTotalCount("ServiceWorker.Database.ReadResult", 3);
+  histogram_tester.ExpectBucketCount("ServiceWorker.Database.ReadResult",
+                                     ServiceWorkerDatabase::STATUS_OK, 2);
+  histogram_tester.ExpectBucketCount(
+      "ServiceWorker.Database.ReadResult",
+      ServiceWorkerDatabase::STATUS_ERROR_CORRUPTED, 1);
+}
+
 }  // namespace content