Make pref service more robust against prefs that change their types without updating user pref values.

The outdated value will be ignored, but can be overwritten by a value of the correct type. Developers
who change types of prefs should provide migration functionality if needed. Changing the pref's path
(i.e., its name) is also advisable, but watch out that you don't leave an orphaned pref value in the
user pref file forever.

This change also moves tracking of preference types out of PrefService::Preference and into the
PrefValueStore, because the latter now needs to know the registered pref types. This is an
implementation change, only "externally" visible to unit tests.

BUG=55552
TEST=covered by unit tests; also, Chrome launched with a pre-June 2010 profile containing
    kWebkitInspectorSettings shouldn't crash
Review URL: http://codereview.chromium.org/3411011

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@59773 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 172 insertions, 62 deletions

diff --git a/chrome/browser/prefs/pref_service.cc b/chrome/browser/prefs/pref_service.cc
index bc87c2f..0668ed4 100644
--- a/chrome/browser/prefs/pref_service.cc
+++ b/chrome/browser/prefs/pref_service.cc
@@ -312,10 +312,7 @@ bool PrefService::HasPrefPath(const char* path) const {
 const PrefService::Preference* PrefService::FindPreference(
     const char* pref_name) const {
   DCHECK(CalledOnValidThread());
-  // We only look up prefs by name, so the type is irrelevant, except that it
-  // must be one that Preference() allows (i.e., neither TYPE_NULL nor
-  // TYPE_BINARY).
-  Preference p(this, pref_name, Value::TYPE_INTEGER);
+  Preference p(this, pref_name);
   PreferenceSet::const_iterator it = prefs_.find(&p);
   return it == prefs_.end() ? NULL : *it;
 }
@@ -377,10 +374,13 @@ void PrefService::RegisterPreference(const char* path, Value* default_value) {
     return;
   }
 
+  Value::ValueType orig_type = default_value->GetType();
+  DCHECK(orig_type != Value::TYPE_NULL && orig_type != Value::TYPE_BINARY) <<
+         "invalid preference type: " << orig_type;
+
   // We set the default value of dictionaries and lists to be null so it's
   // easier for callers to check for empty dict/list prefs. The PrefValueStore
   // accepts ownership of the value (null or default_value).
-  Value::ValueType orig_type = default_value->GetType();
   if (Value::TYPE_LIST == orig_type || Value::TYPE_DICTIONARY == orig_type) {
     pref_value_store_->SetDefaultPrefValue(path, Value::CreateNullValue());
   } else {
@@ -388,7 +388,8 @@ void PrefService::RegisterPreference(const char* path, Value* default_value) {
     pref_value_store_->SetDefaultPrefValue(path, scoped_value.release());
   }
 
-  prefs_.insert(new Preference(this, path, orig_type));
+  pref_value_store_->RegisterPreferenceType(path, orig_type);
+  prefs_.insert(new Preference(this, path));
 }
 
 void PrefService::ClearPref(const char* path) {
@@ -416,11 +417,12 @@ void PrefService::Set(const char* path, const Value& value) {
   // user values.
   bool value_changed = false;
   if (value.GetType() == Value::TYPE_NULL &&
-      (pref->type() == Value::TYPE_DICTIONARY ||
-       pref->type() == Value::TYPE_LIST)) {
+      (pref->GetType() == Value::TYPE_DICTIONARY ||
+       pref->GetType() == Value::TYPE_LIST)) {
     value_changed = pref_value_store_->RemoveUserPrefValue(path);
-  } else if (pref->type() != value.GetType()) {
-    NOTREACHED() << "Trying to set pref " << path << " of type " << pref->type()
+  } else if (pref->GetType() != value.GetType()) {
+    NOTREACHED() << "Trying to set pref " << path
+                 << " of type " << pref->GetType()
                  << " to value of type " << value.GetType();
   } else {
     value_changed = pref_value_store_->SetUserPrefValue(path, value.DeepCopy());
@@ -493,7 +495,7 @@ DictionaryValue* PrefService::GetMutableDictionary(const char* path) {
     NOTREACHED() << "Trying to get an unregistered pref: " << path;
     return NULL;
   }
-  if (pref->type() != Value::TYPE_DICTIONARY) {
+  if (pref->GetType() != Value::TYPE_DICTIONARY) {
     NOTREACHED() << "Wrong type for GetMutableDictionary: " << path;
     return NULL;
   }
@@ -518,7 +520,7 @@ ListValue* PrefService::GetMutableList(const char* path) {
     NOTREACHED() << "Trying to get an unregistered pref: " << path;
     return NULL;
   }
-  if (pref->type() != Value::TYPE_LIST) {
+  if (pref->GetType() != Value::TYPE_LIST) {
     NOTREACHED() << "Wrong type for GetMutableList: " << path;
     return NULL;
   }
@@ -555,8 +557,9 @@ void PrefService::SetUserPrefValue(const char* path, Value* new_value) {
     NOTREACHED() << "Preference is managed: " << path;
     return;
   }
-  if (pref->type() != new_value->GetType()) {
-    NOTREACHED() << "Trying to set pref " << path << " of type " << pref->type()
+  if (pref->GetType() != new_value->GetType()) {
+    NOTREACHED() << "Trying to set pref " << path
+                 << " of type " << pref->GetType()
                  << " to value of type " << new_value->GetType();
     return;
   }
@@ -569,15 +572,15 @@ void PrefService::SetUserPrefValue(const char* path, Value* new_value) {
 // PrefService::Preference
 
 PrefService::Preference::Preference(const PrefService* service,
-                                    const char* name,
-                                    Value::ValueType type)
-      : type_(type),
-        name_(name),
+                                    const char* name)
+      : name_(name),
         pref_service_(service) {
   DCHECK(name);
   DCHECK(service);
-  DCHECK(type != Value::TYPE_NULL && type != Value::TYPE_BINARY) <<
-         "invalid preference type: " << type;
+}
+
+Value::ValueType PrefService::Preference::GetType() const {
+  return pref_service_->pref_value_store_->GetRegisteredType(name_);
 }
 
 const Value* PrefService::Preference::GetValue() const {
@@ -585,18 +588,11 @@ const Value* PrefService::Preference::GetValue() const {
       "Must register pref before getting its value";
 
   Value* found_value = NULL;
-  if (pref_service_->pref_value_store_->GetValue(name_, &found_value)) {
-    Value::ValueType found_type = found_value->GetType();
-    // Dictionaries and lists have default values of TYPE_NULL.
-    if (found_type == type_ ||
-        (found_type == Value::TYPE_NULL
-         && (type_ == Value::TYPE_LIST || type_ == Value::TYPE_DICTIONARY))) {
-      return found_value;
-    }
-  }
+  if (pref_service_->pref_value_store_->GetValue(name_, &found_value))
+    return found_value;
 
   // Every registered preference has at least a default value.
-  NOTREACHED() << "no default value for registered pref " << name_;
+  NOTREACHED() << "no valid value found for registered pref " << name_;
   return NULL;
 }
 
diff --git a/chrome/browser/prefs/pref_service.h b/chrome/browser/prefs/pref_service.h
index 3fd11be..450c6c1 100644
--- a/chrome/browser/prefs/pref_service.h
+++ b/chrome/browser/prefs/pref_service.h
@@ -33,16 +33,16 @@ class PrefService : public NonThreadSafe {
     // dictionary (a branch), or list.  You shouldn't need to construct this on
     // your own; use the PrefService::Register*Pref methods instead.
     Preference(const PrefService* service,
-               const char* name,
-               Value::ValueType type);
+               const char* name);
     ~Preference() {}
 
-    Value::ValueType type() const { return type_; }
-
     // Returns the name of the Preference (i.e., the key, e.g.,
     // browser.window_placement).
     const std::string name() const { return name_; }
 
+    // Returns the registered type of the preference.
+    Value::ValueType GetType() const;
+
     // Returns the value of the Preference, falling back to the registered
     // default value if no other has been set.
     const Value* GetValue() const;
@@ -81,7 +81,6 @@ class PrefService : public NonThreadSafe {
    private:
     friend class PrefService;
 
-    Value::ValueType type_;
     std::string name_;
 
     // Reference to the PrefService in which this pref was created.
diff --git a/chrome/browser/prefs/pref_value_store.cc b/chrome/browser/prefs/pref_value_store.cc
index 1dd5a90..46c825d 100644
--- a/chrome/browser/prefs/pref_value_store.cc
+++ b/chrome/browser/prefs/pref_value_store.cc
@@ -4,7 +4,6 @@
 
 #include "chrome/browser/prefs/pref_value_store.h"
 
-#include "base/values.h"
 #include "chrome/browser/chrome_thread.h"
 #include "chrome/browser/extensions/extension_pref_store.h"
 #include "chrome/browser/policy/configuration_policy_pref_store.h"
@@ -13,6 +12,27 @@
 #include "chrome/common/json_pref_store.h"
 #include "chrome/common/notification_service.h"
 
+namespace {
+
+// Returns true if the actual value is a valid type for the expected type when
+// found in the given store.
+bool IsValidType(Value::ValueType expected, Value::ValueType actual,
+                 PrefNotifier::PrefStoreType store) {
+  if (expected == actual)
+    return true;
+
+  // Dictionaries and lists are allowed to hold TYPE_NULL values too, but only
+  // in the default pref store.
+  if (store == PrefNotifier::DEFAULT_STORE &&
+      actual == Value::TYPE_NULL &&
+      (expected == Value::TYPE_DICTIONARY || expected == Value::TYPE_LIST)) {
+    return true;
+  }
+  return false;
+}
+
+}  // namespace
+
 // static
 PrefValueStore* PrefValueStore::CreatePrefValueStore(
     const FilePath& pref_filename,
@@ -46,17 +66,34 @@ bool PrefValueStore::GetValue(const std::string& name,
                               Value** out_value) const {
   // Check the |PrefStore|s in order of their priority from highest to lowest
   // to find the value of the preference described by the given preference name.
+  // If the found value is not the correct type, keep looking. This allows a
+  // default value to override an outdated user-pref setting.
   for (size_t i = 0; i <= PrefNotifier::PREF_STORE_TYPE_MAX; ++i) {
     if (pref_stores_[i].get() &&
-        pref_stores_[i]->prefs()->Get(name.c_str(), out_value)) {
+        pref_stores_[i]->prefs()->Get(name.c_str(), out_value) &&
+        IsValidType(GetRegisteredType(name), (*out_value)->GetType(),
+                    static_cast<PrefNotifier::PrefStoreType>(i))) {
       return true;
     }
   }
-  // No value found for the given preference name: set the return false.
+  // No valid value found for the given preference name: set the return false.
   *out_value = NULL;
   return false;
 }
 
+void PrefValueStore::RegisterPreferenceType(const std::string& name,
+                                            Value::ValueType type) {
+  pref_types_[name] = type;
+}
+
+Value::ValueType PrefValueStore::GetRegisteredType(
+    const std::string& name) const {
+  PrefTypeMap::const_iterator found = pref_types_.find(name);
+  if (found == pref_types_.end())
+    return Value::TYPE_NULL;
+  return found->second;
+}
+
 bool PrefValueStore::WritePrefs() {
   bool success = true;
   for (size_t i = 0; i <= PrefNotifier::PREF_STORE_TYPE_MAX; ++i) {
@@ -189,13 +226,17 @@ PrefNotifier::PrefStoreType PrefValueStore::ControllingPrefStoreForPref(
 }
 
 bool PrefValueStore::PrefValueInStore(const char* name,
-                                      PrefNotifier::PrefStoreType type) const {
-  if (!pref_stores_[type].get()) {
-    // No store of that type set, so this pref can't be in it.
+                                      PrefNotifier::PrefStoreType store) const {
+  if (!pref_stores_[store].get()) {
+    // No store of that type, so this pref can't be in it.
     return false;
   }
-  Value* tmp_value;
-  return pref_stores_[type]->prefs()->Get(name, &tmp_value);
+  Value* tmp_value = NULL;
+  if (pref_stores_[store]->prefs()->Get(name, &tmp_value) &&
+      IsValidType(GetRegisteredType(name), tmp_value->GetType(), store)) {
+    return true;
+  }
+  return false;
 }
 
 void PrefValueStore::RefreshPolicyPrefsCompletion(
diff --git a/chrome/browser/prefs/pref_value_store.h b/chrome/browser/prefs/pref_value_store.h
index 8ea4236..ba2308d 100644
--- a/chrome/browser/prefs/pref_value_store.h
+++ b/chrome/browser/prefs/pref_value_store.h
@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_PREFS_PREF_VALUE_STORE_H_
 #pragma once
 
+#include <map>
 #include <string>
 #include <vector>
 
@@ -14,6 +15,7 @@
 #include "base/gtest_prod_util.h"
 #include "base/ref_counted.h"
 #include "base/scoped_ptr.h"
+#include "base/values.h"
 #include "chrome/browser/chrome_thread.h"
 #include "chrome/browser/prefs/pref_notifier.h"
 #include "chrome/common/pref_store.h"
@@ -21,7 +23,6 @@
 class FilePath;
 class PrefStore;
 class Profile;
-class Value;
 
 // The PrefValueStore manages various sources of values for Preferences
 // (e.g., configuration policies, extensions, and user settings). It returns
@@ -48,12 +49,20 @@ class PrefValueStore : public base::RefCountedThreadSafe<PrefValueStore> {
 
   ~PrefValueStore();
 
-  // Get the preference value for the given preference name.
-  // Return true if a value for the given preference name was found in any of
-  // the available PrefStores. Most callers should use Preference::GetValue()
-  // instead of calling this method directly.
+  // Gets the value for the given preference name that has a valid value type;
+  // that is, the same type the preference was registered with, or NULL for
+  // default values of Dictionaries and Lists. Returns true if a valid value
+  // was found in any of the available PrefStores. Most callers should use
+  // Preference::GetValue() instead of calling this method directly.
   bool GetValue(const std::string& name, Value** out_value) const;
 
+  // Adds a preference to the mapping of names to types.
+  void RegisterPreferenceType(const std::string& name, Value::ValueType type);
+
+  // Gets the registered value type for the given preference name. Returns
+  // Value::TYPE_NULL if the preference has never been registered.
+  Value::ValueType GetRegisteredType(const std::string& name) const;
+
   // Read preference values into the three PrefStores so that they are available
   // through the GetValue method. Return the first error that occurs (but
   // continue reading the remaining PrefStores).
@@ -68,7 +77,8 @@ class PrefValueStore : public base::RefCountedThreadSafe<PrefValueStore> {
   // the user prefs are expected to be written out.
   void ScheduleWritePrefs();
 
-  // Returns true if the PrefValueStore contains the given preference.
+  // Returns true if the PrefValueStore contains the given preference with a
+  // non-default value set with the applicable (registered) type.
   bool HasPrefPath(const char* name) const;
 
   // Called by the PrefNotifier when the value of the preference at |path| has
@@ -175,8 +185,15 @@ class PrefValueStore : public base::RefCountedThreadSafe<PrefValueStore> {
 
   scoped_ptr<PrefStore> pref_stores_[PrefNotifier::PREF_STORE_TYPE_MAX + 1];
 
+  // A mapping of preference names to their registered types.
+  typedef std::map<std::string, Value::ValueType> PrefTypeMap;
+  PrefTypeMap pref_types_;
+
+  // Returns true if the preference with the given name has a value in the
+  // given PrefStoreType, of the same value type as the preference was
+  // registered with.
   bool PrefValueInStore(const char* name,
-                        PrefNotifier::PrefStoreType type) const;
+                        PrefNotifier::PrefStoreType store) const;
 
   // Called during policy refresh after ReadPrefs completes on the thread
   // that initiated the policy refresh. RefreshPolicyPrefsCompletion takes
diff --git a/chrome/browser/prefs/pref_value_store_unittest.cc b/chrome/browser/prefs/pref_value_store_unittest.cc
index 0719153..fcb5b60 100644
--- a/chrome/browser/prefs/pref_value_store_unittest.cc
+++ b/chrome/browser/prefs/pref_value_store_unittest.cc
@@ -43,16 +43,6 @@ namespace prefs {
 }
 
 // Potentially expected values of all preferences used in this test program.
-// The "user" namespace is defined globally in an ARM system header, so we need
-// something different here.
-namespace user_pref {
-  const int kMaxTabsValue = 31;
-  const bool kDeleteCacheValue = true;
-  const char kCurrentThemeIDValue[] = "abcdefg";
-  const char kHomepageValue[] = "http://www.google.com";
-  const char kApplicationLocaleValue[] = "is-WRONG";
-}
-
 namespace enforced_pref {
   const std::string kHomepageValue = "http://www.topeka.com";
 }
@@ -68,6 +58,16 @@ namespace command_line_pref {
   const char kHomepageValue[] = "http://www.ferretcentral.org";
 }
 
+// The "user" namespace is defined globally in an ARM system header, so we need
+// something different here.
+namespace user_pref {
+  const int kMaxTabsValue = 31;
+  const bool kDeleteCacheValue = true;
+  const char kCurrentThemeIDValue[] = "abcdefg";
+  const char kHomepageValue[] = "http://www.google.com";
+  const char kApplicationLocaleValue[] = "is-WRONG";
+}
+
 namespace recommended_pref {
   const int kMaxTabsValue = 10;
   const bool kRecommendedPrefValue = true;
@@ -75,6 +75,7 @@ namespace recommended_pref {
 
 namespace default_pref {
   const int kDefaultValue = 7;
+  const char kHomepageValue[] = "default homepage";
 }
 
 class PrefValueStoreTest : public testing::Test {
@@ -112,6 +113,26 @@ class PrefValueStoreTest : public testing::Test {
         recommended_pref_store_,
         default_pref_store_);
 
+    // Register prefs with the PrefValueStore.
+    pref_value_store_->RegisterPreferenceType(prefs::kApplicationLocale,
+                                              Value::TYPE_STRING);
+    pref_value_store_->RegisterPreferenceType(prefs::kCurrentThemeID,
+                                              Value::TYPE_STRING);
+    pref_value_store_->RegisterPreferenceType(prefs::kDeleteCache,
+                                              Value::TYPE_BOOLEAN);
+    pref_value_store_->RegisterPreferenceType(prefs::kHomepage,
+                                              Value::TYPE_STRING);
+    pref_value_store_->RegisterPreferenceType(prefs::kMaxTabs,
+                                              Value::TYPE_INTEGER);
+    pref_value_store_->RegisterPreferenceType(prefs::kRecommendedPref,
+                                              Value::TYPE_BOOLEAN);
+    pref_value_store_->RegisterPreferenceType(prefs::kSampleDict,
+                                              Value::TYPE_DICTIONARY);
+    pref_value_store_->RegisterPreferenceType(prefs::kSampleList,
+                                              Value::TYPE_LIST);
+    pref_value_store_->RegisterPreferenceType(prefs::kDefaultPref,
+                                              Value::TYPE_INTEGER);
+
     ui_thread_.reset(new ChromeThread(ChromeThread::UI, &loop_));
     file_thread_.reset(new ChromeThread(ChromeThread::FILE, &loop_));
   }
@@ -306,6 +327,37 @@ TEST_F(PrefValueStoreTest, GetValue) {
   ASSERT_TRUE(v_null == NULL);
 }
 
+// Make sure that if a preference changes type, so the wrong type is stored in
+// the user pref file, it uses the correct fallback value instead.
+TEST_F(PrefValueStoreTest, GetValueChangedType) {
+  // Check falling back to a recommended value.
+  user_pref_store_->prefs()->SetString(prefs::kMaxTabs, "not an integer");
+  Value* value = NULL;
+  ASSERT_TRUE(pref_value_store_->GetValue(prefs::kMaxTabs, &value));
+  ASSERT_TRUE(value != NULL);
+  ASSERT_EQ(Value::TYPE_INTEGER, value->GetType());
+  int actual_int_value = -1;
+  EXPECT_TRUE(value->GetAsInteger(&actual_int_value));
+  EXPECT_EQ(recommended_pref::kMaxTabsValue, actual_int_value);
+
+  // Check falling back multiple times, to a default string.
+  enforced_pref_store_->prefs()->SetInteger(prefs::kHomepage, 1);
+  extension_pref_store_->prefs()->SetInteger(prefs::kHomepage, 1);
+  command_line_pref_store_->prefs()->SetInteger(prefs::kHomepage, 1);
+  user_pref_store_->prefs()->SetInteger(prefs::kHomepage, 1);
+  recommended_pref_store_->prefs()->SetInteger(prefs::kHomepage, 1);
+  default_pref_store_->prefs()->SetString(prefs::kHomepage,
+                                          default_pref::kHomepageValue);
+
+  value = NULL;
+  ASSERT_TRUE(pref_value_store_->GetValue(prefs::kHomepage, &value));
+  ASSERT_TRUE(value != NULL);
+  ASSERT_EQ(Value::TYPE_STRING, value->GetType());
+  std::string actual_str_value;
+  EXPECT_TRUE(value->GetAsString(&actual_str_value));
+  EXPECT_EQ(default_pref::kHomepageValue, actual_str_value);
+}
+
 TEST_F(PrefValueStoreTest, HasPrefPath) {
   // Enforced preference
   EXPECT_TRUE(pref_value_store_->HasPrefPath(prefs::kHomepage));
@@ -322,10 +374,15 @@ TEST_F(PrefValueStoreTest, HasPrefPath) {
 TEST_F(PrefValueStoreTest, PrefHasChanged) {
   // Setup.
   const char managed_pref_path[] = "managed_pref";
+  pref_value_store_->RegisterPreferenceType(managed_pref_path,
+                                            Value::TYPE_STRING);
   enforced_pref_store_->prefs()->SetString(managed_pref_path, "managed value");
   const char user_pref_path[] = "user_pref";
+  pref_value_store_->RegisterPreferenceType(user_pref_path, Value::TYPE_STRING);
   user_pref_store_->prefs()->SetString(user_pref_path, "user value");
   const char default_pref_path[] = "default_pref";
+  pref_value_store_->RegisterPreferenceType(default_pref_path,
+                                            Value::TYPE_STRING);
   default_pref_store_->prefs()->SetString(default_pref_path, "default value");
 
   // Check pref controlled by highest-priority store.
@@ -399,7 +456,7 @@ TEST_F(PrefValueStoreTest, SetUserPrefValue) {
 
   actual_value = NULL;
   std::string key(prefs::kSampleDict);
-  pref_value_store_->GetValue(key , &actual_value);
+  pref_value_store_->GetValue(key, &actual_value);
 
   ASSERT_EQ(expected_dict_value, actual_value);
   ASSERT_TRUE(expected_dict_value->Equals(actual_value));
diff --git a/chrome/browser/sync/glue/preference_model_associator_unittest.cc b/chrome/browser/sync/glue/preference_model_associator_unittest.cc
index 2939db4..b3dca08 100644
--- a/chrome/browser/sync/glue/preference_model_associator_unittest.cc
+++ b/chrome/browser/sync/glue/preference_model_associator_unittest.cc
@@ -44,7 +44,7 @@ class AbstractPreferenceMergeTest : public testing::Test {
     const PrefService::Preference* pref =
         pref_service_->FindPreference(pref_name.c_str());
     ASSERT_TRUE(pref);
-    Value::ValueType type = pref->type();
+    Value::ValueType type = pref->GetType();
     if (type == Value::TYPE_DICTIONARY)
       empty_value.reset(new DictionaryValue);
     else if (type == Value::TYPE_LIST)