commit 58f95c43814244de9523cf0bdb7d52b596e77f58
parent 857bc5cefa51d78c849e88467fb1a9068c1e7d60
Author: Sandor Molnar <smolnar@mozilla.com>
Date: Fri, 19 Dec 2025 19:56:51 +0200
Revert "Bug 2005971: Retry requests failed because of a corrupt Compression Dictionary r=necko-reviewers,valentin" for causing build bustages @ nsHttpChannel.cpp
This reverts commit d654fc7328fe6de7722153f09ac1ea4be92dbf98.
Diffstat:
4 files changed, 48 insertions(+), 327 deletions(-)
diff --git a/netwerk/protocol/http/HttpBaseChannel.cpp b/netwerk/protocol/http/HttpBaseChannel.cpp
@@ -132,11 +132,9 @@ static bool IsHeaderBlacklistedForRedirectCopy(nsHttpAtom const& aHeader) {
&nsHttp::Alternate_Service_Used,
&nsHttp::Authentication,
&nsHttp::Authorization,
- &nsHttp::Available_Dictionary,
&nsHttp::Connection,
&nsHttp::Content_Length,
&nsHttp::Cookie,
- &nsHttp::Dictionary_Id,
&nsHttp::Host,
&nsHttp::If,
&nsHttp::If_Match,
diff --git a/netwerk/protocol/http/nsHttpChannel.cpp b/netwerk/protocol/http/nsHttpChannel.cpp
@@ -649,22 +649,6 @@ nsresult nsHttpChannel::PrepareToConnect() {
mURI, mLoadInfo->GetExternalContentPolicyType(), &mRequestHead, IsHTTPS(),
this, nsHttpChannel::StaticSuspend,
[self = RefPtr(this)](bool aNeedsResume, DictionaryCacheEntry* aDict) {
- // Don't use dictionary if we're retrying after hash mismatch
- if (self->mDictionaryDisabledForRetry) {
- LOG_DICTIONARIES(("Dictionary disabled for retry, skipping [this=%p]",
- self.get()));
- if (self->mUsingDictionary) {
- self->mDictDecompress->UseCompleted();
- self->mUsingDictionary = false;
- }
- self->mDictDecompress = nullptr;
- if (aNeedsResume) {
- self->Resume();
- }
- // Return false to prevent Available-Dictionary header from being
- // added
- return false;
- }
self->mDictDecompress = aDict;
if (aNeedsResume) {
LOG_DICTIONARIES(("Resuming after getting Dictionary headers"));
@@ -705,37 +689,6 @@ nsresult nsHttpChannel::PrepareToConnect() {
self->mUsingDictionary = false;
}
self->mDictDecompress = nullptr;
-
- // On hash mismatch, set up redirect to retry without
- // dictionary. The request may have already been sent
- // with Available-Dictionary header, so we need to cancel
- // (or more to the point ignore) and retry without it.
- if (aResult == NS_ERROR_CORRUPTED_CONTENT) {
- LOG(
- ("nsHttpChannel::Prefetch hash mismatch, setting up "
- "retry [this=%p]",
- self.get()));
- // Set up redirect channel - it will be opened in
- // OnStopRequest
- nsresult rv =
- self->RedirectToNewChannelForDictionaryRetry();
- if (NS_SUCCEEDED(rv)) {
- // Redirect set up successfully. Resume if suspended so
- // OnStopRequest can be called and open the redirect
- // channel.
- if (self->mSuspendedForDictionary) {
- self->mSuspendedForDictionary = false;
- self->Resume();
- }
- return;
- }
- // If redirect setup failed, fall through to cancel
- LOG(
- ("nsHttpChannel::Prefetch redirect setup failed "
- "[this=%p rv=0x%" PRIx32 "]",
- self.get(), static_cast<uint32_t>(rv)));
- }
-
if (self->mSuspendedForDictionary) {
self->mSuspendedForDictionary = false;
self->Cancel(aResult);
@@ -2362,14 +2315,6 @@ void nsHttpChannel::SetCachedContentType() {
nsresult nsHttpChannel::CallOnStartRequest() {
LOG(("nsHttpChannel::CallOnStartRequest [this=%p]", this));
- // If dictionary retry is pending, don't call OnStartRequest on the original
- // channel. The redirect channel will call OnStartRequest instead.
- if (mDictionaryRetryPending) {
- LOG(("CallOnStartRequest skipped due to pending dictionary retry [this=%p]",
- this));
- return NS_OK;
- }
-
MOZ_RELEASE_ASSERT(!LoadRequireCORSPreflight() || LoadIsCorsPreflightDone(),
"CORS preflight must have been finished by the time we "
"call OnStartRequest");
@@ -3947,26 +3892,29 @@ void nsHttpChannel::HandleAsyncRedirectToUnstrippedURI() {
}
}
}
-nsresult nsHttpChannel::RedirectToNewChannelInternal(
- uint32_t redirectFlags,
- std::function<nsresult(nsHttpChannel*)> setupCallback) {
- LOG(("nsHttpChannel::RedirectToNewChannelInternal [this=%p, flags=0x%x]",
- this, redirectFlags));
+nsresult nsHttpChannel::RedirectToNewChannelForAuthRetry() {
+ LOG(("nsHttpChannel::RedirectToNewChannelForAuthRetry %p", this));
+ nsresult rv = NS_OK;
- nsCOMPtr<nsILoadInfo> redirectLoadInfo =
- CloneLoadInfoForRedirect(mURI, redirectFlags);
+ nsCOMPtr<nsILoadInfo> redirectLoadInfo = CloneLoadInfoForRedirect(
+ mURI, nsIChannelEventSink::REDIRECT_INTERNAL |
+ nsIChannelEventSink::REDIRECT_AUTH_RETRY);
nsCOMPtr<nsIIOService> ioService;
- nsresult rv = gHttpHandler->GetIOService(getter_AddRefs(ioService));
+
+ rv = gHttpHandler->GetIOService(getter_AddRefs(ioService));
NS_ENSURE_SUCCESS(rv, rv);
nsCOMPtr<nsIChannel> newChannel;
rv = gHttpHandler->NewProxiedChannel(mURI, mProxyInfo, mProxyResolveFlags,
mProxyURI, mLoadInfo,
getter_AddRefs(newChannel));
+
NS_ENSURE_SUCCESS(rv, rv);
- rv = SetupReplacementChannel(mURI, newChannel, true, redirectFlags);
+ rv = SetupReplacementChannel(mURI, newChannel, true,
+ nsIChannelEventSink::REDIRECT_INTERNAL |
+ nsIChannelEventSink::REDIRECT_AUTH_RETRY);
NS_ENSURE_SUCCESS(rv, rv);
// rewind the upload stream
@@ -3986,112 +3934,70 @@ nsresult nsHttpChannel::RedirectToNewChannelInternal(
RefPtr<nsHttpChannel> httpChannelImpl = do_QueryObject(newChannel);
- rv = setupCallback(httpChannelImpl);
- NS_ENSURE_SUCCESS(rv, rv);
-
- mRedirectChannel = newChannel;
-
- rv = gHttpHandler->AsyncOnChannelRedirect(this, newChannel, redirectFlags);
-
- if (NS_SUCCEEDED(rv)) {
- rv = WaitForRedirectCallback();
- }
-
- if (NS_FAILED(rv)) {
- AutoRedirectVetoNotifier notifier(this, rv);
- mRedirectChannel = nullptr;
- }
-
- return rv;
-}
-
-nsresult nsHttpChannel::SetupChannelForAuthRetry() {
MOZ_ASSERT(mAuthProvider);
- mAuthProvider = std::move(mAuthProvider);
+ httpChannelImpl->mAuthProvider = std::move(mAuthProvider);
- mProxyInfo = mProxyInfo;
+ httpChannelImpl->mProxyInfo = mProxyInfo;
if ((mCaps & NS_HTTP_STICKY_CONNECTION) ||
mTransaction->HasStickyConnection()) {
mConnectionInfo = mTransaction->GetConnInfo();
- mTransactionSticky = mTransaction;
+ httpChannelImpl->mTransactionSticky = mTransaction;
if (mTransaction->Http2Disabled()) {
- mCaps |= NS_HTTP_DISALLOW_SPDY;
+ httpChannelImpl->mCaps |= NS_HTTP_DISALLOW_SPDY;
}
if (mTransaction->Http3Disabled()) {
- mCaps |= NS_HTTP_DISALLOW_HTTP3;
+ httpChannelImpl->mCaps |= NS_HTTP_DISALLOW_HTTP3;
}
}
- mCaps |= NS_HTTP_STICKY_CONNECTION;
+ // always set sticky connection flag
+ httpChannelImpl->mCaps |= NS_HTTP_STICKY_CONNECTION;
if (LoadAuthConnectionRestartable()) {
- mCaps |= NS_HTTP_CONNECTION_RESTARTABLE;
+ httpChannelImpl->mCaps |= NS_HTTP_CONNECTION_RESTARTABLE;
} else {
- mCaps &= ~NS_HTTP_CONNECTION_RESTARTABLE;
+ httpChannelImpl->mCaps &= ~NS_HTTP_CONNECTION_RESTARTABLE;
}
MOZ_ASSERT(mConnectionInfo);
- mConnectionInfo = mConnectionInfo->Clone();
+ httpChannelImpl->mConnectionInfo = mConnectionInfo->Clone();
- // we need to store the state to skip unnecessary checks in the new
- // channel
- StoreAuthRedirectedChannel(true);
+ // we need to store the state to skip unnecessary checks in the new channel
+ httpChannelImpl->StoreAuthRedirectedChannel(true);
// We must copy proxy and auth header to the new channel.
- // Although the new channel can populate auth headers from auth cache,
- // we would still like to use the auth headers generated in this
- // channel. The main reason for doing this is that certain
- // connection-based/stateful auth schemes like NTLM will fail when we
- // try generate the credentials more than the number of times the server
- // has presented us the challenge due to the usage of nonce in
- // generating the credentials Copying the auth header will bypass
- // generation of the credentials
+ // Although the new channel can populate auth headers from auth cache, we
+ // would still like to use the auth headers generated in this channel. The
+ // main reason for doing this is that certain connection-based/stateful auth
+ // schemes like NTLM will fail when we try generate the credentials more than
+ // the number of times the server has presented us the challenge due to the
+ // usage of nonce in generating the credentials Copying the auth header will
+ // bypass generation of the credentials
nsAutoCString authVal;
if (NS_SUCCEEDED(GetRequestHeader("Proxy-Authorization"_ns, authVal))) {
- SetRequestHeader("Proxy-Authorization"_ns, authVal, false);
+ httpChannelImpl->SetRequestHeader("Proxy-Authorization"_ns, authVal, false);
}
if (NS_SUCCEEDED(GetRequestHeader("Authorization"_ns, authVal))) {
- SetRequestHeader("Authorization"_ns, authVal, false);
+ httpChannelImpl->SetRequestHeader("Authorization"_ns, authVal, false);
}
- SetBlockAuthPrompt(LoadBlockAuthPrompt());
-
- return NS_OK;
-}
-
-nsresult nsHttpChannel::RedirectToNewChannelForAuthRetry() {
- LOG(("nsHttpChannel::RedirectToNewChannelForAuthRetry %p", this));
+ httpChannelImpl->SetBlockAuthPrompt(LoadBlockAuthPrompt());
+ mRedirectChannel = newChannel;
- return RedirectToNewChannelInternal(
+ rv = gHttpHandler->AsyncOnChannelRedirect(
+ this, newChannel,
nsIChannelEventSink::REDIRECT_INTERNAL |
- nsIChannelEventSink::REDIRECT_AUTH_RETRY,
- [this](nsHttpChannel* httpChannelImpl) -> nsresult {
- return httpChannelImpl->SetupChannelForAuthRetry();
- });
-}
-
-nsresult nsHttpChannel::RedirectToNewChannelForDictionaryRetry() {
- LOG(("nsHttpChannel::RedirectToNewChannelForDictionaryRetry [this=%p]",
- this));
-
- // Don't retry if it's POST, PUT, etc (pretty unlikely)
- if (!mRequestHead.IsSafeMethod()) {
- return NS_ERROR_FAILURE;
- }
+ nsIChannelEventSink::REDIRECT_AUTH_RETRY);
- mDictionaryRetryPending = true;
+ if (NS_SUCCEEDED(rv)) rv = WaitForRedirectCallback();
- nsresult rv = RedirectToNewChannelInternal(
- nsIChannelEventSink::REDIRECT_INTERNAL,
- [](nsHttpChannel* httpChannelImpl) -> nsresult {
- httpChannelImpl->mDictionaryDisabledForRetry = true;
- return NS_OK;
- });
+ // redirected channel will be opened after we receive the OnStopRequest
if (NS_FAILED(rv)) {
- mDictionaryRetryPending = false;
+ AutoRedirectVetoNotifier notifier(this, rv);
+ mRedirectChannel = nullptr;
}
return rv;
@@ -10451,28 +10357,6 @@ nsresult nsHttpChannel::ContinueOnStopRequest(nsresult aStatus, bool aIsFromNet,
mAuthRetryPending = false;
}
- // Handle dictionary retry - similar to auth retry above
- if (mDictionaryRetryPending) {
- nsresult rv = OpenRedirectChannel(aStatus);
- LOG(("Opening redirect channel for dictionary retry %x",
- static_cast<uint32_t>(rv)));
- if (NS_FAILED(rv)) {
- if (mListener) {
- MOZ_ASSERT(!LoadOnStartRequestCalled(),
- "We should not call OnStartRequest twice.");
- if (!LoadOnStartRequestCalled()) {
- nsCOMPtr<nsIStreamListener> listener(mListener);
- StoreOnStartRequestCalled(true);
- listener->OnStartRequest(this);
- }
- } else {
- StoreOnStartRequestCalled(true);
- NS_WARNING("OnStartRequest skipped because of null listener");
- }
- }
- mDictionaryRetryPending = false;
- }
-
if (LoadUsedNetwork() && !mReportedNEL) {
MaybeGenerateNELReport();
}
@@ -10585,8 +10469,7 @@ nsHttpChannel::OnDataAvailable(nsIRequest* request, nsIInputStream* input,
// don't send out OnDataAvailable notifications if we've been canceled.
if (mCanceled) return mStatus;
- if (mAuthRetryPending || mDictionaryRetryPending ||
- WRONG_RACING_RESPONSE_SOURCE(request) ||
+ if (mAuthRetryPending || WRONG_RACING_RESPONSE_SOURCE(request) ||
(request == mTransactionPump && LoadTransactionReplaced())) {
uint32_t n;
return input->ReadSegments(NS_DiscardSegment, nullptr, count, &n);
@@ -11361,14 +11244,11 @@ nsHttpChannel::OnRedirectVerifyCallback(nsresult result) {
if (!LoadWaitingForRedirectCallback()) {
// We are not waiting for the callback. At this moment we must release
// reference to the redirect target channel, otherwise we may leak.
- // However, if we are redirecting due to auth retries or dictionary retries,
- // we open the redirected channel after OnStopRequest. In that case we
- // should not release the reference.
- bool keepRedirectChannel =
- (StaticPrefs::network_auth_use_redirect_for_retries() &&
- mAuthRetryPending) ||
- mDictionaryRetryPending;
- if (!keepRedirectChannel) {
+ // However, if we are redirecting due to auth retries, we open the
+ // redirected channel after OnStopRequest. In that case we should not
+ // release the reference
+ if (!StaticPrefs::network_auth_use_redirect_for_retries() ||
+ !mAuthRetryPending) {
mRedirectChannel = nullptr;
}
}
diff --git a/netwerk/protocol/http/nsHttpChannel.h b/netwerk/protocol/http/nsHttpChannel.h
@@ -546,25 +546,14 @@ class nsHttpChannel final : public HttpBaseChannel,
// resolve in firing a ServiceWorker FetchEvent.
[[nodiscard]] nsresult RedirectToInterceptedChannel();
- // the internals to set up for AuthRetry
- [[nodiscard]] nsresult SetupChannelForAuthRetry();
-
// Start an internal redirect to a new channel for auth retry
[[nodiscard]] nsresult RedirectToNewChannelForAuthRetry();
- // Start an internal redirect to retry without dictionary after hash mismatch
- [[nodiscard]] nsresult RedirectToNewChannelForDictionaryRetry();
-
// Determines and sets content type in the cache entry. It's called when
// writing a new entry. The content type is used in cache internally only.
void SetCachedContentType();
private:
- // Helper function for RedirectToNewChannelForAuthRetry and
- // RedirectToNewChannelForDictionaryRetry
- [[nodiscard]] nsresult RedirectToNewChannelInternal(
- uint32_t redirectFlags,
- std::function<nsresult(nsHttpChannel*)> setupCallback);
// this section is for main-thread-only object
// all the references need to be proxy released on main thread.
// auth specific data
@@ -933,10 +922,6 @@ class nsHttpChannel final : public HttpBaseChannel,
bool mUsingDictionary{false}; // we added Available-Dictionary
bool mShouldSuspendForDictionary{false};
bool mSuspendedForDictionary{false};
- // Set on retry channel to prevent offering dictionary again
- bool mDictionaryDisabledForRetry{false};
- // Set when redirect channel is ready, waiting for OnStopRequest to open it
- bool mDictionaryRetryPending{false};
protected:
virtual void DoNotifyListenerCleanup() override;
diff --git a/netwerk/test/unit/test_dictionary_replacement.js b/netwerk/test/unit/test_dictionary_replacement.js
@@ -656,148 +656,6 @@ add_task(async function test_dictionary_hash_mismatch() {
dump("**** Test passed: Hash mismatch properly handled\n");
});
-/**
- * Test that hash mismatch triggers an automatic retry WITHOUT the
- * Available-Dictionary header, and the retry succeeds.
- *
- * Steps:
- * 1. Load a resource with Use-As-Dictionary header (creates dictionary entry)
- * 2. Verify Available-Dictionary is sent for matching resources
- * 3. Corrupt the hash and clear dictionary data
- * 4. Request matching resource - should trigger hash mismatch
- * 5. Verify retry occurs WITHOUT Available-Dictionary header
- * 6. Verify the request succeeds
- */
-add_task(async function test_dictionary_hash_mismatch_retry() {
- dump("**** Clear cache and start fresh\n");
- let lci = Services.loadContextInfo.custom(false, {
- partitionKey: `(https,localhost)`,
- });
- evict_cache_entries("all", lci);
-
- let testingInterface = Services.cache2.QueryInterface(Ci.nsICacheTesting);
- testingInterface.clearDictionaryCacheMemory();
-
- await syncCache();
-
- // Register dictionary endpoint
- await server.registerPathHandler(
- "/dict/retry-test",
- function (request, response) {
- response.writeHead(200, {
- "Content-Type": "application/octet-stream",
- "Use-As-Dictionary":
- 'match="/retry-match/*", id="retry-test-dict", type=raw',
- "Cache-Control": "max-age=3600",
- });
- response.end("DICTIONARY_FOR_RETRY_TEST", "binary");
- }
- );
-
- // Register matching resource endpoint that tracks ALL requests
- await server.registerPathHandler(
- "/retry-match/test",
- function (request, response) {
- // Initialize array if needed
- if (!global.retryTestRequests) {
- global.retryTestRequests = [];
- }
- // Record each request's Available-Dictionary header
- global.retryTestRequests.push(
- request.headers["available-dictionary"] || null
- );
- response.writeHead(200, {
- "Content-Type": "text/plain",
- "Cache-Control": "no-cache",
- });
- response.end("RETRY_TEST_CONTENT", "binary");
- }
- );
-
- // Reset request tracking
- await server.execute("global.retryTestRequests = []");
-
- dump("**** Step 1: Load dictionary resource with Use-As-Dictionary\n");
-
- let dictUrl = `https://localhost:${server.port()}/dict/retry-test`;
- let chan = makeChan(dictUrl);
- let [, data] = await channelOpenPromise(chan);
-
- Assert.equal(
- data,
- "DICTIONARY_FOR_RETRY_TEST",
- "Dictionary content should match"
- );
- await verifyDictionaryStored(dictUrl, true);
- await syncCache();
-
- dump("**** Step 2: Verify Available-Dictionary is sent initially\n");
-
- let matchingUrl = `https://localhost:${server.port()}/retry-match/test`;
- chan = makeChan(matchingUrl);
- await channelOpenPromise(chan);
-
- let requests = await server.execute("global.retryTestRequests");
- Assert.equal(requests.length, 1, "Should have one request");
- Assert.notStrictEqual(
- requests[0],
- null,
- "First request should have Available-Dictionary"
- );
-
- dump("**** Step 3: Corrupt hash and clear dictionary data\n");
-
- // Reset request tracking for the retry test
- await server.execute("global.retryTestRequests = []");
-
- testingInterface.corruptDictionaryHash(dictUrl);
- testingInterface.clearDictionaryDataForTesting(dictUrl);
-
- dump(
- "**** Step 4: Request matching resource - triggers hash mismatch and retry\n"
- );
-
- chan = makeChan(matchingUrl);
- [, data] = await channelOpenPromise(chan);
-
- dump("**** Step 5: Verify retry occurred without Available-Dictionary\n");
-
- requests = await server.execute("global.retryTestRequests");
- dump(`**** Received ${requests.length} request(s)\n`);
- for (let i = 0; i < requests.length; i++) {
- dump(`**** Request ${i}: Available-Dictionary = ${requests[i]}\n`);
- }
-
- // We expect 2 requests:
- // 1. First request WITH Available-Dictionary (triggers hash mismatch)
- // 2. Retry request WITHOUT Available-Dictionary
- Assert.equal(
- requests.length,
- 2,
- "Should have two requests (original + retry)"
- );
- Assert.notStrictEqual(
- requests[0],
- null,
- "First request should have Available-Dictionary"
- );
- Assert.equal(
- requests[1],
- null,
- "Retry request should NOT have Available-Dictionary"
- );
-
- dump("**** Step 6: Verify the request succeeded\n");
-
- Assert.equal(
- data,
- "RETRY_TEST_CONTENT",
- "Request should succeed with correct content"
- );
-
- dump("**** Test passed: Hash mismatch retry works correctly\n");
-});
-
// Cleanup
add_task(async function cleanup() {
let lci = Services.loadContextInfo.custom(false, {
