Bug 579846 - nsIHttpChannel::SetResponseHeader should work after the stream has ended, r=bzbarsky+hsivonen, a=betaN+

netwerk/base/public/nsICacheInfoChannel.idl

@@ -59,3 +59,15 @@ interface nsICacheInfoChannel : nsISupports
    */
   boolean isFromCache();
 };
+
+[scriptable, uuid(746064fc-8783-4d19-9c5d-6361ed807b39)]
+interface nsICacheInfoChannel_GECKO_2_0 : 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;
+};

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_GECKO_2_0)
     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<nsHttpChannel> mChannel;
+};
+
+NS_IMPL_ISUPPORTS0(HttpChannelCacheEntryClosePreventer)
+
+NS_IMETHODIMP
+nsHttpChannel::GetCacheEntryClosePreventer(nsISupports** _retval)
+{
+    NS_ADDREF(*_retval = new HttpChannelCacheEntryClosePreventer(this));
+    return NS_OK;
+}
+
 //-----------------------------------------------------------------------------
 // nsHttpChannel::nsICachingChannel
 //-----------------------------------------------------------------------------

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_GECKO_2_0
                     , public nsICachingChannel
                     , public nsICacheListener
                     , public nsITransportEventSink
@@ -89,6 +92,7 @@ public:
     NS_DECL_ISUPPORTS_INHERITED
     NS_DECL_NSIREQUESTOBSERVER
     NS_DECL_NSISTREAMLISTENER
+    NS_DECL_NSICACHEINFOCHANNEL_GECKO_2_0
     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<nsIChannel>              mRedirectChannel;
     PRUint32                          mRedirectType;
 
+    // Hold counter, keeps the number of calls to holdCacheEntry(), 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 hold counter was
+    // positive.
+    PRUint32                          mDeferredCacheEntryClose  : 1;
+    // True if CloseCacheEntry was called with doomOnFailure set to TRUE.
+    PRUint32                          mDoomCacheEntryOnClose    : 1;
 
     nsTArray<nsContinueRedirectionFunc> mRedirectFuncStack;
 
@@ -354,6 +368,8 @@ private:
     nsresult WaitForRedirectCallback();
     void PushRedirectAsyncFunc(nsContinueRedirectionFunc func);
     void PopRedirectAsyncFunc(nsContinueRedirectionFunc func);
+
+    friend class HttpChannelCacheEntryClosePreventer;
 };
 
 #endif // nsHttpChannel_h__

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<nsICacheInfoChannel_GECKO_2_0> cacheInfoChannel =
+      do_QueryInterface(aRequest);
+  if (cacheInfoChannel) {
+    cacheInfoChannel->
+      GetCacheEntryClosePreventer(getter_AddRefs(mCacheEntryClosePreventer));
+  }
+
   mStreamState = STREAM_BEING_READ;
 
   PRBool scriptingEnabled = mExecutor->IsScriptEnabled();

parser/html/nsHtml5StreamParser.h

@@ -331,6 +331,7 @@ class nsHtml5StreamParser : public nsIStreamListener,
 
     nsCOMPtr<nsIRequest>          mRequest;
     nsCOMPtr<nsIRequestObserver>  mObserver;
+    nsCOMPtr<nsISupports>         mCacheEntryClosePreventer;
 
     /**
      * The Unicode decoder

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

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(
+    "<html>\n" +
+    "<head><meta http-equiv='Cache-control' content='no-cache'></head>\n" +
+    "<body onload='parent.testFrameOnLoad(document.body.innerHTML)'>" + now + "</body>" +
+    "</html>"
+  );
+}

parser/htmlparser/tests/mochitest/test_bug579846.html

@@ -0,0 +1,47 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=579846
+-->
+<head>
+  <title>Test for Bug 579846</title>
+  <script type="text/javascript" src="/MochiKit/packed.js"></script>
+  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
+  <script type="text/javascript">
+    SimpleTest.waitForExplicitFinish();
+
+    var nextAction;
+    function testFrameOnLoad(content)
+    {
+      nextAction(content);
+    }
+
+    nextAction = function(acontent)
+    {
+      nextAction = function(bcontent)
+      {
+        ok(parseInt(acontent) < parseInt(bcontent), "Content has changed");
+        SimpleTest.finish();
+      }
+
+      // Give a chance to update
+      window.setTimeout(function() {
+        var testFrame = document.getElementById('_test_iframe2');
+        testFrame.contentWindow.location.href = 'sub_bug579846.sjs';
+      }, 100);
+    }
+
+    window.onload = function()
+    {
+      var testFrame = document.getElementById('_test_iframe1');
+      testFrame.contentWindow.location.href = 'sub_bug579846.sjs';
+    }
+  </script>
+</head>
+<body>
+  <iframe src="" id="_test_iframe1"></iframe>
+  <iframe src="" id="_test_iframe2"></iframe>
+</body>
+</html>
+

toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_594477_clickable_output.js

@@ -4,7 +4,7 @@
  * http://creativecommons.org/publicdomain/zero/1.0/
  *
  * Contributor(s):
- *  Mihai Șucan <mihai.sucan@gmail.com>
+ *  Mihai   ucan <mihai.sucan@gmail.com>
  *
  * ***** END LICENSE BLOCK ***** */
 
@@ -27,7 +27,12 @@ function tabLoad1(aEvent) {
   browser.addEventListener("load", tabLoad2, true);
 
   // Reload so we get some output in the console.
-  browser.contentWindow.location.reload();
+  // Wait a little to let the cache entry for test-console.html be released,
+  // otherwise the network load wouldn't be logged in the console and test
+  // would fail and stuck in tabLoad2.  See bug 579846 comment 73.
+  executeSoon(function() {
+    browser.contentWindow.location.reload();
+  });
   log(document);
 }