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

chrome/browser/profile_impl.h

 // Copyright (c) 2010 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.
 
 // This class gathers state related to a single user profile.
 
 #ifndef CHROME_BROWSER_PROFILE_IMPL_H_
 #define CHROME_BROWSER_PROFILE_IMPL_H_
 #pragma once
 
 #include "base/file_path.h"
 #include "base/ref_counted.h"
 #include "base/scoped_ptr.h"
 #include "base/timer.h"
 #include "chrome/browser/profile.h"
 #include "chrome/browser/prefs/pref_change_registrar.h"
 #include "chrome/browser/spellcheck_host_observer.h"
 #include "chrome/common/notification_observer.h"
 #include "chrome/common/notification_registrar.h"
 
+class ExtensionPrefs;
 class PrefService;
+class PrefStore;
 
 #if defined(OS_CHROMEOS)
 namespace chromeos {
 class Preferences;
 }
 #endif
 
 class NetPrefObserver;
 
 // The default profile implementation.
@@ skipping 27 matching lines @@
   virtual ExtensionMessageService* GetExtensionMessageService();
   virtual ExtensionEventRouter* GetExtensionEventRouter();
   virtual FaviconService* GetFaviconService(ServiceAccessType sat);
   virtual HistoryService* GetHistoryService(ServiceAccessType sat);
   virtual HistoryService* GetHistoryServiceWithoutCreating();
   virtual AutocompleteClassifier* GetAutocompleteClassifier();
   virtual WebDataService* GetWebDataService(ServiceAccessType sat);
   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();
   virtual PersonalDataManager* GetPersonalDataManager();
   virtual BrowserFileSystemContext* GetFileSystemContext();
   virtual void InitThemes();
   virtual void SetTheme(const Extension* extension);
   virtual void SetNativeTheme();
   virtual void ClearTheme();
   virtual const Extension* GetTheme();
@@ skipping 82 matching lines @@
   }
 
   void RegisterComponentExtensions();
   void InstallDefaultApps();
 
   NotificationRegistrar registrar_;
   PrefChangeRegistrar pref_change_registrar_;
 
   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_;
   scoped_ptr<ExtensionProcessManager> extension_process_manager_;
   scoped_refptr<ExtensionMessageService> extension_message_service_;
   scoped_ptr<ExtensionEventRouter> extension_event_router_;
   scoped_ptr<SSLHostState> ssl_host_state_;
   scoped_refptr<net::TransportSecurityState>
       transport_security_state_;
   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_;
   scoped_ptr<BookmarkModel> bookmark_bar_model_;
   scoped_refptr<WebResourceService> web_resource_service_;
   scoped_ptr<NTPResourceCache> ntp_resource_cache_;
 
   scoped_ptr<TokenService> token_service_;
   scoped_ptr<ProfileSyncFactory> profile_sync_factory_;
   scoped_ptr<ProfileSyncService> sync_service_;
@@ skipping 73 matching lines @@
   // The main database tracker for this profile.
   // Should be used only on the file thread.
   scoped_refptr<webkit_database::DatabaseTracker> db_tracker_;
 
   scoped_refptr<history::TopSites> top_sites_;  // For history and thumbnails.
 
   scoped_refptr<ChromeBlobStorageContext> blob_storage_context_;
 
   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_;
 
   scoped_refptr<chromeos::ProxyConfigServiceImpl>
       chromeos_proxy_config_service_impl_;
 #endif
 
   scoped_refptr<PrefProxyConfigTracker> pref_proxy_config_tracker_;
 
   DISALLOW_COPY_AND_ASSIGN(ProfileImpl);
 };
 
 #endif  // CHROME_BROWSER_PROFILE_IMPL_H_