Fix for Bug 50726 "Save extension list and "winning" prefs from extensions"

chrome/browser/profile_impl.h

Index: chrome/browser/profile_impl.h
diff --git a/chrome/browser/profile_impl.h b/chrome/browser/profile_impl.h
index 83a4b935f47f43f64f138d12788c7e94b8b3095e..aa420e7ec838e0c9f2fb18ef430208974bcbc93a 100644
--- a/chrome/browser/profile_impl.h
+++ b/chrome/browser/profile_impl.h
@@ -18,7 +18,9 @@
 #include "chrome/common/notification_observer.h"
 #include "chrome/common/notification_registrar.h"
 
+class ExtensionPrefs;
 class PrefService;
+class PrefStore;
 
 #if defined(OS_CHROMEOS)
 namespace chromeos {
@@ -66,6 +68,8 @@ class ProfileImpl : public Profile,
   virtual WebDataService* GetWebDataServiceWithoutCreating();
   virtual PasswordStore* GetPasswordStore(ServiceAccessType sat);
   virtual PrefService* GetPrefs();
+  virtual PrefStore* GetExtensionPrefStore();
+  virtual void SetExtensionPrefStore(PrefStore* extension_pref_store);
   virtual TemplateURLModel* GetTemplateURLModel();
   virtual TemplateURLFetcher* GetTemplateURLFetcher();
   virtual DownloadManager* GetDownloadManager();
@@ -168,8 +172,12 @@ class ProfileImpl : public Profile,
 
   FilePath path_;
   FilePath base_cache_path_;
+  scoped_ptr<PrefService> prefs_;  // Keep this on top for destruction order

[Aaron Boodman, 2010/12/03 08:58:10]

Scary comment is scary. What is causing the destruction order dependency? Can we
get rid of it?

[battre (please use the other), 2010/12/03 19:32:58]

Well, this is just playing save. The situation has not changed by this CL.
Probably we don't have a real issue here because of threading guarantees. But I
don't know them, yet. I propose to leave this but added a comment "Please remove
comment if you are certain that we don't have an issue here".

[Aaron Boodman, 2010/12/03 20:00:44]

Sorry, I still don't understand. Can you clarify what you are worried about
happening?

[battre (please use the other), 2010/12/06 10:28:50]

Maybe this comment is clearer:
  // Keep prefs_ on top for destruction order because extension_prefs_,
  // net_pref_observer_, web_resource_service_ and background_contents_service_
  // store pointers to prefs_ and shall be destructed first.

[Aaron Boodman, 2010/12/06 18:20:56]

I talked to a few people in this office about this, and we all agreed. This
problem probably exists all over Chrome, and we never comment about it in any of
those places.

If somebody actually changed ExtensionPrefs such that it actually needed to call
PrefService in its destructor, then a better way to document this would be to
reset the extension_prefs_ pointer explicitly in ~Profile.

In general, relying on destruction order seems pretty ugly.

[Mattias Nissler (ping if slow), 2010/12/06 19:29:50]

I agree that relying on implicit destruction order for something as central as
the Profile is really ugly.

Actually, while working on a recent CL that touched the profile, I tried to make
PrefService destruction explicit in the ProfileImpl constructor. I gave up after
I realized that I had to put resets for pretty much everything in the profile
before releasing prefs_. This shows that the current code heavily relies on the
ordering here.

Created http://crbug.com/65637 to track this.

+                                   // because many other class members depend
+                                   // on it.
   scoped_ptr<VisitedLinkEventListener> visited_link_event_listener_;
   scoped_ptr<VisitedLinkMaster> visited_link_master_;
+  scoped_ptr<ExtensionPrefs> extension_prefs_;
   scoped_refptr<ExtensionsService> extensions_service_;
   scoped_refptr<UserScriptMaster> user_script_master_;
   scoped_refptr<ExtensionDevToolsManager> extension_devtools_manager_;
@@ -182,7 +190,6 @@ class ProfileImpl : public Profile,
   scoped_refptr<TransportSecurityPersister>
       transport_security_persister_;
   scoped_ptr<policy::ProfilePolicyContext> profile_policy_context_;
-  scoped_ptr<PrefService> prefs_;
   scoped_ptr<NetPrefObserver> net_pref_observer_;
   scoped_ptr<TemplateURLFetcher> template_url_fetcher_;
   scoped_ptr<TemplateURLModel> template_url_model_;
@@ -276,6 +283,10 @@ class ProfileImpl : public Profile,
 
   scoped_refptr<ExtensionInfoMap> extension_info_map_;
 
+  // weak pointer to ExtensionPrefStore that stores effective extension
+  // controlled preferences.
+  PrefStore* extension_pref_store_;
+
 #if defined(OS_CHROMEOS)
   scoped_ptr<chromeos::Preferences> chromeos_preferences_;