From 9ea9773c0f58183aaa2a6e0999383f3234437223 Mon Sep 17 00:00:00 2001 From: Honza Bambas Date: Sun, 16 Jan 2011 17:58:49 +0100 Subject: [PATCH] Bug 579846 - nsIHttpChannel::SetResponseHeader should work after the stream has ended, r=bzbarsky, a=betaN+ --- netwerk/base/public/nsICacheInfoChannel.idl | 12 +++ netwerk/protocol/http/nsHttpChannel.cpp | 80 +++++++++++++++++-- netwerk/protocol/http/nsHttpChannel.h | 18 ++++- parser/html/nsHtml5StreamParser.cpp | 13 +++ parser/html/nsHtml5StreamParser.h | 1 + parser/htmlparser/tests/mochitest/Makefile.in | 2 + .../tests/mochitest/sub_bug579846.sjs | 17 ++++ .../tests/mochitest/test_bug579846.html | 47 +++++++++++ 8 files changed, 181 insertions(+), 9 deletions(-) create mode 100644 parser/htmlparser/tests/mochitest/sub_bug579846.sjs create mode 100644 parser/htmlparser/tests/mochitest/test_bug579846.html diff --git a/netwerk/base/public/nsICacheInfoChannel.idl b/netwerk/base/public/nsICacheInfoChannel.idl index 3863c408423d..ad78762cfa12 100644 --- a/netwerk/base/public/nsICacheInfoChannel.idl +++ b/netwerk/base/public/nsICacheInfoChannel.idl @@ -59,3 +59,15 @@ interface nsICacheInfoChannel : nsISupports */ boolean isFromCache(); }; + +[scriptable, uuid(746064fc-8783-4d19-9c5d-6361ed807b39)] +interface nsICacheInfoChannel_MOZILLA_2_0_BRANCH : nsISupports +{ + /** + * Return an object that while not released prevents the channel from + * releasing the cache entry after all work on it has been done. Used by + * asynchronous consumers that needs to work with the cache entry long after + * onStopRequest has been called. + */ + readonly attribute nsISupports cacheEntryClosePreventer; +}; diff --git a/netwerk/protocol/http/nsHttpChannel.cpp b/netwerk/protocol/http/nsHttpChannel.cpp index 15a11916afe6..94f43151ced9 100644 --- a/netwerk/protocol/http/nsHttpChannel.cpp +++ b/netwerk/protocol/http/nsHttpChannel.cpp @@ -121,6 +121,7 @@ nsHttpChannel::nsHttpChannel() , mAsyncCacheOpen(PR_FALSE) , mPendingAsyncCallOnResume(nsnull) , mSuspendCount(0) + , mCacheEntryClosePreventionCount(0) , mCachedContentIsValid(PR_FALSE) , mCachedContentIsPartial(PR_FALSE) , mTransactionReplaced(PR_FALSE) @@ -135,6 +136,8 @@ nsHttpChannel::nsHttpChannel() , mFallingBack(PR_FALSE) , mWaitingForRedirectCallback(PR_FALSE) , mRequestTimeInitialized(PR_FALSE) + , mDeferredCacheEntryClose(PR_FALSE) + , mDoomCacheEntryOnClose(PR_FALSE) { LOG(("Creating nsHttpChannel [this=%p]\n", this)); } @@ -2828,12 +2831,34 @@ nsHttpChannel::ReadFromCache() void nsHttpChannel::CloseCacheEntry(PRBool doomOnFailure) { - if (!mCacheEntry) + if (!mCacheEntry || mDeferredCacheEntryClose) return; LOG(("nsHttpChannel::CloseCacheEntry [this=%p] mStatus=%x mCacheAccess=%x", this, mStatus, mCacheAccess)); + mDoomCacheEntryOnClose = doomOnFailure; + + if (mCacheEntryClosePreventionCount) { + LOG((" close is on hold")); + mDeferredCacheEntryClose = PR_TRUE; + return; + } + + CloseCacheEntryInternal(); +} + +void +nsHttpChannel::CloseCacheEntryInternal() +{ + LOG(("nsHttpChannel::CloseCacheEntryInternal [this=%p] mStatus=%x mCacheAccess=%x", + this, mStatus, mCacheAccess)); + + // perform any final cache operations before we close the cache entry. + if ((mCacheAccess & nsICache::ACCESS_WRITE) && mRequestTimeInitialized) { + FinalizeCacheEntry(); + } + // If we have begun to create or replace a cache entry, and that cache // entry is not complete and not resumable, then it needs to be doomed. // Otherwise, CheckCache will make the mistake of thinking that the @@ -2842,7 +2867,7 @@ nsHttpChannel::CloseCacheEntry(PRBool doomOnFailure) PRBool doom = PR_FALSE; if (mInitedCacheEntry) { NS_ASSERTION(mResponseHead, "oops"); - if (NS_FAILED(mStatus) && doomOnFailure && + if (NS_FAILED(mStatus) && mDoomCacheEntryOnClose && (mCacheAccess & nsICache::ACCESS_WRITE) && !mResponseHead->IsResumable()) doom = PR_TRUE; @@ -3490,6 +3515,7 @@ NS_INTERFACE_MAP_BEGIN(nsHttpChannel) NS_INTERFACE_MAP_ENTRY(nsIRequestObserver) NS_INTERFACE_MAP_ENTRY(nsIStreamListener) NS_INTERFACE_MAP_ENTRY(nsIHttpChannel) + NS_INTERFACE_MAP_ENTRY(nsICacheInfoChannel_MOZILLA_2_0_BRANCH) NS_INTERFACE_MAP_ENTRY(nsICacheInfoChannel) NS_INTERFACE_MAP_ENTRY(nsICachingChannel) NS_INTERFACE_MAP_ENTRY(nsIUploadChannel) @@ -4019,12 +4045,6 @@ nsHttpChannel::OnStopRequest(nsIRequest *request, nsISupports *ctxt, nsresult st mIsPending = PR_FALSE; mStatus = status; - // perform any final cache operations before we close the cache entry. - if (mCacheEntry && (mCacheAccess & nsICache::ACCESS_WRITE) && - mRequestTimeInitialized){ - FinalizeCacheEntry(); - } - if (mListener) { LOG((" calling OnStopRequest\n")); mListener->OnStopRequest(this, mListenerContext, status); @@ -4207,6 +4227,50 @@ nsHttpChannel::SetCacheTokenCachedCharset(const nsACString &aCharset) PromiseFlatCString(aCharset).get()); } +//----------------------------------------------------------------------------- +// nsHttpChannel::nsIHttpChannelInternal_GECKO_2_0 +//----------------------------------------------------------------------------- + +class HttpChannelCacheEntryClosePreventer : public nsISupports +{ +public: + NS_DECL_ISUPPORTS + + HttpChannelCacheEntryClosePreventer(nsHttpChannel* channel) + : mChannel(channel) + { + ++mChannel->mCacheEntryClosePreventionCount; + LOG(("mCacheEntryClosePreventionCount increased to %d, [this=%x]", + mChannel->mCacheEntryClosePreventionCount, + mChannel.get())); + } + +private: + ~HttpChannelCacheEntryClosePreventer() + { + --mChannel->mCacheEntryClosePreventionCount; + LOG(("mCacheEntryClosePreventionCount decreased to %d, [this=%x]", + mChannel->mCacheEntryClosePreventionCount, + mChannel.get())); + + if (!mChannel->mCacheEntryClosePreventionCount && + mChannel->mDeferredCacheEntryClose) { + mChannel->CloseCacheEntryInternal(); + } + } + + nsRefPtr mChannel; +}; + +NS_IMPL_ISUPPORTS0(HttpChannelCacheEntryClosePreventer) + +NS_IMETHODIMP +nsHttpChannel::GetCacheEntryClosePreventer(nsISupports** _retval) +{ + NS_ADDREF(*_retval = new HttpChannelCacheEntryClosePreventer(this)); + return NS_OK; +} + //----------------------------------------------------------------------------- // nsHttpChannel::nsICachingChannel //----------------------------------------------------------------------------- diff --git a/netwerk/protocol/http/nsHttpChannel.h b/netwerk/protocol/http/nsHttpChannel.h index fcf013bfe0b7..f07b87a9177f 100644 --- a/netwerk/protocol/http/nsHttpChannel.h +++ b/netwerk/protocol/http/nsHttpChannel.h @@ -51,6 +51,7 @@ #include "nsTArray.h" #include "nsIHttpEventSink.h" +#include "nsICacheInfoChannel.h" #include "nsICachingChannel.h" #include "nsICacheEntryDescriptor.h" #include "nsICacheListener.h" @@ -67,6 +68,7 @@ class nsAHttpConnection; class AutoRedirectVetoNotifier; +class HttpChannelCacheEntryClosePreventer; using namespace mozilla::net; @@ -76,6 +78,7 @@ using namespace mozilla::net; class nsHttpChannel : public HttpBaseChannel , public nsIStreamListener + , public nsICacheInfoChannel_MOZILLA_2_0_BRANCH , public nsICachingChannel , public nsICacheListener , public nsITransportEventSink @@ -89,6 +92,7 @@ public: NS_DECL_ISUPPORTS_INHERITED NS_DECL_NSIREQUESTOBSERVER NS_DECL_NSISTREAMLISTENER + NS_DECL_NSICACHEINFOCHANNEL_MOZILLA_2_0_BRANCH NS_DECL_NSICACHEINFOCHANNEL NS_DECL_NSICACHINGCHANNEL NS_DECL_NSICACHELISTENER @@ -229,6 +233,7 @@ private: nsresult ShouldUpdateOfflineCacheEntry(PRBool *shouldCacheForOfflineUse); nsresult ReadFromCache(); void CloseCacheEntry(PRBool doomOnFailure); + void CloseCacheEntryInternal(); void CloseOfflineCacheEntry(); nsresult InitCacheEntry(); nsresult InitOfflineCacheEntry(); @@ -322,6 +327,10 @@ private: nsCOMPtr mRedirectChannel; PRUint32 mRedirectType; + // Close prevention count, keeps the number of existing prevention objects, + // positive value prevents the cache entry from release in OnStopRequest. + PRUint32 mCacheEntryClosePreventionCount; + // state flags PRUint32 mCachedContentIsValid : 1; PRUint32 mCachedContentIsPartial : 1; @@ -345,7 +354,12 @@ private: PRUint32 mWaitingForRedirectCallback : 1; // True if mRequestTime has been set. In such a case it is safe to update // the cache entry's expiration time. Otherwise, it is not(see bug 567360). - PRUint32 mRequestTimeInitialized : 1; + PRUint32 mRequestTimeInitialized : 1; + // True if CloseCacheEntry was called while cache entry close prevention + // count was positive. + PRUint32 mDeferredCacheEntryClose : 1; + // True if CloseCacheEntry was called with doomOnFailure set to TRUE. + PRUint32 mDoomCacheEntryOnClose : 1; nsTArray mRedirectFuncStack; @@ -354,6 +368,8 @@ private: nsresult WaitForRedirectCallback(); void PushRedirectAsyncFunc(nsContinueRedirectionFunc func); void PopRedirectAsyncFunc(nsContinueRedirectionFunc func); + + friend class HttpChannelCacheEntryClosePreventer; }; #endif // nsHttpChannel_h__ diff --git a/parser/html/nsHtml5StreamParser.cpp b/parser/html/nsHtml5StreamParser.cpp index c97476e78395..8b3dc30d4bb3 100644 --- a/parser/html/nsHtml5StreamParser.cpp +++ b/parser/html/nsHtml5StreamParser.cpp @@ -41,6 +41,7 @@ #include "nsHtml5StreamParser.h" #include "nsICharsetConverterManager.h" #include "nsICharsetAlias.h" +#include "nsICacheInfoChannel.h" #include "nsServiceManagerUtils.h" #include "nsEncoderDecoderUtils.h" #include "nsContentUtils.h" @@ -220,6 +221,7 @@ nsHtml5StreamParser::~nsHtml5StreamParser() mTokenizer->end(); NS_ASSERTION(!mFlushTimer, "Flush timer was not dropped before dtor!"); #ifdef DEBUG + mCacheEntryClosePreventer = nsnull; mRequest = nsnull; mObserver = nsnull; mUnicodeDecoder = nsnull; @@ -558,6 +560,17 @@ nsHtml5StreamParser::OnStartRequest(nsIRequest* aRequest, nsISupports* aContext) } mRequest = aRequest; + // We must keep the cache entry hold lock to prevent the channel from + // dropping the cache entry after OnStopRequest. We may need to modify + // the cache entry asynchronously, after OnStopRequest. + // See bug 579846. + nsCOMPtr cacheInfoChannel = + do_QueryInterface(aRequest); + if (cacheInfoChannel) { + cacheInfoChannel-> + GetCacheEntryClosePreventer(getter_AddRefs(mCacheEntryClosePreventer)); + } + mStreamState = STREAM_BEING_READ; PRBool scriptingEnabled = mExecutor->IsScriptEnabled(); diff --git a/parser/html/nsHtml5StreamParser.h b/parser/html/nsHtml5StreamParser.h index 8c2a9010ef28..db0030f84af6 100644 --- a/parser/html/nsHtml5StreamParser.h +++ b/parser/html/nsHtml5StreamParser.h @@ -331,6 +331,7 @@ class nsHtml5StreamParser : public nsIStreamListener, nsCOMPtr mRequest; nsCOMPtr mObserver; + nsCOMPtr mCacheEntryClosePreventer; /** * The Unicode decoder diff --git a/parser/htmlparser/tests/mochitest/Makefile.in b/parser/htmlparser/tests/mochitest/Makefile.in index 3d4f3d203bfb..2fcad28dc641 100644 --- a/parser/htmlparser/tests/mochitest/Makefile.in +++ b/parser/htmlparser/tests/mochitest/Makefile.in @@ -84,6 +84,8 @@ _TEST_FILES = parser_datreader.js \ file_bug594730-9.html \ test_bug599584.html \ iframe_bug599584.html \ + test_bug579846.html \ + sub_bug579846.sjs \ $(NULL) # Disabled test due to orange on Linux diff --git a/parser/htmlparser/tests/mochitest/sub_bug579846.sjs b/parser/htmlparser/tests/mochitest/sub_bug579846.sjs new file mode 100644 index 000000000000..a926414741c4 --- /dev/null +++ b/parser/htmlparser/tests/mochitest/sub_bug579846.sjs @@ -0,0 +1,17 @@ +function handleRequest(request, response) +{ + var date = new Date(); + var now = date.getTime(); + date.setTime(date.getTime() + 5 * 60 * 1000); + var exp = (date).toGMTString(); + + response.setHeader("Content-Type", "text/html", false); + response.setHeader("Expires", exp, false); + + response.write( + "\n" + + "\n" + + "" + now + "" + + "" + ); +} diff --git a/parser/htmlparser/tests/mochitest/test_bug579846.html b/parser/htmlparser/tests/mochitest/test_bug579846.html new file mode 100644 index 000000000000..bb36aaa0e5f0 --- /dev/null +++ b/parser/htmlparser/tests/mochitest/test_bug579846.html @@ -0,0 +1,47 @@ + + + + + Test for Bug 579846 + + + + + + + + + + +