extensions: Remove ExtensionService::IsExtensionEnabled, part 9

IsExtensionEnabled() has moved to ExtensionRegistrar. Redirect callers
to ExtensionRegistrar.

Add ExtensionRegistry::GetWeakPtr() so SyncWorker can use weak
pointers for both the registry and registrar and not worry about
the relative lifetimes of these objects.

This CL is addressing code review feedback in:
https://chromium-review.googlesource.com/c/chromium/src/+/6306049

Bug: 396722906
Change-Id: I193c93df7c52303f97bb895f0be8856c77a62739
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6340709
Commit-Queue: James Cook <jamescook@chromium.org>
Reviewed-by: David Bertoni <dbertoni@chromium.org>
Reviewed-by: Fergal Daly <fergal@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1431904}

chrome/browser/apps/platform_apps/api/sync_file_system/sync_file_system_browsertest.cc

@@ -27,8 +27,8 @@
 #include "components/signin/public/identity_manager/identity_manager.h"
 #include "components/signin/public/identity_manager/identity_test_environment.h"
 #include "content/public/test/browser_test.h"
+#include "extensions/browser/extension_registrar.h"
 #include "extensions/browser/extension_registry.h"
-#include "extensions/browser/extension_system.h"
 #include "extensions/test/extension_test_message_listener.h"
 #include "extensions/test/result_catcher.h"
 #include "storage/browser/quota/quota_manager.h"
@@ -98,7 +98,7 @@ class SyncFileSystemTest : public extensions::PlatformAppBrowserTest,
               base_dir_.GetPath(),
               /*task_logger=*/nullptr,
               /*notification_manager=*/nullptr,
-              extensions::ExtensionSystem::Get(context)->extension_service(),
+              extensions::ExtensionRegistrar::Get(context),
               extensions::ExtensionRegistry::Get(context),
               identity_test_env_->identity_manager(),
               /*url_loader_factory=*/nullptr,

chrome/browser/sync_file_system/drive_backend/drive_backend_sync_unittest.cc

@@ -42,6 +42,8 @@
 #include "content/public/browser/browser_thread.h"
 #include "content/public/test/browser_task_environment.h"
 #include "content/public/test/test_utils.h"
+#include "extensions/browser/extension_registrar.h"
+#include "extensions/browser/extension_registry.h"
 #include "extensions/common/extension.h"
 #include "google_apis/drive/drive_api_parser.h"
 #include "google_apis/gaia/gaia_id.h"
@@ -125,8 +127,8 @@ class DriveBackendSyncTest : public testing::Test,
         worker_task_runner_.get(), drive_task_runner.get(), base_dir_.GetPath(),
         nullptr,  // task_logger
         nullptr,  // notification_manager
-        nullptr,  // extension_service
-        nullptr,  // extension_registry
+        extensions::ExtensionRegistrar::Get(&profile_),
+        extensions::ExtensionRegistry::Get(&profile_),
         nullptr,  // identity_manager
         nullptr,  // url_loader_factory
         nullptr,  // drive_service

chrome/browser/sync_file_system/drive_backend/sync_engine.cc

@@ -17,7 +17,6 @@
 #include "base/time/time.h"
 #include "base/values.h"
 #include "chrome/browser/drive/drive_notification_manager_factory.h"
-#include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/signin/identity_manager_factory.h"
 #include "chrome/browser/sync_file_system/drive_backend/callback_helper.h"
@@ -52,6 +51,7 @@
 #include "content/public/browser/device_service.h"
 #include "content/public/browser/network_service_instance.h"
 #include "content/public/browser/storage_partition.h"
+#include "extensions/browser/extension_registrar.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/extension_system_provider.h"
@@ -195,13 +195,13 @@ std::unique_ptr<SyncEngine> SyncEngine::CreateForBrowserContext(
   Profile* profile = Profile::FromBrowserContext(context);
   drive::DriveNotificationManager* notification_manager =
       drive::DriveNotificationManagerFactory::GetForBrowserContext(context);
-  extensions::ExtensionService* extension_service =
-      extensions::ExtensionSystem::Get(context)->extension_service();
   signin::IdentityManager* identity_manager =
       IdentityManagerFactory::GetForProfile(profile);
   scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory =
       context->GetDefaultStoragePartition()
           ->GetURLLoaderFactoryForBrowserProcess();
+  extensions::ExtensionRegistrar* extension_registrar =
+      extensions::ExtensionRegistrar::Get(context);
   extensions::ExtensionRegistry* extension_registry =
       extensions::ExtensionRegistry::Get(context);
 
@@ -209,7 +209,7 @@ std::unique_ptr<SyncEngine> SyncEngine::CreateForBrowserContext(
   auto sync_engine = base::WrapUnique(new SyncEngine(
       ui_task_runner.get(), worker_task_runner.get(), drive_task_runner.get(),
       GetSyncFileSystemDir(context->GetPath()), task_logger,
-      notification_manager, extension_service, extension_registry,
+      notification_manager, extension_registrar, extension_registry,
       identity_manager, url_loader_factory,
       std::make_unique<DriveServiceFactory>(), nullptr /* env_override */));
 
@@ -323,15 +323,10 @@ void SyncEngine::InitializeInternal(
   worker_observer_ = std::make_unique<WorkerObserver>(
       ui_task_runner_.get(), weak_ptr_factory_.GetWeakPtr());
 
-  base::WeakPtr<extensions::ExtensionServiceInterface>
-      extension_service_weak_ptr;
-  if (extension_service_)
-    extension_service_weak_ptr = extension_service_->AsWeakPtr();
-
   if (!sync_worker) {
     sync_worker = std::make_unique<SyncWorker>(
-        sync_file_system_dir_, extension_service_weak_ptr, extension_registry_,
-        env_override_);
+        sync_file_system_dir_, extension_registrar_->GetWeakPtr(),
+        extension_registry_->GetWeakPtr(), env_override_);
   }
 
   sync_worker_ = std::move(sync_worker);
@@ -661,7 +656,7 @@ SyncEngine::SyncEngine(
     const base::FilePath& sync_file_system_dir,
     TaskLogger* task_logger,
     drive::DriveNotificationManager* notification_manager,
-    extensions::ExtensionServiceInterface* extension_service,
+    extensions::ExtensionRegistrar* extension_registrar,
     extensions::ExtensionRegistry* extension_registry,
     signin::IdentityManager* identity_manager,
     scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
@@ -673,7 +668,7 @@ SyncEngine::SyncEngine(
       sync_file_system_dir_(sync_file_system_dir),
       task_logger_(task_logger),
       notification_manager_(notification_manager),
-      extension_service_(extension_service),
+      extension_registrar_(extension_registrar),
       extension_registry_(extension_registry),
       identity_manager_(identity_manager),
       url_loader_factory_(url_loader_factory),

chrome/browser/sync_file_system/drive_backend/sync_engine.h

@@ -37,8 +37,8 @@ class DriveUploaderInterface;
 }
 
 namespace extensions {
+class ExtensionRegistrar;
 class ExtensionRegistry;
-class ExtensionServiceInterface;
 }
 
 namespace leveldb {
@@ -163,7 +163,7 @@ class SyncEngine
              const base::FilePath& sync_file_system_dir,
              TaskLogger* task_logger,
              drive::DriveNotificationManager* notification_manager,
-             extensions::ExtensionServiceInterface* extension_service,
+             extensions::ExtensionRegistrar* extension_registrar,
              extensions::ExtensionRegistry* extension_registry,
              signin::IdentityManager* identity_manager,
              scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
@@ -195,8 +195,8 @@ class SyncEngine
   // KeyedService::DependsOn().
   raw_ptr<drive::DriveNotificationManager, DanglingUntriaged>
       notification_manager_;
-  raw_ptr<extensions::ExtensionServiceInterface, DanglingUntriaged>
-      extension_service_;
+  raw_ptr<extensions::ExtensionRegistrar, DanglingUntriaged>
+      extension_registrar_;
   raw_ptr<extensions::ExtensionRegistry, DanglingUntriaged> extension_registry_;
   raw_ptr<signin::IdentityManager> identity_manager_;
 

chrome/browser/sync_file_system/drive_backend/sync_worker.cc

@@ -49,9 +49,8 @@ void InvokeIdleCallback(base::RepeatingClosure idle_callback,
 
 SyncWorker::SyncWorker(
     const base::FilePath& base_dir,
-    const base::WeakPtr<extensions::ExtensionServiceInterface>&
-        extension_service,
-    extensions::ExtensionRegistry* extension_registry,
+    const base::WeakPtr<extensions::ExtensionRegistrar> extension_registrar,
+    const base::WeakPtr<extensions::ExtensionRegistry>& extension_registry,
     leveldb::Env* env_override)
     : base_dir_(base_dir),
       env_override_(env_override),
@@ -60,7 +59,7 @@ SyncWorker::SyncWorker(
       should_check_remote_change_(true),
       listing_remote_changes_(false),
       sync_enabled_(false),
-      extension_service_(extension_service),
+      extension_registrar_(extension_registrar),
       extension_registry_(extension_registry) {
   DETACH_FROM_SEQUENCE(sequence_checker_);
   DCHECK(base_dir_.IsAbsolute());
@@ -364,38 +363,34 @@ void SyncWorker::UpdateRegisteredApps() {
   context_->GetUITaskRunner()->PostTask(
       FROM_HERE,
       base::BindOnce(
-          &SyncWorker::QueryAppStatusOnUIThread, extension_service_,
-          // This is protected by checking the extension_service_
-          // weak pointer, since the underlying ExtensionService
-          // also relies on the ExtensionRegistry.
-          base::Unretained(extension_registry_), base::Owned(app_ids.release()),
-          app_status,
+          &SyncWorker::QueryAppStatusOnUIThread, extension_registrar_,
+          extension_registry_, base::Owned(app_ids.release()), app_status,
           RelayCallbackToTaskRunner(context_->GetWorkerTaskRunner(), FROM_HERE,
                                     std::move(callback))));
 }
 
+// TODO(crbug.com/402790810): Plumb in a WeakPtr<BrowserContext> and if it is
+// valid use it to look up ExtensionRegistrar and ExtensionRegistry.
 void SyncWorker::QueryAppStatusOnUIThread(
-    const base::WeakPtr<extensions::ExtensionServiceInterface>&
-        extension_service_ptr,
-    extensions::ExtensionRegistry* extension_registry,
+    const base::WeakPtr<extensions::ExtensionRegistrar>& extension_registrar,
+    const base::WeakPtr<extensions::ExtensionRegistry>& extension_registry,
     const std::vector<std::string>* app_ids,
     AppStatusMap* status,
     base::OnceClosure callback) {
-  extensions::ExtensionServiceInterface* extension_service =
-      extension_service_ptr.get();
-  if (!extension_service) {
+  if (!extension_registrar.get() || !extension_registry.get()) {
     std::move(callback).Run();
     return;
   }
 
   for (auto itr = app_ids->begin(); itr != app_ids->end(); ++itr) {
     const std::string& app_id = *itr;
-    if (!extension_registry->GetInstalledExtension(app_id))
+    if (!extension_registry->GetInstalledExtension(app_id)) {
       (*status)[app_id] = APP_STATUS_UNINSTALLED;
-    else if (!extension_service->IsExtensionEnabled(app_id))
+    } else if (!extension_registrar->IsExtensionEnabled(app_id)) {
       (*status)[app_id] = APP_STATUS_DISABLED;
-    else
+    } else {
       (*status)[app_id] = APP_STATUS_ENABLED;
+    }
   }
 
   std::move(callback).Run();

chrome/browser/sync_file_system/drive_backend/sync_worker.h

@@ -28,8 +28,8 @@ class DriveUploaderInterface;
 }
 
 namespace extensions {
+class ExtensionRegistrar;
 class ExtensionRegistry;
-class ExtensionServiceInterface;
 }
 
 namespace storage {
@@ -57,11 +57,11 @@ class SyncEngineInitializer;
 class SyncWorker : public SyncWorkerInterface,
                    public SyncTaskManager::Client {
  public:
-  SyncWorker(const base::FilePath& base_dir,
-             const base::WeakPtr<extensions::ExtensionServiceInterface>&
-                 extension_service,
-             extensions::ExtensionRegistry* extension_registry,
-             leveldb::Env* env_override);
+  SyncWorker(
+      const base::FilePath& base_dir,
+      const base::WeakPtr<extensions::ExtensionRegistrar> extension_registrar,
+      const base::WeakPtr<extensions::ExtensionRegistry>& extension_registry,
+      leveldb::Env* env_override);
 
   SyncWorker(const SyncWorker&) = delete;
   SyncWorker& operator=(const SyncWorker&) = delete;
@@ -120,9 +120,8 @@ class SyncWorker : public SyncWorkerInterface,
                      SyncStatusCode status);
   void UpdateRegisteredApps();
   static void QueryAppStatusOnUIThread(
-      const base::WeakPtr<extensions::ExtensionServiceInterface>&
-          extension_service_ptr,
-      extensions::ExtensionRegistry* extension_registry,
+      const base::WeakPtr<extensions::ExtensionRegistrar>& extension_registrar,
+      const base::WeakPtr<extensions::ExtensionRegistry>& extension_registry,
       const std::vector<std::string>* app_ids,
       AppStatusMap* status,
       base::OnceClosure callback);
@@ -167,9 +166,8 @@ class SyncWorker : public SyncWorkerInterface,
 
   std::unique_ptr<SyncTaskManager> task_manager_;
 
-  base::WeakPtr<extensions::ExtensionServiceInterface> extension_service_;
-  // Only guaranteed to be valid if |extension_service_| is not null.
-  raw_ptr<extensions::ExtensionRegistry> extension_registry_;
+  base::WeakPtr<extensions::ExtensionRegistrar> extension_registrar_;
+  base::WeakPtr<extensions::ExtensionRegistry> extension_registry_;
 
   std::unique_ptr<SyncEngineContext> context_;
   base::ObserverList<Observer>::Unchecked observers_;

chrome/browser/sync_file_system/drive_backend/sync_worker_unittest.cc

@@ -15,12 +15,14 @@
 #include "base/task/single_thread_task_runner.h"
 #include "base/values.h"
 #include "chrome/browser/extensions/test_extension_service.h"
+#include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sync_file_system/drive_backend/metadata_database.h"
 #include "chrome/browser/sync_file_system/drive_backend/metadata_database.pb.h"
 #include "chrome/browser/sync_file_system/drive_backend/sync_engine_context.h"
 #include "chrome/browser/sync_file_system/drive_backend/sync_task.h"
 #include "chrome/browser/sync_file_system/drive_backend/sync_task_manager.h"
 #include "chrome/browser/sync_file_system/sync_file_system_test_util.h"
+#include "chrome/test/base/testing_profile.h"
 #include "components/drive/drive_uploader.h"
 #include "components/drive/service/fake_drive_service.h"
 #include "content/public/test/browser_task_environment.h"
@@ -63,7 +65,12 @@ class MockSyncTask : public ExclusiveTask {
 
 class MockExtensionService : public TestExtensionService {
  public:
-  MockExtensionService() : registry_(nullptr) {}
+  // The ExtensionRegistry and ExtensionRegistrar must use the same profile
+  // (as in production) because the registrar uses the profile to look up
+  // the appropriate registry.
+  explicit MockExtensionService(Profile* profile)
+      : registry_(extensions::ExtensionRegistry::Get(profile)),
+        registrar_(extensions::ExtensionRegistrar::Get(profile)) {}
 
   MockExtensionService(const MockExtensionService&) = delete;
   MockExtensionService& operator=(const MockExtensionService&) = delete;
@@ -71,31 +78,30 @@ class MockExtensionService : public TestExtensionService {
   ~MockExtensionService() override = default;
 
   void AddExtension(const extensions::Extension* extension) override {
-    registry_.AddEnabled(base::WrapRefCounted(extension));
-  }
-
-  bool IsExtensionEnabled(const std::string& extension_id) const override {
-    return registry_.enabled_extensions().Contains(extension_id);
+    registry_->AddEnabled(base::WrapRefCounted(extension));
   }
 
   void UninstallExtension(const std::string& extension_id) {
-    EXPECT_TRUE(registry_.RemoveEnabled(extension_id) ||
-                registry_.RemoveDisabled(extension_id));
+    EXPECT_TRUE(registry_->RemoveEnabled(extension_id) ||
+                registry_->RemoveDisabled(extension_id));
   }
 
   void DisableExtension(const std::string& extension_id) {
-    if (!IsExtensionEnabled(extension_id))
+    if (!registrar_->IsExtensionEnabled(extension_id)) {
       return;
+    }
     scoped_refptr<const extensions::Extension> extension =
-        registry_.GetInstalledExtension(extension_id);
-    EXPECT_TRUE(registry_.RemoveEnabled(extension_id));
-    registry_.AddDisabled(extension);
+        registry_->GetInstalledExtension(extension_id);
+    EXPECT_TRUE(registry_->RemoveEnabled(extension_id));
+    registry_->AddDisabled(extension);
   }
 
-  extensions::ExtensionRegistry& registry() { return registry_; }
+  extensions::ExtensionRegistry& registry() { return *registry_; }
+  extensions::ExtensionRegistrar& registrar() { return *registrar_; }
 
  private:
-  extensions::ExtensionRegistry registry_;
+  const raw_ptr<extensions::ExtensionRegistry> registry_;
+  const raw_ptr<extensions::ExtensionRegistrar> registrar_;
 };
 
 class SyncWorkerTest : public testing::Test {
@@ -111,7 +117,8 @@ class SyncWorkerTest : public testing::Test {
     ASSERT_TRUE(profile_dir_.CreateUniqueTempDir());
     in_memory_env_ = leveldb_chrome::NewMemEnv("SyncWorkerTest");
 
-    extension_service_ = std::make_unique<MockExtensionService>();
+    profile_ = std::make_unique<TestingProfile>();
+    extension_service_ = std::make_unique<MockExtensionService>(profile_.get());
     std::unique_ptr<drive::DriveServiceInterface> fake_drive_service(
         new drive::FakeDriveService);
 
@@ -125,8 +132,8 @@ class SyncWorkerTest : public testing::Test {
                 GetCurrentDefault() /* worker_task_runner */));
 
     sync_worker_ = std::make_unique<SyncWorker>(
-        profile_dir_.GetPath(), extension_service_->AsWeakPtr(),
-        &extension_service_->registry(), in_memory_env_.get());
+        profile_dir_.GetPath(), extension_service_->registrar().GetWeakPtr(),
+        extension_service_->registry().GetWeakPtr(), in_memory_env_.get());
     sync_worker_->Initialize(std::move(sync_engine_context));
 
     sync_worker_->SetSyncEnabled(true);
@@ -136,6 +143,7 @@ class SyncWorkerTest : public testing::Test {
   void TearDown() override {
     sync_worker_.reset();
     extension_service_.reset();
+    profile_.reset();
     base::RunLoop().RunUntilIdle();
   }
 
@@ -170,6 +178,7 @@ class SyncWorkerTest : public testing::Test {
   base::ScopedTempDir profile_dir_;
   std::unique_ptr<leveldb::Env> in_memory_env_;
 
+  std::unique_ptr<TestingProfile> profile_;
   std::unique_ptr<MockExtensionService> extension_service_;
   std::unique_ptr<SyncWorker> sync_worker_;
   base::WeakPtrFactory<SyncWorkerTest> weak_ptr_factory_{this};

extensions/browser/extension_registry.cc

@@ -20,6 +20,10 @@ ExtensionRegistry* ExtensionRegistry::Get(content::BrowserContext* context) {
   return ExtensionRegistryFactory::GetForBrowserContext(context);
 }
 
+base::WeakPtr<ExtensionRegistry> ExtensionRegistry::GetWeakPtr() {
+  return weak_factory_.GetWeakPtr();
+}
+
 ExtensionSet ExtensionRegistry::GenerateInstalledExtensionsSet() const {
   return GenerateInstalledExtensionsSet(EVERYTHING);
 }

extensions/browser/extension_registry.h

@@ -10,6 +10,7 @@
 #include "base/compiler_specific.h"
 #include "base/memory/raw_ptr.h"
 #include "base/memory/scoped_refptr.h"
+#include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
 #include "base/version.h"
 #include "components/keyed_service/core/keyed_service.h"
@@ -57,6 +58,8 @@ class ExtensionRegistry : public KeyedService {
   // Returns the instance for the given |browser_context|.
   static ExtensionRegistry* Get(content::BrowserContext* browser_context);
 
+  base::WeakPtr<ExtensionRegistry> GetWeakPtr();
+
   content::BrowserContext* browser_context() const { return browser_context_; }
 
   // NOTE: These sets are *eventually* mutually exclusive, but an extension can
@@ -216,6 +219,8 @@ class ExtensionRegistry : public KeyedService {
       observers_;
 
   const raw_ptr<content::BrowserContext> browser_context_;
+
+  base::WeakPtrFactory<ExtensionRegistry> weak_factory_{this};
 };
 
 }  // namespace extensions