commit d7a5e533b313308f47a0244975a0b426b2a9fdaa
parent 29767dae3f0bbd8b2542ead07b935c286e480487
Author: Randell Jesup <rjesup@mozilla.com>
Date: Mon, 3 Nov 2025 18:26:15 +0000
Bug 1996560: Clear from dictionary cache on file removal, not metadata removal r=necko-reviewers,valentin,sunil
Introduces a new cache entry type to ease eviction scan. Makes dictionary
origin removal depend directly on the origin being empty.
Also fix an issue with RemoveOrigin()
Differential Revision: https://phabricator.services.mozilla.com/D270169
Diffstat:
8 files changed, 108 insertions(+), 53 deletions(-)
diff --git a/netwerk/cache2/CacheEntry.cpp b/netwerk/cache2/CacheEntry.cpp
@@ -1487,6 +1487,11 @@ nsresult CacheEntry::SetMetaDataElement(const char* aKey, const char* aValue) {
return mFile->SetElement(aKey, aValue);
}
+nsresult CacheEntry::GetIsEmpty(bool* aEmpty) {
+ *aEmpty = GetMetadataMemoryConsumption() == 0;
+ return NS_OK;
+}
+
nsresult CacheEntry::VisitMetaData(nsICacheEntryMetaDataVisitor* aVisitor) {
NS_ENSURE_SUCCESS(mFileStatus, NS_ERROR_NOT_AVAILABLE);
@@ -1770,6 +1775,15 @@ 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/CacheEntry.h b/netwerk/cache2/CacheEntry.h
@@ -90,6 +90,7 @@ class CacheEntry final : public nsIRunnable,
nsresult AsyncDoom(nsICacheEntryDoomCallback* aCallback);
nsresult GetMetaDataElement(const char* key, char** aRetval);
nsresult SetMetaDataElement(const char* key, const char* value);
+ nsresult GetIsEmpty(bool* aEmpty);
nsresult VisitMetaData(nsICacheEntryMetaDataVisitor* visitor);
nsresult MetaDataReady(void);
nsresult SetValid(void);
@@ -525,6 +526,9 @@ class CacheEntryHandle final : public nsICacheEntry {
NS_IMETHOD SetMetaDataElement(const char* key, const char* value) override {
return mEntry->SetMetaDataElement(key, value);
}
+ NS_IMETHOD GetIsEmpty(bool* empty) override {
+ return mEntry->GetIsEmpty(empty);
+ }
NS_IMETHOD VisitMetaData(nsICacheEntryMetaDataVisitor* visitor) override {
return mEntry->VisitMetaData(visitor);
}
diff --git a/netwerk/cache2/CacheIndex.cpp b/netwerk/cache2/CacheIndex.cpp
@@ -1349,8 +1349,14 @@ nsresult CacheIndex::GetEntryForEviction(EvictionSortedSnapshot& aSnapshot,
++skipped;
- if (evictMedia && CacheIndexEntry::GetContentType(rec) !=
- nsICacheEntry::CONTENT_TYPE_MEDIA) {
+ uint32_t type = CacheIndexEntry::GetContentType(rec);
+
+ if (evictMedia && type != nsICacheEntry::CONTENT_TYPE_MEDIA) {
+ continue;
+ }
+
+ if (type == nsICacheEntry::CONTENT_TYPE_DICTIONARY) {
+ // Let them be removed by becoming empty and removing themselves
continue;
}
@@ -3871,7 +3877,7 @@ void CacheIndex::DoTelemetryReport() {
static const nsLiteralCString
contentTypeNames[nsICacheEntry::CONTENT_TYPE_LAST] = {
"UNKNOWN"_ns, "OTHER"_ns, "JAVASCRIPT"_ns, "IMAGE"_ns,
- "MEDIA"_ns, "STYLESHEET"_ns, "WASM"_ns};
+ "MEDIA"_ns, "STYLESHEET"_ns, "WASM"_ns, "DICTIONARY"_ns};
for (uint32_t i = 0; i < nsICacheEntry::CONTENT_TYPE_LAST; ++i) {
if (mIndexStats.Size() > 0) {
diff --git a/netwerk/cache2/CacheStorageService.cpp b/netwerk/cache2/CacheStorageService.cpp
@@ -744,15 +744,6 @@ static bool RemoveExactEntry(CacheEntryTable* aEntries, nsACString const& aKey,
return false; // Already replaced...
}
- // Remove from DictionaryCache immediately, to ensure the removal is
- // synchronous
-
- if (aEntry->GetEnhanceID().EqualsLiteral("dict:")) {
- DictionaryCache::RemoveOriginFor(aEntry->GetURI());
- } else {
- DictionaryCache::RemoveDictionaryFor(aEntry->GetURI());
- }
-
LOG(("RemoveExactEntry [entry=%p removed]", aEntry));
aEntries->Remove(aKey);
return true;
diff --git a/netwerk/cache2/Dictionary.cpp b/netwerk/cache2/Dictionary.cpp
@@ -731,10 +731,16 @@ NS_IMETHODIMP DictionaryOriginReader::OnCacheEntryAvailable(
AUTO_PROFILER_FLOW_MARKER("DictionaryOriginReader::VisitMetaData", NETWORK,
Flow::FromPointer(this));
- mOrigin->SetCacheEntry(aCacheEntry);
- // There's no data in the cache entry, just metadata
- nsCOMPtr<nsICacheEntryMetaDataVisitor> metadata(mOrigin);
- aCacheEntry->VisitMetaData(metadata);
+ bool empty = false;
+ aCacheEntry->GetIsEmpty(&empty);
+ if (empty) {
+ // New cache entry, set type
+ mOrigin->SetCacheEntry(aCacheEntry);
+ } else {
+ // There's no data in the cache entry, just metadata
+ nsCOMPtr<nsICacheEntryMetaDataVisitor> metadata(mOrigin);
+ aCacheEntry->VisitMetaData(metadata);
+ }
// This list is the only thing keeping us alive
RefPtr<DictionaryOriginReader> safety(this);
@@ -910,6 +916,8 @@ void DictionaryCache::Clear() {
// static
void DictionaryCache::RemoveDictionaryFor(const nsACString& aKey) {
RefPtr<DictionaryCache> cache = GetInstance();
+ DICTIONARY_LOG(
+ ("Removing dictionary for %s", PromiseFlatCString(aKey).get()));
NS_DispatchToMainThread(NewRunnableMethod<const nsCString>(
"DictionaryCache::RemoveDictionaryFor", cache,
&DictionaryCache::RemoveDictionary, aKey));
@@ -918,7 +926,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 %80s", PromiseFlatCString(aKey).get()));
+ ("Removing dictionary for %s", PromiseFlatCString(aKey).get()));
nsCOMPtr<nsIURI> uri;
if (NS_FAILED(NS_NewURI(getter_AddRefs(uri), aKey))) {
@@ -936,16 +944,14 @@ void DictionaryCache::RemoveDictionary(const nsACString& aKey) {
void DictionaryCache::RemoveOriginFor(const nsACString& aKey) {
RefPtr<DictionaryCache> cache = GetInstance();
DICTIONARY_LOG(
- ("Removing dictionary for %80s", PromiseFlatCString(aKey).get()));
+ ("Removing dictionary origin %s", PromiseFlatCString(aKey).get()));
NS_DispatchToMainThread(NewRunnableMethod<const nsCString>(
- "DictionaryCache::RemoveDictionaryFor", cache,
- &DictionaryCache::RemoveDictionary, aKey));
+ "DictionaryCache::RemoveOriginFor", cache, &DictionaryCache::RemoveOrigin,
+ aKey));
}
-// Remove a dictionary if it exists for the key given
+// Remove a dictionary if it exists for the key given, if it's empty
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;
@@ -953,10 +959,15 @@ void DictionaryCache::RemoveOrigin(const nsACString& aKey) {
nsAutoCString prepath;
if (NS_SUCCEEDED(GetDictPath(uri, prepath))) {
if (auto origin = mDictionaryCache.Lookup(prepath)) {
- origin.Data()->AssertEmpty();
+ if (MOZ_UNLIKELY(origin.Data()->IsEmpty())) {
+ DICTIONARY_LOG(
+ ("Removing origin for %s", PromiseFlatCString(aKey).get()));
+ mDictionaryCache.Remove(prepath);
+ } else {
+ DICTIONARY_LOG(
+ ("Origin not empty: %s", PromiseFlatCString(aKey).get()));
+ }
}
-
- mDictionaryCache.Remove(prepath);
}
}
@@ -1128,6 +1139,7 @@ nsresult DictionaryOrigin::Write(DictionaryCacheEntry* aDictEntry) {
void DictionaryOrigin::SetCacheEntry(nsICacheEntry* aEntry) {
mEntry = aEntry;
+ mEntry->SetContentType(nsICacheEntry::CONTENT_TYPE_DICTIONARY);
if (mDeferredWrites) {
for (auto& entry : mEntries) {
if (NS_FAILED(Write(entry))) {
@@ -1245,12 +1257,13 @@ already_AddRefed<DictionaryCacheEntry> DictionaryOrigin::AddEntry(
nsresult DictionaryOrigin::RemoveEntry(const nsACString& aKey) {
DICTIONARY_LOG(
("DictionaryOrigin::RemoveEntry for %s", PromiseFlatCString(aKey).get()));
+ RefPtr<DictionaryCacheEntry> hold;
for (const auto& dict : mEntries) {
DICTIONARY_LOG(
(" Comparing to %s", PromiseFlatCString(dict->GetURI()).get()));
if (dict->GetURI().Equals(aKey)) {
// Ensure it doesn't disappear on us
- RefPtr<DictionaryCacheEntry> hold(dict);
+ hold = dict;
DICTIONARY_LOG(("Removing %p", dict.get()));
mEntries.RemoveElement(dict);
if (mEntry) {
@@ -1259,25 +1272,34 @@ nsresult DictionaryOrigin::RemoveEntry(const nsACString& aKey) {
// We don't have the cache entry yet. Defer the removal from
// the entry until we do
mPendingRemove.AppendElement(hold);
+ return NS_OK;
}
- return NS_OK;
+ break;
}
}
- DICTIONARY_LOG(("DictionaryOrigin::RemoveEntry (pending) for %s",
- PromiseFlatCString(aKey).get()));
- for (const auto& dict : mPendingEntries) {
- DICTIONARY_LOG(
- (" Comparing to %s", PromiseFlatCString(dict->GetURI()).get()));
- if (dict->GetURI().Equals(aKey)) {
- // Ensure it doesn't disappear on us
- RefPtr<DictionaryCacheEntry> hold(dict);
- DICTIONARY_LOG(("Removing %p", dict.get()));
- mPendingEntries.RemoveElement(dict);
- hold->RemoveEntry(mEntry);
- return NS_OK;
+ if (!hold) {
+ DICTIONARY_LOG(("DictionaryOrigin::RemoveEntry (pending) for %s",
+ PromiseFlatCString(aKey).get()));
+ for (const auto& dict : mPendingEntries) {
+ DICTIONARY_LOG(
+ (" Comparing to %s", PromiseFlatCString(dict->GetURI()).get()));
+ if (dict->GetURI().Equals(aKey)) {
+ // Ensure it doesn't disappear on us
+ RefPtr<DictionaryCacheEntry> hold(dict);
+ DICTIONARY_LOG(("Removing %p", dict.get()));
+ mPendingEntries.RemoveElement(dict);
+ break;
+ }
}
}
- return NS_ERROR_FAILURE;
+ // If this origin has no entries, remove it and doom the entry
+ if (IsEmpty()) {
+ if (mEntry) {
+ mEntry->AsyncDoom(nullptr);
+ }
+ gDictionaryCache->RemoveOrigin(mOrigin);
+ }
+ return hold ? NS_OK : NS_ERROR_FAILURE;
}
void DictionaryOrigin::FinishAddEntry(DictionaryCacheEntry* aEntry) {
@@ -1373,6 +1395,12 @@ nsresult DictionaryOrigin::OnMetaDataElement(const char* asciiKey,
DICTIONARY_LOG(("DictionaryOrigin::OnMetaDataElement %s %s",
asciiKey ? asciiKey : "", asciiValue));
+ // We set the content ID to CONTENT_TYPE_DICTIONARY, ensure that's correct
+ if (strcmp(asciiKey, "ctid") == 0) {
+ MOZ_ASSERT(strcmp(asciiValue, "7") == 0);
+ return NS_OK;
+ }
+ // All other keys should be URLs for dictionaries
// If we already have an entry for this key (pending or in the list),
// don't override it
for (auto& entry : mEntries) {
diff --git a/netwerk/cache2/Dictionary.h b/netwerk/cache2/Dictionary.h
@@ -279,8 +279,9 @@ class DictionaryOrigin : public nsICacheEntryMetaDataVisitor {
void FinishAddEntry(DictionaryCacheEntry* aEntry);
void DumpEntries();
void Clear();
- void AssertEmpty() const {
- MOZ_DIAGNOSTIC_ASSERT(mEntries.IsEmpty() && mPendingEntries.IsEmpty());
+ bool IsEmpty() const {
+ return mEntries.IsEmpty() && mPendingEntries.IsEmpty() &&
+ mPendingRemove.IsEmpty() && mWaitingCacheRead.IsEmpty();
}
private:
diff --git a/netwerk/cache2/nsICacheEntry.idl b/netwerk/cache2/nsICacheEntry.idl
@@ -29,12 +29,13 @@ interface nsICacheEntry : nsISupports
const unsigned long CONTENT_TYPE_MEDIA = 4;
const unsigned long CONTENT_TYPE_STYLESHEET = 5;
const unsigned long CONTENT_TYPE_WASM = 6;
+ const unsigned long CONTENT_TYPE_DICTIONARY = 7;
/**
* Content type that is used internally to check whether the value parsed
* from disk is within allowed limits. Don't pass CONTENT_TYPE_LAST to
* setContentType method.
*/
- const unsigned long CONTENT_TYPE_LAST = 7;
+ const unsigned long CONTENT_TYPE_LAST = 8;
/**
* Placeholder for the initial value of expiration time.
@@ -204,6 +205,11 @@ interface nsICacheEntry : nsISupports
void setMetaDataElement(in string key, in string value);
/**
+ * Get if the entry is empty (new)
+ */
+ readonly attribute boolean isEmpty;
+
+ /**
* Obtain the list of metadata keys this entry keeps.
*
* NOTE: The callback is invoked under the CacheFile's lock. It means
diff --git a/netwerk/protocol/about/nsAboutCacheEntry.cpp b/netwerk/protocol/about/nsAboutCacheEntry.cpp
@@ -491,15 +491,20 @@ nsAboutCacheEntry::Channel::OnMetaDataElement(char const* key,
":</th>\n"
" <td>");
if (mEnhanceId.EqualsLiteral("dict:")) {
- RefPtr<DictionaryCacheEntry> dict = new DictionaryCacheEntry("temp");
- dict->ParseMetadata(value);
- nsAppendEscapedHTML(
- nsPrintfCString(
- "Hash: %s\nPattern: %s\nId: %s\nMatch-Id: ", dict->GetHash().get(),
- dict->GetPattern().get(), dict->GetId().get()),
- *mBuffer);
- dict->AppendMatchDest(*mBuffer);
- mBuffer->AppendLiteral("\n");
+ // We set the content ID to CONTENT_TYPE_DICTIONARY, ensure that's correct
+ if (strcmp(key, "ctid") == 0) {
+ MOZ_ASSERT(strcmp(value, "7") == 0);
+ } else {
+ RefPtr<DictionaryCacheEntry> dict = new DictionaryCacheEntry("temp");
+ dict->ParseMetadata(value);
+ nsAppendEscapedHTML(
+ nsPrintfCString("Hash: %s\nPattern: %s\nId: %s\nMatch-Id: ",
+ dict->GetHash().get(), dict->GetPattern().get(),
+ dict->GetId().get()),
+ *mBuffer);
+ dict->AppendMatchDest(*mBuffer);
+ mBuffer->AppendLiteral("\n");
+ }
}
nsAppendEscapedHTML(nsDependentCString(value), *mBuffer);
mBuffer->AppendLiteral(
