Bug 2058647 r=beth
Two issues here: - There was some inconsistency in how we handled domains with multiple dots at the end. Instead of making this consistent, I just did the easier thing and return an error if a domain ends with more than one dot, because I don't think there's a realistic use case for it. - Also, places that we called `Preferences::UnregisterCallback()` assumed that it succeeded and would always release the last reference to the `PrefCallback`, so if it didn't succeed a `PrefCallback` would get freed but would still be in `mObservers`. So now we check if `UnregisterCallback()` fails and if so, add the `PrefCallback` back to `mObservers`. (I decided to do it this way since `UnregisterCallback()` failing should be very rare so I didn't want to incur a lookup followed by a remove in the normal case) (technically we only need to fix one of these to close the hole, but fixing both made sense) Differential Revision: https://phabricator.services.mozilla.com/D323047
This commit is contained in:
Greg Stoll
committed by
gstoll@mozilla.com
gstoll@mozilla.com
parent
b3bb8982b8
commit
f35e75640c
@@ -1421,6 +1421,21 @@ static nsCString CopyStrippingTrailingDot(const nsACString& aDomain) {
|
||||
return nsCString(aDomain);
|
||||
}
|
||||
|
||||
// A single trailing dot is normalized away; more than one is rejected since
|
||||
// only one would be stripped, leaving an empty trailing segment.
|
||||
static bool DomainEndsWithMultipleDots(const nsACString& aDomain) {
|
||||
return StringEndsWith(aDomain, ".."_ns);
|
||||
}
|
||||
|
||||
static bool DomainEndsWithMultipleDots(const char* const* aDomains) {
|
||||
for (const char* const* p = aDomains; *p; ++p) {
|
||||
if (DomainEndsWithMultipleDots(nsDependentCString(*p))) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
||||
// The fire-time payload of a pref-change callback: the function and its
|
||||
// closure. Shared base of the trie's refcounted CallbackNode and the mirror
|
||||
// list's inline entries (see MirrorCallbackList), so the notify, sweep and
|
||||
@@ -3484,30 +3499,37 @@ nsPrefBranch::AddObserverImpl(const nsACString& aDomain, nsIObserver* aObserver,
|
||||
mozilla::UniquePtr<PrefCallback> existing;
|
||||
mObservers.Remove(&weakKey, &existing);
|
||||
if (existing) {
|
||||
Preferences::UnregisterCallback(NotifyObserver, prefName,
|
||||
existing.get(),
|
||||
/* aPrefixMatch */ true);
|
||||
nsresult rv = Preferences::UnregisterCallback(NotifyObserver, prefName,
|
||||
existing.get(),
|
||||
/* aPrefixMatch */ true);
|
||||
if (NS_WARN_IF(NS_FAILED(rv))) {
|
||||
// The trie still points at the weak PrefCallback; keep it alive and
|
||||
// leave the weak registration in place.
|
||||
mObservers.InsertOrUpdate(&weakKey, std::move(existing));
|
||||
return rv;
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
nsresult rv = NS_OK;
|
||||
mObservers.WithEntryHandle(pCallback.get(), [&](auto&& p) {
|
||||
if (p) {
|
||||
NS_WARNING(
|
||||
nsPrintfCString("Ignoring duplicate observer: %s", prefName.get())
|
||||
.get());
|
||||
} else {
|
||||
// We must pass a fully qualified preference name to the callback
|
||||
// aDomain == nullptr is the only possible failure, and we trapped it with
|
||||
// NS_ENSURE_ARG above.
|
||||
Preferences::RegisterCallback(NotifyObserver, prefName, pCallback.get(),
|
||||
/* aPrefixMatch */ true);
|
||||
|
||||
p.Insert(std::move(pCallback));
|
||||
// We must pass a fully qualified preference name to the callback.
|
||||
rv = Preferences::RegisterCallback(NotifyObserver, prefName,
|
||||
pCallback.get(),
|
||||
/* aPrefixMatch */ true);
|
||||
if (NS_SUCCEEDED(rv)) {
|
||||
p.Insert(std::move(pCallback));
|
||||
}
|
||||
}
|
||||
});
|
||||
|
||||
return NS_OK;
|
||||
return rv;
|
||||
}
|
||||
|
||||
NS_IMETHODIMP
|
||||
@@ -3529,8 +3551,9 @@ nsPrefBranch::RemoveObserverImpl(const nsACString& aDomain,
|
||||
}
|
||||
|
||||
// Remove the relevant PrefCallback from mObservers and get an owning pointer
|
||||
// to it. Unregister the callback first, and then let the owning pointer go
|
||||
// out of scope and destroy the callback.
|
||||
// to it. If unregistering the trie callback fails (e.g. because mObservers
|
||||
// and the trie have drifted out of sync), the CallbackNode in the trie still
|
||||
// holds Data() == this PrefCallback, so put it back.
|
||||
const nsCString& prefName = GetPrefName(aDomain);
|
||||
PrefCallback key(prefName, aObserver, this);
|
||||
mozilla::UniquePtr<PrefCallback> pCallback;
|
||||
@@ -3551,6 +3574,10 @@ nsPrefBranch::RemoveObserverImpl(const nsACString& aDomain,
|
||||
rv = Preferences::UnregisterCallback(NotifyObserver, prefName,
|
||||
pCallback.get(),
|
||||
/* aPrefixMatch */ true);
|
||||
if (NS_FAILED(rv)) {
|
||||
PrefCallback* raw = pCallback.get();
|
||||
mObservers.InsertOrUpdate(raw, std::move(pCallback));
|
||||
}
|
||||
}
|
||||
|
||||
return rv;
|
||||
@@ -5940,6 +5967,7 @@ nsresult PreferencesImpl::RegisterCallbackImpl(PrefChangedFunc aCallback,
|
||||
bool aIsPrefix) {
|
||||
MOZ_ASSERT(NS_IsMainThread());
|
||||
NS_ENSURE_ARG(aCallback);
|
||||
NS_ENSURE_FALSE(DomainEndsWithMultipleDots(aPrefNode), NS_ERROR_INVALID_ARG);
|
||||
NS_ENSURE_TRUE(Preferences::InitStaticMembers(), NS_ERROR_NOT_AVAILABLE);
|
||||
|
||||
RefPtr<CallbackNode> node =
|
||||
@@ -5969,6 +5997,7 @@ nsresult PreferencesImpl::UnregisterCallbackImpl(PrefChangedFunc aCallback,
|
||||
bool aIsPrefix) {
|
||||
MOZ_ASSERT(NS_IsMainThread());
|
||||
MOZ_ASSERT(aCallback);
|
||||
NS_ENSURE_FALSE(DomainEndsWithMultipleDots(aPrefNode), NS_ERROR_INVALID_ARG);
|
||||
if (Preferences::sShutdown) {
|
||||
MOZ_ASSERT(!Preferences::sPreferences);
|
||||
return NS_OK;
|
||||
|
||||
@@ -294,7 +294,9 @@ class Preferences final : public nsIPrefService,
|
||||
// boundaries: the callback fires for aPref itself and for any pref that
|
||||
// extends it by one or more whole '.'-delimited segments, but not for a pref
|
||||
// that merely shares a leading substring (e.g. a "foo.bar" prefix does not
|
||||
// match "foo.barbaz"). A trailing '.' is optional and is normalized away.
|
||||
// match "foo.barbaz"). A trailing '.' is optional and is normalized away;
|
||||
// a domain ending in more than one '.' is rejected with
|
||||
// NS_ERROR_INVALID_ARG.
|
||||
template <typename T = void>
|
||||
static nsresult RegisterPrefixCallback(PrefChangedFunc aCallback,
|
||||
const nsACString& aPref,
|
||||
|
||||
@@ -400,6 +400,30 @@ TEST(PrefsCallbackTrie, TrailingDotEquivalence)
|
||||
EXPECT_EQ(count, 1);
|
||||
}
|
||||
|
||||
// A domain ending in more than one dot is rejected rather than partially
|
||||
// normalized.
|
||||
TEST(PrefsCallbackTrie, MultipleTrailingDotsRejected)
|
||||
{
|
||||
int count = 0;
|
||||
EXPECT_EQ(Preferences::RegisterCallback(IncrementCount,
|
||||
"test.trie.dots.a.."_ns, &count),
|
||||
NS_ERROR_INVALID_ARG);
|
||||
EXPECT_EQ(Preferences::RegisterPrefixCallback(
|
||||
IncrementCount, "test.trie.dots.a.."_ns, &count),
|
||||
NS_ERROR_INVALID_ARG);
|
||||
EXPECT_EQ(Preferences::UnregisterCallback(IncrementCount,
|
||||
"test.trie.dots.a.."_ns, &count),
|
||||
NS_ERROR_INVALID_ARG);
|
||||
|
||||
nsCOMPtr<nsIPrefBranch> prefs = do_GetService(NS_PREFSERVICE_CONTRACTID);
|
||||
ASSERT_TRUE(prefs);
|
||||
RefPtr<TestWeakPrefObserver> observer = new TestWeakPrefObserver();
|
||||
EXPECT_EQ(prefs->AddObserver("test.trie.dots.d.."_ns, observer, false),
|
||||
NS_ERROR_INVALID_ARG);
|
||||
Preferences::SetBool("test.trie.dots.d.e", true);
|
||||
EXPECT_EQ(observer->mNotifyCount, 0);
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Compact / MarkDead tests — verify lazy-unregister behavior.
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
Reference in New Issue
Block a user