Bug 1394578 - Rewrite Preferences::SetPreference(). r=glandium draft
authorNicholas Nethercote <nnethercote@mozilla.com>
Thu, 23 Nov 2017 18:03:02 +1100
changeset 703384 e2209017d344822fa829befc0f6b430ec8d32047
parent 702919 02d7f1b5af99ff2732e338c8c5834f48daaa9e58
child 703385 a6506ae8f7aed688ef5f8d730fb7d2f03bf0e3ab
push id90801
push usernnethercote@mozilla.com
push dateSat, 25 Nov 2017 06:45:10 +0000
reviewersglandium
bugs1394578
milestone59.0a1
Bug 1394578 - Rewrite Preferences::SetPreference(). r=glandium Preferences::SetPreference() is used when setting prefs in the content process from dom::Pref values that are passed from the parent process. Currently we use the high-level Set*InAnyProcess() methods to do this -- basically the same code path as sets done via the API -- but this has several problems. - It is subtly broken. If the content process already has a pref of type T with a default value and then we get a SetPreference() call that tries to change it to type U, it will erroneously fail. In practice this never(?) happens, but it shows that things aren't arranged very well. - SetPreference() also looks up the hashtable twice to get the same pref if both a default value and a user value are present in the dom::Pref. - This happens in content processes, while all other pref set operations occur in the parent process. This is the main reason we have the Set*InAnyProcess() functions. In short, the setting of prefs via IPC is quite different to the setting of prefs via other means -- because it happens in content processes, and it's more of a clobber that can set both values at once -- and so should be handled differently. The solution is to make Preferences::SetPreference() use lower-level operations to do the update. It now does the hash table lookup/add itself, and then calls the new Pref::FromDomPref() method. That method then possibly calls the new PrefValue::FromDomPrefValue() method for both kinds of value, and overwrites them as necessary. SetValueFromDom() is no longer used and the patch removes it. MozReview-Commit-ID: 2Rg8VMOc0Cl
modules/libpref/Preferences.cpp
modules/libpref/Preferences.h
--- a/modules/libpref/Preferences.cpp
+++ b/modules/libpref/Preferences.cpp
@@ -195,16 +195,36 @@ union PrefValue {
       case PrefType::Bool:
         *aDomValue = mBoolVal;
         return;
 
       default:
         MOZ_CRASH();
     }
   }
+
+  PrefType FromDomPrefValue(const dom::PrefValue& aDomValue)
+  {
+    switch (aDomValue.type()) {
+      case dom::PrefValue::TnsCString:
+        mStringVal = aDomValue.get_nsCString().get();
+        return PrefType::String;
+
+      case dom::PrefValue::Tint32_t:
+        mIntVal = aDomValue.get_int32_t();
+        return PrefType::Int;
+
+      case dom::PrefValue::Tbool:
+        mBoolVal = aDomValue.get_bool();
+        return PrefType::Bool;
+
+      default:
+        MOZ_CRASH();
+    }
+  }
 };
 
 #ifdef DEBUG
 const char*
 PrefTypeToString(PrefType aType)
 {
   switch (aType) {
     case PrefType::String:
@@ -421,16 +441,57 @@ public:
 
     MOZ_ASSERT(aDomPref->defaultValue().type() ==
                  dom::MaybePrefValue::Tnull_t ||
                aDomPref->userValue().type() == dom::MaybePrefValue::Tnull_t ||
                (aDomPref->defaultValue().get_PrefValue().type() ==
                 aDomPref->userValue().get_PrefValue().type()));
   }
 
+  void FromDomPref(const dom::Pref& aDomPref, bool* aValueChanged)
+  {
+    MOZ_ASSERT(strcmp(mName, aDomPref.name().get()) == 0);
+
+    const dom::MaybePrefValue& defaultValue = aDomPref.defaultValue();
+    bool defaultValueChanged = false;
+    if (defaultValue.type() == dom::MaybePrefValue::TPrefValue) {
+      PrefValue value;
+      PrefType type = value.FromDomPrefValue(defaultValue.get_PrefValue());
+      if (!ValueMatches(PrefValueKind::Default, type, value)) {
+        // Type() is PrefType::None if it's a newly added pref. This is ok.
+        mDefaultValue.Replace(Type(), type, value);
+        SetType(type);
+        mHasDefaultValue = true;
+        defaultValueChanged = true;
+      }
+    }
+    // Note: we never clear a default value.
+
+    const dom::MaybePrefValue& userValue = aDomPref.userValue();
+    bool userValueChanged = false;
+    if (userValue.type() == dom::MaybePrefValue::TPrefValue) {
+      PrefValue value;
+      PrefType type = value.FromDomPrefValue(userValue.get_PrefValue());
+      if (!ValueMatches(PrefValueKind::User, type, value)) {
+        // Type() is PrefType::None if it's a newly added pref. This is ok.
+        mUserValue.Replace(Type(), type, value);
+        SetType(type);
+        mHasUserValue = true;
+        userValueChanged = true;
+      }
+    } else if (mHasUserValue) {
+      ClearUserValue();
+      userValueChanged = true;
+    }
+
+    if (userValueChanged || (defaultValueChanged && !mHasUserValue)) {
+      *aValueChanged = true;
+    }
+  }
+
   bool HasAdvisablySizedValues()
   {
     if (!IsTypeString()) {
       return true;
     }
 
     const char* stringVal;
     if (mHasDefaultValue) {
@@ -756,17 +817,17 @@ pref_SetPref(const char* aPrefName,
   }
 
   auto pref = static_cast<Pref*>(gHashTable->Add(aPrefName, fallible));
   if (!pref) {
     return NS_ERROR_OUT_OF_MEMORY;
   }
 
   if (!pref->Name()) {
-    // New (zeroed) entry. Initialize it.
+    // New (zeroed) entry. Partially initialize it.
     new (pref) Pref(aPrefName);
     pref->SetType(aType);
   }
 
   bool valueChanged = false, handleDirty = false;
   nsresult rv = pref->SetValue(
     aType, aKind, aValue, aIsSticky, aForceSet, &valueChanged, &handleDirty);
   if (NS_FAILED(rv)) {
@@ -3568,65 +3629,60 @@ Preferences::SavePrefFileAsynchronous()
 
 NS_IMETHODIMP
 Preferences::SavePrefFile(nsIFile* aFile)
 {
   // This is the method accessible from service API. Make it off main thread.
   return SavePrefFileInternal(aFile, SaveMethod::Asynchronous);
 }
 
-/* static */ nsresult
-Preferences::SetValueFromDom(const char* aPrefName,
-                             const dom::PrefValue& aValue,
-                             PrefValueKind aKind)
+/* static */ void
+Preferences::SetPreference(const dom::Pref& aDomPref)
 {
-  switch (aValue.type()) {
-    case dom::PrefValue::TnsCString:
-      return Preferences::SetCStringInAnyProcess(
-        aPrefName, aValue.get_nsCString(), aKind);
-
-    case dom::PrefValue::Tint32_t:
-      return Preferences::SetIntInAnyProcess(
-        aPrefName, aValue.get_int32_t(), aKind);
-
-    case dom::PrefValue::Tbool:
-      return Preferences::SetBoolInAnyProcess(
-        aPrefName, aValue.get_bool(), aKind);
-
-    default:
-      MOZ_CRASH();
+  MOZ_ASSERT(!XRE_IsParentProcess());
+  NS_ENSURE_TRUE(InitStaticMembers(), (void)0);
+
+  const char* prefName = aDomPref.name().get();
+
+  auto pref = static_cast<Pref*>(gHashTable->Add(prefName, fallible));
+  if (!pref) {
+    return;
+  }
+
+  if (!pref->Name()) {
+    // New (zeroed) entry. Partially initialize it.
+    new (pref) Pref(prefName);
+  }
+
+  bool valueChanged = false;
+  pref->FromDomPref(aDomPref, &valueChanged);
+
+  // When the parent process clears a pref's user value we get a DomPref here
+  // with no default value and no user value. There are two possibilities.
+  //
+  // - 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(pref);
+  }
+
+  // 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);
   }
 }
 
-void
-Preferences::SetPreference(const dom::Pref& aDomPref)
-{
-  const char* prefName = aDomPref.name().get();
-  const dom::MaybePrefValue& defaultValue = aDomPref.defaultValue();
-  const dom::MaybePrefValue& userValue = aDomPref.userValue();
-
-  if (defaultValue.type() == dom::MaybePrefValue::TPrefValue) {
-    nsresult rv = SetValueFromDom(
-      prefName, defaultValue.get_PrefValue(), PrefValueKind::Default);
-    if (NS_FAILED(rv)) {
-      return;
-    }
-  }
-
-  if (userValue.type() == dom::MaybePrefValue::TPrefValue) {
-    SetValueFromDom(prefName, userValue.get_PrefValue(), PrefValueKind::User);
-  } else {
-    Preferences::ClearUserInAnyProcess(prefName);
-  }
-
-  // NB: we should never try to clear a default value, that doesn't
-  // make sense
-}
-
-void
+/* static */ void
 Preferences::GetPreference(dom::Pref* aDomPref)
 {
   Pref* pref = pref_HashTableLookup(aDomPref->name().get());
   if (pref && pref->HasAdvisablySizedValues()) {
     pref->ToDomPref(aDomPref);
   }
 }
 
--- a/modules/libpref/Preferences.h
+++ b/modules/libpref/Preferences.h
@@ -401,20 +401,16 @@ public:
   {
     PrefixMatch,
     ExactMatch,
   };
 
 private:
   static mozilla::Result<mozilla::Ok, const char*> InitInitialObjects();
 
-  static nsresult SetValueFromDom(const char* aPrefName,
-                                  const dom::PrefValue& aValue,
-                                  PrefValueKind aKind);
-
   // Functions above that modify prefs will fail if they are not run in the
   // parent process. The "InAnyProcess" functions below will succeed outside
   // the content process, and are used when passing pref values from the parent
   // process to content processes.
 
   static nsresult SetBoolInAnyProcess(
     const char* aPrefName,
     bool aValue,