Add a EarlyExtensionPrefsObserver interface

This commit is part 1 of 2, being a refactor with no change in behavior.
There is an existing observer leak (an AddObserver call with no matching
RemoveObserver call), but this commit keeps that leak. A follow-up
commit will fix the leak.

TBR=seantopping@chromium.org

Change-Id: I73e9087540949c29bb76d6526362b5448452e182
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1585403
Commit-Queue: Nigel Tao <nigeltao@chromium.org>
Reviewed-by: Sean Topping <seantopping@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#654688}

chrome/browser/extensions/api/content_settings/content_settings_service.cc

@@ -69,4 +69,14 @@ void ContentSettingsService::OnExtensionStateChanged(
   content_settings_store_->SetExtensionState(extension_id, state);
 }
 
+void ContentSettingsService::OnExtensionPrefsAvailable(ExtensionPrefs* prefs) {
+  // TODO(nigeltao): Use a ScopedObserver (as a member field, not a local
+  // variable).
+  //
+  // As is, this is a leak: an AddObserver call that has no matching
+  // RemoveObserver call. This leak will become a dangling pointer when this
+  // ContentSettingsService object is destroyed.
+  prefs->AddObserver(this);
+}
+
 }  // namespace extensions

chrome/browser/extensions/api/content_settings/content_settings_service.h

@@ -19,7 +19,8 @@ class ExtensionPrefs;
 // This service hosts a single ContentSettingsStore for the
 // chrome.contentSettings API.
 class ContentSettingsService : public BrowserContextKeyedAPI,
-                               public ExtensionPrefsObserver {
+                               public ExtensionPrefsObserver,
+                               public EarlyExtensionPrefsObserver {
  public:
   explicit ContentSettingsService(content::BrowserContext* context);
   ~ContentSettingsService() override;
@@ -45,6 +46,9 @@ class ContentSettingsService : public BrowserContextKeyedAPI,
   void OnExtensionStateChanged(const std::string& extension_id,
                                bool state) override;
 
+  // EarlyExtensionPrefsObserver implementation.
+  void OnExtensionPrefsAvailable(ExtensionPrefs* prefs) override;
+
  private:
   friend class BrowserContextKeyedAPIFactory<ContentSettingsService>;
 

chrome/browser/extensions/chrome_extensions_browser_client.cc

@@ -251,7 +251,7 @@ PrefService* ChromeExtensionsBrowserClient::GetPrefServiceForContext(
 
 void ChromeExtensionsBrowserClient::GetEarlyExtensionPrefsObservers(
     content::BrowserContext* context,
-    std::vector<ExtensionPrefsObserver*>* observers) const {
+    std::vector<EarlyExtensionPrefsObserver*>* observers) const {
   observers->push_back(ContentSettingsService::Get(context));
 }
 

chrome/browser/extensions/chrome_extensions_browser_client.h

@@ -95,7 +95,7 @@ class ChromeExtensionsBrowserClient : public ExtensionsBrowserClient {
       content::BrowserContext* context) override;
   void GetEarlyExtensionPrefsObservers(
       content::BrowserContext* context,
-      std::vector<ExtensionPrefsObserver*>* observers) const override;
+      std::vector<EarlyExtensionPrefsObserver*>* observers) const override;
   ProcessManagerDelegate* GetProcessManagerDelegate() const override;
   std::unique_ptr<ExtensionHostDelegate> CreateExtensionHostDelegate() override;
   bool DidVersionUpdate(content::BrowserContext* context) override;

chrome/browser/extensions/test_extension_prefs.cc

@@ -124,7 +124,7 @@ void TestExtensionPrefs::RecreateExtensionPrefs() {
   std::unique_ptr<ExtensionPrefs> prefs(ExtensionPrefs::Create(
       &profile_, pref_service_.get(), temp_dir_.GetPath(),
       extension_pref_value_map_.get(), extensions_disabled_,
-      std::vector<ExtensionPrefsObserver*>(),
+      std::vector<EarlyExtensionPrefsObserver*>(),
       // Guarantee that no two extensions get the same installation time
       // stamp and we can reliably assert the installation order in the tests.
       clock_.get()));

chrome/test/base/testing_profile.cc

@@ -488,7 +488,7 @@ void TestingProfile::Init() {
           this, GetPrefs(), extensions_path_,
           ExtensionPrefValueMapFactory::GetForBrowserContext(this),
           extensions_disabled,
-          std::vector<extensions::ExtensionPrefsObserver*>()));
+          std::vector<extensions::EarlyExtensionPrefsObserver*>()));
   extensions::ExtensionPrefsFactory::GetInstance()->SetInstanceForTesting(
       this, std::move(extension_prefs));
 

chromecast/browser/extensions/cast_extensions_browser_client.cc

@@ -161,7 +161,7 @@ PrefService* CastExtensionsBrowserClient::GetPrefServiceForContext(
 
 void CastExtensionsBrowserClient::GetEarlyExtensionPrefsObservers(
     content::BrowserContext* context,
-    std::vector<ExtensionPrefsObserver*>* observers) const {}
+    std::vector<EarlyExtensionPrefsObserver*>* observers) const {}
 
 ProcessManagerDelegate* CastExtensionsBrowserClient::GetProcessManagerDelegate()
     const {

chromecast/browser/extensions/cast_extensions_browser_client.h

@@ -78,7 +78,7 @@ class CastExtensionsBrowserClient : public ExtensionsBrowserClient {
       content::BrowserContext* context) override;
   void GetEarlyExtensionPrefsObservers(
       content::BrowserContext* context,
-      std::vector<ExtensionPrefsObserver*>* observers) const override;
+      std::vector<EarlyExtensionPrefsObserver*>* observers) const override;
   ProcessManagerDelegate* GetProcessManagerDelegate() const override;
   std::unique_ptr<ExtensionHostDelegate> CreateExtensionHostDelegate() override;
   bool DidVersionUpdate(content::BrowserContext* context) override;

extensions/browser/extension_prefs.cc

@@ -322,7 +322,7 @@ ExtensionPrefs* ExtensionPrefs::Create(
     const base::FilePath& root_dir,
     ExtensionPrefValueMap* extension_pref_value_map,
     bool extensions_disabled,
-    const std::vector<ExtensionPrefsObserver*>& early_observers) {
+    const std::vector<EarlyExtensionPrefsObserver*>& early_observers) {
   return ExtensionPrefs::Create(
       browser_context, prefs, root_dir, extension_pref_value_map,
       extensions_disabled, early_observers, base::DefaultClock::GetInstance());
@@ -335,7 +335,7 @@ ExtensionPrefs* ExtensionPrefs::Create(
     const base::FilePath& root_dir,
     ExtensionPrefValueMap* extension_pref_value_map,
     bool extensions_disabled,
-    const std::vector<ExtensionPrefsObserver*>& early_observers,
+    const std::vector<EarlyExtensionPrefsObserver*>& early_observers,
     base::Clock* clock) {
   return new ExtensionPrefs(browser_context, pref_service, root_dir,
                             extension_pref_value_map, clock,
@@ -1815,7 +1815,7 @@ ExtensionPrefs::ExtensionPrefs(
     ExtensionPrefValueMap* extension_pref_value_map,
     base::Clock* clock,
     bool extensions_disabled,
-    const std::vector<ExtensionPrefsObserver*>& early_observers)
+    const std::vector<EarlyExtensionPrefsObserver*>& early_observers)
     : browser_context_(browser_context),
       prefs_(prefs),
       install_directory_(root_dir),
@@ -1827,7 +1827,7 @@ ExtensionPrefs::ExtensionPrefs(
   // Ensure that any early observers are watching before prefs are initialized.
   for (auto iter = early_observers.cbegin(); iter != early_observers.cend();
        ++iter) {
-    AddObserver(*iter);
+    (*iter)->OnExtensionPrefsAvailable(this);
   }
 
   InitPrefStore();

extensions/browser/extension_prefs.h

@@ -52,6 +52,7 @@ class PrefRegistrySyncable;
 namespace extensions {
 
 class AppSorting;
+class EarlyExtensionPrefsObserver;
 class ExtensionPrefsObserver;
 class URLPatternSet;
 
@@ -140,16 +141,16 @@ class ExtensionPrefs : public KeyedService {
   // Creates an ExtensionPrefs object.
   // Does not take ownership of |prefs| or |extension_pref_value_map|.
   // If |extensions_disabled| is true, extension controlled preferences and
-  // content settings do not become effective. ExtensionPrefsObservers should be
-  // included in |early_observers| if they need to observe events which occur
-  // during initialization of the ExtensionPrefs object.
+  // content settings do not become effective. EarlyExtensionPrefsObservers
+  // should be included in |early_observers| if they need to observe events
+  // which occur during initialization of the ExtensionPrefs object.
   static ExtensionPrefs* Create(
       content::BrowserContext* browser_context,
       PrefService* prefs,
       const base::FilePath& root_dir,
       ExtensionPrefValueMap* extension_pref_value_map,
       bool extensions_disabled,
-      const std::vector<ExtensionPrefsObserver*>& early_observers);
+      const std::vector<EarlyExtensionPrefsObserver*>& early_observers);
 
   // A version of Create which allows injection of a custom base::Time provider.
   // Use this as needed for testing.
@@ -159,7 +160,7 @@ class ExtensionPrefs : public KeyedService {
       const base::FilePath& root_dir,
       ExtensionPrefValueMap* extension_pref_value_map,
       bool extensions_disabled,
-      const std::vector<ExtensionPrefsObserver*>& early_observers,
+      const std::vector<EarlyExtensionPrefsObserver*>& early_observers,
       base::Clock* clock);
 
   ~ExtensionPrefs() override;
@@ -618,13 +619,14 @@ class ExtensionPrefs : public KeyedService {
   };
 
   // See the Create methods.
-  ExtensionPrefs(content::BrowserContext* browser_context,
-                 PrefService* prefs,
-                 const base::FilePath& root_dir,
-                 ExtensionPrefValueMap* extension_pref_value_map,
-                 base::Clock* clock,
-                 bool extensions_disabled,
-                 const std::vector<ExtensionPrefsObserver*>& early_observers);
+  ExtensionPrefs(
+      content::BrowserContext* browser_context,
+      PrefService* prefs,
+      const base::FilePath& root_dir,
+      ExtensionPrefValueMap* extension_pref_value_map,
+      base::Clock* clock,
+      bool extensions_disabled,
+      const std::vector<EarlyExtensionPrefsObserver*>& early_observers);
 
   // Converts absolute paths in the pref to paths relative to the
   // install_directory_.

extensions/browser/extension_prefs_factory.cc

@@ -49,7 +49,7 @@ ExtensionPrefsFactory::~ExtensionPrefsFactory() {
 KeyedService* ExtensionPrefsFactory::BuildServiceInstanceFor(
     content::BrowserContext* context) const {
   ExtensionsBrowserClient* client = ExtensionsBrowserClient::Get();
-  std::vector<ExtensionPrefsObserver*> prefs_observers;
+  std::vector<EarlyExtensionPrefsObserver*> prefs_observers;
   client->GetEarlyExtensionPrefsObservers(context, &prefs_observers);
   return ExtensionPrefs::Create(
       context, client->GetPrefServiceForContext(context),

extensions/browser/extension_prefs_observer.h

@@ -49,6 +49,39 @@ class ExtensionPrefsObserver {
       const std::string& extension_id) {}
 };
 
+// An ExtensionPrefsObserver that's part of the GetEarlyExtensionPrefsObservers
+// mechanism, where the ExtensionPrefsObserver needs to connect to an
+// ExtensionPrefs, but the ExtensionPrefs doesn't exist yet. This
+// OnExtensionPrefsAvailable method lets the connection happen during or
+// shortly after the ExtensionPrefs constructor.
+class EarlyExtensionPrefsObserver {
+ public:
+  // Called when "prefs->AddObserver(observer)" should be called, during or
+  // shortly after |prefs|' constructor. OnExtensionPrefsAvailable
+  // implementations should make that AddObserver call, but are also
+  // responsible for making the matching RemoveObserver call at an appropriate
+  // time, no later than during the observer's destructor. Otherwise, the
+  // observee (the |prefs| object) will follow a dangling pointer whenever the
+  // next event occurs.
+  //
+  // Making that RemoveObserver call at the right time has to be the
+  // responsibility of the observer, not the observee, since the observee does
+  // not know when the observer is destroyed or is otherwise no longer
+  // interested in events.
+  //
+  // Given that the observer is responsible for calling RemoveObserver, it is
+  // cleaner for the observer, not the observee, to also be responsible for
+  // calling AddObserver.
+  //
+  // The recommended technique for ensuring matching AddObserver and
+  // RemoveObserver calls is to used a ScopedObserver.
+  //
+  // Unlike other ExtensionPrefsObserver methods, this method does not have an
+  // "const std::string& extension_id" argument. It is not about any one
+  // particular extension, it is about the extension preferences as a whole.
+  virtual void OnExtensionPrefsAvailable(ExtensionPrefs* prefs) = 0;
+};
+
 }  // namespace extensions
 
 #endif  // EXTENSIONS_BROWSER_EXTENSION_PREFS_OBSERVER_H_

extensions/browser/extensions_browser_client.h

@@ -63,7 +63,6 @@ class Extension;
 class ExtensionCache;
 class ExtensionError;
 class ExtensionHostDelegate;
-class ExtensionPrefsObserver;
 class ExtensionApiFrameIdMap;
 class ExtensionApiFrameIdMapHelper;
 class ExtensionNavigationUIData;
@@ -206,7 +205,7 @@ class ExtensionsBrowserClient {
   // are not owned by ExtensionPrefs.
   virtual void GetEarlyExtensionPrefsObservers(
       content::BrowserContext* context,
-      std::vector<ExtensionPrefsObserver*>* observers) const = 0;
+      std::vector<EarlyExtensionPrefsObserver*>* observers) const = 0;
 
   // Returns the ProcessManagerDelegate shared across all BrowserContexts. May
   // return NULL in tests or for simple embedders.

extensions/browser/extensions_test.cc

@@ -97,7 +97,7 @@ void ExtensionsTest::SetUp() {
       browser_context(), pref_service_.get(),
       browser_context()->GetPath().AppendASCII("Extensions"),
       extension_pref_value_map_.get(), false /* extensions_disabled */,
-      std::vector<ExtensionPrefsObserver*>()));
+      std::vector<EarlyExtensionPrefsObserver*>()));
 
   ExtensionPrefsFactory::GetInstance()->SetInstanceForTesting(
       browser_context(), std::move(extension_prefs));

extensions/browser/test_extensions_browser_client.cc

@@ -172,7 +172,7 @@ PrefService* TestExtensionsBrowserClient::GetPrefServiceForContext(
 
 void TestExtensionsBrowserClient::GetEarlyExtensionPrefsObservers(
     content::BrowserContext* context,
-    std::vector<ExtensionPrefsObserver*>* observers) const {}
+    std::vector<EarlyExtensionPrefsObserver*>* observers) const {}
 
 ProcessManagerDelegate* TestExtensionsBrowserClient::GetProcessManagerDelegate()
     const {

extensions/browser/test_extensions_browser_client.h

@@ -112,7 +112,7 @@ class TestExtensionsBrowserClient : public ExtensionsBrowserClient {
       content::BrowserContext* context) override;
   void GetEarlyExtensionPrefsObservers(
       content::BrowserContext* context,
-      std::vector<ExtensionPrefsObserver*>* observers) const override;
+      std::vector<EarlyExtensionPrefsObserver*>* observers) const override;
   ProcessManagerDelegate* GetProcessManagerDelegate() const override;
   std::unique_ptr<ExtensionHostDelegate> CreateExtensionHostDelegate() override;
   bool DidVersionUpdate(content::BrowserContext* context) override;

extensions/shell/browser/shell_extensions_browser_client.cc

@@ -178,8 +178,7 @@ PrefService* ShellExtensionsBrowserClient::GetPrefServiceForContext(
 
 void ShellExtensionsBrowserClient::GetEarlyExtensionPrefsObservers(
     content::BrowserContext* context,
-    std::vector<ExtensionPrefsObserver*>* observers) const {
-}
+    std::vector<EarlyExtensionPrefsObserver*>* observers) const {}
 
 ProcessManagerDelegate*
 ShellExtensionsBrowserClient::GetProcessManagerDelegate() const {

extensions/shell/browser/shell_extensions_browser_client.h

@@ -83,7 +83,7 @@ class ShellExtensionsBrowserClient : public ExtensionsBrowserClient {
       content::BrowserContext* context) override;
   void GetEarlyExtensionPrefsObservers(
       content::BrowserContext* context,
-      std::vector<ExtensionPrefsObserver*>* observers) const override;
+      std::vector<EarlyExtensionPrefsObserver*>* observers) const override;
   ProcessManagerDelegate* GetProcessManagerDelegate() const override;
   std::unique_ptr<ExtensionHostDelegate> CreateExtensionHostDelegate() override;
   bool DidVersionUpdate(content::BrowserContext* context) override;