Bug 985998 - Allow set*Pref() to override type of prefs with no default value. r=bsmedberg,khuey
authorIrving Reid <irving@mozilla.com>
Wed, 23 Apr 2014 09:48:24 -0400
changeset 179993 9fab567879bc
parent 179992 0096df4879f7
child 179994 d36139793476
push id26650
push userryanvm@gmail.com
push dateThu, 24 Apr 2014 17:39:51 +0000
treeherdermozilla-central@3b166b8add93 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbsmedberg, khuey
bugs985998
milestone31.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 985998 - Allow set*Pref() to override type of prefs with no default value. r=bsmedberg,khuey
modules/libpref/public/nsIPrefBranch.idl
modules/libpref/src/prefapi.cpp
modules/libpref/test/unit/test_changeType.js
modules/libpref/test/unit/xpcshell.ini
--- a/modules/libpref/public/nsIPrefBranch.idl
+++ b/modules/libpref/public/nsIPrefBranch.idl
@@ -65,17 +65,18 @@ interface nsIPrefBranch : nsISupports
   boolean getBoolPref(in string aPrefName);
 
   /**
    * Called to set the state of an individual boolean preference.
    *
    * @param aPrefName The boolean preference to set the state of.
    * @param aValue    The boolean value to set the preference to.
    *
-   * @throws Error if setting failed or the value is the wrong type.
+   * @throws Error if setting failed or the preference has a default
+             value of a type other than boolean.
    *
    * @see getBoolPref
    */
   void setBoolPref(in string aPrefName, in boolean aValue);
 
   /**
    * Called to get the state of an individual floating-point preference.
    * "Floating point" preferences are really string preferences that
@@ -101,17 +102,18 @@ interface nsIPrefBranch : nsISupports
   string getCharPref(in string aPrefName);
 
   /**
    * Called to set the state of an individual string preference.
    *
    * @param aPrefName The string preference to set.
    * @param aValue    The string value to set the preference to.
    *
-   * @throws Error if setting failed or the value is the wrong type.
+   * @throws Error if setting failed or the preference has a default
+             value of a type other than string.
    *
    * @see getCharPref
    */
   void setCharPref(in string aPrefName, in string aValue);
 
   /**
    * Called to get the state of an individual integer preference.
    *
@@ -124,17 +126,18 @@ interface nsIPrefBranch : nsISupports
   long getIntPref(in string aPrefName);
 
   /**
    * Called to set the state of an individual integer preference.
    *
    * @param aPrefName The integer preference to set the value of.
    * @param aValue    The integer value to set the preference to.
    *
-   * @throws Error if setting failed or the value is the wrong type.
+   * @throws Error if setting failed or the preference has a default
+             value of a type other than integer.
    *
    * @see getIntPref
    */
   void setIntPref(in string aPrefName, in long aValue);
 
   /**
    * Called to get the state of an individual complex preference. A complex
    * preference is a preference which represents an XPCOM object that can not
--- a/modules/libpref/src/prefapi.cpp
+++ b/modules/libpref/src/prefapi.cpp
@@ -701,29 +701,34 @@ static bool pref_ValueChanged(PrefValue 
     }
     else if (type & PREF_INT)
         changed = oldValue.intVal != newValue.intVal;
     else if (type & PREF_BOOL)
         changed = oldValue.boolVal != newValue.boolVal;
     return changed;
 }
 
-static void pref_SetValue(PrefValue* oldValue, PrefValue newValue, PrefType type)
+/*
+ * Overwrite the type and value of an existing preference. Caller must
+ * ensure that they are not changing the type of a preference that has
+ * a default value.
+ */
+static void pref_SetValue(PrefValue* existingValue, uint16_t *existingFlags,
+                          PrefValue newValue, PrefType newType)
 {
-    switch (type & PREF_VALUETYPE_MASK)
-    {
-        case PREF_STRING:
-            PR_ASSERT(newValue.stringVal);
-            if (oldValue->stringVal)
-                PL_strfree(oldValue->stringVal);
-            oldValue->stringVal = newValue.stringVal ? PL_strdup(newValue.stringVal) : nullptr;
-            break;
-
-        default:
-            *oldValue = newValue;
+    if ((*existingFlags & PREF_STRING) && existingValue->stringVal) {
+        PL_strfree(existingValue->stringVal);
+    }
+    *existingFlags = (*existingFlags & ~PREF_VALUETYPE_MASK) | newType;
+    if (newType & PREF_STRING) {
+        PR_ASSERT(newValue.stringVal);
+        existingValue->stringVal = newValue.stringVal ? PL_strdup(newValue.stringVal) : nullptr;
+    }
+    else {
+        *existingValue = newValue;
     }
     gDirty = true;
 }
 
 PrefHashEntry* pref_HashTableLookup(const void *key)
 {
 #ifndef MOZ_B2G
     MOZ_ASSERT(NS_IsMainThread());
@@ -747,66 +752,67 @@ nsresult pref_HashPref(const char *key, 
     if (!gHashTable.ops)
         return NS_ERROR_OUT_OF_MEMORY;
 
     PrefHashEntry* pref = static_cast<PrefHashEntry*>(PL_DHashTableOperate(&gHashTable, key, PL_DHASH_ADD));
 
     if (!pref)
         return NS_ERROR_OUT_OF_MEMORY;
 
-    // new entry, better intialize
+    // new entry, better initialize
     if (!pref->key) {
 
         // initialize the pref entry
         pref->flags = type;
         pref->key = ArenaStrDup(key, &gPrefNameArena);
         memset(&pref->defaultPref, 0, sizeof(pref->defaultPref));
         memset(&pref->userPref, 0, sizeof(pref->userPref));
     }
-    else if ((((PrefType)(pref->flags)) & PREF_VALUETYPE_MASK) !=
-                 (type & PREF_VALUETYPE_MASK))
+    else if ((pref->flags & PREF_HAS_DEFAULT) && PREF_TYPE(pref) != type)
     {
-        NS_WARNING(nsPrintfCString("Trying to set pref %s to with the wrong type!", key).get());
+        NS_WARNING(nsPrintfCString("Trying to overwrite value of default pref %s with the wrong type!", key).get());
         return NS_ERROR_UNEXPECTED;
     }
 
     bool valueChanged = false;
     if (flags & kPrefSetDefault)
     {
         if (!PREF_IS_LOCKED(pref))
         {       /* ?? change of semantics? */
             if (pref_ValueChanged(pref->defaultPref, value, type) ||
                 !(pref->flags & PREF_HAS_DEFAULT))
             {
-                pref_SetValue(&pref->defaultPref, value, type);
+                pref_SetValue(&pref->defaultPref, &pref->flags, value, type);
                 pref->flags |= PREF_HAS_DEFAULT;
                 if (!PREF_HAS_USER_VALUE(pref))
                     valueChanged = true;
             }
         }
     }
     else
     {
         /* If new value is same as the default value, then un-set the user value.
            Otherwise, set the user value only if it has changed */
-        if (!pref_ValueChanged(pref->defaultPref, value, type) &&
-            (pref->flags & PREF_HAS_DEFAULT) &&
+        if ((pref->flags & PREF_HAS_DEFAULT) &&
+            !pref_ValueChanged(pref->defaultPref, value, type) &&
             !(flags & kPrefForceSet))
         {
             if (PREF_HAS_USER_VALUE(pref))
             {
+                /* XXX should we free a user-set string value if there is one? */
                 pref->flags &= ~PREF_USERSET;
                 if (!PREF_IS_LOCKED(pref))
                     valueChanged = true;
             }
         }
-        else if ( !PREF_HAS_USER_VALUE(pref) ||
-                   pref_ValueChanged(pref->userPref, value, type) )
+        else if (!PREF_HAS_USER_VALUE(pref) ||
+                 PREF_TYPE(pref) != type ||
+                 pref_ValueChanged(pref->userPref, value, type) )
         {
-            pref_SetValue(&pref->userPref, value, type);
+            pref_SetValue(&pref->userPref, &pref->flags, value, type);
             pref->flags |= PREF_USERSET;
             if (!PREF_IS_LOCKED(pref))
                 valueChanged = true;
         }
     }
 
     nsresult rv = NS_OK;
     if (valueChanged) {
new file mode 100644
--- /dev/null
+++ b/modules/libpref/test/unit/test_changeType.js
@@ -0,0 +1,63 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+/* Tests for changing the type of a preference (bug 985998) */
+
+const PREF_INVALID = 0;
+const PREF_BOOL    = 128;
+const PREF_INT     = 64;
+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
+
+  defaultBranch.setBoolPref("TypeTest.existing.bool", true);
+  defaultBranch.setIntPref("TypeTest.existing.int", 23);
+  defaultBranch.setCharPref("TypeTest.existing.char", "hey");
+
+  // 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");
+
+  // All the combinations of attempted type changes
+  do_check_throws(function() {
+    userBranch.setCharPref("TypeTest.existing.bool", "boo"); }, Cr.NS_ERROR_UNEXPECTED);
+  do_check_throws(function() {
+    userBranch.setIntPref("TypeTest.existing.bool", 5); }, Cr.NS_ERROR_UNEXPECTED);
+  do_check_throws(function() {
+    userBranch.setCharPref("TypeTest.existing.int", "boo"); }, Cr.NS_ERROR_UNEXPECTED);
+  do_check_throws(function() {
+    userBranch.setBoolPref("TypeTest.existing.int", true); }, Cr.NS_ERROR_UNEXPECTED);
+  do_check_throws(function() {
+    userBranch.setBoolPref("TypeTest.existing.char", true); }, Cr.NS_ERROR_UNEXPECTED);
+  do_check_throws(function() {
+    userBranch.setIntPref("TypeTest.existing.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);
+}
--- a/modules/libpref/test/unit/xpcshell.ini
+++ b/modules/libpref/test/unit/xpcshell.ini
@@ -5,10 +5,11 @@ support-files =
   data/testPref.js
   extdata/testExt.js
 
 [test_warnings.js]
 [test_bug345529.js]
 [test_bug506224.js]
 [test_bug577950.js]
 [test_bug790374.js]
+[test_changeType.js]
 [test_extprefs.js]
 [test_libPrefs.js]