Revert "service worker: Use mojo version of GetUserDataForAllRegistrations"

This reverts commit 853e0dd9.

Reason for revert: Change seems to have caused consistent unit_tests
failures on the TSAN bot, see
https://bugs.chromium.org/p/chromium/issues/detail?id=1112825

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}

TBR=falken@chromium.org,kinuko@chromium.org,bashi@chromium.org,nhiroki@chromium.org

Change-Id: If41653b493ad11a6af679ee000a207be5b65f73b
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1055677, 1112825
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2337273
Commit-Queue: Rebekah Potter <rbpotter@chromium.org>
Reviewed-by: Rebekah Potter <rbpotter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#794695}

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

@@ -93,7 +93,6 @@ 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;
 };
@@ -249,14 +248,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,
-       array<ServiceWorkerUserData> values);
+      (ServiceWorkerDatabaseStatus status, map<int64, string> values);
   // Gets the user data from all registrations that have user data for
-  // |key_prefix| where |key_prefix| is a prefix of keys.
+  // |key_prefix| where |key_prefix| is a prefix of keys. Returns a map from
+  // registration IDs to their values.
   GetUserDataForAllRegistrationsByKeyPrefix(string key_prefix) =>
-      (ServiceWorkerDatabaseStatus status,
-       array<ServiceWorkerUserData> values);
+      (ServiceWorkerDatabaseStatus status, map<int64, string> 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<storage::mojom::ServiceWorkerUserDataPtr>* user_data) {
+    std::vector<std::pair<int64_t, std::string>>* user_data) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(user_data->empty());
 
@@ -1153,8 +1153,7 @@ ServiceWorkerDatabase::ReadUserDataForAllRegistrations(
         user_data->clear();
         break;
       }
-      user_data->emplace_back(storage::mojom::ServiceWorkerUserData::New(
-          registration_id, user_data_name, value));
+      user_data->push_back(std::make_pair(registration_id, value));
     }
   }
 
@@ -1165,7 +1164,7 @@ ServiceWorkerDatabase::ReadUserDataForAllRegistrations(
 ServiceWorkerDatabase::Status
 ServiceWorkerDatabase::ReadUserDataForAllRegistrationsByKeyPrefix(
     const std::string& user_data_name_prefix,
-    std::vector<storage::mojom::ServiceWorkerUserDataPtr>* user_data) {
+    std::vector<std::pair<int64_t, std::string>>* user_data) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(user_data->empty());
 
@@ -1222,8 +1221,7 @@ ServiceWorkerDatabase::ReadUserDataForAllRegistrationsByKeyPrefix(
         user_data->clear();
         break;
       }
-      user_data->push_back(storage::mojom::ServiceWorkerUserData::New(
-          registration_id, parts[0], value));
+      user_data->push_back(std::make_pair(registration_id, 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<storage::mojom::ServiceWorkerUserDataPtr>* user_data);
+      std::vector<std::pair<int64_t, std::string>>* 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<storage::mojom::ServiceWorkerUserDataPtr>* user_data);
+      std::vector<std::pair<int64_t, std::string>>* 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

@@ -625,8 +625,8 @@ void ServiceWorkerRegistry::StoreUserData(
                              blink::ServiceWorkerStatusCode::kErrorFailed));
       return;
     }
-    user_data.push_back(storage::mojom::ServiceWorkerUserData::New(
-        registration_id, kv.first, kv.second));
+    user_data.push_back(
+        storage::mojom::ServiceWorkerUserData::New(kv.first, kv.second));
   }
 
   GetRemoteStorageControl()->StoreUserData(
@@ -717,7 +717,7 @@ void ServiceWorkerRegistry::GetUserDataForAllRegistrations(
     return;
   }
 
-  GetRemoteStorageControl()->GetUserDataForAllRegistrations(
+  storage()->GetUserDataForAllRegistrations(
       key,
       base::BindOnce(&ServiceWorkerRegistry::DidGetUserDataForAllRegistrations,
                      weak_factory_.GetWeakPtr(), std::move(callback)));
@@ -735,7 +735,7 @@ void ServiceWorkerRegistry::GetUserDataForAllRegistrationsByKeyPrefix(
     return;
   }
 
-  GetRemoteStorageControl()->GetUserDataForAllRegistrationsByKeyPrefix(
+  storage()->GetUserDataForAllRegistrationsByKeyPrefix(
       key_prefix,
       base::BindOnce(&ServiceWorkerRegistry::DidGetUserDataForAllRegistrations,
                      weak_factory_.GetWeakPtr(), std::move(callback)));
@@ -1309,18 +1309,11 @@ void ServiceWorkerRegistry::DidClearUserData(
 
 void ServiceWorkerRegistry::DidGetUserDataForAllRegistrations(
     GetUserDataForAllRegistrationsCallback callback,
-    storage::mojom::ServiceWorkerDatabaseStatus status,
-    std::vector<storage::mojom::ServiceWorkerUserDataPtr> entries) {
+    const std::vector<std::pair<int64_t, std::string>>& user_data,
+    storage::mojom::ServiceWorkerDatabaseStatus status) {
   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

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

content/browser/service_worker/service_worker_storage.cc

@@ -768,10 +768,9 @@ void ServiceWorkerStorage::GetUserDataForAllRegistrations(
   switch (state_) {
     case STORAGE_STATE_DISABLED:
       RunSoon(FROM_HERE,
-              base::BindOnce(
-                  std::move(callback),
-                  ServiceWorkerDatabase::Status::kErrorDisabled,
-                  std::vector<storage::mojom::ServiceWorkerUserDataPtr>()));
+              base::BindOnce(std::move(callback),
+                             std::vector<std::pair<int64_t, std::string>>(),
+                             ServiceWorkerDatabase::Status::kErrorDisabled));
       return;
     case STORAGE_STATE_INITIALIZING:  // Fall-through.
     case STORAGE_STATE_UNINITIALIZED:
@@ -800,10 +799,9 @@ void ServiceWorkerStorage::GetUserDataForAllRegistrationsByKeyPrefix(
   switch (state_) {
     case STORAGE_STATE_DISABLED:
       RunSoon(FROM_HERE,
-              base::BindOnce(
-                  std::move(callback),
-                  ServiceWorkerDatabase::Status::kErrorDisabled,
-                  std::vector<storage::mojom::ServiceWorkerUserDataPtr>()));
+              base::BindOnce(std::move(callback),
+                             std::vector<std::pair<int64_t, std::string>>(),
+                             ServiceWorkerDatabase::Status::kErrorDisabled));
       return;
     case STORAGE_STATE_INITIALIZING:  // Fall-through.
     case STORAGE_STATE_UNINITIALIZED:
@@ -1558,12 +1556,11 @@ void ServiceWorkerStorage::GetUserDataForAllRegistrationsInDB(
     scoped_refptr<base::SequencedTaskRunner> original_task_runner,
     const std::string& key,
     GetUserDataForAllRegistrationsInDBCallback callback) {
-  std::vector<storage::mojom::ServiceWorkerUserDataPtr> user_data;
+  std::vector<std::pair<int64_t, std::string>> user_data;
   ServiceWorkerDatabase::Status status =
       database->ReadUserDataForAllRegistrations(key, &user_data);
   original_task_runner->PostTask(
-      FROM_HERE,
-      base::BindOnce(std::move(callback), status, std::move(user_data)));
+      FROM_HERE, base::BindOnce(std::move(callback), user_data, status));
 }
 
 void ServiceWorkerStorage::GetUserDataForAllRegistrationsByKeyPrefixInDB(
@@ -1571,13 +1568,12 @@ void ServiceWorkerStorage::GetUserDataForAllRegistrationsByKeyPrefixInDB(
     scoped_refptr<base::SequencedTaskRunner> original_task_runner,
     const std::string& key_prefix,
     GetUserDataForAllRegistrationsInDBCallback callback) {
-  std::vector<storage::mojom::ServiceWorkerUserDataPtr> user_data;
+  std::vector<std::pair<int64_t, std::string>> user_data;
   ServiceWorkerDatabase::Status status =
       database->ReadUserDataForAllRegistrationsByKeyPrefix(key_prefix,
                                                            &user_data);
   original_task_runner->PostTask(
-      FROM_HERE,
-      base::BindOnce(std::move(callback), status, std::move(user_data)));
+      FROM_HERE, base::BindOnce(std::move(callback), user_data, status));
 }
 
 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(
-      ServiceWorkerDatabase::Status,
-      std::vector<storage::mojom::ServiceWorkerUserDataPtr>)>;
+      const std::vector<std::pair<int64_t, std::string>>& user_data,
+      ServiceWorkerDatabase::Status)>;
 
   ~ServiceWorkerStorage();
 

content/browser/service_worker/service_worker_storage_control_impl.cc

@@ -13,6 +13,20 @@ 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,
@@ -332,14 +346,17 @@ void ServiceWorkerStorageControlImpl::ClearUserDataByKeyPrefixes(
 void ServiceWorkerStorageControlImpl::GetUserDataForAllRegistrations(
     const std::string& key,
     GetUserDataForAllRegistrationsCallback callback) {
-  storage_->GetUserDataForAllRegistrations(key, std::move(callback));
+  storage_->GetUserDataForAllRegistrations(
+      key,
+      base::BindOnce(&DidGetUserDataForAllRegistrations, std::move(callback)));
 }
 
 void ServiceWorkerStorageControlImpl::GetUserDataForAllRegistrationsByKeyPrefix(
     const std::string& key_prefix,
     GetUserDataForAllRegistrationsByKeyPrefixCallback callback) {
-  storage_->GetUserDataForAllRegistrationsByKeyPrefix(key_prefix,
-                                                      std::move(callback));
+  storage_->GetUserDataForAllRegistrationsByKeyPrefix(
+      key_prefix,
+      base::BindOnce(&DidGetUserDataForAllRegistrations, 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;
-  std::vector<storage::mojom::ServiceWorkerUserDataPtr> values;
+  base::flat_map<int64_t, std::string> values;
 };
 
 ReadResponseHeadResult ReadResponseHead(
@@ -537,14 +537,13 @@ class ServiceWorkerStorageControlImplTest : public testing::Test {
     GetUserDataForAllRegistrationsResult result;
     base::RunLoop loop;
     storage()->GetUserDataForAllRegistrations(
-        key,
-        base::BindLambdaForTesting(
-            [&](DatabaseStatus status,
-                std::vector<storage::mojom::ServiceWorkerUserDataPtr> values) {
-              result.status = status;
-              result.values = std::move(values);
-              loop.Quit();
-            }));
+        key, base::BindLambdaForTesting(
+                 [&](DatabaseStatus status,
+                     const base::flat_map<int64_t, std::string>& values) {
+                   result.status = status;
+                   result.values = values;
+                   loop.Quit();
+                 }));
     loop.Run();
     return result;
   }
@@ -557,9 +556,9 @@ class ServiceWorkerStorageControlImplTest : public testing::Test {
         key_prefix,
         base::BindLambdaForTesting(
             [&](DatabaseStatus status,
-                std::vector<storage::mojom::ServiceWorkerUserDataPtr> values) {
+                const base::flat_map<int64_t, std::string>& values) {
               result.status = status;
-              result.values = std::move(values);
+              result.values = values;
               loop.Quit();
             }));
     loop.Run();
@@ -1164,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(
-        registration_id, "key1", "value1"));
-    user_data.push_back(storage::mojom::ServiceWorkerUserData::New(
-        registration_id, "key2", "value2"));
+    user_data.push_back(
+        storage::mojom::ServiceWorkerUserData::New("key1", "value1"));
+    user_data.push_back(
+        storage::mojom::ServiceWorkerUserData::New("key2", "value2"));
 
     status = StoreUserData(registration_id, kScope.GetOrigin(),
                            std::move(user_data));
@@ -1254,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(
-      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"));
+  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"));
   status =
       StoreUserData(registration_id, kScope.GetOrigin(), std::move(user_data));
   ASSERT_EQ(status, DatabaseStatus::kOk);
@@ -1342,11 +1341,11 @@ TEST_F(ServiceWorkerStorageControlImplTest,
   {
     std::vector<storage::mojom::ServiceWorkerUserDataPtr> user_data;
     user_data.push_back(storage::mojom::ServiceWorkerUserData::New(
-        registration_id1, "key1", "registration1_value1"));
+        "key1", "registration1_value1"));
     user_data.push_back(storage::mojom::ServiceWorkerUserData::New(
-        registration_id1, "key2", "registration1_value2"));
+        "key2", "registration1_value2"));
     user_data.push_back(storage::mojom::ServiceWorkerUserData::New(
-        registration_id1, "prefix1", "registration1_prefix_value1"));
+        "prefix1", "registration1_prefix_value1"));
     status = StoreUserData(registration_id1, kScope1.GetOrigin(),
                            std::move(user_data));
     ASSERT_EQ(status, DatabaseStatus::kOk);
@@ -1354,11 +1353,11 @@ TEST_F(ServiceWorkerStorageControlImplTest,
   {
     std::vector<storage::mojom::ServiceWorkerUserDataPtr> user_data;
     user_data.push_back(storage::mojom::ServiceWorkerUserData::New(
-        registration_id1, "key1", "registration2_value1"));
+        "key1", "registration2_value1"));
     user_data.push_back(storage::mojom::ServiceWorkerUserData::New(
-        registration_id1, "key3", "registration2_value3"));
+        "key3", "registration2_value3"));
     user_data.push_back(storage::mojom::ServiceWorkerUserData::New(
-        registration_id1, "prefix2", "registration2_prefix_value2"));
+        "prefix2", "registration2_prefix_value2"));
     status = StoreUserData(registration_id2, kScope2.GetOrigin(),
                            std::move(user_data));
     ASSERT_EQ(status, DatabaseStatus::kOk);
@@ -1369,23 +1368,19 @@ TEST_F(ServiceWorkerStorageControlImplTest,
   result = GetUserDataForAllRegistrations("key1");
   ASSERT_EQ(result.status, DatabaseStatus::kOk);
   ASSERT_EQ(result.values.size(), 2UL);
-  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");
+  EXPECT_EQ(result.values[registration_id1], "registration1_value1");
+  EXPECT_EQ(result.values[registration_id2], "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[0]->registration_id, registration_id1);
-  EXPECT_EQ(result.values[0]->value, "registration1_value2");
+  EXPECT_EQ(result.values[registration_id1], "registration1_value2");
 
   result = GetUserDataForAllRegistrations("key3");
   ASSERT_EQ(result.status, DatabaseStatus::kOk);
   ASSERT_EQ(result.values.size(), 1UL);
-  EXPECT_EQ(result.values[0]->registration_id, registration_id2);
-  EXPECT_EQ(result.values[0]->value, "registration2_value3");
+  EXPECT_EQ(result.values[registration_id2], "registration2_value3");
 
   // Getting unknown key succeeds but returns an empty value.
   // TODO(bashi): Make sure this is an intentional behavior. The existing
@@ -1401,17 +1396,14 @@ TEST_F(ServiceWorkerStorageControlImplTest,
   result = GetUserDataForAllRegistrations("key1");
   ASSERT_EQ(result.status, DatabaseStatus::kOk);
   ASSERT_EQ(result.values.size(), 1UL);
-  EXPECT_EQ(result.values[0]->registration_id, registration_id2);
-  EXPECT_EQ(result.values[0]->value, "registration2_value1");
+  EXPECT_EQ(result.values[registration_id2], "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[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");
+  EXPECT_EQ(result.values[registration_id1], "registration1_prefix_value1");
+  EXPECT_EQ(result.values[registration_id2], "registration2_prefix_value2");
 
   // Clear prefixed user data.
   status = ClearUserDataForAllRegistrationsByKeyPrefix("prefix");