Support retry for service worker user data methods

This CL is similar to crrev.com/c/2550108 but for user data methods,
except for StoreUserData(). StoreUserData() has a move-only parameter
and needs to be specialized.

This CL also updates some browser_tests to wait for all inflight tasks
are executed before finishing tests. These changes are needed to
satisfy the DCHECK() in generated mojo bindings.


Bug: 1133143
Change-Id: Ia234eb4b84098f7a42450dfda1e70d9f34f9fe5f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2561963
Commit-Queue: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#833140}

content/browser/service_worker/service_worker_registry.cc

@@ -588,10 +588,11 @@ void ServiceWorkerRegistry::GetUserData(int64_t registration_id,
                                         const std::vector<std::string>& keys,
                                         GetUserDataCallback callback) {
   DCHECK_CURRENTLY_ON(ServiceWorkerContext::GetCoreThreadId());
-  GetRemoteStorageControl()->GetUserData(
-      registration_id, keys,
-      base::BindOnce(&ServiceWorkerRegistry::DidGetUserData,
-                     weak_factory_.GetWeakPtr(), std::move(callback)));
+  CreateInvokerAndStartRemoteCall(
+      &storage::mojom::ServiceWorkerStorageControl::GetUserData,
+      base::BindRepeating(&ServiceWorkerRegistry::DidGetUserData,
+                          weak_factory_.GetWeakPtr(), base::Passed(&callback)),
+      static_cast<const int64_t>(registration_id), keys);
 }
 
 void ServiceWorkerRegistry::GetUserDataByKeyPrefix(
@@ -599,10 +600,11 @@ void ServiceWorkerRegistry::GetUserDataByKeyPrefix(
     const std::string& key_prefix,
     GetUserDataCallback callback) {
   DCHECK_CURRENTLY_ON(ServiceWorkerContext::GetCoreThreadId());
-  GetRemoteStorageControl()->GetUserDataByKeyPrefix(
-      registration_id, key_prefix,
-      base::BindOnce(&ServiceWorkerRegistry::DidGetUserData,
-                     weak_factory_.GetWeakPtr(), std::move(callback)));
+  CreateInvokerAndStartRemoteCall(
+      &storage::mojom::ServiceWorkerStorageControl::GetUserDataByKeyPrefix,
+      base::BindRepeating(&ServiceWorkerRegistry::DidGetUserData,
+                          weak_factory_.GetWeakPtr(), base::Passed(&callback)),
+      static_cast<const int64_t>(registration_id), key_prefix);
 }
 
 void ServiceWorkerRegistry::GetUserKeysAndDataByKeyPrefix(
@@ -610,10 +612,12 @@ void ServiceWorkerRegistry::GetUserKeysAndDataByKeyPrefix(
     const std::string& key_prefix,
     GetUserKeysAndDataCallback callback) {
   DCHECK_CURRENTLY_ON(ServiceWorkerContext::GetCoreThreadId());
-  GetRemoteStorageControl()->GetUserKeysAndDataByKeyPrefix(
-      registration_id, key_prefix,
-      base::BindOnce(&ServiceWorkerRegistry::DidGetUserKeysAndData,
-                     weak_factory_.GetWeakPtr(), std::move(callback)));
+  CreateInvokerAndStartRemoteCall(
+      &storage::mojom::ServiceWorkerStorageControl::
+          GetUserKeysAndDataByKeyPrefix,
+      base::BindRepeating(&ServiceWorkerRegistry::DidGetUserKeysAndData,
+                          weak_factory_.GetWeakPtr(), base::Passed(&callback)),
+      static_cast<const int64_t>(registration_id), key_prefix);
 }
 
 void ServiceWorkerRegistry::StoreUserData(
@@ -648,10 +652,11 @@ void ServiceWorkerRegistry::ClearUserData(int64_t registration_id,
                                           const std::vector<std::string>& keys,
                                           StatusCallback callback) {
   DCHECK_CURRENTLY_ON(ServiceWorkerContext::GetCoreThreadId());
-  GetRemoteStorageControl()->ClearUserData(
-      registration_id, keys,
-      base::BindOnce(&ServiceWorkerRegistry::DidClearUserData,
-                     weak_factory_.GetWeakPtr(), std::move(callback)));
+  CreateInvokerAndStartRemoteCall(
+      &storage::mojom::ServiceWorkerStorageControl::ClearUserData,
+      base::BindRepeating(&ServiceWorkerRegistry::DidClearUserData,
+                          weak_factory_.GetWeakPtr(), base::Passed(&callback)),
+      static_cast<const int64_t>(registration_id), keys);
 }
 
 void ServiceWorkerRegistry::ClearUserDataByKeyPrefixes(
@@ -659,40 +664,49 @@ void ServiceWorkerRegistry::ClearUserDataByKeyPrefixes(
     const std::vector<std::string>& key_prefixes,
     StatusCallback callback) {
   DCHECK_CURRENTLY_ON(ServiceWorkerContext::GetCoreThreadId());
-  GetRemoteStorageControl()->ClearUserDataByKeyPrefixes(
-      registration_id, key_prefixes,
-      base::BindOnce(&ServiceWorkerRegistry::DidClearUserData,
-                     weak_factory_.GetWeakPtr(), std::move(callback)));
+  CreateInvokerAndStartRemoteCall(
+      &storage::mojom::ServiceWorkerStorageControl::ClearUserDataByKeyPrefixes,
+      base::BindRepeating(&ServiceWorkerRegistry::DidClearUserData,
+                          weak_factory_.GetWeakPtr(), base::Passed(&callback)),
+      static_cast<const int64_t>(registration_id), key_prefixes);
 }
 
 void ServiceWorkerRegistry::ClearUserDataForAllRegistrationsByKeyPrefix(
     const std::string& key_prefix,
     StatusCallback callback) {
   DCHECK_CURRENTLY_ON(ServiceWorkerContext::GetCoreThreadId());
-  GetRemoteStorageControl()->ClearUserDataForAllRegistrationsByKeyPrefix(
-      key_prefix,
-      base::BindOnce(&ServiceWorkerRegistry::DidClearUserData,
-                     weak_factory_.GetWeakPtr(), std::move(callback)));
+  CreateInvokerAndStartRemoteCall(
+      &storage::mojom::ServiceWorkerStorageControl::
+          ClearUserDataForAllRegistrationsByKeyPrefix,
+      base::BindRepeating(&ServiceWorkerRegistry::DidClearUserData,
+                          weak_factory_.GetWeakPtr(), base::Passed(&callback)),
+      key_prefix);
 }
 
 void ServiceWorkerRegistry::GetUserDataForAllRegistrations(
     const std::string& key,
     GetUserDataForAllRegistrationsCallback callback) {
   DCHECK_CURRENTLY_ON(ServiceWorkerContext::GetCoreThreadId());
-  GetRemoteStorageControl()->GetUserDataForAllRegistrations(
-      key,
-      base::BindOnce(&ServiceWorkerRegistry::DidGetUserDataForAllRegistrations,
-                     weak_factory_.GetWeakPtr(), std::move(callback)));
+  CreateInvokerAndStartRemoteCall(
+      &storage::mojom::ServiceWorkerStorageControl::
+          GetUserDataForAllRegistrations,
+      base::BindRepeating(
+          &ServiceWorkerRegistry::DidGetUserDataForAllRegistrations,
+          weak_factory_.GetWeakPtr(), base::Passed(&callback)),
+      key);
 }
 
 void ServiceWorkerRegistry::GetUserDataForAllRegistrationsByKeyPrefix(
     const std::string& key_prefix,
     GetUserDataForAllRegistrationsCallback callback) {
   DCHECK_CURRENTLY_ON(ServiceWorkerContext::GetCoreThreadId());
-  GetRemoteStorageControl()->GetUserDataForAllRegistrationsByKeyPrefix(
-      key_prefix,
-      base::BindOnce(&ServiceWorkerRegistry::DidGetUserDataForAllRegistrations,
-                     weak_factory_.GetWeakPtr(), std::move(callback)));
+  CreateInvokerAndStartRemoteCall(
+      &storage::mojom::ServiceWorkerStorageControl::
+          GetUserDataForAllRegistrationsByKeyPrefix,
+      base::BindRepeating(
+          &ServiceWorkerRegistry::DidGetUserDataForAllRegistrations,
+          weak_factory_.GetWeakPtr(), base::Passed(&callback)),
+      key_prefix);
 }
 
 void ServiceWorkerRegistry::GetRegisteredOrigins(
@@ -1293,8 +1307,11 @@ void ServiceWorkerRegistry::DidDoomUncommittedResourceIds(
 
 void ServiceWorkerRegistry::DidGetUserData(
     GetUserDataCallback callback,
+    uint64_t call_id,
     storage::mojom::ServiceWorkerDatabaseStatus status,
     const std::vector<std::string>& data) {
+  DCHECK_CURRENTLY_ON(ServiceWorkerContext::GetCoreThreadId());
+  FinishRemoteCall(call_id);
   if (status != storage::mojom::ServiceWorkerDatabaseStatus::kOk &&
       status != storage::mojom::ServiceWorkerDatabaseStatus::kErrorNotFound) {
     ScheduleDeleteAndStartOver();
@@ -1304,9 +1321,11 @@ void ServiceWorkerRegistry::DidGetUserData(
 
 void ServiceWorkerRegistry::DidGetUserKeysAndData(
     GetUserKeysAndDataCallback callback,
+    uint64_t call_id,
     storage::mojom::ServiceWorkerDatabaseStatus status,
     const base::flat_map<std::string, std::string>& data_map) {
   DCHECK_CURRENTLY_ON(ServiceWorkerContext::GetCoreThreadId());
+  FinishRemoteCall(call_id);
   if (status != storage::mojom::ServiceWorkerDatabaseStatus::kOk &&
       status != storage::mojom::ServiceWorkerDatabaseStatus::kErrorNotFound) {
     ScheduleDeleteAndStartOver();
@@ -1330,8 +1349,10 @@ void ServiceWorkerRegistry::DidStoreUserData(
 
 void ServiceWorkerRegistry::DidClearUserData(
     StatusCallback callback,
+    uint64_t call_id,
     storage::mojom::ServiceWorkerDatabaseStatus status) {
   DCHECK_CURRENTLY_ON(ServiceWorkerContext::GetCoreThreadId());
+  FinishRemoteCall(call_id);
   if (status != storage::mojom::ServiceWorkerDatabaseStatus::kOk)
     ScheduleDeleteAndStartOver();
   std::move(callback).Run(DatabaseStatusToStatusCode(status));
@@ -1339,9 +1360,11 @@ void ServiceWorkerRegistry::DidClearUserData(
 
 void ServiceWorkerRegistry::DidGetUserDataForAllRegistrations(
     GetUserDataForAllRegistrationsCallback callback,
+    uint64_t call_id,
     storage::mojom::ServiceWorkerDatabaseStatus status,
     std::vector<storage::mojom::ServiceWorkerUserDataPtr> entries) {
   DCHECK_CURRENTLY_ON(ServiceWorkerContext::GetCoreThreadId());
+  FinishRemoteCall(call_id);
   // TODO(crbug.com/1055677): Update call sites of
   // GetUserDataForAllRegistrations so that we can avoid converting mojo struct
   // to a pair.

content/browser/service_worker/service_worker_registry.h

@@ -346,18 +346,22 @@ class CONTENT_EXPORT ServiceWorkerRegistry {
       uint64_t call_id,
       storage::mojom::ServiceWorkerDatabaseStatus status);
   void DidGetUserData(GetUserDataCallback callback,
+                      uint64_t call_id,
                       storage::mojom::ServiceWorkerDatabaseStatus status,
                       const std::vector<std::string>& data);
   void DidGetUserKeysAndData(
       GetUserKeysAndDataCallback callback,
+      uint64_t call_id,
       storage::mojom::ServiceWorkerDatabaseStatus status,
       const base::flat_map<std::string, std::string>& data_map);
   void DidStoreUserData(StatusCallback callback,
                         storage::mojom::ServiceWorkerDatabaseStatus status);
   void DidClearUserData(StatusCallback callback,
+                        uint64_t call_id,
                         storage::mojom::ServiceWorkerDatabaseStatus status);
   void DidGetUserDataForAllRegistrations(
       GetUserDataForAllRegistrationsCallback callback,
+      uint64_t call_id,
       storage::mojom::ServiceWorkerDatabaseStatus status,
       std::vector<storage::mojom::ServiceWorkerUserDataPtr> entries);
 

content/browser/service_worker/service_worker_registry_unittest.cc

@@ -1580,6 +1580,163 @@ TEST_F(ServiceWorkerRegistryTest,
   }
 }
 
+TEST_F(ServiceWorkerRegistryTest, RetryInflightCalls_UserData) {
+  // Prerequisite: Store two registrations.
+  const GURL kScope1("http://www.example.com/scope/");
+  const GURL kScriptUrl1("http://www.example.com/script.js");
+  const auto kOrigin1(url::Origin::Create(kScope1));
+  scoped_refptr<ServiceWorkerRegistration> registration1 =
+      CreateServiceWorkerRegistrationAndVersion(context(), kScope1, kScriptUrl1,
+                                                /*resource_id=*/1);
+  ASSERT_EQ(StoreRegistration(registration1, registration1->waiting_version()),
+            blink::ServiceWorkerStatusCode::kOk);
+
+  const GURL kScope2("http://www.example.com/scope/foo");
+  const GURL kScriptUrl2("http://www.example.com/fooscript.js");
+  const auto kOrigin2(url::Origin::Create(kScope2));
+  scoped_refptr<ServiceWorkerRegistration> registration2 =
+      CreateServiceWorkerRegistrationAndVersion(context(), kScope2, kScriptUrl2,
+                                                /*resource_id=*/2);
+  ASSERT_EQ(StoreRegistration(registration2, registration2->waiting_version()),
+            blink::ServiceWorkerStatusCode::kOk);
+
+  // Store some user data.
+  {
+    base::RunLoop loop1;
+    registry()->StoreUserData(
+        registration1->id(), kOrigin1,
+        {{"key1", "value1"}, {"prefixed_key1", "prefixed_value1"}},
+        base::BindLambdaForTesting([&](blink::ServiceWorkerStatusCode status) {
+          EXPECT_EQ(status, blink::ServiceWorkerStatusCode::kOk);
+          loop1.Quit();
+        }));
+
+    base::RunLoop loop2;
+    registry()->StoreUserData(
+        registration2->id(), kOrigin2,
+        {{"key2", "value2"}, {"prefixed_key2", "prefixed_value2"}},
+        base::BindLambdaForTesting([&](blink::ServiceWorkerStatusCode status) {
+          EXPECT_EQ(status, blink::ServiceWorkerStatusCode::kOk);
+          loop2.Quit();
+        }));
+
+    // TODO(crbug.com/1133143): Support retry for StoreRegistration() then
+    // simulate storage restart before running loops.
+
+    loop1.Run();
+    loop2.Run();
+  }
+
+  // Tests that get methods for `registration1` work.
+  {
+    base::RunLoop loop1;
+    registry()->GetUserData(
+        registration1->id(), {{"key1"}},
+        base::BindLambdaForTesting([&](const std::vector<std::string>& values,
+                                       blink::ServiceWorkerStatusCode status) {
+          EXPECT_EQ(status, blink::ServiceWorkerStatusCode::kOk);
+          EXPECT_EQ(values.size(), 1U);
+          loop1.Quit();
+        }));
+
+    base::RunLoop loop2;
+    registry()->GetUserDataByKeyPrefix(
+        registration1->id(), "prefixed",
+        base::BindLambdaForTesting([&](const std::vector<std::string>& values,
+                                       blink::ServiceWorkerStatusCode status) {
+          EXPECT_EQ(status, blink::ServiceWorkerStatusCode::kOk);
+          EXPECT_EQ(values.size(), 1U);
+          loop2.Quit();
+        }));
+
+    base::RunLoop loop3;
+    registry()->GetUserKeysAndDataByKeyPrefix(
+        registration1->id(), "prefixed",
+        base::BindLambdaForTesting(
+            [&](blink::ServiceWorkerStatusCode status,
+                const base::flat_map<std::string, std::string>& user_data) {
+              EXPECT_EQ(status, blink::ServiceWorkerStatusCode::kOk);
+              EXPECT_EQ(user_data.size(), 1U);
+              loop3.Quit();
+            }));
+
+    EXPECT_EQ(inflight_call_count(), 3U);
+    registry()->SimulateStorageRestartForTesting();
+
+    loop1.Run();
+    loop2.Run();
+    loop3.Run();
+    EXPECT_EQ(inflight_call_count(), 0U);
+  }
+
+  // Tests that get methods for all registrations work.
+  {
+    base::RunLoop loop1;
+    registry()->GetUserDataForAllRegistrations(
+        "key2",
+        base::BindLambdaForTesting(
+            [&](const std::vector<std::pair<int64_t, std::string>>& user_data,
+                blink::ServiceWorkerStatusCode status) {
+              EXPECT_EQ(status, blink::ServiceWorkerStatusCode::kOk);
+              EXPECT_EQ(user_data.size(), 1U);
+              loop1.Quit();
+            }));
+
+    base::RunLoop loop2;
+    registry()->GetUserDataForAllRegistrationsByKeyPrefix(
+        "prefixed",
+        base::BindLambdaForTesting(
+            [&](const std::vector<std::pair<int64_t, std::string>>& user_data,
+                blink::ServiceWorkerStatusCode status) {
+              EXPECT_EQ(status, blink::ServiceWorkerStatusCode::kOk);
+              EXPECT_EQ(user_data.size(), 2U);
+              loop2.Quit();
+            }));
+
+    EXPECT_EQ(inflight_call_count(), 2U);
+    registry()->SimulateStorageRestartForTesting();
+
+    loop1.Run();
+    loop2.Run();
+    EXPECT_EQ(inflight_call_count(), 0U);
+  }
+
+  // Tests that clear methods work.
+  {
+    base::RunLoop loop1;
+    registry()->ClearUserData(
+        registration1->id(), {{"key1"}},
+        base::BindLambdaForTesting([&](blink::ServiceWorkerStatusCode status) {
+          EXPECT_EQ(status, blink::ServiceWorkerStatusCode::kOk);
+          loop1.Quit();
+        }));
+
+    base::RunLoop loop2;
+    registry()->ClearUserDataByKeyPrefixes(
+        registration2->id(), {{"key2"}},
+        base::BindLambdaForTesting([&](blink::ServiceWorkerStatusCode status) {
+          EXPECT_EQ(status, blink::ServiceWorkerStatusCode::kOk);
+          loop2.Quit();
+        }));
+
+    base::RunLoop loop3;
+    registry()->ClearUserDataForAllRegistrationsByKeyPrefix(
+        "prefixed",
+        base::BindLambdaForTesting([&](blink::ServiceWorkerStatusCode status) {
+          EXPECT_EQ(status, blink::ServiceWorkerStatusCode::kOk);
+          loop3.Quit();
+        }));
+
+    EXPECT_EQ(inflight_call_count(), 3U);
+    registry()->SimulateStorageRestartForTesting();
+
+    loop1.Run();
+    loop2.Run();
+    loop3.Run();
+    EXPECT_EQ(inflight_call_count(), 0U);
+  }
+}
+
 TEST_F(ServiceWorkerRegistryTest, RetryInflightCalls_DeleteAndStartOver) {
   EnsureRemoteCallsAreExecuted();