Bug 1477904: Correctly handle static var caches with changed default values. r?njn draft
authorKris Maglione <maglione.k@gmail.com>
Mon, 23 Jul 2018 22:50:03 -0700
changeset 821894 1536b65f7006b214457d54cd9bf0940289de4c08
parent 821893 750d3a0ab6f054153dc5716e680577cbb9112527
push id117209
push usermaglione.k@gmail.com
push dateTue, 24 Jul 2018 05:50:34 +0000
reviewersnjn
bugs1477904
milestone63.0a1
Bug 1477904: Correctly handle static var caches with changed default values. r?njn MozReview-Commit-ID: H2ImQrmrtAV
modules/libpref/Preferences.cpp
modules/libpref/SharedPrefMap.cpp
modules/libpref/SharedPrefMap.h
modules/libpref/init/StaticPrefList.h
modules/libpref/test/unit_ipc/test_sharedMap_var_caches.js
modules/libpref/test/unit_ipc/xpcshell.ini
--- a/modules/libpref/Preferences.cpp
+++ b/modules/libpref/Preferences.cpp
@@ -461,16 +461,17 @@ class PrefWrapper;
 class Pref
 {
 public:
   explicit Pref(const char* aName)
     : mName(ArenaStrdup(aName, gPrefNameArena))
     , mType(static_cast<uint32_t>(PrefType::None))
     , mIsSticky(false)
     , mIsLocked(false)
+    , mDefaultChanged(false)
     , mHasDefaultValue(false)
     , mHasUserValue(false)
     , mDefaultValue()
     , mUserValue()
   {
   }
 
   ~Pref()
@@ -496,26 +497,32 @@ public:
   bool IsTypeInt() const { return IsType(PrefType::Int); }
   bool IsTypeBool() const { return IsType(PrefType::Bool); }
 
   // Other properties.
 
   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; }
   bool HasUserValue() const { return mHasUserValue; }
 
   template<typename T>
   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());
   }
 
   void AddToMap(SharedPrefMapBuilder& aMap)
   {
     if (IsTypeBool()) {
       AddToMap<bool>(aMap);
@@ -708,16 +715,19 @@ public:
     // Should we set the default value? Only if the pref is not locked, and
     // doing so would change the default value.
     if (!IsLocked()) {
       if (aIsLocked) {
         SetIsLocked(true);
       }
       if (!ValueMatches(PrefValueKind::Default, aType, aValue)) {
         mDefaultValue.Replace(mHasDefaultValue, Type(), aType, aValue);
+        if (mHasDefaultValue) {
+          mDefaultChanged = true;
+        }
         mHasDefaultValue = true;
         if (aIsSticky) {
           mIsSticky = true;
         }
         if (!mHasUserValue) {
           *aValueChanged = true;
         }
         // What if we change the default to be the same as the user value?
@@ -931,16 +941,17 @@ public:
   }
 
 private:
   const char* mName; // allocated in gPrefNameArena
 
   uint32_t mType : 2;
   uint32_t mIsSticky : 1;
   uint32_t mIsLocked : 1;
+  uint32_t mDefaultChanged : 1;
   uint32_t mHasDefaultValue : 1;
   uint32_t mHasUserValue : 1;
 
   PrefValue mDefaultValue;
   PrefValue mUserValue;
 };
 
 class PrefEntry : public PLDHashEntryHdr
@@ -1003,16 +1014,17 @@ public:
     struct Matcher                                                             \
     {                                                                          \
       retType match(const Pref* aPref) { return aPref->method(); }             \
       retType match(SharedPref& aPref) { return aPref.method(); }              \
     };                                                                         \
     return match(Matcher());                                                   \
   }
 
+  FORWARD(bool, DefaultChanged)
   FORWARD(bool, IsLocked)
   FORWARD(bool, IsSticky)
   FORWARD(bool, HasDefaultValue)
   FORWARD(bool, HasUserValue)
   FORWARD(const char*, Name)
   FORWARD(nsCString, NameString)
   FORWARD(PrefType, Type)
 #undef FORWARD
@@ -4825,33 +4837,29 @@ Preferences::InitInitialObjects(bool aIs
 
   if (!XRE_IsParentProcess()) {
     MOZ_ASSERT(gSharedMap);
 
     // We got our initial preference values from the content process, so we
     // 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)                               \
   MOZ_ASSERT(GetPref<StripAtomic<cpp_type>>(name, value) == StaticPrefs::id(), \
              "Incorrect cached value for " name);
 #include "mozilla/StaticPrefList.h"
 #undef PREF
 #undef VARCACHE_PREF
--- a/modules/libpref/SharedPrefMap.cpp
+++ b/modules/libpref/SharedPrefMap.cpp
@@ -93,16 +93,17 @@ SharedPrefMapBuilder::Add(const char* aK
     aKey,
     mKeyTable.Add(aKey),
     { aDefaultValue, aUserValue },
     uint8_t(PrefType::Bool),
     aFlags.mHasDefaultValue,
     aFlags.mHasUserValue,
     aFlags.mIsSticky,
     aFlags.mIsLocked,
+    aFlags.mDefaultChanged,
   });
 }
 
 void
 SharedPrefMapBuilder::Add(const char* aKey,
                           const Flags& aFlags,
                           int32_t aDefaultValue,
                           int32_t aUserValue)
@@ -118,16 +119,17 @@ SharedPrefMapBuilder::Add(const char* aK
     aKey,
     mKeyTable.Add(aKey),
     { index },
     uint8_t(PrefType::Int),
     aFlags.mHasDefaultValue,
     aFlags.mHasUserValue,
     aFlags.mIsSticky,
     aFlags.mIsLocked,
+    aFlags.mDefaultChanged,
   });
 }
 
 void
 SharedPrefMapBuilder::Add(const char* aKey,
                           const Flags& aFlags,
                           const nsCString& aDefaultValue,
                           const nsCString& aUserValue)
@@ -145,16 +147,17 @@ SharedPrefMapBuilder::Add(const char* aK
     aKey,
     mKeyTable.Add(aKey),
     { index },
     uint8_t(PrefType::String),
     aFlags.mHasDefaultValue,
     aFlags.mHasUserValue,
     aFlags.mIsSticky,
     aFlags.mIsLocked,
+    aFlags.mDefaultChanged,
   });
 }
 
 Result<Ok, nsresult>
 SharedPrefMapBuilder::Finalize(loader::AutoMemMap& aMap)
 {
   using Header = SharedPrefMap::Header;
 
@@ -211,17 +214,17 @@ SharedPrefMapBuilder::Finalize(loader::A
   headerPtr[0] = header;
 
   auto* entryPtr = reinterpret_cast<SharedPrefMap::Entry*>(&headerPtr[1]);
   for (auto* entry : entries) {
     *entryPtr = {
       entry->mKey,          GetValue(*entry),
       entry->mType,         entry->mHasDefaultValue,
       entry->mHasUserValue, entry->mIsSticky,
-      entry->mIsLocked,
+      entry->mIsLocked,     entry->mDefaultChanged,
     };
     entryPtr++;
   }
 
   auto ptr = mem.Get<uint8_t>();
 
   mKeyTable.Write(
     { &ptr[header.mKeyStrings.mOffset], header.mKeyStrings.mSize });
--- a/modules/libpref/SharedPrefMap.h
+++ b/modules/libpref/SharedPrefMap.h
@@ -313,16 +313,18 @@ class SharedPrefMap
     uint8_t mHasDefaultValue : 1;
     // True if the preference has a user value. Callers must not attempt to
     // access the entry's user value if this is false.
     uint8_t mHasUserValue : 1;
     // True if the preference is sticky, as defined by the preference service.
     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:
   NS_INLINE_DECL_REFCOUNTING(SharedPrefMap)
 
   // A temporary wrapper class for accessing entries in the array. Instances of
   // this class are valid as long as SharedPrefMap instance is alive, but
   // generally should not be stored long term, or allocated on the heap.
@@ -340,16 +342,17 @@ public:
     nsCString NameString() const { return mMap->KeyTable().Get(mEntry->mKey); }
 
     PrefType Type() const
     {
       MOZ_ASSERT(PrefType(mEntry->mType) != PrefType::None);
       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; }
     bool IsSticky() const { return mEntry->mIsSticky; }
 
     bool GetBoolValue(PrefValueKind aKind = PrefValueKind::User) const
     {
       MOZ_ASSERT(Type() == PrefType::Bool);
@@ -584,16 +587,17 @@ public:
 
   // The set of flags for the preference, as documented in SharedPrefMap::Entry.
   struct Flags
   {
     uint8_t mHasDefaultValue : 1;
     uint8_t mHasUserValue : 1;
     uint8_t mIsSticky : 1;
     uint8_t mIsLocked : 1;
+    uint8_t mDefaultChanged : 1;
   };
 
   void Add(const char* aKey,
            const Flags& aFlags,
            bool aDefaultValue,
            bool aUserValue);
 
   void Add(const char* aKey,
@@ -855,16 +859,17 @@ private:
     StringTableEntry mKey;
     Value mValue;
 
     uint8_t mType : 2;
     uint8_t mHasDefaultValue : 1;
     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
   // serialization. This must not be called before callers have finished adding
   // entries to the value array builders.
   SharedPrefMap::Value GetValue(const Entry& aEntry) const
   {
     switch (PrefType(aEntry.mType)) {
--- a/modules/libpref/init/StaticPrefList.h
+++ b/modules/libpref/init/StaticPrefList.h
@@ -144,24 +144,28 @@ VARCACHE_PREF(
 #endif
 VARCACHE_PREF(
   "dom.animations-api.timelines.enabled",
    dom_animations_api_timelines_enabled,
   bool, PREF_VALUE
 )
 #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,
   bool, false
 )
 
 // 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,
   bool, true
 )
 
 VARCACHE_PREF(
   "dom.performance.enable_scheduler_timing",
new file mode 100644
--- /dev/null
+++ b/modules/libpref/test/unit_ipc/test_sharedMap_var_caches.js
@@ -0,0 +1,66 @@
+/* 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 variable caches in the content process
+// correctly handle values which are different from their
+// statically-defined defaults.
+//
+// Since we can't access variable cache 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}`);
+});
+
--- a/modules/libpref/test/unit_ipc/xpcshell.ini
+++ b/modules/libpref/test/unit_ipc/xpcshell.ini
@@ -4,9 +4,11 @@ skip-if = toolkit == 'android'
 
 [test_existing_prefs.js]
 [test_initial_prefs.js]
 [test_large_pref.js]
 [test_locked_prefs.js]
 [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]