Register SW based script only on first time extension activation.

During browser startup, registering a service worker can be slow (in
order of a few seconds). We currently register SW based extension's
script on every extension activation (when we load extensions),
which isn't necessary as SW registrations are suppose to persist
across browser restart by design.

Add a pref to remember the fact that we have successfully registered
an extension's background SW script for a particular version of
that extension and use that to decide whether or not to register
the SW during OnActivateExtension.

This CL will improve SW based extension's startup time.

Bug: 889687
Test: Demo browser clock extension doesn't lag much with the change (note that this is requires local modification to chromium, specifically Patchset #2)
Change-Id: Icfb7174be27f65a1cb6b8ac6d99ddc6b33fdf1c0
Reviewed-on: https://chromium-review.googlesource.com/c/1265821
Commit-Queue: Istiaque Ahmed <lazyboy@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604823}

chrome/browser/extensions/service_worker_apitest.cc

@@ -49,6 +49,7 @@
 #include "extensions/browser/extension_host.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/process_manager.h"
+#include "extensions/browser/service_worker_task_queue.h"
 #include "extensions/common/api/test.h"
 #include "extensions/common/extension_features.h"
 #include "extensions/common/value_builder.h"
@@ -305,38 +306,129 @@ IN_PROC_BROWSER_TEST_P(ServiceWorkerBasedBackgroundTest, OnInstalledEvent) {
       << message_;
 }
 
-// Listens for "runtime.onStartup" event early so that tests can wait for the
-// event on startup (and not miss it).
-class ServiceWorkerOnStartupTest : public ServiceWorkerBasedBackgroundTest {
+// Listens for |message| from extension Service Worker early so that tests can
+// wait for the message on startup (and not miss it).
+class ServiceWorkerWithEarlyMessageListenerTest
+    : public ServiceWorkerBasedBackgroundTest {
  public:
-  ServiceWorkerOnStartupTest() = default;
-  ~ServiceWorkerOnStartupTest() override = default;
+  explicit ServiceWorkerWithEarlyMessageListenerTest(
+      const std::string& test_message)
+      : test_message_(test_message) {}
+  ~ServiceWorkerWithEarlyMessageListenerTest() override = default;
 
-  bool WaitForOnStartupEvent() { return listener_->WaitUntilSatisfied(); }
+  bool WaitForMessage() { return listener_->WaitUntilSatisfied(); }
 
   void CreatedBrowserMainParts(content::BrowserMainParts* main_parts) override {
     // At this point, the notification service is initialized but the profile
     // and extensions have not.
-    listener_ = std::make_unique<ExtensionTestMessageListener>(
-        "onStartup event", false);
+    listener_ =
+        std::make_unique<ExtensionTestMessageListener>(test_message_, false);
     ServiceWorkerBasedBackgroundTest::CreatedBrowserMainParts(main_parts);
   }
 
  private:
+  const std::string test_message_;
   std::unique_ptr<ExtensionTestMessageListener> listener_;
 
-  DISALLOW_COPY_AND_ASSIGN(ServiceWorkerOnStartupTest);
+  DISALLOW_COPY_AND_ASSIGN(ServiceWorkerWithEarlyMessageListenerTest);
+};
+
+class ServiceWorkerOnStartupEventTest
+    : public ServiceWorkerWithEarlyMessageListenerTest {
+ public:
+  ServiceWorkerOnStartupEventTest()
+      : ServiceWorkerWithEarlyMessageListenerTest("onStartup event") {}
+  ~ServiceWorkerOnStartupEventTest() override = default;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ServiceWorkerOnStartupEventTest);
 };
 
 // Tests "runtime.onStartup" for extension SW.
-IN_PROC_BROWSER_TEST_P(ServiceWorkerOnStartupTest, PRE_Event) {
+IN_PROC_BROWSER_TEST_P(ServiceWorkerOnStartupEventTest, PRE_Event) {
   ASSERT_TRUE(RunExtensionTest(
       "service_worker/worker_based_background/on_startup_event"))
       << message_;
 }
 
-IN_PROC_BROWSER_TEST_P(ServiceWorkerOnStartupTest, Event) {
-  EXPECT_TRUE(WaitForOnStartupEvent());
+IN_PROC_BROWSER_TEST_P(ServiceWorkerOnStartupEventTest, Event) {
+  EXPECT_TRUE(WaitForMessage());
+}
+
+class ServiceWorkerRegistrationAtStartupTest
+    : public ServiceWorkerWithEarlyMessageListenerTest,
+      public ServiceWorkerTaskQueue::TestObserver {
+ public:
+  ServiceWorkerRegistrationAtStartupTest()
+      : ServiceWorkerWithEarlyMessageListenerTest("WORKER_RUNNING") {
+    ServiceWorkerTaskQueue::SetObserverForTest(this);
+  }
+  ~ServiceWorkerRegistrationAtStartupTest() override {
+    ServiceWorkerTaskQueue::SetObserverForTest(nullptr);
+  }
+
+  // ServiceWorkerTaskQueue::TestObserver:
+  void OnActivateExtension(const ExtensionId& extension_id,
+                           bool will_register_service_worker) override {
+    if (extension_id != kExtensionId)
+      return;
+
+    will_register_service_worker_ = will_register_service_worker;
+
+    extension_activated_ = true;
+    if (run_loop_)
+      run_loop_->Quit();
+  }
+
+  void WaitForOnActivateExtension() {
+    if (extension_activated_)
+      return;
+    run_loop_ = std::make_unique<base::RunLoop>();
+    run_loop_->Run();
+  }
+
+  bool WillRegisterServiceWorker() {
+    return will_register_service_worker_.value();
+  }
+
+ protected:
+  static const char kExtensionId[];
+
+ private:
+  bool extension_activated_ = false;
+  base::Optional<bool> will_register_service_worker_;
+  std::unique_ptr<base::RunLoop> run_loop_;
+
+  DISALLOW_COPY_AND_ASSIGN(ServiceWorkerRegistrationAtStartupTest);
+};
+
+// Test extension id at
+// api_test/service_worker/worker_based_background/registration_at_startup/.
+const char ServiceWorkerRegistrationAtStartupTest::kExtensionId[] =
+    "gnchfmandajfaiajniicagenfmhdjila";
+
+// Tests that Service Worker registration for existing extension isn't issued
+// upon browser restart.
+// Regression test for https://crbug.com/889687.
+IN_PROC_BROWSER_TEST_P(ServiceWorkerRegistrationAtStartupTest,
+                       PRE_ExtensionActivationDoesNotReregister) {
+  const Extension* extension = LoadExtension(test_data_dir_.AppendASCII(
+      "service_worker/worker_based_background/registration_at_startup"));
+  ASSERT_TRUE(extension);
+  EXPECT_EQ(kExtensionId, extension->id());
+  // Wait for "WORKER_RUNNING" message from the Service Worker.
+  EXPECT_TRUE(WaitForMessage());
+  EXPECT_TRUE(WillRegisterServiceWorker());
+}
+
+IN_PROC_BROWSER_TEST_P(ServiceWorkerRegistrationAtStartupTest,
+                       ExtensionActivationDoesNotReregister) {
+  // Since the extension has onStartup listener, the Service Worker will run on
+  // browser start and we'll see "WORKER_RUNNING" message from the worker.
+  EXPECT_TRUE(WaitForMessage());
+  // As the extension activated during first run on PRE_ step, it shouldn't
+  // re-register the Service Worker upon browser restart.
+  EXPECT_FALSE(WillRegisterServiceWorker());
 }
 
 // Class that dispatches an event to |extension_id| right after a
@@ -1451,10 +1543,16 @@ INSTANTIATE_TEST_CASE_P(ServiceWorkerTestWithJSBindings,
                         ServiceWorkerBasedBackgroundTest,
                         ::testing::Values(JAVASCRIPT_BINDINGS));
 INSTANTIATE_TEST_CASE_P(ServiceWorkerTestWithNativeBindings,
-                        ServiceWorkerOnStartupTest,
+                        ServiceWorkerOnStartupEventTest,
+                        ::testing::Values(NATIVE_BINDINGS));
+INSTANTIATE_TEST_CASE_P(ServiceWorkerTestWithJSBindings,
+                        ServiceWorkerOnStartupEventTest,
+                        ::testing::Values(JAVASCRIPT_BINDINGS));
+INSTANTIATE_TEST_CASE_P(ServiceWorkerTestWithNativeBindings,
+                        ServiceWorkerRegistrationAtStartupTest,
                         ::testing::Values(NATIVE_BINDINGS));
 INSTANTIATE_TEST_CASE_P(ServiceWorkerTestWithJSBindings,
-                        ServiceWorkerOnStartupTest,
+                        ServiceWorkerRegistrationAtStartupTest,
                         ::testing::Values(JAVASCRIPT_BINDINGS));
 
 }  // namespace extensions

chrome/test/data/extensions/api_test/service_worker/worker_based_background/registration_at_startup/manifest.json

+{
+  "name": "Tests that activating an existing SW based extension does not trigger a SW re-registration.",
+  "description": "Extension id: gnchfmandajfaiajniicagenfmhdjila",
+  "key": "MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEApA/uy1jmpFi89H9xJlXF1hR/BEv8L4KfUvmApSsVqWSKgax35xLxrWQOrdVE6iKITSIGIpOyLTFfIST8kw0P6asFA2aAB6WaBA5ZgV2Xpww6nlRQR+yMGvqSswisDyz4WTNeHYQKuSIyNo89NGPTiUOCQRXjLbFKlzmDvErU69HLT1uxcHnv1YSdglt+ALi3Jz1xBafbU/z7mhvHlFtawZ7gGS/xIJIo5tGJOMqUFFUh2UKd8obuRzJQulGp+11L1JzZO54WAiJuie/pcojiF/NIHRng9dbrCc6yNGp1foO/LlOu7VOX+w6H1r+cmxAogifv/MtSDewX49SYA2xNwQIDAQAB",
+  "version": "0.1",
+  "manifest_version": 2,
+  "background": {"service_worker_script": "service_worker_background.js"}
+}

chrome/test/data/extensions/api_test/service_worker/worker_based_background/registration_at_startup/service_worker_background.js

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+chrome.test.sendMessage('WORKER_RUNNING');
+
+// Attach a dummy listener for onStartup to kick off the service worker on
+// every browser start, so we send "WORKER_RUNNING" message.
+chrome.runtime.onStartup.addListener(() => {});

extensions/browser/service_worker_task_queue.cc

@@ -12,6 +12,7 @@
 #include "content/public/browser/service_worker_context.h"
 #include "content/public/browser/storage_partition.h"
 #include "extensions/browser/event_router.h"
+#include "extensions/browser/extension_prefs.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/lazy_context_id.h"
 #include "extensions/browser/service_worker_task_queue_factory.h"
@@ -24,6 +25,16 @@ namespace extensions {
 
 namespace {
 
+// A preference key storing the information about an extension that was
+// activated and has a registered worker based background page.
+const char kPrefServiceWorkerRegistrationInfo[] =
+    "service_worker_registration_info";
+
+// The extension version of the registered service worker.
+const char kPrefVersion[] = "version";
+
+ServiceWorkerTaskQueue::TestObserver* g_test_observer = nullptr;
+
 void RunTask(LazyContextTaskQueue::PendingTask task,
              const ExtensionId& extension_id,
              int64_t version_id,
@@ -176,6 +187,11 @@ void ServiceWorkerTaskQueue::DidStopServiceWorkerContext(
   waiting_did_start_worker_tasks_.erase(key);
 }
 
+// static
+void ServiceWorkerTaskQueue::SetObserverForTest(TestObserver* observer) {
+  g_test_observer = observer;
+}
+
 bool ServiceWorkerTaskQueue::ShouldEnqueueTask(BrowserContext* context,
                                                const Extension* extension) {
   // We call StartWorker every time we want to dispatch an event to an extension
@@ -205,9 +221,22 @@ void ServiceWorkerTaskQueue::AddPendingTaskToDispatchEvent(
 void ServiceWorkerTaskQueue::ActivateExtension(const Extension* extension) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  // TODO(lazyboy): Should we only register Service Worker during installation
-  // and remember its success/failure state in prefs?
   const ExtensionId extension_id = extension->id();
+  // Note: version.IsValid() = false implies we didn't have any prefs stored.
+  base::Version version = RetrieveRegisteredServiceWorkerVersion(extension_id);
+  const bool service_worker_already_registered =
+      version.IsValid() && version == extension->version();
+  if (g_test_observer) {
+    g_test_observer->OnActivateExtension(extension_id,
+                                         !service_worker_already_registered);
+  }
+
+  if (service_worker_already_registered) {
+    // TODO(https://crbug.com/901101): We should kick off an async check to see
+    // if the registration is *actually* there and re-register if necessary.
+    return;
+  }
+
   pending_registrations_.insert(extension_id);
   GURL script_url = extension->GetResourceURL(
       BackgroundInfo::GetBackgroundServiceWorkerScript(extension));
@@ -226,6 +255,8 @@ void ServiceWorkerTaskQueue::DeactivateExtension(const Extension* extension) {
   GURL script_url = extension->GetResourceURL(
       BackgroundInfo::GetBackgroundServiceWorkerScript(extension));
   const ExtensionId extension_id = extension->id();
+  RemoveRegisteredServiceWorkerInfo(extension_id);
+
   content::BrowserContext::GetStoragePartitionForSite(browser_context_,
                                                       extension->url())
       ->GetServiceWorkerContext()
@@ -264,7 +295,9 @@ void ServiceWorkerTaskQueue::DidRegisterServiceWorker(
     bool success) {
   ExtensionRegistry* registry = ExtensionRegistry::Get(browser_context_);
   DCHECK(registry);
-  if (!registry->enabled_extensions().Contains(extension_id))
+  const Extension* extension =
+      registry->enabled_extensions().GetByID(extension_id);
+  if (!extension)
     return;
 
   std::vector<TaskInfo> pending_tasks;
@@ -273,6 +306,8 @@ void ServiceWorkerTaskQueue::DidRegisterServiceWorker(
   if (!success)  // TODO(lazyboy): Handle.
     return;
 
+  SetRegisteredServiceWorkerInfo(extension->id(), extension->version());
+
   for (TaskInfo& task_info : pending_tasks) {
     LazyContextId context_id(browser_context_, extension_id,
                              task_info.service_worker_scope);
@@ -290,4 +325,35 @@ void ServiceWorkerTaskQueue::DidUnregisterServiceWorker(
   // TODO(lazyboy): Handle success = false case.
 }
 
+base::Version ServiceWorkerTaskQueue::RetrieveRegisteredServiceWorkerVersion(
+    const ExtensionId& extension_id) {
+  const base::DictionaryValue* info = nullptr;
+  if (!ExtensionPrefs::Get(browser_context_)
+           ->ReadPrefAsDictionary(extension_id,
+                                  kPrefServiceWorkerRegistrationInfo, &info)) {
+    return base::Version();
+  }
+  std::string version_string;
+  info->GetString(kPrefVersion, &version_string);
+  return base::Version(version_string);
+}
+
+void ServiceWorkerTaskQueue::SetRegisteredServiceWorkerInfo(
+    const ExtensionId& extension_id,
+    const base::Version& version) {
+  DCHECK(version.IsValid());
+  auto info = std::make_unique<base::DictionaryValue>();
+  info->SetString(kPrefVersion, version.GetString());
+  ExtensionPrefs::Get(browser_context_)
+      ->UpdateExtensionPref(extension_id, kPrefServiceWorkerRegistrationInfo,
+                            std::move(info));
+}
+
+void ServiceWorkerTaskQueue::RemoveRegisteredServiceWorkerInfo(
+    const ExtensionId& extension_id) {
+  ExtensionPrefs::Get(browser_context_)
+      ->UpdateExtensionPref(extension_id, kPrefServiceWorkerRegistrationInfo,
+                            nullptr);
+}
+
 }  // namespace extensions

extensions/browser/service_worker_task_queue.h

@@ -9,6 +9,7 @@
 #include <set>
 
 #include "base/memory/weak_ptr.h"
+#include "base/version.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "extensions/browser/lazy_context_task_queue.h"
 #include "extensions/common/extension_id.h"
@@ -54,6 +55,23 @@ class ServiceWorkerTaskQueue : public KeyedService,
   void DidStopServiceWorkerContext(const ExtensionId& extension_id,
                                    int64_t service_worker_version_id);
 
+  class TestObserver {
+   public:
+    TestObserver() = default;
+    virtual ~TestObserver() = default;
+
+    // Called when an extension with id |extension_id| is going to be activated.
+    // |will_register_service_worker| is true if a Service Worker will be
+    // registered.
+    virtual void OnActivateExtension(const ExtensionId& extension_id,
+                                     bool will_register_service_worker) = 0;
+
+   private:
+    DISALLOW_COPY_AND_ASSIGN(TestObserver);
+  };
+
+  static void SetObserverForTest(TestObserver* observer);
+
  private:
   struct TaskInfo;
 
@@ -84,6 +102,21 @@ class ServiceWorkerTaskQueue : public KeyedService,
                               int process_id,
                               int thread_id);
 
+  // The following three methods retrieve, store, and remove information
+  // about Service Worker registration of SW based background pages:
+  //
+  // Retrieves the last version of the extension, returns invalid version if
+  // there isn't any such extension.
+  base::Version RetrieveRegisteredServiceWorkerVersion(
+      const ExtensionId& extension_id);
+  // Records that the extension with |extension_id| and |version| successfully
+  // registered a Service Worker.
+  void SetRegisteredServiceWorkerInfo(const ExtensionId& extension_id,
+                                      const base::Version& version);
+  // Clears any record of registered Service Worker for the given extension with
+  // |extension_id|.
+  void RemoveRegisteredServiceWorkerInfo(const ExtensionId& extension_id);
+
   // Set of extension ids that hasn't completed Service Worker registration.
   std::set<ExtensionId> pending_registrations_;