Reland "service worker: Use mojo version of GetUserDataForAllRegistrations"

This is a reland of 853e0dd9

This is the third attempt to land. The second attempt was reverted
due to a flaky test. A speculative fix was landed in
crrev.com/c/2352268.

Original change's description:
> service worker: Use mojo version of GetUserDataForAllRegistrations
>
> This CL starts using mojo version of
> GetUserDataForAllRegistrations{,ByKeyPrefix}().
>
> The type of the return value of these methods is also changed from
> map<registration_id, value> to array<ServiceWorkerUserData>,
> and ServiceWorkerUserData now has registration_id.
> These changes are required to handle a situation where a
> registration_id is associated with multiple values. Some features like
> payments could associate multiple values for a registration_id.
>
> Bug: 1055677
> Change-Id: I29e9ae7c1722316b2ac2f163cfc5b50fe7590d54
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2331376
> Commit-Queue: Kenichi Ishibashi <bashi@chromium.org>
> Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
> Reviewed-by: Matt Falkenhagen <falken@chromium.org>
> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#794447}

Bug: 1055677
Change-Id: I5bcaef59bfe96715684a11b41a728505c828f315
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2365732Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Kenichi Ishibashi <bashi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#799992}

components/services/storage/public/mojom/service_worker_storage_control.mojom

@@ -93,6 +93,7 @@ interface ServiceWorkerResourceMetadataWriter {
 // lifetime of user data is tied up with the registration.
 // It will be deleted when the corresponding registration is deleted.
 struct ServiceWorkerUserData {
+  int64 registration_id;
   string key;
   string value;
 };
@@ -251,14 +252,14 @@ interface ServiceWorkerStorageControl {
       (ServiceWorkerDatabaseStatus status);
 
   // Gets the user data from all registrations that have user data for |key|.
-  // Returns a map from registration IDs to their values.
   GetUserDataForAllRegistrations(string key) =>
-      (ServiceWorkerDatabaseStatus status, map<int64, string> values);
+      (ServiceWorkerDatabaseStatus status,
+       array<ServiceWorkerUserData> values);
   // Gets the user data from all registrations that have user data for
-  // |key_prefix| where |key_prefix| is a prefix of keys. Returns a map from
-  // registration IDs to their values.
+  // |key_prefix| where |key_prefix| is a prefix of keys.
   GetUserDataForAllRegistrationsByKeyPrefix(string key_prefix) =>
-      (ServiceWorkerDatabaseStatus status, map<int64, string> values);
+      (ServiceWorkerDatabaseStatus status,
+       array<ServiceWorkerUserData> values);
   // Clears the user data from all registrations using |key_prefix| as a prefix
   // of keys.
   ClearUserDataForAllRegistrationsByKeyPrefix(string key_prefix) =>

content/browser/service_worker/service_worker_database.cc

@@ -1111,7 +1111,7 @@ ServiceWorkerDatabase::Status ServiceWorkerDatabase::RewriteDB() {
 ServiceWorkerDatabase::Status
 ServiceWorkerDatabase::ReadUserDataForAllRegistrations(
     const std::string& user_data_name,
-    std::vector<std::pair<int64_t, std::string>>* user_data) {
+    std::vector<storage::mojom::ServiceWorkerUserDataPtr>* user_data) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(user_data->empty());
 
@@ -1153,7 +1153,8 @@ ServiceWorkerDatabase::ReadUserDataForAllRegistrations(
         user_data->clear();
         break;
       }
-      user_data->push_back(std::make_pair(registration_id, value));
+      user_data->emplace_back(storage::mojom::ServiceWorkerUserData::New(
+          registration_id, user_data_name, value));
     }
   }
 
@@ -1164,7 +1165,7 @@ ServiceWorkerDatabase::ReadUserDataForAllRegistrations(
 ServiceWorkerDatabase::Status
 ServiceWorkerDatabase::ReadUserDataForAllRegistrationsByKeyPrefix(
     const std::string& user_data_name_prefix,
-    std::vector<std::pair<int64_t, std::string>>* user_data) {
+    std::vector<storage::mojom::ServiceWorkerUserDataPtr>* user_data) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(user_data->empty());
 
@@ -1221,7 +1222,8 @@ ServiceWorkerDatabase::ReadUserDataForAllRegistrationsByKeyPrefix(
         user_data->clear();
         break;
       }
-      user_data->push_back(std::make_pair(registration_id, value));
+      user_data->push_back(storage::mojom::ServiceWorkerUserData::New(
+          registration_id, parts[0], value));
     }
   }
 

content/browser/service_worker/service_worker_database.h

@@ -217,14 +217,14 @@ class CONTENT_EXPORT ServiceWorkerDatabase {
   // from the database. Returns OK if they are successfully read or not found.
   Status ReadUserDataForAllRegistrations(
       const std::string& user_data_name,
-      std::vector<std::pair<int64_t, std::string>>* user_data);
+      std::vector<storage::mojom::ServiceWorkerUserDataPtr>* user_data);
 
   // Reads user data for all registrations that have data with
   // |user_data_name_prefix| from the database. Returns OK if they are
   // successfully read or not found.
   Status ReadUserDataForAllRegistrationsByKeyPrefix(
       const std::string& user_data_name_prefix,
-      std::vector<std::pair<int64_t, std::string>>* user_data);
+      std::vector<storage::mojom::ServiceWorkerUserDataPtr>* user_data);
 
   // Deletes user data for all registrations that have data with
   // |user_data_name_prefix| from the database. Returns OK if all are

content/browser/service_worker/service_worker_registry.cc

@@ -616,8 +616,8 @@ void ServiceWorkerRegistry::StoreUserData(
                              blink::ServiceWorkerStatusCode::kErrorFailed));
       return;
     }
-    user_data.push_back(
-        storage::mojom::ServiceWorkerUserData::New(kv.first, kv.second));
+    user_data.push_back(storage::mojom::ServiceWorkerUserData::New(
+        registration_id, kv.first, kv.second));
   }
 
   GetRemoteStorageControl()->StoreUserData(
@@ -708,7 +708,7 @@ void ServiceWorkerRegistry::GetUserDataForAllRegistrations(
     return;
   }
 
-  storage()->GetUserDataForAllRegistrations(
+  GetRemoteStorageControl()->GetUserDataForAllRegistrations(
       key,
       base::BindOnce(&ServiceWorkerRegistry::DidGetUserDataForAllRegistrations,
                      weak_factory_.GetWeakPtr(), std::move(callback)));
@@ -726,7 +726,7 @@ void ServiceWorkerRegistry::GetUserDataForAllRegistrationsByKeyPrefix(
     return;
   }
 
-  storage()->GetUserDataForAllRegistrationsByKeyPrefix(
+  GetRemoteStorageControl()->GetUserDataForAllRegistrationsByKeyPrefix(
       key_prefix,
       base::BindOnce(&ServiceWorkerRegistry::DidGetUserDataForAllRegistrations,
                      weak_factory_.GetWeakPtr(), std::move(callback)));
@@ -1287,11 +1287,18 @@ void ServiceWorkerRegistry::DidClearUserData(
 
 void ServiceWorkerRegistry::DidGetUserDataForAllRegistrations(
     GetUserDataForAllRegistrationsCallback callback,
-    const std::vector<std::pair<int64_t, std::string>>& user_data,
-    storage::mojom::ServiceWorkerDatabaseStatus status) {
+    storage::mojom::ServiceWorkerDatabaseStatus status,
+    std::vector<storage::mojom::ServiceWorkerUserDataPtr> entries) {
   DCHECK_CURRENTLY_ON(ServiceWorkerContext::GetCoreThreadId());
+  // TODO(crbug.com/1055677): Update call sites of
+  // GetUserDataForAllRegistrations so that we can avoid converting mojo struct
+  // to a pair.
+  std::vector<std::pair<int64_t, std::string>> user_data;
   if (status != storage::mojom::ServiceWorkerDatabaseStatus::kOk)
     ScheduleDeleteAndStartOver();
+  for (auto& entry : entries) {
+    user_data.emplace_back(entry->registration_id, entry->value);
+  }
   std::move(callback).Run(user_data, DatabaseStatusToStatusCode(status));
 }
 

content/browser/service_worker/service_worker_registry.h

@@ -337,8 +337,8 @@ class CONTENT_EXPORT ServiceWorkerRegistry {
                         storage::mojom::ServiceWorkerDatabaseStatus status);
   void DidGetUserDataForAllRegistrations(
       GetUserDataForAllRegistrationsCallback callback,
-      const std::vector<std::pair<int64_t, std::string>>& user_data,
-      storage::mojom::ServiceWorkerDatabaseStatus status);
+      storage::mojom::ServiceWorkerDatabaseStatus status,
+      std::vector<storage::mojom::ServiceWorkerUserDataPtr> entries);
 
   void DidGetNewRegistrationId(
       blink::mojom::ServiceWorkerRegistrationOptions options,

content/browser/service_worker/service_worker_storage.cc

@@ -786,9 +786,10 @@ void ServiceWorkerStorage::GetUserDataForAllRegistrations(
   switch (state_) {
     case STORAGE_STATE_DISABLED:
       RunSoon(FROM_HERE,
-              base::BindOnce(std::move(callback),
-                             std::vector<std::pair<int64_t, std::string>>(),
-                             ServiceWorkerDatabase::Status::kErrorDisabled));
+              base::BindOnce(
+                  std::move(callback),
+                  ServiceWorkerDatabase::Status::kErrorDisabled,
+                  std::vector<storage::mojom::ServiceWorkerUserDataPtr>()));
       return;
     case STORAGE_STATE_INITIALIZING:  // Fall-through.
     case STORAGE_STATE_UNINITIALIZED:
@@ -817,9 +818,10 @@ void ServiceWorkerStorage::GetUserDataForAllRegistrationsByKeyPrefix(
   switch (state_) {
     case STORAGE_STATE_DISABLED:
       RunSoon(FROM_HERE,
-              base::BindOnce(std::move(callback),
-                             std::vector<std::pair<int64_t, std::string>>(),
-                             ServiceWorkerDatabase::Status::kErrorDisabled));
+              base::BindOnce(
+                  std::move(callback),
+                  ServiceWorkerDatabase::Status::kErrorDisabled,
+                  std::vector<storage::mojom::ServiceWorkerUserDataPtr>()));
       return;
     case STORAGE_STATE_INITIALIZING:  // Fall-through.
     case STORAGE_STATE_UNINITIALIZED:
@@ -1607,11 +1609,12 @@ void ServiceWorkerStorage::GetUserDataForAllRegistrationsInDB(
     scoped_refptr<base::SequencedTaskRunner> original_task_runner,
     const std::string& key,
     GetUserDataForAllRegistrationsInDBCallback callback) {
-  std::vector<std::pair<int64_t, std::string>> user_data;
+  std::vector<storage::mojom::ServiceWorkerUserDataPtr> user_data;
   ServiceWorkerDatabase::Status status =
       database->ReadUserDataForAllRegistrations(key, &user_data);
   original_task_runner->PostTask(
-      FROM_HERE, base::BindOnce(std::move(callback), user_data, status));
+      FROM_HERE,
+      base::BindOnce(std::move(callback), status, std::move(user_data)));
 }
 
 void ServiceWorkerStorage::GetUserDataForAllRegistrationsByKeyPrefixInDB(
@@ -1619,12 +1622,13 @@ void ServiceWorkerStorage::GetUserDataForAllRegistrationsByKeyPrefixInDB(
     scoped_refptr<base::SequencedTaskRunner> original_task_runner,
     const std::string& key_prefix,
     GetUserDataForAllRegistrationsInDBCallback callback) {
-  std::vector<std::pair<int64_t, std::string>> user_data;
+  std::vector<storage::mojom::ServiceWorkerUserDataPtr> user_data;
   ServiceWorkerDatabase::Status status =
       database->ReadUserDataForAllRegistrationsByKeyPrefix(key_prefix,
                                                            &user_data);
   original_task_runner->PostTask(
-      FROM_HERE, base::BindOnce(std::move(callback), user_data, status));
+      FROM_HERE,
+      base::BindOnce(std::move(callback), status, std::move(user_data)));
 }
 
 void ServiceWorkerStorage::DeleteAllDataForOriginsFromDB(

content/browser/service_worker/service_worker_storage.h

@@ -107,8 +107,8 @@ class CONTENT_EXPORT ServiceWorkerStorage {
   using GetUserKeysAndDataInDBCallback = storage::mojom::
       ServiceWorkerStorageControl::GetUserKeysAndDataByKeyPrefixCallback;
   using GetUserDataForAllRegistrationsInDBCallback = base::OnceCallback<void(
-      const std::vector<std::pair<int64_t, std::string>>& user_data,
-      ServiceWorkerDatabase::Status)>;
+      ServiceWorkerDatabase::Status,
+      std::vector<storage::mojom::ServiceWorkerUserDataPtr>)>;
 
   ~ServiceWorkerStorage();
 

content/browser/service_worker/service_worker_storage_control_impl.cc

@@ -13,20 +13,6 @@ namespace content {
 
 namespace {
 
-void DidGetUserDataForAllRegistrations(
-    ServiceWorkerStorageControlImpl::GetUserDataForAllRegistrationsCallback
-        callback,
-    const std::vector<std::pair<int64_t, std::string>>& user_data,
-    storage::mojom::ServiceWorkerDatabaseStatus status) {
-  // TODO(bashi): Change ServiceWorkerStorage::GetUserDataForAllRegistrations()
-  // to return base::flat_map.
-  base::flat_map<int64_t, std::string> values;
-  for (auto& entry : user_data) {
-    values[entry.first] = entry.second;
-  }
-  std::move(callback).Run(status, std::move(values));
-}
-
 void DidGetAllRegistrations(
     ServiceWorkerStorageControlImpl::GetAllRegistrationsDeprecatedCallback
         callback,
@@ -357,17 +343,14 @@ void ServiceWorkerStorageControlImpl::ClearUserDataByKeyPrefixes(
 void ServiceWorkerStorageControlImpl::GetUserDataForAllRegistrations(
     const std::string& key,
     GetUserDataForAllRegistrationsCallback callback) {
-  storage_->GetUserDataForAllRegistrations(
-      key,
-      base::BindOnce(&DidGetUserDataForAllRegistrations, std::move(callback)));
+  storage_->GetUserDataForAllRegistrations(key, std::move(callback));
 }
 
 void ServiceWorkerStorageControlImpl::GetUserDataForAllRegistrationsByKeyPrefix(
     const std::string& key_prefix,
     GetUserDataForAllRegistrationsByKeyPrefixCallback callback) {
-  storage_->GetUserDataForAllRegistrationsByKeyPrefix(
-      key_prefix,
-      base::BindOnce(&DidGetUserDataForAllRegistrations, std::move(callback)));
+  storage_->GetUserDataForAllRegistrationsByKeyPrefix(key_prefix,
+                                                      std::move(callback));
 }
 
 void ServiceWorkerStorageControlImpl::

content/browser/service_worker/service_worker_storage_control_impl_unittest.cc

@@ -84,7 +84,7 @@ struct GetUserKeysAndDataByKeyPrefixResult {
 
 struct GetUserDataForAllRegistrationsResult {
   DatabaseStatus status;
-  base::flat_map<int64_t, std::string> values;
+  std::vector<storage::mojom::ServiceWorkerUserDataPtr> values;
 };
 
 ReadResponseHeadResult ReadResponseHead(
@@ -536,13 +536,14 @@ class ServiceWorkerStorageControlImplTest : public testing::Test {
     GetUserDataForAllRegistrationsResult result;
     base::RunLoop loop;
     storage()->GetUserDataForAllRegistrations(
-        key, base::BindLambdaForTesting(
-                 [&](DatabaseStatus status,
-                     const base::flat_map<int64_t, std::string>& values) {
-                   result.status = status;
-                   result.values = values;
-                   loop.Quit();
-                 }));
+        key,
+        base::BindLambdaForTesting(
+            [&](DatabaseStatus status,
+                std::vector<storage::mojom::ServiceWorkerUserDataPtr> values) {
+              result.status = status;
+              result.values = std::move(values);
+              loop.Quit();
+            }));
     loop.Run();
     return result;
   }
@@ -555,9 +556,9 @@ class ServiceWorkerStorageControlImplTest : public testing::Test {
         key_prefix,
         base::BindLambdaForTesting(
             [&](DatabaseStatus status,
-                const base::flat_map<int64_t, std::string>& values) {
+                std::vector<storage::mojom::ServiceWorkerUserDataPtr> values) {
               result.status = status;
-              result.values = values;
+              result.values = std::move(values);
               loop.Quit();
             }));
     loop.Run();
@@ -1162,10 +1163,10 @@ TEST_F(ServiceWorkerStorageControlImplTest, StoreAndGetUserData) {
   // Store user data with two entries.
   {
     std::vector<storage::mojom::ServiceWorkerUserDataPtr> user_data;
-    user_data.push_back(
-        storage::mojom::ServiceWorkerUserData::New("key1", "value1"));
-    user_data.push_back(
-        storage::mojom::ServiceWorkerUserData::New("key2", "value2"));
+    user_data.push_back(storage::mojom::ServiceWorkerUserData::New(
+        registration_id, "key1", "value1"));
+    user_data.push_back(storage::mojom::ServiceWorkerUserData::New(
+        registration_id, "key2", "value2"));
 
     status = StoreUserData(registration_id, kScope.GetOrigin(),
                            std::move(user_data));
@@ -1252,14 +1253,14 @@ TEST_F(ServiceWorkerStorageControlImplTest, StoreAndGetUserDataByKeyPrefix) {
 
   // Store some user data with prefixes.
   std::vector<storage::mojom::ServiceWorkerUserDataPtr> user_data;
-  user_data.push_back(
-      storage::mojom::ServiceWorkerUserData::New("prefixA", "value1"));
-  user_data.push_back(
-      storage::mojom::ServiceWorkerUserData::New("prefixA2", "value2"));
-  user_data.push_back(
-      storage::mojom::ServiceWorkerUserData::New("prefixB", "value3"));
-  user_data.push_back(
-      storage::mojom::ServiceWorkerUserData::New("prefixC", "value4"));
+  user_data.push_back(storage::mojom::ServiceWorkerUserData::New(
+      registration_id, "prefixA", "value1"));
+  user_data.push_back(storage::mojom::ServiceWorkerUserData::New(
+      registration_id, "prefixA2", "value2"));
+  user_data.push_back(storage::mojom::ServiceWorkerUserData::New(
+      registration_id, "prefixB", "value3"));
+  user_data.push_back(storage::mojom::ServiceWorkerUserData::New(
+      registration_id, "prefixC", "value4"));
   status =
       StoreUserData(registration_id, kScope.GetOrigin(), std::move(user_data));
   ASSERT_EQ(status, DatabaseStatus::kOk);
@@ -1340,11 +1341,11 @@ TEST_F(ServiceWorkerStorageControlImplTest,
   {
     std::vector<storage::mojom::ServiceWorkerUserDataPtr> user_data;
     user_data.push_back(storage::mojom::ServiceWorkerUserData::New(
-        "key1", "registration1_value1"));
+        registration_id1, "key1", "registration1_value1"));
     user_data.push_back(storage::mojom::ServiceWorkerUserData::New(
-        "key2", "registration1_value2"));
+        registration_id1, "key2", "registration1_value2"));
     user_data.push_back(storage::mojom::ServiceWorkerUserData::New(
-        "prefix1", "registration1_prefix_value1"));
+        registration_id1, "prefix1", "registration1_prefix_value1"));
     status = StoreUserData(registration_id1, kScope1.GetOrigin(),
                            std::move(user_data));
     ASSERT_EQ(status, DatabaseStatus::kOk);
@@ -1352,11 +1353,11 @@ TEST_F(ServiceWorkerStorageControlImplTest,
   {
     std::vector<storage::mojom::ServiceWorkerUserDataPtr> user_data;
     user_data.push_back(storage::mojom::ServiceWorkerUserData::New(
-        "key1", "registration2_value1"));
+        registration_id1, "key1", "registration2_value1"));
     user_data.push_back(storage::mojom::ServiceWorkerUserData::New(
-        "key3", "registration2_value3"));
+        registration_id1, "key3", "registration2_value3"));
     user_data.push_back(storage::mojom::ServiceWorkerUserData::New(
-        "prefix2", "registration2_prefix_value2"));
+        registration_id1, "prefix2", "registration2_prefix_value2"));
     status = StoreUserData(registration_id2, kScope2.GetOrigin(),
                            std::move(user_data));
     ASSERT_EQ(status, DatabaseStatus::kOk);
@@ -1367,19 +1368,23 @@ TEST_F(ServiceWorkerStorageControlImplTest,
   result = GetUserDataForAllRegistrations("key1");
   ASSERT_EQ(result.status, DatabaseStatus::kOk);
   ASSERT_EQ(result.values.size(), 2UL);
-  EXPECT_EQ(result.values[registration_id1], "registration1_value1");
-  EXPECT_EQ(result.values[registration_id2], "registration2_value1");
+  EXPECT_EQ(result.values[0]->registration_id, registration_id1);
+  EXPECT_EQ(result.values[0]->value, "registration1_value1");
+  EXPECT_EQ(result.values[1]->registration_id, registration_id2);
+  EXPECT_EQ(result.values[1]->value, "registration2_value1");
 
   // Get uncommon user data.
   result = GetUserDataForAllRegistrations("key2");
   ASSERT_EQ(result.status, DatabaseStatus::kOk);
   ASSERT_EQ(result.values.size(), 1UL);
-  EXPECT_EQ(result.values[registration_id1], "registration1_value2");
+  EXPECT_EQ(result.values[0]->registration_id, registration_id1);
+  EXPECT_EQ(result.values[0]->value, "registration1_value2");
 
   result = GetUserDataForAllRegistrations("key3");
   ASSERT_EQ(result.status, DatabaseStatus::kOk);
   ASSERT_EQ(result.values.size(), 1UL);
-  EXPECT_EQ(result.values[registration_id2], "registration2_value3");
+  EXPECT_EQ(result.values[0]->registration_id, registration_id2);
+  EXPECT_EQ(result.values[0]->value, "registration2_value3");
 
   // Getting unknown key succeeds but returns an empty value.
   // TODO(bashi): Make sure this is an intentional behavior. The existing
@@ -1395,14 +1400,17 @@ TEST_F(ServiceWorkerStorageControlImplTest,
   result = GetUserDataForAllRegistrations("key1");
   ASSERT_EQ(result.status, DatabaseStatus::kOk);
   ASSERT_EQ(result.values.size(), 1UL);
-  EXPECT_EQ(result.values[registration_id2], "registration2_value1");
+  EXPECT_EQ(result.values[0]->registration_id, registration_id2);
+  EXPECT_EQ(result.values[0]->value, "registration2_value1");
 
   // Get prefixed user data.
   result = GetUserDataForAllRegistrationsByKeyPrefix("prefix");
   ASSERT_EQ(result.status, DatabaseStatus::kOk);
   ASSERT_EQ(result.values.size(), 2UL);
-  EXPECT_EQ(result.values[registration_id1], "registration1_prefix_value1");
-  EXPECT_EQ(result.values[registration_id2], "registration2_prefix_value2");
+  EXPECT_EQ(result.values[0]->registration_id, registration_id1);
+  EXPECT_EQ(result.values[0]->value, "registration1_prefix_value1");
+  EXPECT_EQ(result.values[1]->registration_id, registration_id2);
+  EXPECT_EQ(result.values[1]->value, "registration2_prefix_value2");
 
   // Clear prefixed user data.
   status = ClearUserDataForAllRegistrationsByKeyPrefix("prefix");