Bug 1477904: Correctly handle static var caches with changed default values. r=njn

MozReview-Commit-ID: H2ImQrmrtAV

--HG--
extra : rebase_source : bc1c43aba3d19b48e9ee3fc3d477138404009d10

modules/libpref/Preferences.cpp

@@ -466,6 +466,7 @@ public:
     , mType(static_cast<uint32_t>(PrefType::None))
     , mIsSticky(false)
     , mIsLocked(false)
+    , mDefaultChanged(false)
     , mHasDefaultValue(false)
     , mHasUserValue(false)
     , mDefaultValue()
@@ -501,6 +502,8 @@ public:
   bool IsLocked() const { return mIsLocked; }
   void SetIsLocked(bool aValue) { mIsLocked = aValue; }
 
+  bool DefaultChanged() const { return mDefaultChanged; }
+
   bool IsSticky() const { return mIsSticky; }
 
   bool HasDefaultValue() const { return mHasDefaultValue; }
@@ -510,7 +513,11 @@ public:
   void AddToMap(SharedPrefMapBuilder& aMap)
   {
     aMap.Add(Name(),
-             { HasDefaultValue(), HasUserValue(), IsSticky(), IsLocked() },
+             { HasDefaultValue(),
+               HasUserValue(),
+               IsSticky(),
+               IsLocked(),
+               DefaultChanged() },
              HasDefaultValue() ? mDefaultValue.Get<T>() : T(),
              HasUserValue() ? mUserValue.Get<T>() : T());
   }
@@ -713,6 +720,9 @@ public:
       }
       if (!ValueMatches(PrefValueKind::Default, aType, aValue)) {
         mDefaultValue.Replace(mHasDefaultValue, Type(), aType, aValue);
+        if (mHasDefaultValue) {
+          mDefaultChanged = true;
+        }
         mHasDefaultValue = true;
         if (aIsSticky) {
           mIsSticky = true;
@@ -936,6 +946,7 @@ private:
   uint32_t mType : 2;
   uint32_t mIsSticky : 1;
   uint32_t mIsLocked : 1;
+  uint32_t mDefaultChanged : 1;
   uint32_t mHasDefaultValue : 1;
   uint32_t mHasUserValue : 1;
 
@@ -1008,6 +1019,7 @@ public:
     return match(Matcher());                                                   \
   }
 
+  FORWARD(bool, DefaultChanged)
   FORWARD(bool, IsLocked)
   FORWARD(bool, IsSticky)
   FORWARD(bool, HasDefaultValue)
@@ -4830,23 +4842,19 @@ Preferences::InitInitialObjects(bool aIsStartup)
     // don't need to add them to the DB. For static var caches, though, the
     // current preference values may differ from their static defaults. So we
     // still need to notify callbacks for each of our shared prefs which have
-    // user values.
-    //
-    // While it is technically also possible for the default values to have
-    // changed at runtime, and therefore not match the static defaults, we don't
-    // support that for static preferences in this configuration, and therefore
-    // ignore the possibility.
+    // user values, of whose default values have changed since they were
+    // initialized.
     for (auto& pref : gSharedMap->Iter()) {
-      if (pref.HasUserValue() || pref.IsLocked()) {
+      if (pref.HasUserValue() || pref.DefaultChanged()) {
         NotifyCallbacks(pref.Name(), PrefWrapper(pref));
       }
     }
 
 #ifdef DEBUG
-    // Check that all varcache preferences match their current values. This can
-    // currently fail if the default value of a static varcache preference is
-    // changed in a preference file or at runtime, rather than in
-    // StaticPrefList.h.
+      // Check that all varcache preferences match their current values. This
+      // can currently fail if the default value of a static varcache preference
+      // is changed in a preference file or at runtime, rather than in
+      // StaticPrefList.h.
 
 #define PREF(name, cpp_type, value)
 #define VARCACHE_PREF(name, id, cpp_type, value)                               \

modules/libpref/SharedPrefMap.cpp

@@ -98,6 +98,7 @@ SharedPrefMapBuilder::Add(const char* aKey,
     aFlags.mHasUserValue,
     aFlags.mIsSticky,
     aFlags.mIsLocked,
+    aFlags.mDefaultChanged,
   });
 }
 
@@ -123,6 +124,7 @@ SharedPrefMapBuilder::Add(const char* aKey,
     aFlags.mHasUserValue,
     aFlags.mIsSticky,
     aFlags.mIsLocked,
+    aFlags.mDefaultChanged,
   });
 }
 
@@ -150,6 +152,7 @@ SharedPrefMapBuilder::Add(const char* aKey,
     aFlags.mHasUserValue,
     aFlags.mIsSticky,
     aFlags.mIsLocked,
+    aFlags.mDefaultChanged,
   });
 }
 
@@ -216,7 +219,7 @@ SharedPrefMapBuilder::Finalize(loader::AutoMemMap& aMap)
       entry->mKey,          GetValue(*entry),
       entry->mType,         entry->mHasDefaultValue,
       entry->mHasUserValue, entry->mIsSticky,
-      entry->mIsLocked,
+      entry->mIsLocked,     entry->mDefaultChanged,
     };
     entryPtr++;
   }

modules/libpref/SharedPrefMap.h

@@ -318,6 +318,8 @@ class SharedPrefMap
     uint8_t mIsSticky : 1;
     // True if the preference is locked, as defined by the preference service.
     uint8_t mIsLocked : 1;
+    // True if the preference's default value has changed since it was first set.
+    uint8_t mDefaultChanged : 1;
   };
 
 public:
@@ -345,6 +347,7 @@ public:
       return PrefType(mEntry->mType);
     }
 
+    bool DefaultChanged() const { return mEntry->mDefaultChanged; }
     bool HasDefaultValue() const { return mEntry->mHasDefaultValue; }
     bool HasUserValue() const { return mEntry->mHasUserValue; }
     bool IsLocked() const { return mEntry->mIsLocked; }
@@ -589,6 +592,7 @@ public:
     uint8_t mHasUserValue : 1;
     uint8_t mIsSticky : 1;
     uint8_t mIsLocked : 1;
+    uint8_t mDefaultChanged : 1;
   };
 
   void Add(const char* aKey,
@@ -860,6 +864,7 @@ private:
     uint8_t mHasUserValue : 1;
     uint8_t mIsSticky : 1;
     uint8_t mIsLocked : 1;
+    uint8_t mDefaultChanged : 1;
   };
 
   // Converts a builder Value struct to a SharedPrefMap::Value struct for

modules/libpref/init/StaticPrefList.h

@@ -149,6 +149,8 @@ VARCACHE_PREF(
 )
 #undef PREF_VALUE
 
+// NOTE: This preference is used in unit tests. If it is removed or its default
+// value changes, please update test_sharedMap_var_caches.js accordingly.
 VARCACHE_PREF(
   "dom.webcomponents.shadowdom.report_usage",
    dom_webcomponents_shadowdom_report_usage,
@@ -157,6 +159,8 @@ VARCACHE_PREF(
 
 // Whether we disable triggering mutation events for changes to style
 // attribute via CSSOM.
+// NOTE: This preference is used in unit tests. If it is removed or its default
+// value changes, please update test_sharedMap_var_caches.js accordingly.
 VARCACHE_PREF(
   "dom.mutation-events.cssom.disabled",
    dom_mutation_events_cssom_disabled,

modules/libpref/test/unit_ipc/test_sharedMap_var_caches.js

@@ -0,0 +1,65 @@
+/* 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/. */
+"use strict";
+
+// Tests that static preference varcaches in the content process
+// correctly handle values which are different from their
+// statically-defined defaults.
+//
+// Since we can't access varcaches values from JS, this tests relies on
+// assertions in debug builds to detect mismatches. The default and user
+// values of two preferences are changed (respectively) before a content
+// process is started. Once the content process is launched, the
+// preference service asserts that the values stored in all var caches
+// match their current values as known to the preference service. If
+// there's a mismatch, the shell will crash, and the test will fail.
+//
+// For sanity, we also check that the dynamically retrieved preference
+// values in the content process match our expectations, though this is
+// not strictly part of the test.
+
+const PREF1_NAME = "dom.webcomponents.shadowdom.report_usage";
+const PREF1_VALUE = false;
+
+const PREF2_NAME = "dom.mutation-events.cssom.disabled"
+const PREF2_VALUE = true;
+
+ChromeUtils.import("resource://gre/modules/Services.jsm");
+ChromeUtils.import("resource://testing-common/ExtensionXPCShellUtils.jsm");
+
+ExtensionTestUtils.init(this);
+
+let contentPage;
+
+const {prefs} = Services;
+const defaultPrefs = prefs.getDefaultBranch("");
+
+add_task(async function test_sharedMap_var_caches() {
+  equal(prefs.getBoolPref(PREF1_NAME), PREF1_VALUE,
+        `Expected initial value for ${PREF1_NAME}`);
+  equal(prefs.getBoolPref(PREF2_NAME), PREF2_VALUE,
+        `Expected initial value for ${PREF2_NAME}`);
+
+  defaultPrefs.setBoolPref(PREF1_NAME, !PREF1_VALUE);
+  prefs.setBoolPref(PREF2_NAME, !PREF2_VALUE);
+
+  equal(prefs.getBoolPref(PREF1_NAME), !PREF1_VALUE,
+        `Expected updated value for ${PREF1_NAME}`);
+  equal(prefs.getBoolPref(PREF2_NAME), !PREF2_VALUE,
+        `Expected updated value for ${PREF2_NAME}`);
+
+  let contentPage = await ExtensionTestUtils.loadContentPage("about:blank", {remote: true});
+  registerCleanupFunction(() => contentPage.close());
+
+  let values = await contentPage.spawn([PREF1_NAME, PREF2_NAME], (prefs) => {
+    ChromeUtils.import("resource://gre/modules/Services.jsm");
+    return prefs.map(pref => Services.prefs.getBoolPref(pref));
+  })
+
+  equal(values[0], !PREF1_VALUE,
+        `Expected content value for ${PREF1_NAME}`);
+  equal(values[1], !PREF2_VALUE,
+        `Expected content value for ${PREF2_NAME}`);
+});
+

modules/libpref/test/unit_ipc/xpcshell.ini

@@ -9,4 +9,6 @@ skip-if = toolkit == 'android'
 [test_observed_prefs.js]
 [test_update_prefs.js]
 [test_sharedMap.js]
+[test_sharedMap_var_caches.js]
+skip-if = !debug # Relies on debug assertions to catch failure cases.
 [test_user_default_prefs.js]