commit 0b834e0ecd40295f11cf8218ff144035827f9866
parent ba6106a65317ab93cae85888ec656a36219cf47a
Author: Cristian Tuns <ctuns@mozilla.com>
Date: Wed, 29 Oct 2025 17:38:06 -0400
Revert "Bug 1996848, Bug 1996560, Bug 1996305, Bug 1996298 - Disable AssertEmpty() from release builds for crashing frequently. r=gstoll" for causing bug 1996848, 1996764 and increase volume of bug 1994773
This reverts commit c5da1f9e6dc21588b13579943235c25c91b2a187.
Revert "Bug 1996560: Clear from dictionary cache on file removal, not metadata removal r=necko-reviewers,valentin"
This reverts commit 17d6947e95f4de6de8848f118a4691a4924f9091.
Revert "Bug 1996305: Ensure Suspend() is called before starting async dictionary origin reads r=necko-reviewers,valentin"
This reverts commit 10a3b590155247b10b3c0bf0c10f7d8e0aa734ec.
Revert "Bug 1996298: PurgeByFrecency was over-purging due to holding references r=necko-reviewers,jstutte,valentin" for causing bug 1996848, 1996764 and increase volume of bug 1994773
This reverts commit 3d93084ce35156797daf624969e2eb42445773f8.
Diffstat:
7 files changed, 42 insertions(+), 82 deletions(-)
diff --git a/netwerk/cache2/CacheEntry.cpp b/netwerk/cache2/CacheEntry.cpp
@@ -1770,15 +1770,6 @@ void CacheEntry::DoomAlreadyRemoved() {
mIsDoomed = true;
- // Remove from DictionaryCache immediately, to ensure the removal is
- // synchronous
- LOG(("DoomAlreadyRemoved [entry=%p removed]", this));
- if (mEnhanceID.EqualsLiteral("dict:")) {
- DictionaryCache::RemoveOriginFor(mURI);
- } else {
- DictionaryCache::RemoveDictionaryFor(mURI);
- }
-
// Pretend pinning is know. This entry is now doomed for good, so don't
// bother with defering doom because of unknown pinning state any more.
mPinningKnown = true;
diff --git a/netwerk/cache2/CacheStorageService.cpp b/netwerk/cache2/CacheStorageService.cpp
@@ -744,6 +744,10 @@ static bool RemoveExactEntry(CacheEntryTable* aEntries, nsACString const& aKey,
return false; // Already replaced...
}
+ // Remove from DictionaryCache immediately, to ensure the removal is
+ // synchronous
+ DictionaryCache::RemoveDictionaryFor(aEntry->GetURI());
+
LOG(("RemoveExactEntry [entry=%p removed]", aEntry));
aEntries->Remove(aKey);
return true;
@@ -1506,8 +1510,8 @@ Result<size_t, nsresult> CacheStorageService::MemoryPool::PurgeByFrecency(
} else {
if (entry->GetEnhanceID().EqualsLiteral("dict:")) {
LOG(
- ("*** Ignored Entry is a dictionary origin, metadata size %d, "
- "referenced %d, Frecency %f",
+ ("*** Entry is a dictionary origin, metadata size %d, referenced "
+ "%d, Frecency %f",
entry->GetMetadataMemoryConsumption(), entry->IsReferenced(),
entry->GetFrecency()));
}
@@ -1526,9 +1530,7 @@ Result<size_t, nsresult> CacheStorageService::MemoryPool::PurgeByFrecency(
break;
}
- // Ensure it's deleted immediately if purged so we can record the
- // mMemorySize savings
- RefPtr<CacheEntry> entry = std::move(checkPurge.mEntry);
+ RefPtr<CacheEntry> entry = checkPurge.mEntry;
if (entry->Purge(CacheEntry::PURGE_WHOLE)) {
numPurged++;
@@ -1542,10 +1544,7 @@ Result<size_t, nsresult> CacheStorageService::MemoryPool::PurgeByFrecency(
}
}
- LOG(
- ("MemoryPool::PurgeByFrecency done, purged %zu - mMemorySize %u, "
- "memoryLimit %u",
- numPurged, (uint32_t)mMemorySize, memoryLimit));
+ LOG(("MemoryPool::PurgeByFrecency done"));
return numPurged;
}
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(
- bool aCreate, DictionaryOrigin* aOrigin, nsACString& aKey, nsIURI* aURI,
+ 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,
- aCreate
+ aOrigin
? nsICacheStorage::OPEN_NORMALLY |
nsICacheStorage::CHECK_MULTITHREADED
: nsICacheStorage::OPEN_READONLY | nsICacheStorage::OPEN_SECRETLY |
@@ -858,13 +858,12 @@ 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(
- true, origin, prepath, aURI, ExtContentPolicy::TYPE_OTHER, this,
+ origin, prepath, aURI, ExtContentPolicy::TYPE_OTHER, this,
[entry = RefPtr(aDictEntry)](
bool, DictionaryCacheEntry* aDict) { // XXX avoid so many lambdas
// which cause allocations
@@ -918,7 +917,7 @@ void DictionaryCache::RemoveDictionaryFor(const nsACString& aKey) {
// Remove a dictionary if it exists for the key given
void DictionaryCache::RemoveDictionary(const nsACString& aKey) {
DICTIONARY_LOG(
- ("Removing dictionary for %s", PromiseFlatCString(aKey).get()));
+ ("Removing dictionary for %80s", PromiseFlatCString(aKey).get()));
nsCOMPtr<nsIURI> uri;
if (NS_FAILED(NS_NewURI(getter_AddRefs(uri), aKey))) {
@@ -932,34 +931,6 @@ void DictionaryCache::RemoveDictionary(const nsACString& aKey) {
}
}
-// static
-void DictionaryCache::RemoveOriginFor(const nsACString& aKey) {
- RefPtr<DictionaryCache> cache = GetInstance();
- DICTIONARY_LOG(
- ("Removing dictionary origin %s", PromiseFlatCString(aKey).get()));
- NS_DispatchToMainThread(NewRunnableMethod<const nsCString>(
- "DictionaryCache::RemoveOriginFor", cache, &DictionaryCache::RemoveOrigin,
- 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
@@ -1031,9 +1002,10 @@ 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, nsHttpChannel* aChan,
- void (*aSuspend)(nsHttpChannel*),
+ nsIURI* aURI, ExtContentPolicyType aType, bool& aAsync,
+ 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
@@ -1063,10 +1035,9 @@ void DictionaryCache::GetDictionaryFor(
RefPtr<DictionaryOriginReader> reader = new DictionaryOriginReader();
// Must do this before calling start, which can run the callbacks and call
// Resume
- DICTIONARY_LOG(("Suspending to get Dictionary headers"));
+ aAsync = true;
aSuspend(aChan);
- reader->Start(false, existing.Data(), prepath, aURI, aType, this,
- aCallback);
+ reader->Start(existing.Data(), prepath, aURI, aType, this, aCallback);
}
return;
}
@@ -1102,9 +1073,8 @@ 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
- DICTIONARY_LOG(("Suspending to get Dictionary headers"));
- aSuspend(aChan);
- reader->Start(false, origin, prepath, aURI, aType, this, aCallback);
+ reader->Start(origin, prepath, aURI, aType, this, aCallback);
+ aAsync = true;
return;
}
// No dictionaries for origin
@@ -1331,16 +1301,10 @@ void DictionaryOrigin::DumpEntries() {
void DictionaryOrigin::Clear() {
mEntries.Clear();
mPendingEntries.Clear();
- mPendingRemove.Clear();
// We may be under a lock; doom this asynchronously
- if (mEntry) {
- // This will attempt to delete the DictionaryOrigin, but we'll do
- // that more directly
- NS_DispatchBackgroundTask(NS_NewRunnableFunction(
- "DictionaryOrigin::Clear",
- [entry = mEntry]() { entry->AsyncDoom(nullptr); }));
- }
- DictionaryCache::RemoveOriginFor(mOrigin); // async
+ NS_DispatchBackgroundTask(NS_NewRunnableFunction(
+ "DictionaryOrigin::Clear",
+ [entry = mEntry]() { entry->AsyncDoom(nullptr); }));
}
// caller will throw this into a RefPtr
diff --git a/netwerk/cache2/Dictionary.h b/netwerk/cache2/Dictionary.h
@@ -237,7 +237,7 @@ class DictionaryOriginReader final : public nsICacheEntryOpenCallback,
DictionaryOriginReader() {}
void Start(
- bool aCreate, DictionaryOrigin* aOrigin, nsACString& aKey, nsIURI* aURI,
+ DictionaryOrigin* aOrigin, nsACString& aKey, nsIURI* aURI,
ExtContentPolicyType aType, DictionaryCache* aCache,
const std::function<nsresult(bool, DictionaryCacheEntry*)>& aCallback);
void FinishMatch();
@@ -279,9 +279,6 @@ class DictionaryOrigin : public nsICacheEntryMetaDataVisitor {
void FinishAddEntry(DictionaryCacheEntry* aEntry);
void DumpEntries();
void Clear();
- void AssertEmpty() const {
- MOZ_ASSERT(mEntries.IsEmpty() && mPendingEntries.IsEmpty());
- }
private:
virtual ~DictionaryOrigin() {}
@@ -333,13 +330,9 @@ 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);
@@ -351,8 +344,8 @@ class DictionaryCache final {
// return an entry
void GetDictionaryFor(
- nsIURI* aURI, ExtContentPolicyType aType, nsHttpChannel* aChan,
- void (*aSuspend)(nsHttpChannel*),
+ nsIURI* aURI, ExtContentPolicyType aType, bool& aAsync,
+ 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
@@ -633,14 +633,16 @@ 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(),
- this, nsHttpChannel::StaticSuspend,
+ aAsync, this, nsHttpChannel::StaticSuspend,
[self = RefPtr(this)](bool aNeedsResume, DictionaryCacheEntry* aDict) {
self->mDictDecompress = aDict;
if (aNeedsResume) {
@@ -687,6 +689,11 @@ 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
@@ -658,7 +658,8 @@ nsresult nsHttpHandler::InitConnectionMgr() {
// dictionary load.
nsresult nsHttpHandler::AddAcceptAndDictionaryHeaders(
nsIURI* aURI, ExtContentPolicyType aType, nsHttpRequestHead* aRequest,
- bool aSecure, nsHttpChannel* aChan, void (*aSuspend)(nsHttpChannel*),
+ bool aSecure, bool& aAsync, nsHttpChannel* aChan,
+ void (*aSuspend)(nsHttpChannel*),
const std::function<bool(bool, DictionaryCacheEntry*)>& aCallback) {
LOG(("Adding Dictionary headers"));
auto guard = MakeScopeExit([&]() { (aCallback)(false, nullptr); });
@@ -672,7 +673,7 @@ nsresult nsHttpHandler::AddAcceptAndDictionaryHeaders(
// aCallback will now be owned by GetDictionaryFor
guard.release();
mDictionaryCache->GetDictionaryFor(
- aURI, aType, aChan, aSuspend,
+ aURI, aType, aAsync, aChan, aSuspend,
[self = RefPtr(this), aRequest, aCallback](
bool aNeedsResume, DictionaryCacheEntry* aDict) {
if (!aDict) {
@@ -724,7 +725,11 @@ 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,7 +119,8 @@ class nsHttpHandler final : public nsIHttpProtocolHandler,
[[nodiscard]] nsresult AddAcceptAndDictionaryHeaders(
nsIURI* aURI, ExtContentPolicyType aType, nsHttpRequestHead* aRequest,
- bool aSecure, nsHttpChannel* aChan, void (*aSuspend)(nsHttpChannel*),
+ bool aSecure, bool& aAsync, nsHttpChannel* aChan,
+ void (*aSuspend)(nsHttpChannel*),
const std::function<bool(bool, DictionaryCacheEntry*)>& aCallback);
[[nodiscard]] nsresult AddStandardRequestHeaders(
nsHttpRequestHead*, nsIURI* aURI, bool aIsHTTPS,
