commit 19e809294cafc7ef9643211d6b09a17f314a4b56
parent 4c4a17eaeb33a194c56c6fd33e42246aabd69b1f
Author: Randell Jesup <rjesup@mozilla.com>
Date: Tue, 7 Oct 2025 14:07:11 +0000
Bug 1978495: Make clear-site-data synchronous r=necko-reviewers,valentin
Also makes CacheStorageService::Clear() synchronous
Fixes a long-standing violation of the spec
Differential Revision: https://phabricator.services.mozilla.com/D267490
Diffstat:
10 files changed, 140 insertions(+), 62 deletions(-)
diff --git a/netwerk/cache2/CacheEntry.cpp b/netwerk/cache2/CacheEntry.cpp
@@ -533,7 +533,7 @@ already_AddRefed<CacheEntryHandle> CacheEntry::ReopenTruncated(
mLock.AssertCurrentThreadOwns();
// Hold callbacks invocation, AddStorageEntry would invoke from doom
- // prematurly
+ // prematurely
mPreventCallbacks = true;
RefPtr<CacheEntryHandle> handle;
diff --git a/netwerk/cache2/CacheFileContextEvictor.cpp b/netwerk/cache2/CacheFileContextEvictor.cpp
@@ -672,7 +672,7 @@ void CacheFileContextEvictor::EvictEntries() {
};
rv = CacheIndex::HasEntry(hash, &status, callback);
// This must never fail, since eviction (this code) happens only when the
- // index is up-to-date and thus the informatin is known.
+ // index is up-to-date and thus the information is known.
MOZ_ASSERT(NS_SUCCEEDED(rv));
if (pinned != mEntries[0]->mPinned) {
@@ -822,7 +822,7 @@ void CacheFileContextEvictor::EvictEntries() {
LOG(("CacheFileContextEvictor::EvictEntries - Removing entry."));
file->Remove(false);
- CacheIndex::RemoveEntry(&hash, metadata->GetKey());
+ CacheIndex::RemoveEntry(&hash, metadata->GetKey(), false);
}
MOZ_ASSERT_UNREACHABLE("We should never get here");
diff --git a/netwerk/cache2/CacheFileIOManager.cpp b/netwerk/cache2/CacheFileIOManager.cpp
@@ -2437,7 +2437,8 @@ nsresult CacheFileIOManager::DoomFile(CacheFileHandle* aHandle,
}
nsresult CacheFileIOManager::DoomFileInternal(
- CacheFileHandle* aHandle, PinningDoomRestriction aPinningDoomRestriction) {
+ CacheFileHandle* aHandle, PinningDoomRestriction aPinningDoomRestriction,
+ bool aClearDictionary) {
LOG(("CacheFileIOManager::DoomFileInternal() [handle=%p]", aHandle));
aHandle->Log();
@@ -2515,7 +2516,7 @@ nsresult CacheFileIOManager::DoomFileInternal(
if (!aHandle->IsSpecialFile()) {
// Ensure the string doesn't disappear with the handle
RefPtr<CacheFileHandle> handle(aHandle);
- CacheIndex::RemoveEntry(aHandle->Hash(), aHandle->Key());
+ CacheIndex::RemoveEntry(aHandle->Hash(), aHandle->Key(), aClearDictionary);
}
aHandle->mIsDoomed = true;
@@ -3415,6 +3416,14 @@ nsresult CacheFileIOManager::EvictByContext(
if (NS_WARN_IF(NS_FAILED(rv))) {
return rv;
}
+ // Clear the entries from the Index immediately, to comply with
+ // https://www.w3.org/TR/clear-site-data/#fetch-integration
+ // aBaseDomain isn't needed for Clear-Site-Data, but is for
+ // ClearBaseDomain. This can also make CacheStorageService::Clear() and
+ // ClearBaseDomain() be synchronous.
+ // Note that we will effectively hide the entries until the actual evict
+ // happens.
+ CacheIndex::EvictByContext(aOrigin, aBaseDomain);
return NS_OK;
}
@@ -3448,6 +3457,9 @@ nsresult CacheFileIOManager::EvictByContextInternal(
// This happens in xpcshell tests that use cache without profile. We need
// to notify observers in this case since the tests are waiting for it.
// Also notify for aPinned == true, those are interested as well.
+
+ // XXX This doesn't actually clear anything in this case (is there anything
+ // to clear?)
if (!aLoadContextInfo) {
RefPtr<EvictionNotifierRunnable> r = new EvictionNotifierRunnable();
NS_DispatchToMainThread(r);
@@ -3470,6 +3482,9 @@ nsresult CacheFileIOManager::EvictByContextInternal(
NS_ConvertUTF16toUTF8 baseDomain(aBaseDomain);
// Doom all active handles that matches the load context
+ // NOTE: Dictionaries have already been cleared synchronously,
+ // so there's no need to re-clear them (which might cause
+ // problems if they were re-created in to interim).
nsTArray<RefPtr<CacheFileHandle>> handles;
mHandles.GetActiveHandles(&handles);
@@ -3553,7 +3568,8 @@ nsresult CacheFileIOManager::EvictByContextInternal(
// doom decision will be deferred until pinning status is determined.
rv = DoomFileInternal(handle,
aPinned ? CacheFileIOManager::DOOM_WHEN_PINNED
- : CacheFileIOManager::DOOM_WHEN_NON_PINNED);
+ : CacheFileIOManager::DOOM_WHEN_NON_PINNED,
+ false);
if (NS_WARN_IF(NS_FAILED(rv))) {
LOG(
("CacheFileIOManager::EvictByContextInternal() - Cannot doom handle"
@@ -3587,7 +3603,7 @@ nsresult CacheFileIOManager::CacheIndexStateChanged() {
// non-null here.
MOZ_ASSERT(gInstance);
- // We have to re-distatch even if we are on IO thread to prevent reentering
+ // We have to re-dispatch even if we are on IO thread to prevent reentering
// the lock in CacheIndex
nsCOMPtr<nsIRunnable> ev = NewRunnableMethod(
"net::CacheFileIOManager::CacheIndexStateChangedInternal",
diff --git a/netwerk/cache2/CacheFileIOManager.h b/netwerk/cache2/CacheFileIOManager.h
@@ -414,7 +414,8 @@ class CacheFileIOManager final : public nsITimerCallback, public nsINamed {
bool aTruncate);
nsresult DoomFileInternal(
CacheFileHandle* aHandle,
- PinningDoomRestriction aPinningDoomRestriction = NO_RESTRICTION);
+ PinningDoomRestriction aPinningDoomRestriction = NO_RESTRICTION,
+ bool aClearDirectory = true);
nsresult DoomFileByKeyInternal(const SHA1Sum::Hash* aHash);
nsresult MaybeReleaseNSPRHandleInternal(CacheFileHandle* aHandle,
bool aIgnoreShutdownLag = false);
diff --git a/netwerk/cache2/CacheIndex.cpp b/netwerk/cache2/CacheIndex.cpp
@@ -841,14 +841,28 @@ nsresult CacheIndex::InitEntry(const SHA1Sum::Hash* aHash,
// static
nsresult CacheIndex::RemoveEntry(const SHA1Sum::Hash* aHash,
- const nsACString& aKey) {
- LOG(("CacheIndex::RemoveEntry() [hash=%08x%08x%08x%08x%08x] key=%s",
- LOGSHA1(aHash), PromiseFlatCString(aKey).get()));
+ const nsACString& aKey,
+ bool aClearDictionary) {
+ LOG(
+ ("CacheIndex::RemoveEntry() [hash=%08x%08x%08x%08x%08x] key=%s "
+ "clear_dictionary=%d",
+ LOGSHA1(aHash), PromiseFlatCString(aKey).get(), aClearDictionary));
MOZ_ASSERT(CacheFileIOManager::IsOnIOThread());
- // Remove the dictionary even if we later error out - async
- DictionaryCache::RemoveDictionaryFor(aKey);
+ // Remove any dictionary associated with this entry even if we later
+ // error out - async since removal happens on MainThread.
+
+ // TODO XXX There may be a hole here where a dictionary entry can get
+ // referenced for a request before RemoveDictionaryFor can run, but after
+ // the entry is removed here.
+
+ // Note: we don't want to (re)clear dictionaries when the
+ // CacheFileContextEvictor purges entries; they've already been cleared
+ // via CacheIndex::EvictByContext synchronously
+ if (aClearDictionary) {
+ DictionaryCache::RemoveDictionaryFor(aKey);
+ }
StaticMutexAutoLock lock(sLock);
@@ -1083,6 +1097,32 @@ nsresult CacheIndex::UpdateEntry(const SHA1Sum::Hash* aHash,
return NS_OK;
}
+// Clear the entries from the Index immediately, to comply with
+// https://www.w3.org/TR/clear-site-data/#fetch-integration
+// Note that we will effectively hide the entries until the actual evict
+// happens.
+
+// aOrigin == "" means clear all unless aBaseDomain is set to something
+// static
+void CacheIndex::EvictByContext(const nsAString& aOrigin,
+ const nsAString& aBaseDomain) {
+ StaticMutexAutoLock lock(sLock);
+
+ RefPtr<CacheIndex> index = gInstance;
+
+ // Store in hashset that this origin has been evicted; we'll remove it
+ // when CacheFileIOManager::EvictByContextInternal() finishes.
+ // Not valid to set both aOrigin and aBaseDomain
+ if (!aOrigin.IsEmpty() && aBaseDomain.IsEmpty()) {
+ // likely CacheStorageService::ClearByPrincipal
+ nsCOMPtr<nsIURI> uri;
+ if (NS_SUCCEEDED(NS_NewURI(getter_AddRefs(uri), aOrigin))) {
+ // Remove the dictionary entries for this origin immediately
+ DictionaryCache::RemoveDictionariesForOrigin(uri);
+ }
+ }
+}
+
// static
nsresult CacheIndex::RemoveAll() {
LOG(("CacheIndex::RemoveAll()"));
diff --git a/netwerk/cache2/CacheIndex.h b/netwerk/cache2/CacheIndex.h
@@ -748,7 +748,8 @@ class CacheIndex final : public CacheFileIOListener, public nsIRunnable {
// Remove entry from index. The entry should be present in index.
static nsresult RemoveEntry(const SHA1Sum::Hash* aHash,
- const nsACString& aKey);
+ const nsACString& aKey,
+ bool aClearDictionary = true);
// Update some information in entry. The entry MUST be present in index and
// MUST be initialized. Call to AddEntry() or EnsureEntryExists() and to
@@ -762,6 +763,11 @@ class CacheIndex final : public CacheFileIOListener, public nsIRunnable {
const uint8_t* aContentType,
const uint32_t* aSize);
+ // Mark entries so we won't find them. Used to implement synchronous
+ // clearing for Clear-Site-Data: cache for Compression Dictionaries
+ static void EvictByContext(const nsAString& aOrigin,
+ const nsAString& aBaseDomain);
+
// Remove all entries from the index. Called when clearing the whole cache.
static nsresult RemoveAll();
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;
@@ -899,7 +903,7 @@ nsresult CacheStorageService::ClearOriginInternal(
NS_IMETHODIMP CacheStorageService::ClearOriginDictionary(nsIURI* aURI) {
LOG(("CacheStorageService::ClearOriginDictionary"));
// Note: due to cookie samesite rules, we need to clean for all ports
- DictionaryCache::RemoveDictionaries(aURI);
+ DictionaryCache::RemoveDictionariesForOrigin(aURI);
return NS_OK;
}
diff --git a/netwerk/cache2/Dictionary.cpp b/netwerk/cache2/Dictionary.cpp
@@ -98,6 +98,13 @@ DictionaryCacheEntry::DictionaryCacheEntry(const char* aKey) {
DICTIONARY_LOG(("Created DictionaryCacheEntry %p, uri=%s", this, aKey));
}
+DictionaryCacheEntry::~DictionaryCacheEntry() {
+ MOZ_ASSERT(mUsers == 0);
+ DICTIONARY_LOG(
+ ("Destroyed DictionaryCacheEntry %p, uri=%s, pattern=%s, id=%s", this,
+ mURI.get(), mPattern.get(), mId.get()));
+}
+
DictionaryCacheEntry::DictionaryCacheEntry(const nsACString& aURI,
const nsACString& aPattern,
nsTArray<nsCString>& aMatchDest,
@@ -896,26 +903,17 @@ void DictionaryCache::RemoveDictionaryFor(const nsACString& aKey) {
// Remove a dictionary if it exists for the key given
void DictionaryCache::RemoveDictionary(const nsACString& aKey) {
- nsCString enhance;
- nsCString urlstring;
- nsCOMPtr<nsILoadContextInfo> info =
- CacheFileUtils::ParseKey(aKey, &enhance, &urlstring);
- MOZ_ASSERT(info);
- if (!info) {
- DICTIONARY_LOG(("DictionaryCache::RemoveDictionary() - Cannot parse key!"));
- return;
- }
- DICTIONARY_LOG(("Removing dictionary for %s, enhance %s", urlstring.get(),
- enhance.get()));
+ DICTIONARY_LOG(
+ ("Removing dictionary for %80s", PromiseFlatCString(aKey).get()));
nsCOMPtr<nsIURI> uri;
- if (NS_FAILED(NS_NewURI(getter_AddRefs(uri), urlstring))) {
+ 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()->RemoveEntry(urlstring);
+ origin.Data()->RemoveEntry(aKey);
}
}
}
@@ -923,37 +921,48 @@ void DictionaryCache::RemoveDictionary(const nsACString& aKey) {
// 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
-void DictionaryCache::RemoveDictionaries(nsIURI* aURI) {
+void DictionaryCache::RemoveDictionariesForOrigin(nsIURI* aURI) {
+ // There's no PrePathNoPort()
+ nsAutoCString temp;
+ aURI->GetScheme(temp);
+ nsCString origin(temp);
+ aURI->GetUserPass(temp);
+ origin += "://"_ns + temp;
+ aURI->GetHost(temp);
+ origin += temp;
+
+ DICTIONARY_LOG(("Removing all dictionaries for origin of %s (%zu)",
+ PromiseFlatCString(origin).get(), origin.Length()));
RefPtr<DictionaryCache> cache = GetInstance();
- nsDependentCSubstring spec; // aka https://site/ with no port
- if (NS_FAILED(aURI->GetSpec(spec))) {
- return;
- }
-
- DICTIONARY_LOG(("Removing all dictionaries for %s (%zu)",
- PromiseFlatCString(spec).get(), spec.Length()));
- RefPtr<DictionaryOrigin> origin;
- // We can't just use Remove here; the ClearSiteData service strips the port.
- // We need to clear all that match the host with any port or none
- cache->mDictionaryCache.RemoveIf([&spec](auto& entry) {
- // We need to drop any port. Assuming they're the same up to the / or : in
- // mOrigin, we want to limit the host there. Verify that:
+ // We can't just use Remove here; the ClearSiteData service strips the
+ // port. In that case, We need to clear all that match the host with any
+ // port or none.
+ cache->mDictionaryCache.RemoveIf([&origin](auto& entry) {
+ // We need to drop any port from entry (and origin). Assuming they're
+ // the same up to the / or : in mOrigin, we want to limit the host
+ // there. We also know that entry is https://.
+ // Verify that:
// a) they're equal to that point
// b) that the next character of mOrigin is '/' or ':', which avoids
- // issues like matching https://foo.bar/ to (mOrigin)
+ // issues like matching https://foo.bar to (mOrigin)
// https://foo.barsoom.com:666/
- if (entry.Data()->mOrigin.Length() > spec.Length() &&
- (entry.Data()->mOrigin[spec.Length() - 1] == '/' || // no port
- entry.Data()->mOrigin[spec.Length() - 1] == ':')) { // port
+ DICTIONARY_LOG(
+ ("Possibly removing dictionary origin for %s (vs %s), %zu vs %zu",
+ entry.Data()->mOrigin.get(), PromiseFlatCString(origin).get(),
+ entry.Data()->mOrigin.Length(), origin.Length()));
+ if (entry.Data()->mOrigin.Length() > origin.Length() &&
+ (entry.Data()->mOrigin[origin.Length()] == '/' || // no port
+ entry.Data()->mOrigin[origin.Length()] == ':')) { // port
// no strncmp() for nsCStrings...
nsDependentCSubstring host =
Substring(entry.Data()->mOrigin, 0,
- spec.Length() - 1); // not including '/' or ':'
- nsDependentCSubstring temp =
- Substring(spec, 0, spec.Length() - 1); // Drop '/'
- if (temp.Equals(host)) {
+ origin.Length()); // not including '/' or ':'
+ DICTIONARY_LOG(("Compare %s vs %s", entry.Data()->mOrigin.get(),
+ PromiseFlatCString(host).get()));
+ if (origin.Equals(host)) {
DICTIONARY_LOG(
- ("Removing dictionary for %s", entry.Data()->mOrigin.get()));
+ ("RemoveDictionaries: Removing dictionary origin %p for %s",
+ entry.Data().get(), entry.Data()->mOrigin.get()));
entry.Data()->Clear();
return true;
}
@@ -1190,6 +1199,8 @@ nsresult DictionaryOrigin::RemoveEntry(const nsACString& aKey) {
DICTIONARY_LOG(
("DictionaryOrigin::RemoveEntry for %s", PromiseFlatCString(aKey).get()));
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);
@@ -1208,6 +1219,8 @@ nsresult DictionaryOrigin::RemoveEntry(const nsACString& aKey) {
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);
@@ -1271,7 +1284,10 @@ void DictionaryOrigin::DumpEntries() {
void DictionaryOrigin::Clear() {
mEntries.Clear();
mPendingEntries.Clear();
- mEntry->AsyncDoom(nullptr);
+ // We may be under a lock; doom this asynchronously
+ 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
@@ -51,7 +51,7 @@ class DictionaryCacheEntry final : public nsICacheEntryOpenCallback,
friend class DictionaryOrigin;
private:
- ~DictionaryCacheEntry() { MOZ_ASSERT(mUsers == 0); }
+ ~DictionaryCacheEntry();
public:
NS_DECL_THREADSAFE_ISUPPORTS
@@ -123,14 +123,6 @@ class DictionaryCacheEntry final : public nsICacheEntryOpenCallback,
const nsACString& GetURI() const { return mURI; }
- void Clear() {
- MOZ_ASSERT(NS_IsMainThread());
- mHash.Truncate(0);
- mDictionaryData.clear();
- mDictionaryDataComplete = false;
- MOZ_ASSERT(mWaitingPrefetch.IsEmpty());
- }
-
const Vector<uint8_t>& GetDictionary() const { return mDictionaryData; }
// Accumulate a hash while saving a file being received to the cache
@@ -340,7 +332,7 @@ class DictionaryCache final {
nsresult RemoveEntry(nsIURI* aURI, const nsACString& aKey);
- static void RemoveDictionaries(nsIURI* aURI);
+ static void RemoveDictionariesForOrigin(nsIURI* aURI);
static void RemoveAllDictionaries();
// Clears all ports at host
diff --git a/netwerk/protocol/http/nsHttpChannel.cpp b/netwerk/protocol/http/nsHttpChannel.cpp
@@ -659,6 +659,9 @@ nsresult nsHttpChannel::PrepareToConnect() {
PROFILER_MARKER("Dictionary Prefetch", NETWORK,
MarkerTiming::IntervalStart(), FlowMarker,
Flow::FromPointer(self));
+ // XXX if this fails, retry the connection (we assume that the
+ // DictionaryCacheEntry has been removed). Failure should be only in
+ // weird cases like no storage service.
return NS_SUCCEEDED(self->mDictDecompress->Prefetch(
GetLoadContextInfo(self), self->mShouldSuspendForDictionary,
[self]() {
