commit 9eadb586ca759f088ada51f86a95f5bcf66db93f
parent 6c989f8755f6b51b8a0ee9cb296deb23a3ecd65c
Author: Valentin Gosu <valentin.gosu@gmail.com>
Date: Thu, 16 Oct 2025 14:06:22 +0000
Bug 1966494 - Make sure channels waiting for cache entry aren't blocked by suspended channels r=necko-reviewers,jesup
- adds a timer to nsHttpChannel::Suspend so after 5 seconds of being suspended it makes other listeners of the cache entry continue without one
- Listeners that passed the OPEN_BYPASS_IF_BUSY flag will already do that, but that is not commonly used, and listeners can't add the flag after calling cacheStorage->AsyncOpenURI()
- This patch doesn't fix the concurrent read/write case, where listeners are actively reading chunks as they're being written. For that case to work we'd need to pass the transaction from the suspended channel to one of the listening channels, or a new channel that's only intended for writing.
Differential Revision: https://phabricator.services.mozilla.com/D266672
Diffstat:
7 files changed, 277 insertions(+), 12 deletions(-)
diff --git a/modules/libpref/init/StaticPrefList.yaml b/modules/libpref/init/StaticPrefList.yaml
@@ -15538,6 +15538,14 @@
value: false
mirror: always
+# The number of milliseconds after which a suspended channel writing
+# to a cache entry will notify all readers waiting for a callback to
+# continue without a cache entry.
+- name: network.cache.suspended_writer_delay_ms
+ type: RelaxedAtomicUint32
+ value: 5000
+ mirror: always
+
# This is used for a temporary workaround for a web-compat issue. If pref is
# true CORS preflight requests are allowed to send client certificates.
- name: network.cors_preflight.allow_client_cert
diff --git a/netwerk/cache2/CacheEntry.cpp b/netwerk/cache2/CacheEntry.cpp
@@ -217,6 +217,7 @@ CacheEntry::CacheEntry(const nsACString& aStorageID, const nsACString& aURI,
mPreventCallbacks(false),
mHasData(false),
mPinningKnown(false),
+ mBypassWriterLock(false),
mCacheEntryId(GetNextId()) {
LOG(("CacheEntry::CacheEntry [this=%p]", this));
@@ -644,8 +645,11 @@ bool CacheEntry::InvokeCallbacks(bool aReadOnly) MOZ_REQUIRES(mLock) {
}
if (!mIsDoomed && (mState == WRITING || mState == REVALIDATING)) {
- LOG((" entry is being written/revalidated"));
- return false;
+ if (!mBypassWriterLock) {
+ LOG((" entry is being written/revalidated"));
+ return false;
+ }
+ LOG((" entry is being written/revalidated but bypassing writer lock"));
}
bool recreate;
@@ -717,12 +721,15 @@ bool CacheEntry::InvokeCallback(Callback& aCallback) MOZ_REQUIRES(mLock) {
MOZ_ASSERT(mState > LOADING);
if (mState == WRITING || mState == REVALIDATING) {
- // Prevent invoking other callbacks since one of them is now writing
- // or revalidating this entry. No consumers should get this entry
- // until metadata are filled with values downloaded from the server
- // or the entry revalidated and output stream has been opened.
- LOG((" entry is being written/revalidated, callback bypassed"));
- return false;
+ if (!mBypassWriterLock) {
+ // Prevent invoking other callbacks since one of them is now writing
+ // or revalidating this entry. No consumers should get this entry
+ // until metadata are filled with values downloaded from the server
+ // or the entry revalidated and output stream has been opened.
+ LOG((" entry is being written/revalidated, callback bypassed"));
+ return false;
+ }
+ LOG((" entry is being written/revalidated but bypassing writer lock"));
}
// mRecheckAfterWrite flag already set means the callback has already passed
@@ -982,6 +989,12 @@ void CacheEntry::OnHandleClosed(CacheEntryHandle const* aHandle) {
mWriter = nullptr;
+ // Reset bypass flag when writer is cleared
+ if (mBypassWriterLock) {
+ mBypassWriterLock = false;
+ LOG((" reset bypass writer lock flag due to writer cleared"));
+ }
+
if (mState == WRITING) {
LOG((" reverting to state EMPTY - write failed"));
mState = EMPTY;
@@ -1023,6 +1036,17 @@ bool CacheEntry::IsReferenced() const {
return mHandlesCount > 0;
}
+void CacheEntry::SetBypassWriterLock(bool aBypass) {
+ mozilla::MutexAutoLock lock(mLock);
+ LOG(("CacheEntry::SetBypassWriterLock [this=%p, bypass=%d]", this, aBypass));
+ mBypassWriterLock = aBypass;
+
+ if (aBypass) {
+ // Invoke callbacks that were blocked by writer state
+ InvokeCallbacks();
+ }
+}
+
bool CacheEntry::IsFileDoomed() {
if (NS_SUCCEEDED(mFileStatus)) {
return mFile->IsDoomed();
@@ -1477,7 +1501,15 @@ nsresult CacheEntry::MetaDataReady() {
MOZ_ASSERT(mState > EMPTY);
- if (mState == WRITING) mState = READY;
+ if (mState == WRITING) {
+ mState = READY;
+
+ // Reset bypass flag when transitioning to READY state
+ if (mBypassWriterLock) {
+ mBypassWriterLock = false;
+ LOG((" reset bypass writer lock flag due to state transition to READY"));
+ }
+ }
InvokeCallbacks();
@@ -1497,6 +1529,12 @@ nsresult CacheEntry::SetValid() {
mState = READY;
mHasData = true;
+ // Reset bypass flag when transitioning to READY state
+ if (mBypassWriterLock) {
+ mBypassWriterLock = false;
+ LOG((" reset bypass writer lock flag due to state transition to READY"));
+ }
+
InvokeCallbacks();
outputStream.swap(mOutputStream);
diff --git a/netwerk/cache2/CacheEntry.h b/netwerk/cache2/CacheEntry.h
@@ -120,6 +120,12 @@ class CacheEntry final : public nsIRunnable,
bool IsDoomed() const { return mIsDoomed; }
bool IsPinned() const { return mPinned; }
+ // Mark entry to allow bypassing writer lock for new listeners
+ void SetBypassWriterLock(bool aBypass);
+ bool ShouldBypassWriterLock() const MOZ_REQUIRES(mLock) {
+ return mBypassWriterLock;
+ }
+
// Methods for entry management (eviction from memory),
// called only on the management thread.
@@ -366,6 +372,8 @@ class CacheEntry final : public nsIRunnable,
// Whether the pinning state of the entry is known (equals to the actual state
// of the cache file)
bool mPinningKnown : 1 MOZ_GUARDED_BY(mLock);
+ // Whether to bypass writer lock for new listeners (when writer is suspended)
+ bool mBypassWriterLock : 1 MOZ_GUARDED_BY(mLock);
static char const* StateString(uint32_t aState);
@@ -554,6 +562,10 @@ class CacheEntryHandle final : public nsICacheEntry {
NS_IMETHOD SetDictionary(DictionaryCacheEntry* aDict) override {
return mEntry->SetDictionary(aDict);
}
+ NS_IMETHOD SetBypassWriterLock(bool aBypass) override {
+ mEntry->SetBypassWriterLock(aBypass);
+ return NS_OK;
+ }
// Specific implementation:
NS_IMETHOD Dismiss() override;
diff --git a/netwerk/cache2/nsICacheEntry.idl b/netwerk/cache2/nsICacheEntry.idl
@@ -328,6 +328,10 @@ interface nsICacheEntry : nsISupports
*/
[noscript] void SetDictionary(in DictionaryCacheEntry dict);
+ /**
+ * Set bypass flag to allow waiting listeners to continue even while entry is being written
+ */
+ void setBypassWriterLock(in boolean aBypass);
};
/**
diff --git a/netwerk/protocol/http/nsHttpChannel.cpp b/netwerk/protocol/http/nsHttpChannel.cpp
@@ -6288,6 +6288,7 @@ nsresult nsHttpChannel::DoInstallCacheListener(bool aIsDictionaryCompressed,
if (NS_FAILED(rv)) return rv;
mListener = tee;
+ mWritingToCache = true;
// If this is Use-As-Dictionary we need to be able to read it quickly for
// dictionary use, OR if it's encoded in dcb or dcz (using a dictionary),
// we must decompress it before storing since we won't have the dictionary
@@ -7122,6 +7123,7 @@ void nsHttpChannel::CancelNetworkRequest(nsresult aStatus) {
NS_IMETHODIMP
nsHttpChannel::Suspend() {
+ MOZ_ASSERT(NS_IsMainThread());
NS_ENSURE_TRUE(LoadIsPending(), NS_ERROR_NOT_AVAILABLE);
PROFILER_MARKER("nsHttpChannel::Suspend", NETWORK, {}, FlowMarker,
@@ -7133,6 +7135,21 @@ nsHttpChannel::Suspend() {
if (mSuspendCount == 1) {
mSuspendTimestamp = TimeStamp::NowLoRes();
+
+ // Start timer to detect if we're suspended too long
+ // This helps unblock waiting cache listeners when a channel is suspended
+ // for an unreasonably long time.
+ uint32_t delay = StaticPrefs::network_cache_suspended_writer_delay_ms();
+ if (!mSuspendTimer && delay) {
+ mSuspendTimer = NS_NewTimer();
+ }
+
+ if (mSuspendTimer && delay) {
+ RefPtr<TimerCallback> timerCallback = new TimerCallback(this);
+ mSuspendTimer->InitWithCallback(timerCallback, delay,
+ nsITimer::TYPE_ONE_SHOT);
+ LOG((" started suspend timer, will fire in %dms", delay));
+ }
}
nsresult rvTransaction = NS_OK;
@@ -7152,6 +7169,7 @@ void nsHttpChannel::StaticSuspend(nsHttpChannel* aChan) { aChan->Suspend(); }
NS_IMETHODIMP
nsHttpChannel::Resume() {
+ MOZ_ASSERT(NS_IsMainThread());
NS_ENSURE_TRUE(mSuspendCount > 0, NS_ERROR_UNEXPECTED);
AUTO_PROFILER_FLOW_MARKER("nsHttpChannel::Resume", NETWORK,
@@ -7162,6 +7180,18 @@ nsHttpChannel::Resume() {
if (--mSuspendCount == 0) {
mSuspendTotalTime += TimeStamp::NowLoRes() - mSuspendTimestamp;
+ // Cancel suspend timer since we're resuming
+ if (mSuspendTimer) {
+ mSuspendTimer->Cancel();
+ LOG((" cancelled suspend timer"));
+ }
+
+ // Reset bypass flag since the writer is resuming
+ if (mCacheEntry && (mWritingToCache || LoadCacheEntryIsWriteOnly())) {
+ mCacheEntry->SetBypassWriterLock(false);
+ LOG((" reset bypass writer lock flag"));
+ }
+
if (mCallOnResume) {
// Resume the interrupted procedure first, then resume
// the pump to continue process the input stream.
@@ -10101,6 +10131,7 @@ nsresult nsHttpChannel::ContinueOnStopRequest(nsresult aStatus, bool aIsFromNet,
}
CloseCacheEntry(!aContentComplete);
+ mWritingToCache = false;
if (mLoadGroup) {
mLoadGroup->RemoveRequest(this, nullptr, aStatus);
@@ -11686,6 +11717,21 @@ nsresult nsHttpChannel::TriggerNetwork() {
return ContinueConnect();
}
+nsresult nsHttpChannel::OnSuspendTimeout() {
+ MOZ_ASSERT(NS_IsMainThread(), "Must be called on the main thread");
+
+ LOG(("nsHttpChannel::OnSuspendTimeout [this=%p]\n", this));
+
+ // If we're still suspended and have a cache entry, enable bypass mode
+ // This allows any waiting or future listeners to continue
+ if (mSuspendCount > 0 && mCacheEntry) {
+ LOG((" suspend timeout: bypassing writer lock"));
+ mCacheEntry->SetBypassWriterLock(true);
+ }
+
+ return NS_OK;
+}
+
void nsHttpChannel::MaybeRaceCacheWithNetwork() {
nsresult rv;
@@ -11793,6 +11839,9 @@ nsHttpChannel::TimerCallback::Notify(nsITimer* aTimer) {
if (aTimer == mChannel->mNetworkTriggerTimer) {
return mChannel->TriggerNetwork();
}
+ if (aTimer == mChannel->mSuspendTimer) {
+ return mChannel->OnSuspendTimeout();
+ }
MOZ_CRASH("Unknown timer");
return NS_OK;
diff --git a/netwerk/protocol/http/nsHttpChannel.h b/netwerk/protocol/http/nsHttpChannel.h
@@ -835,6 +835,7 @@ class nsHttpChannel final : public HttpBaseChannel,
nsresult TriggerNetworkWithDelay(uint32_t aDelay);
nsresult TriggerNetwork();
+ nsresult OnSuspendTimeout();
void CancelNetworkRequest(nsresult aStatus);
nsresult LogConsoleError(const char* aTag);
@@ -848,6 +849,12 @@ class nsHttpChannel final : public HttpBaseChannel,
nsCOMPtr<nsITimer> mNetworkTriggerTimer;
// Is true if the network request has been triggered.
bool mNetworkTriggered = false;
+
+ // Timer to detect if channel has been suspended too long while writing to
+ // cache. When the timer fires we'll notify the cache entry to make
+ // all other listeners continue.
+ nsCOMPtr<nsITimer> mSuspendTimer;
+ bool mWritingToCache = false;
bool mWaitingForProxy = false;
bool mStaleRevalidation = false;
// Will be true if the onCacheEntryAvailable callback is not called by the
diff --git a/netwerk/test/browser/browser_fetch_after_suspending_request.js b/netwerk/test/browser/browser_fetch_after_suspending_request.js
@@ -15,7 +15,10 @@ let wrappedChannel;
async function test_fetch_after_suspend(rcwnEnabled) {
info("Set network.http.rcwn.enabled to " + rcwnEnabled);
await SpecialPowers.pushPrefEnv({
- set: [["network.http.rcwn.enabled", rcwnEnabled]],
+ set: [
+ ["network.http.rcwn.enabled", rcwnEnabled],
+ ["network.cache.suspended_writer_delay_ms", 300],
+ ],
});
info("Add a new test tab");
@@ -55,9 +58,8 @@ async function test_fetch_after_suspend(rcwnEnabled) {
await promise;
info("Fetch the same URL again");
- let second = fetch(tab.linkedBrowser, testBlockedUrl);
let secondCompleted = false;
- second.then(() => {
+ let second = fetch(tab.linkedBrowser, testBlockedUrl).then(() => {
secondCompleted = true;
});
@@ -85,3 +87,148 @@ function fetch(browser, url) {
await response.text();
});
}
+
+add_task(async function test_fetch_after_suspended_timer_fires() {
+ await SpecialPowers.pushPrefEnv({
+ set: [
+ ["network.http.rcwn.enabled", false],
+ ["network.cache.suspended_writer_delay_ms", 300],
+ ],
+ });
+
+ info("Add a new test tab");
+ const tab = BrowserTestUtils.addTab(
+ gBrowser,
+ "https://example.com/document-builder.sjs?html=tab"
+ );
+ await BrowserTestUtils.browserLoaded(tab.linkedBrowser);
+
+ const testBlockedUrl = "https://example.com/?test-blocked";
+
+ info(`Add an observer to suspend the next channel to ${testBlockedUrl}`);
+ const { promise, resolve } = Promise.withResolvers();
+ const onExamineResponse = subject => {
+ if (!(subject instanceof Ci.nsIHttpChannel)) {
+ return;
+ }
+
+ const channel = subject.QueryInterface(Ci.nsIHttpChannel);
+ if (channel.URI.displaySpec !== testBlockedUrl) {
+ return;
+ }
+
+ wrappedChannel = ChannelWrapper.get(channel);
+ wrappedChannel.suspend("test-blocked-suspend");
+ Services.obs.removeObserver(onExamineResponse, "http-on-examine-response");
+ resolve();
+ };
+ Services.obs.addObserver(onExamineResponse, "http-on-examine-response");
+
+ info(`Send fetch call for ${testBlockedUrl}`);
+ let first = fetch(tab.linkedBrowser, testBlockedUrl);
+
+ info(
+ "Wait for the fetch request to be suspended in http-on-examine-response"
+ );
+ await promise;
+
+ // eslint-disable-next-line mozilla/no-arbitrary-setTimeout
+ await new Promise(resolve => setTimeout(resolve, 500));
+
+ info("Fetch the same URL again");
+ let secondCompleted = false;
+ let second = fetch(tab.linkedBrowser, testBlockedUrl).then(() => {
+ secondCompleted = true;
+ });
+
+ // eslint-disable-next-line mozilla/no-arbitrary-setTimeout
+ let timer = new Promise(resolve => setTimeout(resolve, 2000));
+ await Promise.race([second, timer]);
+
+ Assert.equal(
+ secondCompleted,
+ true,
+ "The second fetch should resolve successfully"
+ );
+
+ // Resume the first channel and await its completion so we don't leak anything.
+ wrappedChannel.resume();
+ await first;
+
+ info("Cleanup");
+ gBrowser.removeTab(tab);
+});
+
+add_task(async function test_fetch_after_suspended_and_resumed() {
+ await SpecialPowers.pushPrefEnv({
+ set: [
+ ["network.http.rcwn.enabled", false],
+ ["network.cache.suspended_writer_delay_ms", 1000],
+ ],
+ });
+
+ info("Add a new test tab");
+ const tab = BrowserTestUtils.addTab(
+ gBrowser,
+ "https://example.com/document-builder.sjs?html=tab"
+ );
+ await BrowserTestUtils.browserLoaded(tab.linkedBrowser);
+
+ const testBlockedUrl = "https://example.com/?test-blocked";
+
+ info(`Add an observer to suspend the next channel to ${testBlockedUrl}`);
+ const { promise, resolve } = Promise.withResolvers();
+ const onExamineResponse = subject => {
+ if (!(subject instanceof Ci.nsIHttpChannel)) {
+ return;
+ }
+
+ const channel = subject.QueryInterface(Ci.nsIHttpChannel);
+ if (channel.URI.displaySpec !== testBlockedUrl) {
+ return;
+ }
+
+ wrappedChannel = ChannelWrapper.get(channel);
+ wrappedChannel.suspend("test-blocked-suspend");
+ Services.obs.removeObserver(onExamineResponse, "http-on-examine-response");
+ resolve();
+ };
+ Services.obs.addObserver(onExamineResponse, "http-on-examine-response");
+
+ info(`Send fetch call for ${testBlockedUrl}`);
+ let first = fetch(tab.linkedBrowser, testBlockedUrl);
+
+ info(
+ "Wait for the fetch request to be suspended in http-on-examine-response"
+ );
+ await promise;
+
+ // eslint-disable-next-line mozilla/no-arbitrary-setTimeout
+ await new Promise(resolve => setTimeout(resolve, 100));
+
+ // Resume the channel to make sure we cancel timer
+ wrappedChannel.resume();
+
+ // eslint-disable-next-line mozilla/no-arbitrary-setTimeout
+ await new Promise(resolve => setTimeout(resolve, 1500));
+
+ info("Fetch the same URL again");
+ let secondCompleted = false;
+ let second = fetch(tab.linkedBrowser, testBlockedUrl).then(() => {
+ secondCompleted = true;
+ });
+
+ // eslint-disable-next-line mozilla/no-arbitrary-setTimeout
+ let timer = new Promise(resolve => setTimeout(resolve, 2000));
+ await Promise.race([second, timer]);
+
+ Assert.equal(
+ secondCompleted,
+ true,
+ "The second fetch should resolve successfully"
+ );
+ await first;
+
+ info("Cleanup");
+ gBrowser.removeTab(tab);
+});
