commit 4c7312620865aee9739abccef2c90f8dab6d8cd6
parent d7cb9a5e0b91481697d6f1ebff33aba1b8e0a916
Author: Randell Jesup <rjesup@mozilla.com>
Date: Tue, 7 Oct 2025 14:07:10 +0000
Bug 1917970: Update handling of removing encodings for CompressionDictionaries r=necko-reviewers,valentin,kershaw
We need to ensure that we remove all entries from the Content-Encoding if
it's a dictionary or if it's dictionary-encoded
Differential Revision: https://phabricator.services.mozilla.com/D265926
Diffstat:
4 files changed, 65 insertions(+), 89 deletions(-)
diff --git a/netwerk/protocol/http/HttpBaseChannel.cpp b/netwerk/protocol/http/HttpBaseChannel.cpp
@@ -1518,10 +1518,9 @@ HttpBaseChannel::DoApplyContentConversions(nsIStreamListener* aNextListener,
// being a stack with the last converter created being the first one
// to accept the raw network data.
- nsAutoCString newEncoding;
char* cePtr = contentEncoding.BeginWriting();
uint32_t count = 0;
- bool removed_encoding = false;
+ bool removeEncodings = false;
while (char* val = nsCRT::strtok(cePtr, HTTP_LWS ",", &cePtr)) {
if (++count > 16) {
// For compatibility with old code, we will just carry on without
@@ -1562,30 +1561,34 @@ HttpBaseChannel::DoApplyContentConversions(nsIStreamListener* aNextListener,
glean::http::content_encoding.AccumulateSingleSample(mode);
}
if (from.EqualsLiteral("dcb") || from.EqualsLiteral("dcz")) {
- removed_encoding = true;
+ MOZ_ASSERT(XRE_IsParentProcess());
+ removeEncodings = true;
}
nextListener = converter;
} else {
- if (val) LOG(("Unknown content encoding '%s', ignoring\n", val));
- // leave it in content-encoding if we didn't decompress it, though if
- // there are following decoders, this will just be wrong, and we
- // should error out. Or maybe error out on any unknown encoding
- newEncoding += val;
+ if (val) {
+ LOG(("Unknown content encoding '%s'\n", val));
+ }
+ // we *should* return NS_ERROR_UNEXPECTED here, but that will break sites
+ // that use things like content-encoding: x-gzip, x-gzip (or any other
+ // weird encoding)
}
}
// dcb and dcz encodings are removed when it's decompressed (always in
// the parent process). However, in theory you could have
// Content-Encoding: dcb,gzip
- // in which case we should pass it down to the content process as
- // Content-Encoding: gzip
- // This of course would be silly, but supported by the spec
- if (removed_encoding) {
- LOG(("Changing Content-Encoding from %s to %s", contentEncoding.get(),
- newEncoding.get()));
+ // in which case we could pass it down to the content process as
+ // Content-Encoding: gzip. We won't do that; we'll remove all compressors
+ // if we need to remove any.
+ // This double compression of course is silly, but supported by the spec.
+ if (removeEncodings) {
+ // if we have dcb or dcz, all content-encodings in the header should
+ // be removed as we're decompressing before the tee in the parent
+ // process
+ LOG(("Changing Content-Encoding from '%s' to ''", contentEncoding.get()));
// Can't use SetHeader; we need to overwrite the current value
- rv =
- mResponseHead->SetHeaderOverride(nsHttp::Content_Encoding, newEncoding);
+ rv = mResponseHead->SetHeaderOverride(nsHttp::Content_Encoding, ""_ns);
}
*aNewNextListener = do_AddRef(nextListener).take();
return NS_OK;
diff --git a/netwerk/protocol/http/nsHttpChannel.cpp b/netwerk/protocol/http/nsHttpChannel.cpp
@@ -6002,23 +6002,7 @@ nsresult DoAddCacheEntryHeaders(nsHttpChannel* self, nsICacheEntry* entry,
rv = StoreAuthorizationMetaData(entry, requestHead);
if (NS_FAILED(rv)) return rv;
- rv = self->ProcessVaryCacheEntryHeaders(entry, nullptr);
- if (NS_FAILED(rv)) return rv;
-
- // Store the received HTTP head with the cache entry as an element of
- // the meta data.
- nsAutoCString head;
- responseHead->Flatten(head, true);
- rv = entry->SetMetaDataElement("response-head", head.get());
- if (NS_FAILED(rv)) return rv;
- head.Truncate();
- responseHead->FlattenNetworkOriginalHeaders(head);
- rv = entry->SetMetaDataElement("original-response-headers", head.get());
- if (NS_FAILED(rv)) return rv;
-
- // Indicate we have successfully finished setting metadata on the cache entry.
- rv = entry->MetaDataReady();
-
+ rv = self->UpdateCacheEntryHeaders(entry, nullptr);
return rv;
}
@@ -6028,8 +6012,8 @@ nsresult nsHttpChannel::AddCacheEntryHeaders(nsICacheEntry* entry,
mSecurityInfo, aModified);
}
-nsresult nsHttpChannel::ProcessVaryCacheEntryHeaders(nsICacheEntry* entry,
- const nsHttpAtom* aAtom) {
+nsresult nsHttpChannel::UpdateCacheEntryHeaders(nsICacheEntry* entry,
+ const nsHttpAtom* aAtom) {
nsresult rv = NS_OK;
// Iterate over the headers listed in the Vary response header, and
@@ -6094,18 +6078,23 @@ nsresult nsHttpChannel::ProcessVaryCacheEntryHeaders(nsICacheEntry* entry,
}
}
}
- return rv;
-}
-
-nsresult nsHttpChannel::ModifiedCacheEntryHeaders(nsICacheEntry* entry,
- const nsHttpAtom& aAtom) {
- nsresult rv = ProcessVaryCacheEntryHeaders(entry, &aAtom);
- if (NS_SUCCEEDED(rv)) {
- // Indicate we have successfully finished setting metadata on the cache
- // entry.
- rv = entry->MetaDataReady();
+ if (NS_FAILED(rv)) {
+ return rv;
}
- return rv;
+ // Store the received HTTP head with the cache entry as an element of
+ // the meta data.
+ nsAutoCString head;
+ mResponseHead->Flatten(head, true);
+ rv = entry->SetMetaDataElement("response-head", head.get());
+ if (NS_FAILED(rv)) return rv;
+ head.Truncate();
+ mResponseHead->FlattenNetworkOriginalHeaders(head);
+ rv = entry->SetMetaDataElement("original-response-headers", head.get());
+ if (NS_FAILED(rv)) return rv;
+
+ // Indicate we have successfully finished setting metadata on the cache
+ // entry.
+ return entry->MetaDataReady();
}
bool nsHttpChannel::ParseDictionary(nsICacheEntry* aEntry,
@@ -6219,7 +6208,10 @@ nsresult nsHttpChannel::InstallCacheListener(int64_t offset) {
// normal listener chain, which often will eventually include a
// decompressor. If the Content-Encoding is dcb or dcz, we'll include a
// decompressor *before* the tee, so the cache will see decompressed data
-// (we can't decompress dcb/dcz when reading from the cache).
+// (we can't decompress dcb/dcz when reading from the cache). Also, if an
+// entry is being used as a dictionary (Use-As-Dictionary), we want the data
+// to in the cache to be decompressed, so we should install a decompressor
+// before the tee as well.
nsresult nsHttpChannel::DoInstallCacheListener(bool aIsDictionaryCompressed,
nsACString* aDictionary,
int64_t offset) {
@@ -6299,7 +6291,7 @@ nsresult nsHttpChannel::DoInstallCacheListener(bool aIsDictionaryCompressed,
// before the content process gets to see it
// XXX We could recompress this with e.g. gzip to save space and improve
// hitrate, at the cost of some CPU.
- bool removedEncoding = false;
+
// Note: this doesn't handle cases like "dcb, gzip" or (worse?) "gzip, dcb".
// We could in theory handle them.
if (aDictionary || aIsDictionaryCompressed) {
@@ -6311,38 +6303,36 @@ nsresult nsHttpChannel::DoInstallCacheListener(bool aIsDictionaryCompressed,
if (NS_FAILED(rv)) {
return rv;
}
+ // Remove Available-Dictionary from Vary header if present. This
+ // avoids us refusing to match on a future load, for example if this
+ // dictionary was decoded from an earlier version using a dictionary
+ // (i.e. the update jquery to new version using the old version as a
+ // dictionary; no future load will use that old version).
+
+ // XXX It would be slightly more efficient to remove all at once
+ // instead of sequentially by passing an array of strings
+ RemoveFromVary(mResponseHead.get(), "available-dictionary"_ns);
+ RemoveFromVary(mResponseHead.get(), "accept-encoding"_ns);
+
if (listener) {
LOG_DICTIONARIES(
("Installed nsHTTPCompressConv %p before tee", listener.get()));
mListener = listener;
mCompressListener = listener;
StoreHasAppliedConversion(true);
- removedEncoding = true;
+
} else {
LOG_DICTIONARIES(("Didn't install decompressor before tee"));
}
- if (removedEncoding) {
- // Remove Available-Dictionary from Vary header if present if we
- // decompressed and changed the content type. This avoids us refusing to
- // match on a future load, for example if this dictionary was decoded from
- // an earlier version using a dictionary (i.e. the update jquery to new
- // version using the old version as a dictionary; no future load will use
- // that old version).
- // XXX It would be slightly more efficient to remove all at once
- // instead of sequentially by passing an array of strings
- RemoveFromVary(mResponseHead.get(), "available-dictionary"_ns);
- RemoveFromVary(mResponseHead.get(), "accept-encoding"_ns);
- }
- }
-
- if (removedEncoding) {
- // After decompressors, we modified Content-Encoding
- rv = ModifiedCacheEntryHeaders(mCacheEntry, nsHttp::Content_Encoding);
+ // We may have modified Content-Encoding; make sure cache metadata
+ // reflects that. Pass nullptr so we pick up the Vary updates above
+ rv = UpdateCacheEntryHeaders(mCacheEntry, nullptr);
if (NS_FAILED(rv)) {
mCacheEntry->AsyncDoom(nullptr);
return rv;
}
}
+
return NS_OK;
}
diff --git a/netwerk/protocol/http/nsHttpChannel.h b/netwerk/protocol/http/nsHttpChannel.h
@@ -421,10 +421,8 @@ class nsHttpChannel final : public HttpBaseChannel,
bool aModified);
[[nodiscard]] nsresult AddCacheEntryHeaders(nsICacheEntry* entry,
bool aModified);
- [[nodiscard]] nsresult ProcessVaryCacheEntryHeaders(nsICacheEntry* entry,
- const nsHttpAtom* aAtom);
- [[nodiscard]] nsresult ModifiedCacheEntryHeaders(nsICacheEntry* entry,
- const nsHttpAtom& aAtom);
+ [[nodiscard]] nsresult UpdateCacheEntryHeaders(nsICacheEntry* entry,
+ const nsHttpAtom* aAtom);
[[nodiscard]] nsresult FinalizeCacheEntry();
[[nodiscard]] nsresult InstallCacheListener(int64_t offset = 0);
[[nodiscard]] nsresult DoInstallCacheListener(bool aIsDictionaryCompressed,
diff --git a/netwerk/protocol/http/nsHttpHandler.cpp b/netwerk/protocol/http/nsHttpHandler.cpp
@@ -674,13 +674,10 @@ nsresult nsHttpHandler::AddAcceptAndDictionaryHeaders(
aURI, aType, aAsync,
[self = RefPtr(this), aRequest, aCallback](
bool aNeedsResume, DictionaryCacheEntry* aDict) {
- nsresult rv;
if (!aDict) {
- rv = aRequest->SetHeader(
- nsHttp::Accept_Encoding, self->mHttpsAcceptEncodings, false,
- nsHttpHeaderArray::eVarietyRequestOverride);
- (aCallback)(false, nullptr);
- return rv;
+ // Accept-Encoding was already set in AddStandardHeaders
+ (aCallback)(aNeedsResume, nullptr);
+ return NS_OK;
}
nsAutoCStringN<64> encodedHash = ":"_ns + aDict->GetHash() + ":"_ns;
@@ -704,7 +701,7 @@ nsresult nsHttpHandler::AddAcceptAndDictionaryHeaders(
if ((aCallback)(aNeedsResume, aDict)) {
LOG_DICTIONARIES(
("Setting Available-Dictionary: %s", encodedHash.get()));
- rv = aRequest->SetHeader(
+ nsresult rv = aRequest->SetHeader(
nsHttp::Available_Dictionary, encodedHash, false,
nsHttpHeaderArray::eVarietyRequestOverride);
if (NS_FAILED(rv)) {
@@ -727,21 +724,9 @@ nsresult nsHttpHandler::AddAcceptAndDictionaryHeaders(
return NS_OK;
});
} else {
- rv = aRequest->SetHeader(nsHttp::Accept_Encoding, mHttpsAcceptEncodings,
- false,
- nsHttpHeaderArray::eVarietyRequestOverride);
aAsync = false;
}
} else {
- // We need to not override a previous setting of 'identity' (for range
- // requests)
- nsAutoCString encoding;
- Unused << aRequest->GetHeader(nsHttp::Accept_Encoding, encoding);
- if (!encoding.EqualsLiteral("identity")) {
- rv = aRequest->SetHeader(nsHttp::Accept_Encoding, mHttpAcceptEncodings,
- false,
- nsHttpHeaderArray::eVarietyRequestOverride);
- }
aAsync = false;
}
// guard may call aCallback here
