Bug 1419654 - Prevent possible pref default/user value type mismatches. r=glandium
authorNicholas Nethercote <nnethercote@mozilla.com>
Tue, 21 Nov 2017 16:00:23 +1100
changeset 437951 4c24fa3a323b29d2a64188f83412eaa89a26a231
parent 437950 5bd0686052829656cd02ed55f5cd16c29f4894b7
child 438028 5482a1a3cfee4351402acc357c30dcd92c697521
push id117
push userfmarier@mozilla.com
push dateTue, 28 Nov 2017 20:17:16 +0000
reviewersglandium
bugs1419654
milestone59.0a1
Bug 1419654 - Prevent possible pref default/user value type mismatches. r=glandium Currently, you can create a pref that only has a user value, and then later give it a default value with a different type. The entire pref is then recorded as having this second type. This causes problems later when interpreting the user value. This patch makes SetValue() fail if it tries to set a default value whose type differs from an existing user value. It also expands an existing test to cover this case and some similar ones. MozReview-Commit-ID: 89tvISQ7RNT
modules/libpref/Preferences.cpp
modules/libpref/test/unit/test_changeType.js
--- a/modules/libpref/Preferences.cpp
+++ b/modules/libpref/Preferences.cpp
@@ -475,38 +475,49 @@ public:
     if (Type() == PrefType::String) {
       free(const_cast<char*>(mUserValue.mStringVal));
       mUserValue.mStringVal = nullptr;
     }
 
     mHasUserValue = false;
   }
 
-  void SetValue(PrefType aType,
-                PrefValue aValue,
-                uint32_t aFlags,
-                bool* aValueChanged,
-                bool* aDirty)
+  nsresult SetValue(PrefType aType,
+                    PrefValue aValue,
+                    uint32_t aFlags,
+                    bool* aValueChanged,
+                    bool* aDirty)
   {
     if (aFlags & kPrefSetDefault) {
+      // Types must always match when setting the default value.
+      if (!IsType(aType)) {
+        return NS_ERROR_UNEXPECTED;
+      }
+
       if (!IsLocked()) {
         // ?? change of semantics?
         if (!mHasDefaultValue || !mDefaultValue.Equals(aType, aValue)) {
           ReplaceValue(PrefValueKind::Default, aType, aValue);
           if (aFlags & kPrefSticky) {
             mIsSticky = true;
           }
           if (!mHasUserValue) {
             *aValueChanged = true;
           }
         }
         // What if we change the default to be the same as the user value?
         // Should we clear the user value?
       }
     } else {
+      // If we have a default value, types must match when setting the user
+      // value.
+      if (mHasDefaultValue && !IsType(aType)) {
+        return NS_ERROR_UNEXPECTED;
+      }
+
       // If new value is same as the default value and it's not a "sticky"
       // pref, then un-set the user value. Otherwise, set the user value only
       // if it has changed.
       if (mHasDefaultValue && !mIsSticky &&
           mDefaultValue.Equals(aType, aValue) && !(aFlags & kPrefForceSet)) {
         if (mHasUserValue) {
           ClearUserValue();
           if (!IsLocked()) {
@@ -518,16 +529,17 @@ public:
                  !mUserValue.Equals(aType, aValue)) {
         ReplaceValue(PrefValueKind::User, aType, aValue);
         if (!IsLocked()) {
           *aDirty = true;
           *aValueChanged = true;
         }
       }
     }
+    return NS_OK;
   }
 
   // Returns false if this pref doesn't have a user value worth saving.
   bool UserValueToStringForSaving(nsCString& aStr)
   {
     if (mHasUserValue && (!mHasDefaultValue || mIsSticky ||
                           !mDefaultValue.Equals(Type(), mUserValue))) {
       if (IsTypeString()) {
@@ -748,33 +760,34 @@ pref_SetPref(const char* aPrefName,
   auto pref = static_cast<PrefHashEntry*>(gHashTable->Add(aPrefName, fallible));
   if (!pref) {
     return NS_ERROR_OUT_OF_MEMORY;
   }
 
   if (!pref->Name()) {
     // New (zeroed) entry. Initialize it.
     new (pref) PrefHashEntry(aPrefName, aType);
-
-  } else if (pref->HasDefaultValue() && !pref->IsType(aType)) {
+  }
+
+  bool valueChanged = false, handleDirty = false;
+  nsresult rv =
+    pref->SetValue(aType, aValue, aFlags, &valueChanged, &handleDirty);
+  if (NS_FAILED(rv)) {
     NS_WARNING(
       nsPrintfCString(
-        "Ignoring attempt to overwrite value of default pref %s (type %s) with "
-        "the wrong type (%s)!",
+        "Rejected attempt to change type of pref %s's %s value from %s to %s",
         aPrefName,
+        (aFlags & kPrefSetDefault) ? "default" : "user",
         PrefTypeToString(pref->Type()),
         PrefTypeToString(aType))
         .get());
 
-    return NS_ERROR_UNEXPECTED;
+    return rv;
   }
 
-  bool valueChanged = false, handleDirty = false;
-  pref->SetValue(aType, aValue, aFlags, &valueChanged, &handleDirty);
-
   if (handleDirty) {
     Preferences::HandleDirty();
   }
   if (valueChanged) {
     NotifyCallbacks(aPrefName);
   }
 
   return NS_OK;
--- a/modules/libpref/test/unit/test_changeType.js
+++ b/modules/libpref/test/unit/test_changeType.js
@@ -12,52 +12,130 @@ const PREF_STRING  = 32;
 function run_test() {
 
   var ps = Cc["@mozilla.org/preferences-service;1"].
             getService(Ci.nsIPrefService);
 
   let defaultBranch = ps.getDefaultBranch("");
   let userBranch = ps.getBranch("");
 
-  //**************************************************************************//
-  // Can't change the type of prefs that have default values
+  // Prefs that only have a default value -- we can't change their type.
+  defaultBranch.setBoolPref("TypeTest.default.bool", true);
+  defaultBranch.setIntPref("TypeTest.default.int", 23);
+  defaultBranch.setCharPref("TypeTest.default.char", "hey");
+
+  do_check_eq(userBranch.getBoolPref("TypeTest.default.bool"), true);
+  do_check_eq(userBranch.getIntPref("TypeTest.default.int"), 23);
+  do_check_eq(userBranch.getCharPref("TypeTest.default.char"), "hey");
 
-  defaultBranch.setBoolPref("TypeTest.existing.bool", true);
-  defaultBranch.setIntPref("TypeTest.existing.int", 23);
-  defaultBranch.setCharPref("TypeTest.existing.char", "hey");
+  // Prefs that only have a user value -- we can change their type, but only
+  // when we set the user value.
+  userBranch.setBoolPref("TypeTest.user.bool", false);
+  userBranch.setIntPref("TypeTest.user.int", 24);
+  userBranch.setCharPref("TypeTest.user.char", "hi");
+
+  do_check_eq(userBranch.getBoolPref("TypeTest.user.bool"), false);
+  do_check_eq(userBranch.getIntPref("TypeTest.user.int"), 24);
+  do_check_eq(userBranch.getCharPref("TypeTest.user.char"), "hi");
+
+  // Prefs that have both a default and a user value -- we can't change their
+  // type.
+  defaultBranch.setBoolPref("TypeTest.both.bool", true);
+     userBranch.setBoolPref("TypeTest.both.bool", false);
+  defaultBranch.setIntPref("TypeTest.both.int", 25);
+     userBranch.setIntPref("TypeTest.both.int", 26);
+  defaultBranch.setCharPref("TypeTest.both.char", "yo");
+     userBranch.setCharPref("TypeTest.both.char", "ya");
 
-  // The user branch reads back the expected default
-  do_check_eq(userBranch.getBoolPref("TypeTest.existing.bool"), true);
-  do_check_eq(userBranch.getIntPref("TypeTest.existing.int"), 23);
-  do_check_eq(userBranch.getCharPref("TypeTest.existing.char"), "hey");
+  do_check_eq(userBranch.getBoolPref("TypeTest.both.bool"), false);
+  do_check_eq(userBranch.getIntPref("TypeTest.both.int"), 26);
+  do_check_eq(userBranch.getCharPref("TypeTest.both.char"), "ya");
 
-  // All the combinations of attempted type changes
+  // We only have a default value, and we try to set a default value of a
+  // different type --> fails.
+  do_check_throws(function() {
+    defaultBranch.setCharPref("TypeTest.default.bool", "boo"); }, Cr.NS_ERROR_UNEXPECTED);
+  do_check_throws(function() {
+    defaultBranch.setIntPref("TypeTest.default.bool", 5); }, Cr.NS_ERROR_UNEXPECTED);
+  do_check_throws(function() {
+    defaultBranch.setCharPref("TypeTest.default.int", "boo"); }, Cr.NS_ERROR_UNEXPECTED);
+  do_check_throws(function() {
+    defaultBranch.setBoolPref("TypeTest.default.int", true); }, Cr.NS_ERROR_UNEXPECTED);
   do_check_throws(function() {
-    userBranch.setCharPref("TypeTest.existing.bool", "boo"); }, Cr.NS_ERROR_UNEXPECTED);
+    defaultBranch.setBoolPref("TypeTest.default.char", true); }, Cr.NS_ERROR_UNEXPECTED);
   do_check_throws(function() {
-    userBranch.setIntPref("TypeTest.existing.bool", 5); }, Cr.NS_ERROR_UNEXPECTED);
+    defaultBranch.setIntPref("TypeTest.default.char", 6); }, Cr.NS_ERROR_UNEXPECTED);
+
+  // We only have a default value, and we try to set a user value of a
+  // different type --> fails.
   do_check_throws(function() {
-    userBranch.setCharPref("TypeTest.existing.int", "boo"); }, Cr.NS_ERROR_UNEXPECTED);
+    userBranch.setCharPref("TypeTest.default.bool", "boo"); }, Cr.NS_ERROR_UNEXPECTED);
+  do_check_throws(function() {
+    userBranch.setIntPref("TypeTest.default.bool", 5); }, Cr.NS_ERROR_UNEXPECTED);
   do_check_throws(function() {
-    userBranch.setBoolPref("TypeTest.existing.int", true); }, Cr.NS_ERROR_UNEXPECTED);
+    userBranch.setCharPref("TypeTest.default.int", "boo"); }, Cr.NS_ERROR_UNEXPECTED);
+  do_check_throws(function() {
+    userBranch.setBoolPref("TypeTest.default.int", true); }, Cr.NS_ERROR_UNEXPECTED);
+  do_check_throws(function() {
+    userBranch.setBoolPref("TypeTest.default.char", true); }, Cr.NS_ERROR_UNEXPECTED);
   do_check_throws(function() {
-    userBranch.setBoolPref("TypeTest.existing.char", true); }, Cr.NS_ERROR_UNEXPECTED);
+    userBranch.setIntPref("TypeTest.default.char", 6); }, Cr.NS_ERROR_UNEXPECTED);
+
+  // We only have a user value, and we try to set a default value of a
+  // different type --> fails.
+  do_check_throws(function() {
+    defaultBranch.setCharPref("TypeTest.user.bool", "boo"); }, Cr.NS_ERROR_UNEXPECTED);
   do_check_throws(function() {
-    userBranch.setIntPref("TypeTest.existing.char", 6); }, Cr.NS_ERROR_UNEXPECTED);
-
+    defaultBranch.setIntPref("TypeTest.user.bool", 5); }, Cr.NS_ERROR_UNEXPECTED);
+  do_check_throws(function() {
+    defaultBranch.setCharPref("TypeTest.user.int", "boo"); }, Cr.NS_ERROR_UNEXPECTED);
+  do_check_throws(function() {
+    defaultBranch.setBoolPref("TypeTest.user.int", true); }, Cr.NS_ERROR_UNEXPECTED);
+  do_check_throws(function() {
+    defaultBranch.setBoolPref("TypeTest.user.char", true); }, Cr.NS_ERROR_UNEXPECTED);
+  do_check_throws(function() {
+    defaultBranch.setIntPref("TypeTest.user.char", 6); }, Cr.NS_ERROR_UNEXPECTED);
 
-  //**************************************************************************//
-  // Prefs that don't have default values can mutate
-  let pref = "TypeTest.user";
-  userBranch.setBoolPref(pref, true);
-  userBranch.setCharPref(pref, "yay");
-  do_check_eq(userBranch.getCharPref(pref), "yay");
-  userBranch.setIntPref(pref, 7);
-  do_check_eq(userBranch.getIntPref(pref), 7);
-  userBranch.setBoolPref(pref, false);
-  do_check_eq(userBranch.getBoolPref(pref), false);
-  userBranch.setIntPref(pref, 8);
-  do_check_eq(userBranch.getIntPref(pref), 8);
-  userBranch.setCharPref(pref, "whee");
-  do_check_eq(userBranch.getCharPref(pref), "whee");
-  userBranch.setBoolPref(pref, true);
-  do_check_eq(userBranch.getBoolPref(pref), true);
+  // We only have a user value, and we try to set a user value of a
+  // different type --> SUCCEEDS.
+  userBranch.setCharPref("TypeTest.user.bool", "boo");
+  do_check_eq(userBranch.getCharPref("TypeTest.user.bool"), "boo");
+  userBranch.setIntPref("TypeTest.user.bool", 5);
+  do_check_eq(userBranch.getIntPref("TypeTest.user.bool"), 5);
+  userBranch.setCharPref("TypeTest.user.int", "boo");
+  do_check_eq(userBranch.getCharPref("TypeTest.user.int"), "boo");
+  userBranch.setBoolPref("TypeTest.user.int", true);
+  do_check_eq(userBranch.getBoolPref("TypeTest.user.int"), true);
+  userBranch.setBoolPref("TypeTest.user.char", true);
+  do_check_eq(userBranch.getBoolPref("TypeTest.user.char"), true);
+  userBranch.setIntPref("TypeTest.user.char", 6);
+  do_check_eq(userBranch.getIntPref("TypeTest.user.char"), 6);
+
+  // We have both a default value and user value, and we try to set a default
+  // value of a different type --> fails.
+  do_check_throws(function() {
+    defaultBranch.setCharPref("TypeTest.both.bool", "boo"); }, Cr.NS_ERROR_UNEXPECTED);
+  do_check_throws(function() {
+    defaultBranch.setIntPref("TypeTest.both.bool", 5); }, Cr.NS_ERROR_UNEXPECTED);
+  do_check_throws(function() {
+    defaultBranch.setCharPref("TypeTest.both.int", "boo"); }, Cr.NS_ERROR_UNEXPECTED);
+  do_check_throws(function() {
+    defaultBranch.setBoolPref("TypeTest.both.int", true); }, Cr.NS_ERROR_UNEXPECTED);
+  do_check_throws(function() {
+    defaultBranch.setBoolPref("TypeTest.both.char", true); }, Cr.NS_ERROR_UNEXPECTED);
+  do_check_throws(function() {
+    defaultBranch.setIntPref("TypeTest.both.char", 6); }, Cr.NS_ERROR_UNEXPECTED);
+
+  // We have both a default value and user value, and we try to set a user
+  // value of a different type --> fails.
+  do_check_throws(function() {
+    userBranch.setCharPref("TypeTest.both.bool", "boo"); }, Cr.NS_ERROR_UNEXPECTED);
+  do_check_throws(function() {
+    userBranch.setIntPref("TypeTest.both.bool", 5); }, Cr.NS_ERROR_UNEXPECTED);
+  do_check_throws(function() {
+    userBranch.setCharPref("TypeTest.both.int", "boo"); }, Cr.NS_ERROR_UNEXPECTED);
+  do_check_throws(function() {
+    userBranch.setBoolPref("TypeTest.both.int", true); }, Cr.NS_ERROR_UNEXPECTED);
+  do_check_throws(function() {
+    userBranch.setBoolPref("TypeTest.both.char", true); }, Cr.NS_ERROR_UNEXPECTED);
+  do_check_throws(function() {
+    userBranch.setIntPref("TypeTest.both.char", 6); }, Cr.NS_ERROR_UNEXPECTED);
 }