Bug 1471025: Part 7a - Look up preferences from both dynamic and shared preference tables. r=njn

This patch changes our preference look-up behavior to first check the dynamic
hashtable, and then fall back to the shared map.

In order for this to work, we need to make several other changes as well:

- Attempts to modify a preference that only exists in the shared table
  requires that we copy it to the dynamic table, and change the value of the
  new entry.

- Attempts to clear a user preference with no default value, but which also
  exists in the shared map, requires that we keep an entry in the dynamic
  table to mask the shared entry. To make this work, we change the type of
  these entries to None, and ignore them during look-ups and iteration.

- Iteration needs to take both hashtables into consideration. The
  serialization iterator for changed preferences only needs to care about
  dynamic values, so it remains unchanged. Most of the others need to use
  PrefsIter() instead.

MozReview-Commit-ID: 9PWmSZxoC9Z

--HG--
extra : source : 5051f15fc2005667cfe76ccae0afb1fb0657c103
extra : histedit_source : 28f2216b8c1e9d08ed9b2c03910d4b8434e55421

modules/libpref/Preferences.cpp

@@ -160,6 +160,23 @@ union PrefValue {
   int32_t mIntVal;
   bool mBoolVal;
 
+  PrefValue() = default;
+
+  explicit PrefValue(bool aVal)
+    : mBoolVal(aVal)
+  {
+  }
+
+  explicit PrefValue(int32_t aVal)
+    : mIntVal(aVal)
+  {
+  }
+
+  explicit PrefValue(const char* aVal)
+    : mStringVal(aVal)
+  {
+  }
+
   bool Equals(PrefType aType, PrefValue aValue)
   {
     switch (aType) {
@@ -437,6 +454,8 @@ struct PrefsSizes
 
 static ArenaAllocator<8192, 1> gPrefNameArena;
 
+class PrefWrapper;
+
 class Pref
 {
 public:
@@ -662,6 +681,8 @@ public:
     }
   }
 
+  void FromWrapper(PrefWrapper& aWrapper);
+
   bool HasAdvisablySizedValues()
   {
     MOZ_ASSERT(XRE_IsParentProcess());
@@ -1079,6 +1100,21 @@ public:
   FORWARD(const char*, GetBareStringValue)
 #undef FORWARD
 
+  PrefValue GetValue(PrefValueKind aKind = PrefValueKind::User) const
+  {
+    switch (Type()) {
+      case PrefType::Bool:
+        return PrefValue{ GetBoolValue(aKind) };
+      case PrefType::Int:
+        return PrefValue{ GetIntValue(aKind) };
+      case PrefType::String:
+        return PrefValue{ GetBareStringValue(aKind) };
+      default:
+        MOZ_ASSERT_UNREACHABLE("Unexpected pref type");
+        return PrefValue{};
+    }
+  }
+
   Result<PrefValueKind, nsresult> WantValueKind(PrefType aType,
                                                 PrefValueKind aKind) const
   {
@@ -1121,8 +1157,67 @@ public:
     aResult = GetStringValue(kind);
     return NS_OK;
   }
+
+  bool Matches(PrefType aType,
+               PrefValueKind aKind,
+               PrefValue& aValue,
+               bool aIsSticky,
+               bool aIsLocked) const
+  {
+    return (ValueMatches(aKind, aType, aValue) && aIsSticky == IsSticky() &&
+            aIsLocked == IsLocked());
+  }
+
+  bool ValueMatches(PrefValueKind aKind,
+                    PrefType aType,
+                    const PrefValue& aValue) const
+  {
+    if (!IsType(aType)) {
+      return false;
+    }
+    if (!(aKind == PrefValueKind::Default ? HasDefaultValue()
+                                          : HasUserValue())) {
+      return false;
+    }
+    switch (aType) {
+      case PrefType::Bool:
+        return GetBoolValue(aKind) == aValue.mBoolVal;
+      case PrefType::Int:
+        return GetIntValue(aKind) == aValue.mIntVal;
+      case PrefType::String:
+        return strcmp(GetBareStringValue(aKind), aValue.mStringVal) == 0;
+      default:
+        MOZ_ASSERT_UNREACHABLE("Unexpected preference type");
+        return false;
+    }
+  }
 };
 
+void
+Pref::FromWrapper(PrefWrapper& aWrapper)
+{
+  MOZ_ASSERT(aWrapper.is<SharedPrefMap::Pref>());
+  auto pref = aWrapper.as<SharedPrefMap::Pref>();
+
+  MOZ_ASSERT(IsTypeNone());
+  MOZ_ASSERT(strcmp(mName, pref.Name()) == 0);
+
+  mType = uint32_t(pref.Type());
+
+  mIsLocked = pref.IsLocked();
+  mIsSticky = pref.IsSticky();
+
+  mHasDefaultValue = pref.HasDefaultValue();
+  mHasUserValue = pref.HasUserValue();
+
+  if (mHasDefaultValue) {
+    mDefaultValue.Init(Type(), aWrapper.GetValue(PrefValueKind::Default));
+  }
+  if (mHasUserValue) {
+    mUserValue.Init(Type(), aWrapper.GetValue(PrefValueKind::User));
+  }
+}
+
 class CallbackNode
 {
 public:
@@ -1550,7 +1645,7 @@ pref_HashTableLookup(const char* aPrefName)
 static const PrefWrapper* gCallbackPref;
 
 Maybe<PrefWrapper>
-pref_Lookup(const char* aPrefName)
+pref_Lookup(const char* aPrefName, bool aIncludeTypeNone = false)
 {
   Maybe<PrefWrapper> result;
 
@@ -1561,12 +1656,43 @@ pref_Lookup(const char* aPrefName)
   if (gCallbackPref && strcmp(aPrefName, gCallbackPref->Name()) == 0) {
     result.emplace(*gCallbackPref);
   } else if (Pref* pref = pref_HashTableLookup(aPrefName)) {
-    result.emplace(pref);
+    if (aIncludeTypeNone || !pref->IsTypeNone()) {
+      result.emplace(pref);
+    }
+  } else if (gSharedMap) {
+    Maybe<SharedPrefMap::Pref> pref = gSharedMap->Get(aPrefName);
+    if (pref.isSome()) {
+      result.emplace(*pref);
+    }
   }
 
   return result;
 }
 
+static Result<Pref*, nsresult>
+pref_LookupForModify(const char* aPrefName,
+                     const std::function<bool(const PrefWrapper&)>& aCheckFn)
+{
+  Maybe<PrefWrapper> wrapper = pref_Lookup(aPrefName, /* includeTypeNone */ true);
+  if (wrapper.isNothing()) {
+    return Err(NS_ERROR_INVALID_ARG);
+  }
+  if (!aCheckFn(*wrapper)) {
+    return nullptr;
+  }
+  if (wrapper->is<Pref*>()) {
+    return wrapper->as<Pref*>();
+  }
+
+  auto entry = static_cast<PrefEntry*>(gHashTable->Add(aPrefName, fallible));
+  if (!entry) {
+    return Err(NS_ERROR_OUT_OF_MEMORY);
+  }
+  Pref* pref = entry->mPref;
+  pref->FromWrapper(*wrapper);
+  return pref;
+}
+
 static nsresult
 pref_SetPref(const char* aPrefName,
              PrefType aType,
@@ -1582,15 +1708,29 @@ pref_SetPref(const char* aPrefName,
     return NS_ERROR_OUT_OF_MEMORY;
   }
 
-  auto entry = static_cast<PrefEntry*>(gHashTable->Add(aPrefName, fallible));
-  if (!entry) {
-    return NS_ERROR_OUT_OF_MEMORY;
+  Pref* pref = nullptr;
+  if (gSharedMap) {
+    auto result =
+      pref_LookupForModify(aPrefName, [&](const PrefWrapper& aWrapper) {
+        return !aWrapper.Matches(aType, aKind, aValue, aIsSticky, aIsLocked);
+      });
+    if (result.isOk() && !(pref = result.unwrap())) {
+      // No changes required.
+      return NS_OK;
+    }
   }
 
-  Pref* pref = entry->mPref;
-  if (pref->IsTypeNone()) {
-    // New entry. Set the type.
-    pref->SetType(aType);
+  if (!pref) {
+    auto entry = static_cast<PrefEntry*>(gHashTable->Add(aPrefName, fallible));
+    if (!entry) {
+      return NS_ERROR_OUT_OF_MEMORY;
+    }
+    pref = entry->mPref;
+
+    if (pref->IsTypeNone()) {
+      // New entry. Set the type.
+      pref->SetType(aType);
+    }
   }
 
   bool valueChanged = false;
@@ -2769,10 +2909,9 @@ nsPrefBranch::GetChildList(const char* aStartingAt,
 
   const PrefName& parent = GetPrefName(aStartingAt);
   size_t parentLen = parent.Length();
-  for (auto iter = gHashTable->Iter(); !iter.Done(); iter.Next()) {
-    Pref* pref = static_cast<PrefEntry*>(iter.Get())->mPref;
+  for (auto& pref : PrefsIter(gHashTable, gSharedMap)) {
     if (strncmp(pref->Name(), parent.get(), parentLen) == 0) {
-      prefArray.AppendElement(pref->Name());
+      prefArray.AppendElement(pref->NameString());
     }
   }
 
@@ -4029,6 +4168,10 @@ Preferences::ResetPrefs()
 {
   ENSURE_PARENT_PROCESS("Preferences::ResetPrefs", "all prefs");
 
+  if (gSharedMap) {
+    return NS_ERROR_NOT_AVAILABLE;
+  }
+
   gHashTable->ClearAndPrepareForLength(PREF_HASHTABLE_INITIAL_LENGTH);
   gPrefNameArena.Clear();
 
@@ -4143,7 +4286,13 @@ Preferences::SetPreference(const dom::Pref& aDomPref)
   //   needlessly, but that's ok because this case is rare.
   //
   if (!pref->HasDefaultValue() && !pref->HasUserValue()) {
-    gHashTable->RemoveEntry(entry);
+    // If the preference exists in the shared map, we need to keep the dynamic
+    // entry around to mask it.
+    if (gSharedMap->Has(pref->Name())) {
+      pref->SetType(PrefType::None);
+    } else {
+      gHashTable->RemoveEntry(entry);
+    }
     pref = nullptr;
   }
 
@@ -4967,12 +5116,13 @@ Preferences::Lock(const char* aPrefName)
   ENSURE_PARENT_PROCESS("Lock", aPrefName);
   NS_ENSURE_TRUE(InitStaticMembers(), NS_ERROR_NOT_AVAILABLE);
 
-  Pref* pref = pref_HashTableLookup(aPrefName);
-  if (!pref) {
-    return NS_ERROR_UNEXPECTED;
-  }
+  Pref* pref;
+  MOZ_TRY_VAR(pref,
+              pref_LookupForModify(aPrefName, [](const PrefWrapper& aPref) {
+                return !aPref.IsLocked();
+              }));
 
-  if (!pref->IsLocked()) {
+  if (pref) {
     pref->SetIsLocked(true);
     NotifyCallbacks(aPrefName, PrefWrapper(pref));
   }
@@ -4986,12 +5136,13 @@ Preferences::Unlock(const char* aPrefName)
   ENSURE_PARENT_PROCESS("Unlock", aPrefName);
   NS_ENSURE_TRUE(InitStaticMembers(), NS_ERROR_NOT_AVAILABLE);
 
-  Pref* pref = pref_HashTableLookup(aPrefName);
-  if (!pref) {
-    return NS_ERROR_UNEXPECTED;
-  }
+  Pref* pref;
+  MOZ_TRY_VAR(pref,
+              pref_LookupForModify(aPrefName, [](const PrefWrapper& aPref) {
+                return aPref.IsLocked();
+              }));
 
-  if (pref->IsLocked()) {
+  if (pref) {
     pref->SetIsLocked(false);
     NotifyCallbacks(aPrefName, PrefWrapper(pref));
   }
@@ -5014,16 +5165,27 @@ Preferences::ClearUser(const char* aPrefName)
   ENSURE_PARENT_PROCESS("ClearUser", aPrefName);
   NS_ENSURE_TRUE(InitStaticMembers(), NS_ERROR_NOT_AVAILABLE);
 
-  PrefEntry* entry = pref_HashTableLookupInner(aPrefName);
-  Pref* pref;
-  if (entry && (pref = entry->mPref) && pref->HasUserValue()) {
+  auto result = pref_LookupForModify(
+    aPrefName, [](const PrefWrapper& aPref) { return aPref.HasUserValue(); });
+  if (result.isErr()) {
+    return NS_OK;
+  }
+
+  if (Pref* pref = result.unwrap()) {
     pref->ClearUserValue();
 
     if (!pref->HasDefaultValue()) {
-      gHashTable->RemoveEntry(entry);
+      if (!gSharedMap || !gSharedMap->Has(pref->Name())) {
+        gHashTable->Remove(aPrefName);
+      } else {
+        pref->SetType(PrefType::None);
+      }
+
+      NotifyCallbacks(aPrefName);
+    } else {
+      NotifyCallbacks(aPrefName, PrefWrapper(pref));
     }
 
-    NotifyCallbacks(aPrefName);
     Preferences::HandleDirty();
   }
   return NS_OK;

modules/libpref/test/unit/test_libPrefs.js

@@ -235,9 +235,9 @@ function run_test() {
 
   // locking and unlocking a nonexistent pref should throw
   do_check_throws(function() {
-    ps.lockPref("DefaultPref.nonexistent");}, Cr.NS_ERROR_UNEXPECTED);
+    ps.lockPref("DefaultPref.nonexistent");}, Cr.NS_ERROR_ILLEGAL_VALUE);
   do_check_throws(function() {
-    ps.unlockPref("DefaultPref.nonexistent");}, Cr.NS_ERROR_UNEXPECTED);
+    ps.unlockPref("DefaultPref.nonexistent");}, Cr.NS_ERROR_ILLEGAL_VALUE);
 
   // getting a locked pref branch should return the "default" value
   Assert.ok(!ps.prefIsLocked("DefaultPref.char"));

modules/libpref/test/unit_ipc/test_large_pref.js

@@ -52,6 +52,14 @@ function run_test() {
 
   let isParent = isParentProcess();
   if (isParent) {
+    // Preferences with large values will still appear in the shared memory
+    // snapshot that we share with all processes. They should not, however, be
+    // sent with the list of changes on top of the snapshot.
+    //
+    // So, make sure we've generated the initial snapshot before we set the
+    // preference values by launching a child process with an empty test.
+    sendCommand("");
+
     // Set all combinations of none, small and large, for default and user prefs.
     for (let def of testValues) {
       for (let user of testValues) {
@@ -82,8 +90,8 @@ function run_test() {
         // large, so the preference should not be set.
         let prefExists;
         try {
-          pb.getCharPref(pref_name);
-          prefExists = true;
+          let val = pb.getCharPref(pref_name);
+          prefExists = val.length > 128;
         } catch(e) {
           prefExists = false;
         }