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
--- 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,