commit 29767dae3f0bbd8b2542ead07b935c286e480487
parent 257d067d01948c89d76ae518520f2a0389452aaa
Author: Randell Jesup <rjesup@mozilla.com>
Date: Mon, 3 Nov 2025 18:26:14 +0000
Bug 1996305: Ensure Suspend() is called before starting async dictionary origin reads r=necko-reviewers,valentin
Also adds an explict path for deleting dictionary origin entries, so we
don't have empty DictionaryOrigin objects hanging around.
Differential Revision: https://phabricator.services.mozilla.com/D270002
Diffstat:
6 files changed, 61 insertions(+), 33 deletions(-)
diff --git a/netwerk/cache2/CacheStorageService.cpp b/netwerk/cache2/CacheStorageService.cpp
@@ -746,7 +746,12 @@ static bool RemoveExactEntry(CacheEntryTable* aEntries, nsACString const& aKey,
// Remove from DictionaryCache immediately, to ensure the removal is
// synchronous
- DictionaryCache::RemoveDictionaryFor(aEntry->GetURI());
+
+ if (aEntry->GetEnhanceID().EqualsLiteral("dict:")) {
+ DictionaryCache::RemoveOriginFor(aEntry->GetURI());
+ } else {
+ DictionaryCache::RemoveDictionaryFor(aEntry->GetURI());
+ }
LOG(("RemoveExactEntry [entry=%p removed]", aEntry));
aEntries->Remove(aKey);
@@ -1511,8 +1516,7 @@ Result<size_t, nsresult> CacheStorageService::MemoryPool::PurgeByFrecency(
if (entry->GetEnhanceID().EqualsLiteral("dict:")) {
LOG(
("*** Ignored Entry is a dictionary origin, metadata size %d, "
- "referenced "
- "%d, Frecency %f",
+ "referenced %d, Frecency %f",
entry->GetMetadataMemoryConsumption(), entry->IsReferenced(),
entry->GetFrecency()));
}
diff --git a/netwerk/cache2/Dictionary.cpp b/netwerk/cache2/Dictionary.cpp
@@ -644,7 +644,7 @@ DictionaryCacheEntry::OnCacheEntryAvailable(nsICacheEntry* entry, bool isNew,
// Read the metadata for an Origin and parse it, creating DictionaryCacheEntrys
// as needed. If aType is TYPE_OTHER, there is no Match() to do
void DictionaryOriginReader::Start(
- DictionaryOrigin* aOrigin, nsACString& aKey, nsIURI* aURI,
+ bool aCreate, DictionaryOrigin* aOrigin, nsACString& aKey, nsIURI* aURI,
ExtContentPolicyType aType, DictionaryCache* aCache,
const std::function<nsresult(bool, DictionaryCacheEntry*)>& aCallback) {
mOrigin = aOrigin;
@@ -668,7 +668,7 @@ void DictionaryOriginReader::Start(
PromiseFlatCString(aKey).get(), this));
DictionaryCache::sCacheStorage->AsyncOpenURIString(
aKey, META_DICTIONARY_PREFIX,
- aOrigin
+ aCreate
? nsICacheStorage::OPEN_NORMALLY |
nsICacheStorage::CHECK_MULTITHREADED
: nsICacheStorage::OPEN_READONLY | nsICacheStorage::OPEN_SECRETLY |
@@ -858,12 +858,13 @@ already_AddRefed<DictionaryCacheEntry> DictionaryCache::AddEntry(
// This creates a cycle until the dictionary is removed from the cache
aDictEntry->SetOrigin(origin);
+ DICTIONARY_LOG(("Creating cache entry for origin %s", prepath.get()));
// Open (and parse metadata) or create
RefPtr<DictionaryOriginReader> reader = new DictionaryOriginReader();
// the type is irrelevant; we won't be calling Match()
reader->Start(
- origin, prepath, aURI, ExtContentPolicy::TYPE_OTHER, this,
+ true, origin, prepath, aURI, ExtContentPolicy::TYPE_OTHER, this,
[entry = RefPtr(aDictEntry)](
bool, DictionaryCacheEntry* aDict) { // XXX avoid so many lambdas
// which cause allocations
@@ -931,6 +932,34 @@ void DictionaryCache::RemoveDictionary(const nsACString& aKey) {
}
}
+// static
+void DictionaryCache::RemoveOriginFor(const nsACString& aKey) {
+ RefPtr<DictionaryCache> cache = GetInstance();
+ DICTIONARY_LOG(
+ ("Removing dictionary for %80s", PromiseFlatCString(aKey).get()));
+ NS_DispatchToMainThread(NewRunnableMethod<const nsCString>(
+ "DictionaryCache::RemoveDictionaryFor", cache,
+ &DictionaryCache::RemoveDictionary, aKey));
+}
+
+// Remove a dictionary if it exists for the key given
+void DictionaryCache::RemoveOrigin(const nsACString& aKey) {
+ DICTIONARY_LOG(("Removing origin for %80s", PromiseFlatCString(aKey).get()));
+
+ nsCOMPtr<nsIURI> uri;
+ if (NS_FAILED(NS_NewURI(getter_AddRefs(uri), aKey))) {
+ return;
+ }
+ nsAutoCString prepath;
+ if (NS_SUCCEEDED(GetDictPath(uri, prepath))) {
+ if (auto origin = mDictionaryCache.Lookup(prepath)) {
+ origin.Data()->AssertEmpty();
+ }
+
+ mDictionaryCache.Remove(prepath);
+ }
+}
+
// Remove a dictionary if it exists for the key given. Mainthread only.
// Note: due to cookie samesite rules, we need to clean for all ports
// static
@@ -1002,10 +1031,9 @@ void DictionaryCache::RemoveAllDictionaries() {
// Once we have a DictionaryOrigin (in-memory or parsed), scan it for matches.
// If it's not in the cache, return nullptr via callback.
void DictionaryCache::GetDictionaryFor(
- nsIURI* aURI, ExtContentPolicyType aType, bool& aAsync,
- nsHttpChannel* aChan, void (*aSuspend)(nsHttpChannel*),
+ nsIURI* aURI, ExtContentPolicyType aType, nsHttpChannel* aChan,
+ void (*aSuspend)(nsHttpChannel*),
const std::function<nsresult(bool, DictionaryCacheEntry*)>& aCallback) {
- aAsync = false;
// Note: IETF 2.2.3 Multiple Matching Directories
// We need to return match-dest matches first
// If no match-dest, then the longest match
@@ -1035,9 +1063,10 @@ void DictionaryCache::GetDictionaryFor(
RefPtr<DictionaryOriginReader> reader = new DictionaryOriginReader();
// Must do this before calling start, which can run the callbacks and call
// Resume
- aAsync = true;
+ DICTIONARY_LOG(("Suspending to get Dictionary headers"));
aSuspend(aChan);
- reader->Start(existing.Data(), prepath, aURI, aType, this, aCallback);
+ reader->Start(false, existing.Data(), prepath, aURI, aType, this,
+ aCallback);
}
return;
}
@@ -1073,8 +1102,9 @@ void DictionaryCache::GetDictionaryFor(
RefPtr<DictionaryOriginReader> reader = new DictionaryOriginReader();
// After Start(), if we drop this ref reader will kill itself on
// completion; it holds a self-ref
- reader->Start(origin, prepath, aURI, aType, this, aCallback);
- aAsync = true;
+ DICTIONARY_LOG(("Suspending to get Dictionary headers"));
+ aSuspend(aChan);
+ reader->Start(false, origin, prepath, aURI, aType, this, aCallback);
return;
}
// No dictionaries for origin
diff --git a/netwerk/cache2/Dictionary.h b/netwerk/cache2/Dictionary.h
@@ -237,7 +237,7 @@ class DictionaryOriginReader final : public nsICacheEntryOpenCallback,
DictionaryOriginReader() {}
void Start(
- DictionaryOrigin* aOrigin, nsACString& aKey, nsIURI* aURI,
+ bool aCreate, DictionaryOrigin* aOrigin, nsACString& aKey, nsIURI* aURI,
ExtContentPolicyType aType, DictionaryCache* aCache,
const std::function<nsresult(bool, DictionaryCacheEntry*)>& aCallback);
void FinishMatch();
@@ -279,6 +279,9 @@ class DictionaryOrigin : public nsICacheEntryMetaDataVisitor {
void FinishAddEntry(DictionaryCacheEntry* aEntry);
void DumpEntries();
void Clear();
+ void AssertEmpty() const {
+ MOZ_DIAGNOSTIC_ASSERT(mEntries.IsEmpty() && mPendingEntries.IsEmpty());
+ }
private:
virtual ~DictionaryOrigin() {}
@@ -330,9 +333,13 @@ class DictionaryCache final {
nsIURI* aURI, bool aNewEntry, DictionaryCacheEntry* aDictEntry);
static void RemoveDictionaryFor(const nsACString& aKey);
+ // remove the entire origin (should be empty!)
+ static void RemoveOriginFor(const nsACString& aKey);
// Remove a dictionary if it exists for the key given
void RemoveDictionary(const nsACString& aKey);
+ // Remove an origin for the key given
+ void RemoveOrigin(const nsACString& aKey);
nsresult RemoveEntry(nsIURI* aURI, const nsACString& aKey);
@@ -344,8 +351,8 @@ class DictionaryCache final {
// return an entry
void GetDictionaryFor(
- nsIURI* aURI, ExtContentPolicyType aType, bool& aAsync,
- nsHttpChannel* aChan, void (*aSuspend)(nsHttpChannel*),
+ nsIURI* aURI, ExtContentPolicyType aType, nsHttpChannel* aChan,
+ void (*aSuspend)(nsHttpChannel*),
const std::function<nsresult(bool, DictionaryCacheEntry*)>& aCallback);
size_t SizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf) const {
diff --git a/netwerk/protocol/http/nsHttpChannel.cpp b/netwerk/protocol/http/nsHttpChannel.cpp
@@ -634,16 +634,14 @@ nsresult nsHttpChannel::PrepareToConnect() {
// This may be async; the dictionary headers may need to fetch an origin
// dictionary cache entry from disk before adding the headers. We can
// continue with channel creation, and just block on this being done later
- bool async = false;
AUTO_PROFILER_FLOW_MARKER("nsHttpHandler::AddAcceptAndDictionaryHeaders",
NETWORK, Flow::FromPointer(this));
// AddAcceptAndDictionaryHeaders must call this->Suspend before kicking
// off the async operation that can result in calling the lambda (which
// will Resume), to avoid a race condition.
- bool aAsync;
nsresult rv = gHttpHandler->AddAcceptAndDictionaryHeaders(
mURI, mLoadInfo->GetExternalContentPolicyType(), &mRequestHead, IsHTTPS(),
- aAsync, this, nsHttpChannel::StaticSuspend,
+ this, nsHttpChannel::StaticSuspend,
[self = RefPtr(this)](bool aNeedsResume, DictionaryCacheEntry* aDict) {
self->mDictDecompress = aDict;
if (aNeedsResume) {
@@ -690,11 +688,6 @@ nsresult nsHttpChannel::PrepareToConnect() {
return true;
});
if (NS_FAILED(rv)) return rv;
- if (async) {
- // we'll continue later if GetDictionaryFor is still reading
- LOG_DICTIONARIES(("Suspending to get Dictionary headers"));
- Suspend();
- }
// notify "http-on-modify-request-before-cookies" observers
gHttpHandler->OnModifyRequestBeforeCookies(this);
diff --git a/netwerk/protocol/http/nsHttpHandler.cpp b/netwerk/protocol/http/nsHttpHandler.cpp
@@ -659,8 +659,7 @@ nsresult nsHttpHandler::InitConnectionMgr() {
// dictionary load.
nsresult nsHttpHandler::AddAcceptAndDictionaryHeaders(
nsIURI* aURI, ExtContentPolicyType aType, nsHttpRequestHead* aRequest,
- bool aSecure, bool& aAsync, nsHttpChannel* aChan,
- void (*aSuspend)(nsHttpChannel*),
+ bool aSecure, nsHttpChannel* aChan, void (*aSuspend)(nsHttpChannel*),
const std::function<bool(bool, DictionaryCacheEntry*)>& aCallback) {
LOG(("Adding Dictionary headers"));
auto guard = MakeScopeExit([&]() { (aCallback)(false, nullptr); });
@@ -674,7 +673,7 @@ nsresult nsHttpHandler::AddAcceptAndDictionaryHeaders(
// aCallback will now be owned by GetDictionaryFor
guard.release();
mDictionaryCache->GetDictionaryFor(
- aURI, aType, aAsync, aChan, aSuspend,
+ aURI, aType, aChan, aSuspend,
[self = RefPtr(this), aRequest, aCallback](
bool aNeedsResume, DictionaryCacheEntry* aDict) {
if (!aDict) {
@@ -726,11 +725,7 @@ nsresult nsHttpHandler::AddAcceptAndDictionaryHeaders(
}
return NS_OK;
});
- } else {
- aAsync = false;
}
- } else {
- aAsync = false;
}
// guard may call aCallback here
return rv;
diff --git a/netwerk/protocol/http/nsHttpHandler.h b/netwerk/protocol/http/nsHttpHandler.h
@@ -119,8 +119,7 @@ class nsHttpHandler final : public nsIHttpProtocolHandler,
[[nodiscard]] nsresult AddAcceptAndDictionaryHeaders(
nsIURI* aURI, ExtContentPolicyType aType, nsHttpRequestHead* aRequest,
- bool aSecure, bool& aAsync, nsHttpChannel* aChan,
- void (*aSuspend)(nsHttpChannel*),
+ bool aSecure, nsHttpChannel* aChan, void (*aSuspend)(nsHttpChannel*),
const std::function<bool(bool, DictionaryCacheEntry*)>& aCallback);
[[nodiscard]] nsresult AddStandardRequestHeaders(
nsHttpRequestHead*, nsIURI* aURI, bool aIsHTTPS,
