Handle changes in extension ID generation logic better.

chrome/browser/extensions/extension_prefs.cc

 // Copyright (c) 2011 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.
 
 #include "chrome/browser/extensions/extension_prefs.h"
 
 #include "base/string_number_conversions.h"
 #include "base/string_util.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/extensions/extension_pref_store.h"
@@ skipping 101 matching lines @@
 
 // A preference that contains any extension-controlled preferences.
 const char kPrefPreferences[] = "preferences";
 
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
 
 namespace {
 
-// TODO(asargent) - This is cleanup code for a key that was introduced into
-// the extensions.settings sub-dictionary which wasn't a valid extension
-// id. We can remove this in a couple of months. (See http://crbug.com/40017
-// and http://crbug.com/39745 for more details).
-static void CleanupBadExtensionKeys(PrefService* prefs) {
+// TODO(mihaip): This is cleanup code for keys for unpacked extensions (which
+// are derived from paths). As part of the wstring removal, we changed the way
+// we hash paths, so we need to move prefs from their old synthesized IDs to
+// their new ones. We can remove this by July 2011. (See http://crbug.com/75945
+// for more details).
+static void CleanupBadExtensionKeys(const FilePath& root_dir,
+                                    PrefService* prefs) {
   DictionaryValue* dictionary =
       prefs->GetMutableDictionary(ExtensionPrefs::kExtensionsPref);
-  std::set<std::string> bad_keys;
+  std::map<std::string, std::string> remapped_keys;
   for (DictionaryValue::key_iterator i = dictionary->begin_keys();
        i != dictionary->end_keys(); ++i) {
-    const std::string& key_name(*i);
-    if (!Extension::IdIsValid(key_name)) {
-      bad_keys.insert(key_name);
+    DictionaryValue* ext;
+    if (!dictionary->GetDictionaryWithoutPathExpansion(*i, &ext))
+      continue;
+
+    int location;
+    FilePath::StringType path_str;
+    if (!ext->GetInteger(kPrefLocation, &location) ||
+        !ext->GetString(kPrefPath, &path_str)) {
+      continue;
+    }
+
+    // Only unpacked extensions have generated IDs.
+    if (location != Extension::LOAD)
+      continue;
+
+    const std::string& prefs_id(*i);
+    FilePath path(path_str);
+    // The persisted path can be relative to the root dir (see
+    // MakePath(s)Relative), but the ID is generated before that, using the
+    // absolute path, so we need to undo that.
+    if (!path.IsAbsolute()) {
+      path = root_dir.Append(path);
+    }
+    std::string computed_id = Extension::GenerateIdForPath(path);
+
+    if (prefs_id != computed_id) {
+      DCHECK(!dictionary->HasKey(computed_id));

[asargent_no_longer_on_chrome, 2011/04/04 23:46:15]

Are you positive it's not possible for someone's prefs to get into this state
where there is already an entry under the correct key? Would it make sense to
just delete the value under the incorrect key and leave whatever *is* there
under the correct one if this actually happened?

[Mihai Parparita -not on Chrome, 2011/04/05 00:22:47]

Just deleting the value under the incorrect key seems safer, switched to that.

+      remapped_keys[prefs_id] = computed_id;
     }
   }
-  bool dirty = false;
-  for (std::set<std::string>::iterator i = bad_keys.begin();
-       i != bad_keys.end(); ++i) {
-    dirty = true;
-    dictionary->Remove(*i, NULL);
+
+  if (!remapped_keys.empty()) {
+    for (std::map<std::string, std::string>::const_iterator i =
+            remapped_keys.begin();
+        i != remapped_keys.end();
+        ++i) {
+      Value* extension_prefs;

[asargent_no_longer_on_chrome, 2011/04/04 23:46:15]

nit: initialize to NULL

[Mihai Parparita -not on Chrome, 2011/04/05 00:22:47]

Done.

+      DCHECK(dictionary->RemoveWithoutPathExpansion(
+          i->first, &extension_prefs));

[asargent_no_longer_on_chrome, 2011/04/04 23:46:15]

You should probably either change this to a CHECK, or handle the failure
gracefully (otherwise you'll assign the garbage/NULL in extension_prefs into the
dictionary).

[Mihai Parparita -not on Chrome, 2011/04/05 00:22:47]

Changed to CHECK.

+      dictionary->SetWithoutPathExpansion(i->second, extension_prefs);
+    }
+
+    prefs->ScheduleSavePersistentPrefs();
   }
-  if (dirty)
-    prefs->ScheduleSavePersistentPrefs();
 }
 
 static void ExtentToStringSet(const ExtensionExtent& host_extent,
                               std::set<std::string>* result) {
   ExtensionExtent::PatternList patterns = host_extent.patterns();
   ExtensionExtent::PatternList::const_iterator i;
 
   for (i = patterns.begin(); i != patterns.end(); ++i)
     result->insert(i->GetAsString());
 }
 
 }  // namespace
 
 ExtensionPrefs::ExtensionPrefs(
     PrefService* prefs,
     const FilePath& root_dir,
     ExtensionPrefValueMap* extension_pref_value_map)
     : prefs_(prefs),
       install_directory_(root_dir),
       extension_pref_value_map_(extension_pref_value_map) {
-  // TODO(asargent) - Remove this in a couple of months. (See comment above
-  // CleanupBadExtensionKeys).
-  CleanupBadExtensionKeys(prefs_);
+  // TODO(mihaip): Remove this by July 2011 (see comment above).
+  CleanupBadExtensionKeys(root_dir, prefs_);
 
   MakePathsRelative();
 
   InitPrefStore();
 }
 
 ExtensionPrefs::~ExtensionPrefs() {}
 
 // static
 const char ExtensionPrefs::kExtensionsPref[] = "extensions.settings";
@@ skipping 1252 matching lines @@
 void ExtensionPrefs::RegisterUserPrefs(PrefService* prefs) {
   prefs->RegisterDictionaryPref(kExtensionsPref);
   prefs->RegisterListPref(kExtensionToolbar);
   prefs->RegisterIntegerPref(prefs::kExtensionToolbarSize, -1);
   prefs->RegisterDictionaryPref(kExtensionsBlacklistUpdate);
   prefs->RegisterListPref(prefs::kExtensionInstallAllowList);
   prefs->RegisterListPref(prefs::kExtensionInstallDenyList);
   prefs->RegisterListPref(prefs::kExtensionInstallForceList);
   prefs->RegisterStringPref(kWebStoreLogin, std::string() /* default_value */);
 }