Call ServiceWorker deletion callback early in case of error

ServiceWorker deletion consists of two steps: unregistration and
deletion. If there is an error during unregistration, the deletion
might not occur. To catch this error and avoid waiting infinitely
for deletion, the error is reported immediately, ignoring all
remaining expected calls.

Bug: 927474
Change-Id: Ic71c8957e9cb6b78f4e2a54c25d450d74ef771bc
Reviewed-on: https://chromium-review.googlesource.com/c/1454549
Commit-Queue: Christian Dullweber <dullweber@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#629526}

content/browser/service_worker/service_worker_context_core.cc

@@ -70,23 +70,23 @@ void CheckFetchHandlerOfInstalledServiceWorker(
           : ServiceWorkerCapability::SERVICE_WORKER_NO_FETCH_HANDLER);
 }
 
-void SuccessCollectorCallback(base::OnceClosure done_closure,
-                              bool* overall_success,
-                              blink::ServiceWorkerStatusCode status) {
+// This function will call |callback| after |*expected_calls| reaches zero or
+// when an error occurs. In case of an error, |*expected_calls| is set to -1
+// to prevent calling |callback| again.
+void SuccessReportingCallback(
+    int* expected_calls,
+    const base::RepeatingCallback<void(blink::ServiceWorkerStatusCode)>&
+        callback,
+    blink::ServiceWorkerStatusCode status) {
   if (status != blink::ServiceWorkerStatusCode::kOk) {
-    *overall_success = false;
+    *expected_calls = -1;
+    callback.Run(blink::ServiceWorkerStatusCode::kErrorFailed);
+    return;
+  }
+  (*expected_calls)--;
+  if (*expected_calls == 0) {
+    callback.Run(blink::ServiceWorkerStatusCode::kOk);
   }
-  std::move(done_closure).Run();
-}
-
-void SuccessReportingCallback(
-    const bool* success,
-    base::OnceCallback<void(blink::ServiceWorkerStatusCode)> callback) {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  bool result = *success;
-  std::move(callback).Run(result
-                              ? blink::ServiceWorkerStatusCode::kOk
-                              : blink::ServiceWorkerStatusCode::kErrorFailed);
 }
 
 bool IsSameOriginClientProviderHost(const GURL& origin,
@@ -512,24 +512,29 @@ void ServiceWorkerContextCore::DidGetRegistrationsForDeleteForOrigin(
     std::move(callback).Run(status);
     return;
   }
-  bool* overall_success = new bool(true);
+  if (registrations.empty()) {
+    std::move(callback).Run(blink::ServiceWorkerStatusCode::kOk);
+    return;
+  }
+
+  int* expected_calls = new int(2 * registrations.size());
   // The barrier must be executed twice for each registration: once for
-  // unregistration and once for deletion.
-  base::RepeatingClosure barrier = base::BarrierClosure(
-      2 * registrations.size(),
-      base::BindOnce(&SuccessReportingCallback, base::Owned(overall_success),
-                     std::move(callback)));
+  // unregistration and once for deletion. It will call |callback| immediately
+  // if an error occurs.
+  base::RepeatingCallback<void(blink::ServiceWorkerStatusCode)> barrier =
+      base::BindRepeating(SuccessReportingCallback, base::Owned(expected_calls),
+                          base::AdaptCallbackForRepeating(std::move(callback)));
   for (const auto& registration : registrations) {
     DCHECK(registration);
     if (!registration->is_deleted()) {
-      RegistrationDeletionListener::WaitForDeletion(registration, barrier);
+      RegistrationDeletionListener::WaitForDeletion(
+          registration,
+          base::BindOnce(barrier, blink::ServiceWorkerStatusCode::kOk));
     } else {
-      barrier.Run();
+      barrier.Run(blink::ServiceWorkerStatusCode::kOk);
     }
     job_coordinator_->Abort(registration->scope());
-    UnregisterServiceWorker(
-        registration->scope(),
-        base::BindOnce(&SuccessCollectorCallback, barrier, overall_success));
+    UnregisterServiceWorker(registration->scope(), barrier);
   }
 }
 

content/browser/service_worker/service_worker_context_core_unittest.cc

@@ -6,16 +6,23 @@
 
 #include <memory>
 
+#include "base/test/bind_test_util.h"
 #include "content/browser/service_worker/embedded_worker_test_helper.h"
+#include "content/browser/service_worker/service_worker_context_core.h"
+#include "content/browser/service_worker/service_worker_context_core_observer.h"
+#include "content/browser/service_worker/service_worker_context_wrapper.h"
+#include "content/browser/service_worker/service_worker_registration.h"
 #include "content/browser/service_worker/service_worker_test_utils.h"
+#include "content/browser/service_worker/service_worker_version.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "content/public/test/test_utils.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace content {
 
-class ServiceWorkerContextCoreTest : public testing::Test {
- protected:
+class ServiceWorkerContextCoreTest : public testing::Test,
+                                     public ServiceWorkerContextCoreObserver {
+ public:
   ServiceWorkerContextCoreTest()
       : thread_bundle_(TestBrowserThreadBundle::IO_MAINLOOP) {}
 
@@ -23,14 +30,107 @@ class ServiceWorkerContextCoreTest : public testing::Test {
     helper_.reset(new EmbeddedWorkerTestHelper(base::FilePath()));
   }
 
-  void TearDown() override { helper_.reset(); }
+  void TearDown() override {
+    if (is_observing_context_) {
+      helper_->context_wrapper()->RemoveObserver(this);
+      helper_.reset();
+    }
+  }
 
   ServiceWorkerContextCore* context() { return helper_->context(); }
 
   TestBrowserThreadBundle thread_bundle_;
   std::unique_ptr<EmbeddedWorkerTestHelper> helper_;
 
+  // Runs until |registration| has an active version and it is activated.
+  void RunUntilActivatedVersion(ServiceWorkerRegistration* registration) {
+    if (registration->active_version() &&
+        registration->active_version()->status() ==
+            ServiceWorkerVersion::ACTIVATED)
+      return;
+    if (!is_observing_context_) {
+      helper_->context_wrapper()->AddObserver(this);
+      is_observing_context_ = true;
+    }
+    base::RunLoop loop;
+    scope_for_wait_for_activated_ = registration->scope();
+    quit_closure_for_wait_for_activated_ = loop.QuitClosure();
+    loop.Run();
+  }
+
+  // Registers |script| and waits for the service worker to become activated.
+  void RegisterServiceWorker(
+      const GURL& script,
+      blink::mojom::ServiceWorkerRegistrationOptions options) {
+    base::RunLoop loop;
+    blink::ServiceWorkerStatusCode status;
+    int64_t registration_id;
+    context()->RegisterServiceWorker(
+        script, options,
+        base::BindLambdaForTesting(
+            [&](blink::ServiceWorkerStatusCode result_status,
+                const std::string& /* status_message */,
+                int64_t result_registration_id) {
+              status = result_status;
+              registration_id = result_registration_id;
+              loop.Quit();
+            }));
+    loop.Run();
+    EXPECT_EQ(blink::ServiceWorkerStatusCode::kOk, status);
+    scoped_refptr<ServiceWorkerRegistration> registration =
+        context()->GetLiveRegistration(registration_id);
+    ASSERT_TRUE(registration);
+    RunUntilActivatedVersion(registration.get());
+    EXPECT_TRUE(registration->active_version());
+  }
+
+  // Wrapper for ServiceWorkerStorage::FindRegistrationForScope.
+  blink::ServiceWorkerStatusCode FindRegistrationForScope(const GURL& scope) {
+    base::RunLoop loop;
+    blink::ServiceWorkerStatusCode status;
+    context()->storage()->FindRegistrationForScope(
+        scope,
+        base::BindLambdaForTesting(
+            [&](blink::ServiceWorkerStatusCode result_status,
+                scoped_refptr<ServiceWorkerRegistration> result_registration) {
+              status = result_status;
+              loop.Quit();
+            }));
+    loop.Run();
+    return status;
+  }
+
+  // Wrapper for ServiceWorkerContextCore::DeleteForOrigin.
+  blink::ServiceWorkerStatusCode DeleteForOrigin(const GURL& origin) {
+    blink::ServiceWorkerStatusCode status;
+    base::RunLoop loop;
+    context()->DeleteForOrigin(
+        origin, base::BindLambdaForTesting(
+                    [&](blink::ServiceWorkerStatusCode result_status) {
+                      status = result_status;
+                      loop.Quit();
+                    }));
+    loop.Run();
+    return status;
+  }
+
+ protected:
+  // ServiceWorkerContextCoreObserver overrides:
+  void OnVersionStateChanged(int64_t version_id,
+                             const GURL& scope,
+                             ServiceWorkerVersion::Status status) override {
+    if (status == ServiceWorkerVersion::ACTIVATED &&
+        scope == scope_for_wait_for_activated_ &&
+        quit_closure_for_wait_for_activated_) {
+      std::move(quit_closure_for_wait_for_activated_).Run();
+    }
+  }
+
  private:
+  GURL scope_for_wait_for_activated_;
+  base::OnceClosure quit_closure_for_wait_for_activated_;
+  bool is_observing_context_ = false;
+
   DISALLOW_COPY_AND_ASSIGN(ServiceWorkerContextCoreTest);
 };
 
@@ -62,4 +162,52 @@ TEST_F(ServiceWorkerContextCoreTest, FailureInfo) {
   EXPECT_FALSE(base::ContainsKey(context()->failure_counts_, kVersionId));
 }
 
+TEST_F(ServiceWorkerContextCoreTest, DeleteForOrigin) {
+  const GURL script("https://www.example.com/a/sw.js");
+  const GURL scope("https://www.example.com/a");
+  const GURL origin("https://www.example.com");
+
+  // Register a service worker.
+  blink::mojom::ServiceWorkerRegistrationOptions options;
+  options.scope = scope;
+  RegisterServiceWorker(scope, options);
+
+  // Delete for origin.
+  EXPECT_EQ(blink::ServiceWorkerStatusCode::kOk, DeleteForOrigin(origin));
+
+  // The registration should be deleted.
+  EXPECT_EQ(blink::ServiceWorkerStatusCode::kErrorNotFound,
+            FindRegistrationForScope(scope));
+}
+
+// Tests that DeleteForOrigin() doesn't get stuck forever even upon an error
+// when trying to unregister.
+TEST_F(ServiceWorkerContextCoreTest, DeleteForOrigin_UnregisterFail) {
+  const GURL script("https://www.example.com/a/sw.js");
+  const GURL scope("https://www.example.com/a");
+  const GURL origin("https://www.example.com");
+
+  // Register a service worker.
+  blink::mojom::ServiceWorkerRegistrationOptions options;
+  options.scope = scope;
+  RegisterServiceWorker(scope, options);
+
+  // Start DeleteForOrigin().
+  base::RunLoop loop;
+  blink::ServiceWorkerStatusCode status;
+  context()->DeleteForOrigin(
+      origin, base::BindLambdaForTesting(
+                  [&](blink::ServiceWorkerStatusCode result_status) {
+                    status = result_status;
+                    loop.Quit();
+                  }));
+  // Disable storage before it finishes. This causes the Unregister job to
+  // complete with an error.
+  context()->storage()->Disable();
+  loop.Run();
+
+  // The operation should still complete.
+  EXPECT_EQ(blink::ServiceWorkerStatusCode::kErrorFailed, status);
+}
+
 }  // namespace content