Avoiding getting all SW Registrations during Browser Startup

Upon investigation of a recent bug reporting the Chrome browser hanging
during startup, it was found that ServiceWorkerStorage::GetAll
Registrations() is called which may take a considerable amount of time.
Hence, by removing this call and just getting Registered Origins instead
of all registrations, this issue should be resolved.

The call to the ServiceWorkerContextWatcher class was also removed from
ServiceWorkerContextWrapper, along with GetAllRegistrations() being
replaced with GetRegisteredOrigins().

Bug: 807440
Change-Id: I99ef9895ca28dd5ba8977fe422a3d92099527765
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2239590
Commit-Queue: Nidhi Jaju <nidhijaju@google.com>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#779749}

content/browser/service_worker/service_worker_context_core.cc

@@ -699,6 +699,7 @@ void ServiceWorkerContextCore::AddLiveRegistration(
 }
 
 void ServiceWorkerContextCore::RemoveLiveRegistration(int64_t id) {
+  DCHECK(live_registrations_.find(id) != live_registrations_.end());
   live_registrations_.erase(id);
 }
 
@@ -874,6 +875,15 @@ void ServiceWorkerContextCore::NotifyRegistrationStored(int64_t registration_id,
       registration_id, scope);
 }
 
+void ServiceWorkerContextCore::NotifyAllRegistrationsDeletedForOrigin(
+    const url::Origin& origin) {
+  DCHECK_CURRENTLY_ON(ServiceWorkerContext::GetCoreThreadId());
+  observer_list_->Notify(
+      FROM_HERE,
+      &ServiceWorkerContextCoreObserver::OnAllRegistrationsDeletedForOrigin,
+      origin);
+}
+
 void ServiceWorkerContextCore::OnStorageWiped() {
   observer_list_->Notify(FROM_HERE,
                          &ServiceWorkerContextCoreObserver::OnStorageWiped);

content/browser/service_worker/service_worker_context_core.h

@@ -289,6 +289,8 @@ class CONTENT_EXPORT ServiceWorkerContextCore
   // does not own these object or influence their lifetime.
   ServiceWorkerRegistration* GetLiveRegistration(int64_t registration_id);
   void AddLiveRegistration(ServiceWorkerRegistration* registration);
+  // RemoveLiveRegistration removes registration from |live_registrations_|
+  // and notifies all observers of the id of the registration removed.
   void RemoveLiveRegistration(int64_t registration_id);
   const std::map<int64_t, ServiceWorkerRegistration*>& GetLiveRegistrations()
       const {
@@ -339,6 +341,9 @@ class CONTENT_EXPORT ServiceWorkerContextCore
 
   // Called by ServiceWorkerStorage when StoreRegistration() succeeds.
   void NotifyRegistrationStored(int64_t registration_id, const GURL& scope);
+  // Called on the core thread and notifies observers that all registrations
+  // have been deleted for a particular origin.
+  void NotifyAllRegistrationsDeletedForOrigin(const url::Origin& origin);
 
   URLLoaderFactoryGetter* loader_factory_getter() {
     return loader_factory_getter_.get();

content/browser/service_worker/service_worker_context_core_observer.h

@@ -95,9 +95,23 @@ class ServiceWorkerContextCoreObserver {
   // add user data to the registration.
   virtual void OnRegistrationStored(int64_t registration_id,
                                     const GURL& scope) {}
+
+  // Called after a task has been posted to delete a registration from storage.
+  // This is roughly equivalent to the same time that the promise for
+  // unregister() would be resolved. This means the live
+  // ServiceWorkerRegistration may still exist, and the deletion operator may
+  // not yet have finished.
   virtual void OnRegistrationDeleted(int64_t registration_id,
                                      const GURL& scope) {}
 
+  // Called after all registrations for |origin| are deleted from storage. There
+  // may still be live registrations for this origin in the kUninstalling or
+  // kUninstalled state.
+  //
+  // This is called after OnRegistrationDeleted(). It is called once
+  // ServiceWorkerRegistry gets confirmation that the delete operation finished.
+  virtual void OnAllRegistrationsDeletedForOrigin(const url::Origin& origin) {}
+
   // Notified when the storage corruption recovery is completed and all stored
   // data is wiped out.
   virtual void OnStorageWiped() {}

content/browser/service_worker/service_worker_context_wrapper.cc

@@ -27,7 +27,6 @@
 #include "content/browser/loader/navigation_url_loader_impl.h"
 #include "content/browser/service_worker/embedded_worker_status.h"
 #include "content/browser/service_worker/service_worker_container_host.h"
-#include "content/browser/service_worker/service_worker_context_watcher.h"
 #include "content/browser/service_worker/service_worker_host.h"
 #include "content/browser/service_worker/service_worker_metrics.h"
 #include "content/browser/service_worker/service_worker_object_host.h"
@@ -225,12 +224,6 @@ ServiceWorkerContextWrapper::ServiceWorkerContextWrapper(
   // Add this object as an observer of the wrapped |context_core_|. This lets us
   // forward observer methods to observers outside of content.
   core_observer_list_->AddObserver(this);
-
-  watcher_ = base::MakeRefCounted<ServiceWorkerContextWatcher>(
-      this,
-      base::BindRepeating(&ServiceWorkerContextWrapper::OnRegistrationUpdated,
-                          base::Unretained(this)),
-      base::DoNothing(), base::DoNothing());
 }
 
 void ServiceWorkerContextWrapper::Init(
@@ -270,11 +263,6 @@ void ServiceWorkerContextWrapper::Init(
           base::RetainedRef(loader_factory_getter),
           std::move(
               non_network_pending_loader_factory_bundle_for_update_check)));
-
-  // The watcher also runs or posts a core thread task which must run after
-  // InitOnCoreThread(), so start it after posting that task above.
-  if (watcher_)
-    watcher_->Start();
 }
 
 void ServiceWorkerContextWrapper::Shutdown() {
@@ -282,10 +270,6 @@ void ServiceWorkerContextWrapper::Shutdown() {
 
   storage_partition_ = nullptr;
   process_manager_->Shutdown();
-  if (watcher_) {
-    watcher_->Stop();
-    watcher_ = nullptr;
-  }
 
   // Use explicit feature check here instead of RunOrPostTaskOnThread(), since
   // the feature may be disabled but in unit tests we are considered both on the
@@ -366,10 +350,18 @@ void ServiceWorkerContextWrapper::OnRegistrationStored(int64_t registration_id,
                                                        const GURL& scope) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
+  registered_origins_.insert(url::Origin::Create(scope.GetOrigin()));
+
   for (auto& observer : observer_list_)
     observer.OnRegistrationStored(registration_id, scope);
 }
 
+void ServiceWorkerContextWrapper::OnAllRegistrationsDeletedForOrigin(
+    const url::Origin& origin) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  registered_origins_.erase(origin);
+}
+
 void ServiceWorkerContextWrapper::OnReportConsoleMessage(
     int64_t version_id,
     const ConsoleMessage& message) {
@@ -1547,6 +1539,11 @@ void ServiceWorkerContextWrapper::InitOnCoreThread(
       special_storage_policy, loader_factory_getter,
       std::move(non_network_pending_loader_factory_bundle_for_update_check),
       core_observer_list_.get(), this);
+
+  if (storage_partition_) {
+    context()->registry()->storage()->GetRegisteredOrigins(base::BindOnce(
+        &ServiceWorkerContextWrapper::DidGetRegisteredOrigins, this));
+  }
 }
 
 void ServiceWorkerContextWrapper::FindRegistrationForScopeOnCoreThread(
@@ -2009,9 +2006,13 @@ void ServiceWorkerContextWrapper::DidSetUpLoaderFactoryForUpdateCheck(
 bool ServiceWorkerContextWrapper::HasRegistrationForOrigin(
     const url::Origin& origin) const {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  return !registrations_initialized_ ||
-         registrations_for_origin_.find(origin) !=
-             registrations_for_origin_.end();
+  if (!registrations_initialized_) {
+    return true;
+  }
+  if (registered_origins_.find(origin) != registered_origins_.end()) {
+    return true;
+  }
+  return false;
 }
 
 void ServiceWorkerContextWrapper::WaitForRegistrationsInitializedForTest() {
@@ -2023,25 +2024,21 @@ void ServiceWorkerContextWrapper::WaitForRegistrationsInitializedForTest() {
   loop.Run();
 }
 
-void ServiceWorkerContextWrapper::OnRegistrationUpdated(
-    const std::vector<ServiceWorkerRegistrationInfo>& registrations) {
+void ServiceWorkerContextWrapper::DidGetRegisteredOrigins(
+    std::vector<url::Origin> origins) {
+  DCHECK_CURRENTLY_ON(GetCoreThreadId());
+  GetUIThreadTaskRunner({})->PostTask(
+      FROM_HERE,
+      base::BindOnce(
+          &ServiceWorkerContextWrapper::InitializeRegisteredOriginsOnUI, this,
+          std::move(origins)));
+}
+
+void ServiceWorkerContextWrapper::InitializeRegisteredOriginsOnUI(
+    std::vector<url::Origin> origins) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  // The first call will initialize stored registrations.
+  registered_origins_.insert(origins.begin(), origins.end());
   registrations_initialized_ = true;
-
-  for (const auto& registration : registrations) {
-    url::Origin origin = url::Origin::Create(registration.scope);
-    int64_t registration_id = registration.registration_id;
-    if (registration.delete_flag == ServiceWorkerRegistrationInfo::IS_DELETED) {
-      auto& registration_ids = registrations_for_origin_[origin];
-      registration_ids.erase(registration_id);
-      if (registration_ids.empty())
-        registrations_for_origin_.erase(origin);
-    } else {
-      registrations_for_origin_[origin].insert(registration_id);
-    }
-  }
-
   if (on_registrations_initialized_)
     std::move(on_registrations_initialized_).Run();
 }

content/browser/service_worker/service_worker_context_wrapper.h

@@ -47,7 +47,6 @@ class BrowserContext;
 class ChromeBlobStorageContext;
 class ResourceContext;
 class ServiceWorkerContextObserver;
-class ServiceWorkerContextWatcher;
 class StoragePartitionImpl;
 class URLLoaderFactoryGetter;
 
@@ -123,6 +122,7 @@ class CONTENT_EXPORT ServiceWorkerContextWrapper
                                const GURL& scope) override;
   void OnRegistrationStored(int64_t registration_id,
                             const GURL& scope) override;
+  void OnAllRegistrationsDeletedForOrigin(const url::Origin& origin) override;
   void OnReportConsoleMessage(int64_t version_id,
                               const ConsoleMessage& message) override;
   void OnControlleeAdded(int64_t version_id,
@@ -330,7 +330,11 @@ class CONTENT_EXPORT ServiceWorkerContextWrapper
   // DeleteAndStartOver fails.
   ServiceWorkerContextCore* context();
 
-  // Whether |origin| has any registrations. Must be called on UI thread.
+  // Whether |origin| has any registrations. Uninstalling and uninstalled
+  // registrations do not cause this to return true, that is, only registrations
+  // with status ServiceWorkerRegistration::Status::kIntact are considered, such
+  // as even if the corresponding live registrations may still exist. Must be
+  // called on the UI thread.
   bool HasRegistrationForOrigin(const url::Origin& origin) const;
   void WaitForRegistrationsInitializedForTest();
 
@@ -462,10 +466,11 @@ class CONTENT_EXPORT ServiceWorkerContextWrapper
       base::OnceCallback<void(scoped_refptr<network::SharedURLLoaderFactory>)>
           callback);
 
-  // Called when the stored registrations are loaded, and each time a new
-  // service worker is registered.
-  void OnRegistrationUpdated(
-      const std::vector<ServiceWorkerRegistrationInfo>& registrations);
+  // These methods are used as a callback for GetRegisteredOrigins when
+  // initialising on the core thread, so registered origins can be tracked
+  // on the UI thread as well.
+  void DidGetRegisteredOrigins(std::vector<url::Origin> origins);
+  void InitializeRegisteredOriginsOnUI(std::vector<url::Origin> origins);
 
   // Temporary for crbug.com/824858.
   void GetAllOriginsInfoOnCoreThread(
@@ -576,17 +581,14 @@ class CONTENT_EXPORT ServiceWorkerContextWrapper
   base::flat_map<int64_t /* version_id */, ServiceWorkerRunningInfo>
       running_service_workers_;
 
-  // Maps the origin to a set of registration ids for that origin. Must be
-  // accessed on UI thread.
+  // A set of origins that have at least one registration. See
+  // HasRegistrationForOrigin() for details. Must be accessed on the UI thread.
   // TODO(http://crbug.com/824858): This can be removed when service workers are
   // fully converted to running on the UI thread.
-  base::flat_map<url::Origin, base::flat_set<int64_t>>
-      registrations_for_origin_;
+  std::set<url::Origin> registered_origins_;
   bool registrations_initialized_ = false;
   base::OnceClosure on_registrations_initialized_;
 
-  scoped_refptr<ServiceWorkerContextWatcher> watcher_;
-
   // Temporary for moving context core to the UI thread.
   scoped_refptr<base::TaskRunner> core_thread_task_runner_;
 

content/browser/service_worker/service_worker_context_wrapper_unittest.cc

@@ -108,4 +108,136 @@ TEST_F(ServiceWorkerContextWrapperTest, HasRegistration) {
       url::Origin::Create(GURL("https://example.org"))));
 }
 
+// This test involves storing two registrations for the same origin to storage
+// and deleting one of them to check that HasRegistrationForOrigin still
+// correctly returns TRUE since there is still one registration for the origin,
+// and should only return FALSE when ALL registrations for that origin have been
+// deleted from storage.
+TEST_F(ServiceWorkerContextWrapperTest, DeleteRegistrationsForSameOrigin) {
+  wrapper_->WaitForRegistrationsInitializedForTest();
+
+  // Make two registrations for same origin.
+  GURL scope1("https://example1.com/abc/");
+  GURL script1("https://example1.com/abc/sw.js");
+  scoped_refptr<ServiceWorkerRegistration> registration1 =
+      CreateServiceWorkerRegistrationAndVersion(context(), scope1, script1,
+                                                /*resource_id=*/1);
+  GURL scope2("https://example1.com/xyz/");
+  GURL script2("https://example1.com/xyz/sw.js");
+  scoped_refptr<ServiceWorkerRegistration> registration2 =
+      CreateServiceWorkerRegistrationAndVersion(context(), scope2, script2, 1);
+
+  // Store both registrations.
+  base::RunLoop loop1;
+  registry()->StoreRegistration(
+      registration1.get(), registration1->waiting_version(),
+      base::BindLambdaForTesting(
+          [&loop1](blink::ServiceWorkerStatusCode status1) {
+            ASSERT_EQ(blink::ServiceWorkerStatusCode::kOk, status1);
+            loop1.Quit();
+          }));
+  loop1.Run();
+
+  base::RunLoop loop2;
+  registry()->StoreRegistration(
+      registration2.get(), registration2->waiting_version(),
+      base::BindLambdaForTesting(
+          [&loop2](blink::ServiceWorkerStatusCode status2) {
+            ASSERT_EQ(blink::ServiceWorkerStatusCode::kOk, status2);
+            loop2.Quit();
+          }));
+  loop2.Run();
+
+  // Delete one of the registrations.
+  base::RunLoop loop3;
+  registry()->DeleteRegistration(
+      registration1.get(), registration1->scope().GetOrigin(),
+      base::BindLambdaForTesting(
+          [&loop3](blink::ServiceWorkerStatusCode status3) {
+            ASSERT_EQ(blink::ServiceWorkerStatusCode::kOk, status3);
+            loop3.Quit();
+          }));
+  loop3.Run();
+
+  // Run loop until idle to wait for
+  // ServiceWorkerRegistry::DidDeleteRegistration() to be executed, and make
+  // sure that NotifyAllRegistrationsDeletedForOrigin() is not called.
+  base::RunLoop().RunUntilIdle();
+
+  // Now test that a registration for an origin is still recognized.
+  EXPECT_TRUE(wrapper_->HasRegistrationForOrigin(
+      url::Origin::Create(GURL("https://example1.com"))));
+
+  // Remove second registration.
+  base::RunLoop loop4;
+  registry()->DeleteRegistration(
+      registration2.get(), registration2->scope().GetOrigin(),
+      base::BindLambdaForTesting(
+          [&loop4](blink::ServiceWorkerStatusCode status4) {
+            ASSERT_EQ(blink::ServiceWorkerStatusCode::kOk, status4);
+            loop4.Quit();
+          }));
+  loop4.Run();
+
+  // Run loop until idle to wait for
+  // ServiceWorkerRegistry::DidDeleteRegistration() to be executed, and make
+  // sure that this time NotifyAllRegistrationsDeletedForOrigin() is called.
+  base::RunLoop().RunUntilIdle();
+
+  // Now test that origin does not have any registrations.
+  EXPECT_FALSE(wrapper_->HasRegistrationForOrigin(
+      url::Origin::Create(GURL("https://example1.com"))));
+}
+
+// This tests deleting registrations from storage and checking that even if live
+// registrations may exist, HasRegistrationForOrigin correctly returns FALSE
+// since the registrations do not exist in storage.
+TEST_F(ServiceWorkerContextWrapperTest, DeleteRegistration) {
+  wrapper_->WaitForRegistrationsInitializedForTest();
+
+  // Make registration.
+  GURL scope1("https://example2.com/");
+  GURL script1("https://example2.com/");
+  scoped_refptr<ServiceWorkerRegistration> registration =
+      CreateServiceWorkerRegistrationAndVersion(context(), scope1, script1,
+                                                /*resource_id=*/1);
+
+  // Store registration.
+  base::RunLoop loop1;
+  registry()->StoreRegistration(
+      registration.get(), registration->waiting_version(),
+      base::BindLambdaForTesting(
+          [&loop1](blink::ServiceWorkerStatusCode status1) {
+            ASSERT_EQ(blink::ServiceWorkerStatusCode::kOk, status1);
+            loop1.Quit();
+          }));
+  loop1.Run();
+
+  wrapper_->OnRegistrationCompleted(registration->id(), registration->scope());
+  base::RunLoop().RunUntilIdle();
+
+  // Now test that a registration is recognized.
+  EXPECT_TRUE(wrapper_->HasRegistrationForOrigin(
+      url::Origin::Create(GURL("https://example2.com"))));
+
+  // Delete registration from storage.
+  base::RunLoop loop2;
+  registry()->DeleteRegistration(
+      registration.get(), registration->scope().GetOrigin(),
+      base::BindLambdaForTesting(
+          [&loop2](blink::ServiceWorkerStatusCode status2) {
+            ASSERT_EQ(blink::ServiceWorkerStatusCode::kOk, status2);
+            loop2.Quit();
+          }));
+  loop2.Run();
+
+  // Finish deleting registration from storage.
+  base::RunLoop().RunUntilIdle();
+
+  // Now test that origin does not have any registrations. This should return
+  // FALSE even when live registrations may exist, as the registrations have
+  // been deleted from storage.
+  EXPECT_FALSE(wrapper_->HasRegistrationForOrigin(
+      url::Origin::Create(GURL("https://example2.com"))));
+}
 }  // namespace content

content/browser/service_worker/service_worker_registry.cc

@@ -1202,9 +1202,12 @@ void ServiceWorkerRegistry::DidDeleteRegistration(
   if (registration)
     registration->UnsetStored();
 
-  if (special_storage_policy_ &&
-      origin_state == ServiceWorkerStorage::OriginState::kDelete) {
-    tracked_origins_for_policy_update_.erase(url::Origin::Create(origin));
+  if (origin_state == ServiceWorkerStorage::OriginState::kDelete) {
+    context_->NotifyAllRegistrationsDeletedForOrigin(
+        url::Origin::Create(origin));
+    if (special_storage_policy_) {
+      tracked_origins_for_policy_update_.erase(url::Origin::Create(origin));
+    }
   }
 
   std::move(callback).Run(status);