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

chrome/browser/extensions/extension_prefs.cc

Index: chrome/browser/extensions/extension_prefs.cc
diff --git a/chrome/browser/extensions/extension_prefs.cc b/chrome/browser/extensions/extension_prefs.cc
index 96e497f2e1bc8e913d696b887332ed2f4ae45811..1586c3376a0924aaefa4f028ccd1de563039a2e6 100644
--- a/chrome/browser/extensions/extension_prefs.cc
+++ b/chrome/browser/extensions/extension_prefs.cc
@@ -7,6 +7,7 @@
 #include "base/string_util.h"
 #include "base/string_number_conversions.h"
 #include "base/utf_string_conversions.h"
+#include "chrome/browser/profile.h"
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/extensions/url_pattern.h"
 #include "chrome/common/notification_service.h"
@@ -91,6 +92,14 @@ const char kPrefGrantedPermissionsAPI[] = "granted_permissions.api";
 const char kPrefGrantedPermissionsHost[] = "granted_permissions.host";
 const char kPrefGrantedPermissionsAll[] = "granted_permissions.full";
 
+// A preference that indicates when an extension was installed.
+const char kPrefInstallTime[] = "install_time";
+
+// A preference that indicates the last effective preference values of an
+// extension. The value is a dictionary mapping (non-expanded) preference keys
+// to the values configured by the extension.
+const char kPrefPreferences[] = "preferences";
+
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -133,14 +142,19 @@ static void ExtentToStringSet(const ExtensionExtent& host_extent,
 
 }  // namespace
 
-ExtensionPrefs::ExtensionPrefs(PrefService* prefs, const FilePath& root_dir)
-    : prefs_(prefs),
+ExtensionPrefs::ExtensionPrefs(Profile* profile,
+                               PrefService* prefs,
+                               const FilePath& root_dir)
+    : profile_(profile),
+      prefs_(prefs),
       install_directory_(root_dir) {
   // TODO(asargent) - Remove this in a couple of months. (See comment above
   // CleanupBadExtensionKeys).
   CleanupBadExtensionKeys(prefs);
 
   MakePathsRelative();
+
+  InstallPersistedExtensionControlledPrefs();
 }
 
 ExtensionPrefs::~ExtensionPrefs() {}
@@ -642,7 +656,7 @@ void ExtensionPrefs::GetKilledExtensionIds(std::set<std::string>* killed_ids) {
 }
 
 std::vector<std::string> ExtensionPrefs::GetToolbarOrder() {
-  std::vector<std::string> extension_ids;
+  ExtensionPrefs::ExtensionIdSet extension_ids;
   const ListValue* toolbar_order = prefs_->GetList(kExtensionToolbar);
   if (toolbar_order) {
     for (size_t i = 0; i < toolbar_order->GetSize(); ++i) {
@@ -669,12 +683,18 @@ void ExtensionPrefs::OnExtensionInstalled(
     const Extension* extension, Extension::State initial_state,
     bool initial_incognito_enabled) {
   const std::string& id = extension->id();
+  const base::Time install_time = GetCurrentTime();
   UpdateExtensionPref(id, kPrefState,
                       Value::CreateIntegerValue(initial_state));
   UpdateExtensionPref(id, kPrefIncognitoEnabled,
                       Value::CreateBooleanValue(initial_incognito_enabled));
   UpdateExtensionPref(id, kPrefLocation,
                       Value::CreateIntegerValue(extension->location()));
+  UpdateExtensionPref(id, kPrefInstallTime,
+                      Value::CreateStringValue(
+                          base::Int64ToString(install_time.ToInternalValue())));
+  UpdateExtensionPref(id, kPrefPreferences, new DictionaryValue());
+
   FilePath::StringType path = MakePathRelative(install_directory_,
       extension->path(), NULL);
   UpdateExtensionPref(id, kPrefPath, Value::CreateStringValue(path));
@@ -692,6 +712,9 @@ void ExtensionPrefs::OnExtensionInstalled(
 void ExtensionPrefs::OnExtensionUninstalled(const std::string& extension_id,
                                             const Extension::Location& location,
                                             bool external_uninstall) {
+  PrefKeySet pref_keys;
+  GetExtensionControlledPrefKeys(extension_id, &pref_keys);
+
   // For external extensions, we save a preference reminding ourself not to try
   // and install the extension anymore (except when |external_uninstall| is
   // true, which signifies that the registry key was deleted or the pref file
@@ -703,10 +726,13 @@ void ExtensionPrefs::OnExtensionUninstalled(const std::string& extension_id,
   } else {
     DeleteExtensionPrefs(extension_id);
   }
+
+  for (PrefKeySet::iterator i = pref_keys.begin(); i != pref_keys.end(); ++i)
+    UpdateWinningPref(*i, false);

[Mattias Nissler (ping if slow), 2010/12/01 10:36:36]

I believe Aaron already asked to factor this loop into its own function, since
you have it 3 times in the code. Any reason not to do so?

[battre (please use the other), 2010/12/01 17:44:38]

Done.

 }
 
 Extension::State ExtensionPrefs::GetExtensionState(
-    const std::string& extension_id) {
+    const std::string& extension_id) const {
   DictionaryValue* extension = GetExtensionPref(extension_id);
 
   // If the extension doesn't have a pref, it's a --load-extension.
@@ -727,6 +753,12 @@ void ExtensionPrefs::SetExtensionState(const Extension* extension,
                                        Extension::State state) {
   UpdateExtensionPref(extension->id(), kPrefState,
                       Value::CreateIntegerValue(state));
+
+  PrefKeySet pref_keys;
+  GetExtensionControlledPrefKeys(extension->id(), &pref_keys);
+  for (PrefKeySet::iterator i = pref_keys.begin(); i != pref_keys.end(); ++i)
+    UpdateWinningPref(*i, false);
+
   SavePrefsAndNotify();
 }
 
@@ -830,6 +862,17 @@ DictionaryValue* ExtensionPrefs::GetExtensionPref(
   return extension;
 }
 
+DictionaryValue* ExtensionPrefs::GetExtensionControlledPrefs(
+    const std::string& extension_id) const {
+  DictionaryValue* extension = GetExtensionPref(extension_id);
+  // If the extension doesn't have a pref, it's a --load-extension.
+  if (!extension)
+    return NULL;

[Mattias Nissler (ping if slow), 2010/12/01 10:36:36]

Add a newline here and above before the comment? Jochen wouldn't like it, but I
think the rest of this file has these newlines, so we should be consistent.

[battre (please use the other), 2010/12/01 17:44:38]

Done.

+  DictionaryValue* preferences = NULL;
+  extension->GetDictionary(kPrefPreferences, &preferences);
+  return preferences;
+}
+
 // Helper function for GetInstalledExtensionsInfo.
 static ExtensionInfo* GetInstalledExtensionInfoImpl(
     DictionaryValue* extension_data,
@@ -1089,6 +1132,181 @@ std::string ExtensionPrefs::GetUpdateUrlData(const std::string& extension_id) {
   return data;
 }
 
+base::Time ExtensionPrefs::GetCurrentTime() const {
+  return base::Time::Now();
+}
+
+base::Time ExtensionPrefs::GetInstallTime(
+    const std::string& extension_id) const {
+  const DictionaryValue* extension = GetExtensionPref(extension_id);
+  // If the extension doesn't have a pref, it's a --load-extension.
+  if (!extension)
+    return base::Time::Time();

[Mattias Nissler (ping if slow), 2010/12/01 10:36:36]

Some whitespace here, too?

[battre (please use the other), 2010/12/01 17:44:38]

Done.

+  std::string install_time_str("0");
+  extension->GetString(kPrefInstallTime, &install_time_str);
+  int64 install_time_i64 = 0;
+  base::StringToInt64(install_time_str, &install_time_i64);
+  LOG_IF(ERROR, install_time_i64 == 0)
+      << "Error parsing installation time of an extension.";
+  return base::Time::FromInternalValue(install_time_i64);
+}
+
+void ExtensionPrefs::GetEnabledExtensions(ExtensionIdSet* out) const {
+  DCHECK(out);
+  const DictionaryValue* extensions =
+      pref_service()->GetDictionary(kExtensionsPref);
+
+  for (DictionaryValue::key_iterator ext_id = extensions->begin_keys();
+       ext_id != extensions->end_keys(); ++ext_id) {
+    if (GetExtensionState(*ext_id) != Extension::ENABLED)
+      continue;
+    out->push_back(*ext_id);
+  }
+}
+
+void ExtensionPrefs::FixMissingPrefs(const ExtensionIdSet& extension_ids) {
+  // Fix old entries that did not get an installation time entry when they
+  // were installed or don't have a preferences field.
+  bool persist_required = false;
+  for (ExtensionIdSet::const_iterator ext_id = extension_ids.begin();
+       ext_id != extension_ids.end(); ++ext_id) {
+    DictionaryValue* extension = GetExtensionPref(*ext_id);
+    // If the extension doesn't have a pref, it's a --load-extension.
+    if (extension == NULL)
+      continue;
+
+    if (GetInstallTime(*ext_id) == base::Time::Time()) {
+      const base::Time install_time = GetCurrentTime();
+      extension->Set(kPrefInstallTime,
+                     Value::CreateStringValue(
+                         base::Int64ToString(install_time.ToInternalValue())));
+      persist_required = true;
+    }
+  }
+  if (persist_required)
+    SavePrefsAndNotify();
+}
+
+void ExtensionPrefs::InstallPersistedExtensionControlledPrefs() {
+  // When this is called, the PrefService is initialized and provides access
+  // to the user preferences stored in a JSON file.
+  ExtensionIdSet extension_ids;
+  GetEnabledExtensions(&extension_ids);
+  FixMissingPrefs(extension_ids);
+
+  // Collect the unique extension controlled preference keys of all extensions.
+  std::set<std::string> ext_controlled_prefs;

[Mattias Nissler (ping if slow), 2010/12/01 10:36:36]

Isn't this a PrefKeySet?

[battre (please use the other), 2010/12/01 17:44:38]

No, we need to filter for *unique* entries. The PrefKeySet is a vector.

I have changed this to collect everything in a PrefKeySet, then sort and
unique-ify it. Then we have three invocations of the same loop of
UpdateWinningPref(...) calls, which I have factored out into a function.

Done.

+  for (ExtensionIdSet::iterator ext_id = extension_ids.begin();
+       ext_id != extension_ids.end(); ++ext_id) {
+    DictionaryValue* preferences = GetExtensionControlledPrefs(*ext_id);

[Mattias Nissler (ping if slow), 2010/12/01 10:36:36]

Why not just call GetExtensionControlledPrefKeys(&ext_controlled_prefs) here?

[battre (please use the other), 2010/12/01 17:44:38]

See above. Done.

+    if (!preferences)
+      continue;
+    for (DictionaryValue::key_iterator prefkey = preferences->begin_keys();
+         prefkey != preferences->end_keys(); ++prefkey) {
+      ext_controlled_prefs.insert(*prefkey);
+    }
+  }
+
+  // Store winning preference for each extension controlled preference.
+  // Note that we need to suppress notifications here as the preferences
+  // have not been registered with the PrefService, when this function
+  // is called.

[Mattias Nissler (ping if slow), 2010/12/01 10:36:36]

I don't understand why it would be necessary to suppress the notifications then?

[battre (please use the other), 2010/12/01 17:44:38]

The notification mechanism checked that the value for which we send
notifications has a governor. Usually the default pref store becomes the
governor for preferences when they are registered. However, this has not
happened at this time. Therefore, everything we insert here, has not default
preference, yet. Therefore, the notification triggered a DCHECK violation.

After discussion with mnissler, I have changed PrefValueStore to ignore
notifications of preferences that have not been registered, yet. Now
ExtensionPrefs verifies in SetExtensionControlledPref that preferences have been
registered before they are set.

Done.

+  for (std::set<std::string>::iterator i = ext_controlled_prefs.begin();
+       i != ext_controlled_prefs.end(); ++i) {
+    UpdateWinningPref(*i, true);
+  }
+}
+
+const Value* ExtensionPrefs::WinningExtensionControlledPrefValue(
+    const std::string& key) const {
+  Value *winner = NULL;
+  base::Time winners_install_time = base::Time::Time();
+
+  ExtensionIdSet extension_ids;
+  GetEnabledExtensions(&extension_ids);
+  for (ExtensionIdSet::iterator ext_id = extension_ids.begin();
+       ext_id != extension_ids.end(); ++ext_id) {
+    base::Time extension_install_time = GetInstallTime(*ext_id);
+
+    // We do not need to consider extensions that were installed before the
+    // most recent extension found that provides the requested preference.
+    if (extension_install_time < winners_install_time)
+      continue;
+
+    DictionaryValue* preferences = GetExtensionControlledPrefs(*ext_id);
+    Value *value = NULL;
+    if (preferences && preferences->GetWithoutPathExpansion(key, &value)) {
+      // This extension is more recent than the last one providing this pref.
+      winner = value;
+      winners_install_time = extension_install_time;
+    }
+  }
+
+  return winner;
+}
+
+void ExtensionPrefs::UpdateWinningPref(const std::string& pref_key,
+                                       bool suppressNotification) {
+  PrefStore* extension_pref_store = profile_->GetExtensionPrefStore();
+  const Value* winning_pref_value =
+      WinningExtensionControlledPrefValue(pref_key);
+  Value* old_value = NULL;
+  extension_pref_store->prefs()->Get(pref_key, &old_value);
+  bool changed = !Value::Equals(winning_pref_value, old_value);
+
+  if (winning_pref_value)
+    extension_pref_store->prefs()->Set(pref_key,
+                                       winning_pref_value->DeepCopy());

[Mattias Nissler (ping if slow), 2010/12/01 10:36:36]

Need curlies here.

[battre (please use the other), 2010/12/01 17:44:38]

Done.

+  else
+    extension_pref_store->prefs()->Remove(pref_key, NULL);
+
+  if (changed && !suppressNotification)

[Mattias Nissler (ping if slow), 2010/12/01 10:36:36]

FYI to other reviewers: Note that there is work underway that will change the
PrefStore interface to not allow to write to prefs(). This means that the whole
business of updating the value in ExtensionPrefStore and sending out
notifications will move to there.

+    pref_service()->pref_notifier()->OnPreferenceSet(
+        pref_key.c_str(), PrefNotifier::EXTENSION_STORE);

[Mattias Nissler (ping if slow), 2010/12/01 10:36:36]

curlies here as well.

[battre (please use the other), 2010/12/01 17:44:38]

Done.

+}
+
+void ExtensionPrefs::SetExtensionControlledPref(const std::string& extension_id,
+                                                const std::string& pref_key,
+                                                Value* value) {
+  DictionaryValue* extension_preferences =
+      GetExtensionControlledPrefs(extension_id);
+
+  if (extension_preferences == NULL) {  // May be pruned when writing to disk.
+    DictionaryValue* extension = GetExtensionPref(extension_id);
+    CHECK(extension);
+    // TODO(battre) what do we do with --load-extension extensions?
+    // extension might be null.

[Mattias Nissler (ping if slow), 2010/12/01 10:36:36]

They just can't override prefs? Or should we create the entry? If we had
separate pref paths for regular prefs and the override cache, we wouldn't run
into this problem, right? I don't want to start this discussion again though.
Aaron?

However, the CHECK() is the wrong answer, since that will make us crash
reproducibly. So please at least turn it into a LOG(WARNING) + return

[battre (please use the other), 2010/12/01 17:44:38]

Done.

+    extension_preferences = new DictionaryValue;
+    extension->Set(kPrefPreferences, extension_preferences);
+  }
+
+  Value* oldValue = NULL;
+  extension_preferences->GetWithoutPathExpansion(pref_key, &oldValue);
+  bool modified = !Value::Equals(oldValue, value);
+  if (!modified)
+    return;
+
+  if (value == NULL)
+    extension_preferences->RemoveWithoutPathExpansion(pref_key, NULL);
+  else
+    extension_preferences->SetWithoutPathExpansion(pref_key, value);
+  pref_service()->ScheduleSavePersistentPrefs();
+
+  UpdateWinningPref(pref_key, false);
+}
+
+void ExtensionPrefs::GetExtensionControlledPrefKeys(
+    const std::string& extension_id, PrefKeySet *out) const {
+  DCHECK(out != NULL);
+  DictionaryValue* ext_prefs = GetExtensionControlledPrefs(extension_id);
+  if (ext_prefs) {
+    for (DictionaryValue::key_iterator i = ext_prefs->begin_keys();
+         i != ext_prefs->end_keys(); ++i) {
+      out->push_back(*i);
+    }
+  }
+}
+
 // static
 void ExtensionPrefs::RegisterUserPrefs(PrefService* prefs) {
   prefs->RegisterDictionaryPref(kExtensionsPref);