ExtensionRegistrar for enabling and disabling extensions

Classes will use ExtensionRegistrar to enable and disable extensions, as
well as add, remove, reload, and terminate them (later CLs).

Currently Chrome's ExtensionService does this, among many other things.
A standalone class for this is preferred because:
* Code outside //chrome uses similar steps to run extensions, so we
  should share this code in //extensions
* ExtensionService already does too much and is very complex

This CL adds EnableExtension() and DisableExtension() to
ExtensionRegistrar. ExtensionService still does some Chrome-specific
work, but later we'll factor that out into a delegate.

Later CLs will add the other extension lifecycle methods, so eventually
ExtensionRegistrar is the only class allowed to change the
ExtensionRegistry (thus the name). We'll also update calls to
ExtensionService methods to use ExtensionRegistrar directly when it
makes sense.


Design doc: https://goo.gl/trZKep (Google-internal).

Bug: 762642
Test: extensions_unittests: ExtensionRegistrarTest
Change-Id: I2910a76f873122fbc405b7cc76c766b00a8b216f
Reviewed-on: https://chromium-review.googlesource.com/671206
Commit-Queue: Michael Giuffrida <michaelpg@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#505206}

chrome/browser/extensions/extension_service.cc

@@ -76,6 +76,7 @@
 #include "extensions/browser/event_router.h"
 #include "extensions/browser/extension_file_task_runner.h"
 #include "extensions/browser/extension_host.h"
+#include "extensions/browser/extension_registrar.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/extension_util.h"
@@ -83,7 +84,6 @@
 #include "extensions/browser/external_install_info.h"
 #include "extensions/browser/install_flag.h"
 #include "extensions/browser/lazy_background_task_queue.h"
-#include "extensions/browser/renderer_startup_helper.h"
 #include "extensions/browser/runtime_data.h"
 #include "extensions/browser/uninstall_reason.h"
 #include "extensions/browser/update_observer.h"
@@ -331,10 +331,9 @@ ExtensionService::ExtensionService(Profile* profile,
       extensions_enabled_(extensions_enabled),
       ready_(ready),
       shared_module_service_(new extensions::SharedModuleService(profile_)),
-      renderer_helper_(
-          extensions::RendererStartupHelperFactory::GetForBrowserContext(
-              profile_)),
-      app_data_migrator_(new extensions::AppDataMigrator(profile_, registry_)) {
+      app_data_migrator_(new extensions::AppDataMigrator(profile_, registry_)),
+      extension_registrar_(
+          std::make_unique<extensions::ExtensionRegistrar>(profile_)) {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   TRACE_EVENT0("browser,startup", "ExtensionService::ExtensionService::ctor");
 
@@ -900,11 +899,12 @@ void ExtensionService::EnableExtension(const std::string& extension_id) {
   if (IsExtensionEnabled(extension_id) ||
       extension_prefs_->IsExtensionBlacklisted(extension_id))
     return;
-  const Extension* extension =
+  scoped_refptr<const Extension> extension =
       registry_->disabled_extensions().GetByID(extension_id);
 
   ManagementPolicy* policy = system_->management_policy();
-  if (extension && policy->MustRemainDisabled(extension, nullptr, nullptr)) {
+  if (extension &&
+      policy->MustRemainDisabled(extension.get(), nullptr, nullptr)) {
     UMA_HISTOGRAM_COUNTS_100("Extensions.EnableDeniedByPolicy", 1);
     return;
   }
@@ -916,12 +916,10 @@ void ExtensionService::EnableExtension(const std::string& extension_id) {
     return;
 
   // Move it over to the enabled list.
-  registry_->AddEnabled(make_scoped_refptr(extension));
-  registry_->RemoveDisabled(extension->id());
-
-  NotifyExtensionLoaded(extension);
+  extension_registrar_->EnableExtension(extension);
+  NotifyExtensionLoaded(extension.get());
 
-  MaybeSpinUpLazyBackgroundPage(extension);
+  MaybeSpinUpLazyBackgroundPage(extension.get());
 }
 
 void ExtensionService::DisableExtension(const std::string& extension_id,
@@ -980,11 +978,11 @@ void ExtensionService::DisableExtension(const std::string& extension_id,
 
   // Move it over to the disabled list. Don't send a second unload notification
   // for terminated extensions being disabled.
-  registry_->AddDisabled(make_scoped_refptr(extension));
   if (registry_->enabled_extensions().Contains(extension->id())) {
-    registry_->RemoveEnabled(extension->id());
-    NotifyExtensionUnloaded(extension, UnloadedExtensionReason::DISABLE);
+    extension_registrar_->DisableExtension(extension);
+    PostDeactivateExtension(extension);
   } else {
+    registry_->AddDisabled(make_scoped_refptr(extension));
     registry_->RemoveTerminated(extension->id());
   }
 }
@@ -1096,28 +1094,6 @@ void ExtensionService::RecordPermissionMessagesHistogram(
 }
 
 void ExtensionService::NotifyExtensionLoaded(const Extension* extension) {
-  // The URLRequestContexts need to be first to know that the extension
-  // was loaded, otherwise a race can arise where a renderer that is created
-  // for the extension may try to load an extension URL with an extension id
-  // that the request context doesn't yet know about. The profile is responsible
-  // for ensuring its URLRequestContexts appropriately discover the loaded
-  // extension.
-  system_->RegisterExtensionWithRequestContexts(
-      extension,
-      base::Bind(&ExtensionService::OnExtensionRegisteredWithRequestContexts,
-                 AsWeakPtr(), make_scoped_refptr(extension)));
-
-  renderer_helper_->OnExtensionLoaded(*extension);
-
-  // Tell subsystems that use the ExtensionRegistryObserver::OnExtensionLoaded
-  // about the new extension.
-  //
-  // NOTE: It is important that this happen after notifying the renderers about
-  // the new extensions so that if we navigate to an extension URL in
-  // ExtensionRegistryObserver::OnExtensionLoaded the renderer is guaranteed to
-  // know about it.
-  registry_->TriggerOnLoaded(extension);
-
   // TODO(kalman): Convert ExtensionSpecialStoragePolicy to a
   // BrowserContextKeyedService and use ExtensionRegistryObserver.
   profile_->GetExtensionSpecialStoragePolicy()->
@@ -1153,25 +1129,12 @@ void ExtensionService::NotifyExtensionLoaded(const Extension* extension) {
   }
 }
 
-void ExtensionService::OnExtensionRegisteredWithRequestContexts(
-    scoped_refptr<const extensions::Extension> extension) {
-  registry_->AddReady(extension);
-  if (registry_->enabled_extensions().Contains(extension->id()))
-    registry_->TriggerOnReady(extension.get());
-}
-
-void ExtensionService::NotifyExtensionUnloaded(const Extension* extension,
-                                               UnloadedExtensionReason reason) {
-  registry_->TriggerOnUnloaded(extension, reason);
-
-  renderer_helper_->OnExtensionUnloaded(*extension);
-
-  system_->UnregisterExtensionWithRequestContexts(extension->id(), reason);
-
+void ExtensionService::PostDeactivateExtension(
+    scoped_refptr<const Extension> extension) {
   // TODO(kalman): Convert ExtensionSpecialStoragePolicy to a
   // BrowserContextKeyedService and use ExtensionRegistryObserver.
-  profile_->GetExtensionSpecialStoragePolicy()->
-      RevokeRightsForExtension(extension);
+  profile_->GetExtensionSpecialStoragePolicy()->RevokeRightsForExtension(
+      extension.get());
 
 #if defined(OS_CHROMEOS)
   // Revoke external file access for the extension from its file system context.
@@ -1375,42 +1338,37 @@ void ExtensionService::OnAllExternalProvidersReady() {
 
 void ExtensionService::UnloadExtension(const std::string& extension_id,
                                        UnloadedExtensionReason reason) {
-  // Make sure the extension gets deleted after we return from this function.
   int include_mask =
       ExtensionRegistry::EVERYTHING & ~ExtensionRegistry::TERMINATED;
   scoped_refptr<const Extension> extension(
       registry_->GetExtensionById(extension_id, include_mask));
 
-  // This method can be called via PostTask, so the extension may have been
-  // unloaded by the time this runs.
-  if (!extension.get()) {
-    // In case the extension may have crashed/uninstalled. Allow the profile to
-    // clean up its RequestContexts.
-    system_->UnregisterExtensionWithRequestContexts(extension_id, reason);
-    return;
+  // TODO(michaelpg): Move this block to ExtensionRegistrar once it learns to
+  // reload extensions.
+  if (extension) {
+    // Keep information about the extension so that we can reload it later
+    // even if it's not permanently installed.
+    unloaded_extension_paths_[extension->id()] = extension->path();
+
+    // Clean up if the extension is meant to be enabled after a reload.
+    reloading_extensions_.erase(extension->id());
   }
 
-  // Keep information about the extension so that we can reload it later
-  // even if it's not permanently installed.
-  unloaded_extension_paths_[extension->id()] = extension->path();
+  bool already_disabled =
+      registry_->disabled_extensions().Contains(extension_id);
+  extension_registrar_->RemoveExtension(extension_id, reason);
 
-  // Clean up if the extension is meant to be enabled after a reload.
-  reloading_extensions_.erase(extension->id());
+  // If the extension was only in the terminated set, the removal notification
+  // will be sent when UntrackTerminatedExtension is called.
+  if (!extension)
+    return;
 
-  if (registry_->disabled_extensions().Contains(extension->id())) {
-    registry_->RemoveDisabled(extension->id());
-    // Make sure the profile cleans up its RequestContexts when an already
-    // disabled extension is unloaded (since they are also tracking the disabled
-    // extensions).
-    system_->UnregisterExtensionWithRequestContexts(extension_id, reason);
-    // Don't send the unloaded notification. It was sent when the extension
-    // was disabled.
-  } else {
-    // Remove the extension from the enabled list.
-    registry_->RemoveEnabled(extension->id());
-    NotifyExtensionUnloaded(extension.get(), reason);
-  }
+  if (!already_disabled)
+    PostDeactivateExtension(extension);
 
+  // TODO(michaelpg): Move to ExtensionRegistrar::RemoveExtension() once that
+  // can call PostDeactivateExtension() on a delegate. For now, keep this here
+  // so it runs in the same order.
   content::NotificationService::current()->Notify(
       extensions::NOTIFICATION_EXTENSION_REMOVED,
       content::Source<Profile>(profile_),
@@ -1500,7 +1458,8 @@ void ExtensionService::AddExtension(const Extension* extension) {
     if (!Manifest::IsUnpackedLocation(extension->location()))
       CHECK_GE(version_compare_result, 0);
   }
-  // If the extension was disabled for a reload, then enable it.
+
+  // If the extension was disabled for a reload, we will enable it.
   bool reloading = reloading_extensions_.erase(extension->id()) > 0;
 
   // Set the upgraded bit; we consider reloads upgrades.
@@ -1569,7 +1528,7 @@ void ExtensionService::AddExtension(const Extension* extension) {
                                        syncer::StringOrdinal());
     }
 
-    registry_->AddEnabled(extension);
+    extension_registrar_->EnableExtension(extension);
     NotifyExtensionLoaded(extension);
   }
   system_->runtime_data()->SetBeingUpgraded(extension->id(), false);

chrome/browser/extensions/extension_service.h

@@ -58,12 +58,12 @@ class ComponentLoader;
 class CrxInstaller;
 class ExtensionActionStorageManager;
 class ExtensionErrorController;
+class ExtensionRegistrar;
 class ExtensionRegistry;
 class ExtensionSystem;
 class ExtensionUpdater;
 class ExternalInstallManager;
 class OneShotEvent;
-class RendererStartupHelper;
 class SharedModuleService;
 class UpdateObserver;
 }  // namespace extensions
@@ -150,7 +150,9 @@ class ExtensionServiceInterface
   virtual void CheckForUpdatesSoon() = 0;
 
   // Adds |extension| to this ExtensionService and notifies observers that the
-  // extensions have been loaded.
+  // extension has been loaded.
+  // TODO(michaelpg): Refactor this function into single-purpose functions and
+  // migrate common code into ExtensionRegistrar.
   virtual void AddExtension(const extensions::Extension* extension) = 0;
 
   // Check if we have preferences for the component extension and, if not or if
@@ -515,17 +517,16 @@ class ExtensionService
                                 const std::string& install_parameter);
 
   // Handles sending notification that |extension| was loaded.
+  // TODO(michaelpg): Move to a delegate provided to ExtensionRegistrar, so
+  // ExtensionRegistrar is responsible for calling this at the right times.
   void NotifyExtensionLoaded(const extensions::Extension* extension);
 
-  // Completes extension loading after URLRequestContexts have been updated
-  // on the IO thread.
-  void OnExtensionRegisteredWithRequestContexts(
+  // Handles updating the profile when |extension| is disabled or removed.
+  // TODO(michaelpg): Move to a delegate provided to ExtensionRegistrar, so
+  // ExtensionRegistrar is responsible for calling this at the right times.
+  void PostDeactivateExtension(
       scoped_refptr<const extensions::Extension> extension);
 
-  // Handles sending notification that |extension| was unloaded.
-  void NotifyExtensionUnloaded(const extensions::Extension* extension,
-                               extensions::UnloadedExtensionReason reason);
-
   // Common helper to finish installing the given extension.
   void FinishInstallation(const extensions::Extension* extension);
 
@@ -719,15 +720,14 @@ class ExtensionService
   // The SharedModuleService used to check for import dependencies.
   std::unique_ptr<extensions::SharedModuleService> shared_module_service_;
 
-  // The associated RendererStartupHelper. Guaranteed to outlive the
-  // ExtensionSystem, and thus us.
-  extensions::RendererStartupHelper* renderer_helper_;
-
   base::ObserverList<extensions::UpdateObserver, true> update_observers_;
 
   // Migrates app data when upgrading a legacy packaged app to a platform app
   std::unique_ptr<extensions::AppDataMigrator> app_data_migrator_;
 
+  // Helper to register and unregister extensions.
+  std::unique_ptr<extensions::ExtensionRegistrar> extension_registrar_;
+
   using InstallGateRegistry = std::map<extensions::ExtensionPrefs::DelayReason,
                                        extensions::InstallGate*>;
   InstallGateRegistry install_delayer_registry_;

extensions/browser/BUILD.gn

@@ -149,6 +149,8 @@ source_set("browser_sources") {
     "extension_prefs_scope.h",
     "extension_protocols.cc",
     "extension_protocols.h",
+    "extension_registrar.cc",
+    "extension_registrar.h",
     "extension_registry.cc",
     "extension_registry.h",
     "extension_registry_factory.cc",
@@ -528,6 +530,7 @@ source_set("unit_tests") {
     "extension_api_frame_id_map_unittest.cc",
     "extension_icon_image_unittest.cc",
     "extension_pref_value_map_unittest.cc",
+    "extension_registrar_unittest.cc",
     "extension_registry_unittest.cc",
     "extension_throttle_simulation_unittest.cc",
     "extension_throttle_test_support.cc",

extensions/browser/extension_registrar.cc

+// Copyright 2017 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.
+
+#include "extensions/browser/extension_registrar.h"
+
+#include "base/logging.h"
+#include "content/public/browser/browser_thread.h"
+#include "extensions/browser/extension_registry.h"
+#include "extensions/browser/extension_system.h"
+#include "extensions/browser/renderer_startup_helper.h"
+
+namespace extensions {
+
+ExtensionRegistrar::ExtensionRegistrar(content::BrowserContext* browser_context)
+    : extension_system_(ExtensionSystem::Get(browser_context)),
+      registry_(ExtensionRegistry::Get(browser_context)),
+      renderer_helper_(
+          RendererStartupHelperFactory::GetForBrowserContext(browser_context)),
+      weak_factory_(this) {}
+
+ExtensionRegistrar::~ExtensionRegistrar() = default;
+
+void ExtensionRegistrar::EnableExtension(
+    scoped_refptr<const Extension> extension) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  registry_->RemoveDisabled(extension->id());
+  registry_->AddEnabled(extension);
+
+  // The URLRequestContexts need to be first to know that the extension
+  // was loaded. Otherwise a race can arise where a renderer that is created
+  // for the extension may try to load an extension URL with an extension id
+  // that the request context doesn't yet know about. The BrowserContext should
+  // ensure its URLRequestContexts appropriately discover the loaded extension.
+  extension_system_->RegisterExtensionWithRequestContexts(
+      extension.get(),
+      base::Bind(&ExtensionRegistrar::OnExtensionRegisteredWithRequestContexts,
+                 weak_factory_.GetWeakPtr(), extension));
+
+  renderer_helper_->OnExtensionLoaded(*extension);
+
+  // Tell subsystems that use the ExtensionRegistryObserver::OnExtensionLoaded
+  // about the new extension.
+  //
+  // NOTE: It is important that this happen after notifying the renderers about
+  // the new extensions so that if we navigate to an extension URL in
+  // ExtensionRegistryObserver::OnExtensionLoaded the renderer is guaranteed to
+  // know about it.
+  registry_->TriggerOnLoaded(extension.get());
+}
+
+void ExtensionRegistrar::DisableExtension(
+    scoped_refptr<const Extension> extension) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  // Move the extension to the disabled list.
+  registry_->RemoveEnabled(extension->id());
+  registry_->AddDisabled(extension);
+
+  DeactivateExtension(extension.get(), UnloadedExtensionReason::DISABLE);
+  NotifyExtensionDisabledOrRemoved(extension->id(),
+                                   UnloadedExtensionReason::DISABLE);
+}
+
+void ExtensionRegistrar::RemoveExtension(const ExtensionId& extension_id,
+                                         UnloadedExtensionReason reason) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  int include_mask =
+      ExtensionRegistry::EVERYTHING & ~ExtensionRegistry::TERMINATED;
+  scoped_refptr<const Extension> extension(
+      registry_->GetExtensionById(extension_id, include_mask));
+
+  if (registry_->disabled_extensions().Contains(extension_id)) {
+    registry_->RemoveDisabled(extension_id);
+    // The extension is already deactivated.
+  } else if (extension) {
+    registry_->RemoveEnabled(extension_id);
+    DeactivateExtension(extension.get(), reason);
+  }
+
+  NotifyExtensionDisabledOrRemoved(extension_id, reason);
+}
+
+void ExtensionRegistrar::DeactivateExtension(const Extension* extension,
+                                             UnloadedExtensionReason reason) {
+  registry_->TriggerOnUnloaded(extension, reason);
+  renderer_helper_->OnExtensionUnloaded(*extension);
+}
+
+void ExtensionRegistrar::NotifyExtensionDisabledOrRemoved(
+    const ExtensionId& extension_id,
+    UnloadedExtensionReason reason) {
+  extension_system_->UnregisterExtensionWithRequestContexts(extension_id,
+                                                            reason);
+}
+
+void ExtensionRegistrar::OnExtensionRegisteredWithRequestContexts(
+    scoped_refptr<const Extension> extension) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  registry_->AddReady(extension);
+  if (registry_->enabled_extensions().Contains(extension->id()))
+    registry_->TriggerOnReady(extension.get());
+}
+
+}  // namespace extensions

extensions/browser/extension_registrar.h

+// Copyright 2017 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.
+
+#ifndef EXTENSIONS_BROWSER_EXTENSION_REGISTRAR_H_
+#define EXTENSIONS_BROWSER_EXTENSION_REGISTRAR_H_
+
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
+#include "extensions/common/extension.h"
+
+namespace content {
+class BrowserContext;
+}  // namespace content
+
+namespace extensions {
+
+class Extension;
+class ExtensionRegistry;
+class ExtensionSystem;
+class RendererStartupHelper;
+
+// ExtensionRegistrar drives the stages of registering and unregistering
+// extensions for a BrowserContext.
+// TODO(michaelpg): Add functionality for reloading and terminating extensions.
+class ExtensionRegistrar {
+ public:
+  explicit ExtensionRegistrar(content::BrowserContext* browser_context);
+  virtual ~ExtensionRegistrar();
+
+  // Marks |extension| enabled and notifies other components about it.
+  // TODO(michaelpg): Move other steps and checks for enabling the extension
+  // from ExtensionService::EnableExtension into here.
+  void EnableExtension(scoped_refptr<const Extension> extension);
+
+  // Marks |extension| disabled and notifies other components that it is
+  // disabled. The ExtensionRegistry retains a reference to it in
+  // disabled_extensions().
+  // TODO(michaelpg): Move other steps for disabling the extension from
+  // ExtensionService::DisableExtension into here.
+  void DisableExtension(scoped_refptr<const Extension> extension);
+
+  // Removes |extension| from the extension system by deactivating it if it is
+  // enabled and removing references to it from the ExtensionRegistry's enabled
+  // or disabled sets.
+  // Note: Extensions will not be removed from other sets (terminated,
+  // blacklisted or blocked). ExtensionService handles that, since it also adds
+  // it to those sets. TODO(michaelpg): Make ExtensionRegistrar the sole mutator
+  // of ExtensionRegsitry to simplify this usage.
+  void RemoveExtension(const ExtensionId& extension_id,
+                       UnloadedExtensionReason reason);
+
+ private:
+  // Triggers the unloaded notifications to deactivate an extension.
+  void DeactivateExtension(const Extension* extension,
+                           UnloadedExtensionReason reason);
+
+  // Updates the ExtensionSystem when an extension is disabled or removed (even
+  // if it was already disabled or terminated). Necessary because
+  // ExtensionSystem tracks enabled and disabled extensions separately.
+  void NotifyExtensionDisabledOrRemoved(const ExtensionId& extension_id,
+                                        UnloadedExtensionReason reason);
+
+  // Marks the extension ready after URLRequestContexts have been updated on
+  // the IO thread.
+  void OnExtensionRegisteredWithRequestContexts(
+      scoped_refptr<const Extension> extension);
+
+  // These objects should outlive the service that owns us (ExtensionService).
+  ExtensionSystem* const extension_system_;
+  ExtensionRegistry* const registry_;
+  extensions::RendererStartupHelper* renderer_helper_;
+
+  base::WeakPtrFactory<ExtensionRegistrar> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(ExtensionRegistrar);
+};
+
+}  // namespace extensions
+
+#endif  // EXTENSIONS_BROWSER_EXTENSION_REGISTRAR_H_

extensions/browser/extension_registrar_unittest.cc

+// Copyright 2017 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.
+
+#include "extensions/browser/extension_registrar.h"
+
+#include <memory>
+
+#include "base/location.h"
+#include "base/macros.h"
+#include "base/threading/sequenced_task_runner_handle.h"
+#include "content/public/browser/browser_context.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "extensions/browser/extension_registry.h"
+#include "extensions/browser/extensions_test.h"
+#include "extensions/browser/test_extension_registry_observer.h"
+#include "extensions/browser/test_extensions_browser_client.h"
+#include "extensions/common/extension.h"
+#include "extensions/common/extension_builder.h"
+
+namespace extensions {
+
+namespace {
+
+class TestExtensionSystem : public MockExtensionSystem {
+ public:
+  explicit TestExtensionSystem(content::BrowserContext* context)
+      : MockExtensionSystem(context) {}
+
+  ~TestExtensionSystem() override {}
+
+  void RegisterExtensionWithRequestContexts(
+      const Extension* extension,
+      const base::Closure& callback) override {
+    base::SequencedTaskRunnerHandle::Get()->PostTask(FROM_HERE, callback);
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TestExtensionSystem);
+};
+
+}  // namespace
+
+class ExtensionRegistrarTest : public ExtensionsTest {
+ public:
+  ExtensionRegistrarTest()
+      : ExtensionsTest(std::make_unique<content::TestBrowserThreadBundle>()) {}
+  ~ExtensionRegistrarTest() override = default;
+
+  void SetUp() override {
+    ExtensionsTest::SetUp();
+    extensions_browser_client()->set_extension_system_factory(&factory_);
+    extension_ = ExtensionBuilder("extension").Build();
+  }
+
+ protected:
+  // Verifies that the extension is in the given set in the ExtensionRegistry
+  // and not in other sets.
+  void ExpectInSet(ExtensionRegistry::IncludeFlag set_id) {
+    ExtensionRegistry* registry = ExtensionRegistry::Get(browser_context());
+
+    EXPECT_EQ(set_id == ExtensionRegistry::ENABLED,
+              registry->enabled_extensions().Contains(extension_->id()));
+
+    EXPECT_EQ(set_id == ExtensionRegistry::DISABLED,
+              registry->disabled_extensions().Contains(extension_->id()));
+
+    EXPECT_EQ(set_id == ExtensionRegistry::TERMINATED,
+              registry->terminated_extensions().Contains(extension_->id()));
+
+    EXPECT_EQ(set_id == ExtensionRegistry::BLACKLISTED,
+              registry->blacklisted_extensions().Contains(extension_->id()));
+
+    EXPECT_EQ(set_id == ExtensionRegistry::BLOCKED,
+              registry->blocked_extensions().Contains(extension_->id()));
+  }
+
+  bool IsExtensionReady() {
+    return ExtensionRegistry::Get(browser_context())
+        ->ready_extensions()
+        .Contains(extension_->id());
+  }
+
+  scoped_refptr<const Extension> extension() const { return extension_; }
+
+ private:
+  MockExtensionSystemFactory<TestExtensionSystem> factory_;
+  scoped_refptr<const Extension> extension_;
+
+  DISALLOW_COPY_AND_ASSIGN(ExtensionRegistrarTest);
+};
+
+// Adds and removes an extension.
+TEST_F(ExtensionRegistrarTest, EnableAndRemove) {
+  ExtensionRegistry* extension_registry =
+      ExtensionRegistry::Get(browser_context());
+  ExtensionRegistrar registrar(browser_context());
+
+  registrar.EnableExtension(extension());
+  ExpectInSet(ExtensionRegistry::ENABLED);
+  EXPECT_FALSE(IsExtensionReady());
+
+  TestExtensionRegistryObserver observer(extension_registry);
+  observer.WaitForExtensionReady();
+  EXPECT_TRUE(IsExtensionReady());
+
+  // Calling RemoveExtension removes its entry from the enabled list and then
+  // removes the extension.
+  registrar.RemoveExtension(extension()->id(),
+                            UnloadedExtensionReason::UNINSTALL);
+  ExpectInSet(ExtensionRegistry::NONE);
+}
+
+// Disables an extension before removing it.
+TEST_F(ExtensionRegistrarTest, EnableDisableAndRemove) {
+  ExtensionRegistry* extension_registry =
+      ExtensionRegistry::Get(browser_context());
+  ExtensionRegistrar registrar(browser_context());
+
+  registrar.EnableExtension(extension());
+  ExpectInSet(ExtensionRegistry::ENABLED);
+  EXPECT_FALSE(IsExtensionReady());
+
+  TestExtensionRegistryObserver observer(extension_registry);
+  observer.WaitForExtensionReady();
+  EXPECT_TRUE(IsExtensionReady());
+
+  // Disabling the extension deactivates it.
+  registrar.DisableExtension(extension());
+  ExpectInSet(ExtensionRegistry::DISABLED);
+  EXPECT_FALSE(IsExtensionReady());
+
+  // We can still call RemoveExtension to remove its entry from the disabled
+  // list and remove the extension.
+  registrar.RemoveExtension(extension()->id(),
+                            UnloadedExtensionReason::UNINSTALL);
+  ExpectInSet(ExtensionRegistry::NONE);
+  EXPECT_FALSE(IsExtensionReady());
+}
+
+// Enables a disabled extension.
+TEST_F(ExtensionRegistrarTest, EnableDisabled) {
+  ExtensionRegistry* extension_registry =
+      ExtensionRegistry::Get(browser_context());
+  ExtensionRegistrar registrar(browser_context());
+
+  registrar.EnableExtension(extension());
+  ExpectInSet(ExtensionRegistry::ENABLED);
+  EXPECT_FALSE(IsExtensionReady());
+  TestExtensionRegistryObserver(extension_registry).WaitForExtensionReady();
+
+  registrar.DisableExtension(extension());
+  ExpectInSet(ExtensionRegistry::DISABLED);
+  EXPECT_FALSE(IsExtensionReady());
+
+  registrar.EnableExtension(extension());
+  ExpectInSet(ExtensionRegistry::ENABLED);
+  TestExtensionRegistryObserver(extension_registry).WaitForExtensionReady();
+
+  registrar.RemoveExtension(extension()->id(),
+                            UnloadedExtensionReason::UNINSTALL);
+  ExpectInSet(ExtensionRegistry::NONE);
+}
+
+}  // namespace extensions

extensions/browser/extension_system.h

@@ -57,7 +57,8 @@ class ExtensionSystem : public KeyedService {
   // controlled by |extensions_enabled|.
   virtual void InitForRegularProfile(bool extensions_enabled) = 0;
 
-  // The ExtensionService is created at startup.
+  // The ExtensionService is created at startup. ExtensionService is only
+  // defined in Chrome.
   virtual ExtensionService* extension_service() = 0;
 
   // Per-extension data that can change during the life of the process but

extensions/browser/test_extension_registry_observer.cc

@@ -46,6 +46,7 @@ TestExtensionRegistryObserver::TestExtensionRegistryObserver(
     : will_be_installed_waiter_(new Waiter()),
       uninstalled_waiter_(new Waiter()),
       loaded_waiter_(new Waiter()),
+      ready_waiter_(new Waiter()),
       unloaded_waiter_(new Waiter()),
       extension_registry_observer_(this),
       extension_id_(extension_id) {
@@ -72,6 +73,10 @@ const Extension* TestExtensionRegistryObserver::WaitForExtensionUnloaded() {
   return Wait(&unloaded_waiter_);
 }
 
+const Extension* TestExtensionRegistryObserver::WaitForExtensionReady() {
+  return Wait(&ready_waiter_);
+}
+
 void TestExtensionRegistryObserver::OnExtensionWillBeInstalled(
     content::BrowserContext* browser_context,
     const Extension* extension,
@@ -96,6 +101,13 @@ void TestExtensionRegistryObserver::OnExtensionLoaded(
     loaded_waiter_->OnObserved(extension);
 }
 
+void TestExtensionRegistryObserver::OnExtensionReady(
+    content::BrowserContext* browser_context,
+    const Extension* extension) {
+  if (extension_id_.empty() || extension->id() == extension_id_)
+    ready_waiter_->OnObserved(extension);
+}
+
 void TestExtensionRegistryObserver::OnExtensionUnloaded(
     content::BrowserContext* browser_context,
     const Extension* extension,

extensions/browser/test_extension_registry_observer.h

@@ -30,6 +30,7 @@ class TestExtensionRegistryObserver : public ExtensionRegistryObserver {
   const Extension* WaitForExtensionWillBeInstalled();
   const Extension* WaitForExtensionUninstalled();
   const Extension* WaitForExtensionLoaded();
+  const Extension* WaitForExtensionReady();
   const Extension* WaitForExtensionUnloaded();
 
  private:
@@ -45,6 +46,8 @@ class TestExtensionRegistryObserver : public ExtensionRegistryObserver {
                               extensions::UninstallReason reason) override;
   void OnExtensionLoaded(content::BrowserContext* browser_context,
                          const Extension* extension) override;
+  void OnExtensionReady(content::BrowserContext* browser_context,
+                        const Extension* extension) override;
   void OnExtensionUnloaded(content::BrowserContext* browser_context,
                            const Extension* extension,
                            UnloadedExtensionReason reason) override;
@@ -54,6 +57,7 @@ class TestExtensionRegistryObserver : public ExtensionRegistryObserver {
   std::unique_ptr<Waiter> will_be_installed_waiter_;
   std::unique_ptr<Waiter> uninstalled_waiter_;
   std::unique_ptr<Waiter> loaded_waiter_;
+  std::unique_ptr<Waiter> ready_waiter_;
   std::unique_ptr<Waiter> unloaded_waiter_;
 
   ScopedObserver<ExtensionRegistry, ExtensionRegistryObserver>