Bug 1471025: Part 7a - Look up preferences from both dynamic and shared preference tables. r=njn draft
authorKris Maglione <maglione.k@gmail.com>
Mon, 02 Jul 2018 22:52:53 -0700
changeset 815380 3a0b14b81e335dd1514cf075fe3d525cc0a88bd2
parent 815379 8bd27d0b04812c453e759ed0397330be61ec8aef
child 815381 b255da769062497e19724fdcb316f5131e6b8e89
push id115503
push usermaglione.k@gmail.com
push dateSat, 07 Jul 2018 19:50:37 +0000
reviewersnjn
bugs1471025
milestone63.0a1
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
modules/libpref/Preferences.cpp
modules/libpref/test/unit/test_libPrefs.js
modules/libpref/test/unit_ipc/test_large_pref.js
--- a/modules/libpref/Preferences.cpp
+++ b/modules/libpref/Preferences.cpp
@@ -155,16 +155,33 @@ DeserializeString(char* aChars, nsCStrin
 }
 
 // Keep this in sync with PrefValue in prefs_parser/src/lib.rs.
 union PrefValue {
   const char* mStringVal;
   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) {
       case PrefType::String: {
         if (mStringVal && aValue.mStringVal) {
           return strcmp(mStringVal, aValue.mStringVal) == 0;
         }
         if (!mStringVal && !aValue.mStringVal) {
@@ -432,16 +449,18 @@ struct PrefsSizes
   size_t mCallbacksObjects;
   size_t mCallbacksDomains;
   size_t mMisc;
 };
 }
 
 static ArenaAllocator<8192, 1> gPrefNameArena;
 
+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)
@@ -657,16 +676,18 @@ public:
 
     mHasChangedSinceInit = true;
 
     if (userValueChanged || (defaultValueChanged && !mHasUserValue)) {
       *aValueChanged = true;
     }
   }
 
+  void FromWrapper(PrefWrapper& aWrapper);
+
   bool HasAdvisablySizedValues()
   {
     MOZ_ASSERT(XRE_IsParentProcess());
 
     if (!IsTypeString()) {
       return true;
     }
 
@@ -1074,16 +1095,31 @@ public:
   }
 
   FORWARD(bool, GetBoolValue)
   FORWARD(int32_t, GetIntValue)
   FORWARD(nsCString, GetStringValue)
   FORWARD(const char*, GetBareStringValue)
 #undef FORWARD
 
+  PrefValue GetValue(PrefValueKind aKind = PrefValueKind::Default) 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
   {
     if (Type() != aType) {
       return Err(NS_ERROR_UNEXPECTED);
     }
 
     if (aKind == PrefValueKind::Default || IsLocked() || !HasUserValue()) {
@@ -1116,18 +1152,77 @@ public:
   nsresult GetCStringValue(PrefValueKind aKind, nsACString& aResult) const
   {
     PrefValueKind kind;
     MOZ_TRY_VAR(kind, WantValueKind(PrefType::String, aKind));
 
     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:
   CallbackNode(const nsACString& aDomain,
                PrefChangedFunc aFunc,
                void* aData,
                Preferences::MatchKind aMatchKind)
     : mDomain(aDomain)
@@ -1540,57 +1635,102 @@ pref_HashTableLookup(const char* aPrefNa
 // While notifying preference callbacks, this holds the wrapper for the
 // preference being notified, in order to optimize lookups.
 //
 // Note: Callbacks and lookups only happen on the main thread, so this is safe
 // to use without locking.
 static const PrefWrapper* gCallbackPref;
 
 Maybe<PrefWrapper>
-pref_Lookup(const char* aPrefName)
+pref_Lookup(const char* aPrefName, bool aIncludeTypeNone = false)
 {
   Maybe<PrefWrapper> result;
 
   MOZ_ASSERT(NS_IsMainThread());
 
   AddAccessCount(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,
              PrefValueKind aKind,
              PrefValue aValue,
              bool aIsSticky,
              bool aIsLocked,
              bool aFromInit)
 {
   MOZ_ASSERT(NS_IsMainThread());
 
   if (!gHashTable) {
     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 = entry->mPref;
-  if (pref->IsTypeNone()) {
-    // New entry. Set the type.
-    pref->SetType(aType);
+  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;
+    }
+  }
+
+  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;
   nsresult rv;
   if (aKind == PrefValueKind::Default) {
     rv = pref->SetDefaultValue(
       aType, aValue, aIsSticky, aIsLocked, aFromInit, &valueChanged);
   } else {
@@ -2759,20 +2899,19 @@ nsPrefBranch::GetChildList(const char* a
   *aChildArray = nullptr;
   *aCount = 0;
 
   // This will contain a list of all the pref name strings. Allocated on the
   // stack for speed.
 
   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());
     }
   }
 
   // Now that we've built up the list, run the callback on all the matching
   // elements.
   numPrefs = prefArray.Length();
 
   if (numPrefs) {
@@ -4017,16 +4156,20 @@ Preferences::ReadUserPrefsFromFile(nsIFi
   return openPrefFile(aFile, PrefValueKind::User);
 }
 
 NS_IMETHODIMP
 Preferences::ResetPrefs()
 {
   ENSURE_PARENT_PROCESS("Preferences::ResetPrefs", "all prefs");
 
+  if (gSharedMap) {
+    return NS_ERROR_NOT_AVAILABLE;
+  }
+
   gHashTable->ClearAndPrepareForLength(PREF_HASHTABLE_INITIAL_LENGTH);
   gPrefNameArena.Clear();
 
   return InitInitialObjects(/* isStartup */ false).isOk() ? NS_OK
                                                           : NS_ERROR_FAILURE;
 }
 
 NS_IMETHODIMP
@@ -4131,17 +4274,23 @@ Preferences::SetPreference(const dom::Pr
   // - There was an existing pref with only a user value. FromDomPref() will
   //   have just cleared that user value, so the pref can be removed.
   //
   // - There was no existing pref. FromDomPref() will have done nothing, and
   //   `pref` will be valueless. We will end up adding and removing the value
   //   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);
+    }
   }
 
   // Note: we don't have to worry about HandleDirty() because we are setting
   // prefs in the content process that have come from the parent process.
 
   if (valueChanged) {
     NotifyCallbacks(prefName, PrefWrapper(pref));
   }
@@ -4950,41 +5099,43 @@ Preferences::SetComplex(const char* aPre
 }
 
 /* static */ nsresult
 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;
-  }
-
-  if (!pref->IsLocked()) {
+  Pref* pref;
+  MOZ_TRY_VAR(pref,
+              pref_LookupForModify(aPrefName, [](const PrefWrapper& aPref) {
+                return !aPref.IsLocked();
+              }));
+
+  if (pref) {
     pref->SetIsLocked(true);
     NotifyCallbacks(aPrefName, PrefWrapper(pref));
   }
 
   return NS_OK;
 }
 
 /* static */ nsresult
 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;
-  }
-
-  if (pref->IsLocked()) {
+  Pref* pref;
+  MOZ_TRY_VAR(pref,
+              pref_LookupForModify(aPrefName, [](const PrefWrapper& aPref) {
+                return aPref.IsLocked();
+              }));
+
+  if (pref) {
     pref->SetIsLocked(false);
     NotifyCallbacks(aPrefName, PrefWrapper(pref));
   }
 
   return NS_OK;
 }
 
 /* static */ bool
@@ -4997,26 +5148,37 @@ Preferences::IsLocked(const char* aPrefN
 }
 
 /* static */ nsresult
 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;
 }
 
 /* static */ bool
 Preferences::HasUserValue(const char* aPrefName)
 {
--- a/modules/libpref/test/unit/test_libPrefs.js
+++ b/modules/libpref/test/unit/test_libPrefs.js
@@ -230,19 +230,19 @@ function run_test() {
   Assert.ok(pb1.prefHasUserValue("DefaultPref.char"));
   Assert.equal(ps.getCharPref("DefaultPref.char"), "_user"); 
 
   //**************************************************************************//
   // pref Locking/Unlocking tests
 
   // 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"));
   ps.lockPref("DefaultPref.char");
   Assert.equal(ps.getCharPref("DefaultPref.char"), "_default"); 
   Assert.ok(ps.prefIsLocked("DefaultPref.char"));
 
   // getting an unlocked pref branch should return the "user" value 
--- a/modules/libpref/test/unit_ipc/test_large_pref.js
+++ b/modules/libpref/test/unit_ipc/test_large_pref.js
@@ -47,16 +47,24 @@ function expectedPrefValue(def, user) {
 
 function run_test() {
   let pb = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch);
   let ps = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefService);
   let defaultBranch = ps.getDefaultBranch("");
 
   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) {
         let currPref = prefName(def, user);
         if (def.value) {
           defaultBranch.setCharPref(currPref, def.value);
         }
         if (user.value) {
@@ -77,18 +85,18 @@ function run_test() {
       let pref_name = prefName(def, user);
       if (isParent || (def.name != "Large" && user.name != "Large")) {
         Assert.equal(pb.getCharPref(pref_name), expectedPrefValue(def, user));
       } else {
         // This is the child, and either the default or user value is
         // 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;
         }
         ok(!prefExists,
            "Pref " + pref_name + " should not be set in the child");
       }
     }
   }