Bug 561276 - Cookie dependency on cache determination for image redirects r=bz

netwerk/protocol/http/HttpBaseChannel.cpp

@@ -76,6 +76,7 @@ HttpBaseChannel::HttpBaseChannel()
   , mChooseApplicationCache(PR_FALSE)
   , mLoadedFromApplicationCache(PR_FALSE)
   , mChannelIsForDownload(PR_FALSE)
+  , mRedirectedCachekeys(nsnull)
 {
   LOG(("Creating HttpBaseChannel @%x\n", this));
 
@@ -87,6 +88,9 @@ HttpBaseChannel::~HttpBaseChannel()
 {
   LOG(("Destroying HttpBaseChannel @%x\n", this));
 
+  // Make sure we don't leak
+  CleanRedirectCacheChainIfNecessary();
+
   gHttpHandler->Release();
 }
 
@@ -1166,6 +1170,13 @@ HttpBaseChannel::SetChannelIsForDownload(PRBool aChannelIsForDownload)
   return NS_OK;
 }
 
+NS_IMETHODIMP
+HttpBaseChannel::SetCacheKeysRedirectChain(nsTArray<nsCString> *cacheKeys)
+{
+  mRedirectedCachekeys = cacheKeys;
+  return NS_OK;
+}
+
 //-----------------------------------------------------------------------------
 // HttpBaseChannel::nsISupportsPriority
 //-----------------------------------------------------------------------------
@@ -1384,7 +1395,16 @@ HttpBaseChannel::SetupReplacementChannel(nsIURI       *newURI,
       httpInternal->SetDocumentURI(newURI);
     else
       httpInternal->SetDocumentURI(mDocumentURI);
-  } 
+
+    // if there is a chain of keys for redirect-responses we transfer it to
+    // the new channel (see bug #561276)
+    if (mRedirectedCachekeys) {
+        LOG(("HttpBaseChannel::SetupReplacementChannel "
+             "[this=%p] transferring chain of redirect cache-keys", this));
+        httpInternal->SetCacheKeysRedirectChain(mRedirectedCachekeys);
+        mRedirectedCachekeys = nsnull;
+    }
+  }
   
   // transfer application cache information
   nsCOMPtr<nsIApplicationCacheChannel> appCacheChannel =

netwerk/protocol/http/HttpBaseChannel.h

@@ -154,6 +154,14 @@ public:
   NS_IMETHOD GetCanceled(PRBool *aCanceled);
   NS_IMETHOD GetChannelIsForDownload(PRBool *aChannelIsForDownload);
   NS_IMETHOD SetChannelIsForDownload(PRBool aChannelIsForDownload);
+  NS_IMETHOD SetCacheKeysRedirectChain(nsTArray<nsCString> *cacheKeys);
+  inline void CleanRedirectCacheChainIfNecessary()
+  {
+      if (mRedirectedCachekeys) {
+          delete mRedirectedCachekeys;
+          mRedirectedCachekeys = nsnull;
+      }
+  }
 
   // nsISupportsPriority
   NS_IMETHOD GetPriority(PRInt32 *value);
@@ -250,6 +258,8 @@ protected:
   PRUint32                          mChooseApplicationCache     : 1;
   PRUint32                          mLoadedFromApplicationCache : 1;
   PRUint32                          mChannelIsForDownload       : 1;
+
+  nsTArray<nsCString>              *mRedirectedCachekeys;
 };
 
 

netwerk/protocol/http/nsHttpChannel.cpp

@@ -384,6 +384,9 @@ nsHttpChannel::DoNotifyListener()
     // We have to make sure to drop the reference to the callbacks too
     mCallbacks = nsnull;
     mProgressSink = nsnull;
+
+    // We don't need this info anymore
+    CleanRedirectCacheChainIfNecessary();
 }
 
 void
@@ -2637,12 +2640,27 @@ nsHttpChannel::CheckCache()
             (buf.IsEmpty() && mRequestHead.PeekHeader(nsHttp::Authorization));
     }
 
-    if (!doValidation) {
-        // Sites redirect back to the original URI after setting a session/tracking
-        // cookie. In such cases, force revalidation so that we hit the net and do not
-        // cycle thru cached responses.
-        if (isCachedRedirect && mRequestHead.PeekHeader(nsHttp::Cookie))
+    // Bug #561276: We maintain a chain of cache-keys which returns cached
+    // 3xx-responses (redirects) in order to detect cycles. If a cycle is
+    // found, ignore the cached response and hit the net. Otherwise, use
+    // the cached response and add the cache-key to the chain. Note that
+    // a limited number of redirects (cached or not) is allowed and is
+    // enforced independently of this mechanism
+    if (!doValidation && isCachedRedirect) {
+        nsCAutoString cacheKey;
+        GenerateCacheKey(mPostID, cacheKey);
+
+        if (!mRedirectedCachekeys)
+            mRedirectedCachekeys = new nsTArray<nsCString>();
+        else if (mRedirectedCachekeys->Contains(cacheKey))
             doValidation = PR_TRUE;
+
+        LOG(("Redirection-chain %s key %s\n",
+             doValidation ? "contains" : "does not contain", cacheKey.get()));
+
+        // Append cacheKey if not in the chain already
+        if (!doValidation)
+            mRedirectedCachekeys->AppendElement(cacheKey);
     }
 
     mCachedContentIsValid = !doValidation;
@@ -2678,6 +2696,9 @@ nsHttpChannel::CheckCache()
                 mRequestHead.SetHeader(nsHttp::If_None_Match,
                                        nsDependentCString(val));
         }
+
+        // We don't need this info anymore
+        CleanRedirectCacheChainIfNecessary();
     }
 
     LOG(("nsHTTPChannel::CheckCache exit [this=%p doValidation=%d]\n", this, doValidation));
@@ -4043,6 +4064,9 @@ nsHttpChannel::OnStopRequest(nsIRequest *request, nsISupports *ctxt, nsresult st
     if (mLoadGroup)
         mLoadGroup->RemoveRequest(this, nsnull, status);
 
+    // We don't need this info anymore
+    CleanRedirectCacheChainIfNecessary();
+
     mCallbacks = nsnull;
     mProgressSink = nsnull;
     

netwerk/protocol/http/nsIHttpChannelInternal.idl

@@ -37,6 +37,12 @@
 
 #include "nsISupports.idl"
 
+%{C++
+#include "nsTArray.h"
+class nsCString;
+%}
+[ptr] native StringArray(nsTArray<nsCString>);
+
 interface nsIURI;
 interface nsIProxyInfo;
 
@@ -45,7 +51,7 @@ interface nsIProxyInfo;
  * using any feature exposed by this interface, be aware that this interface 
  * will change and you will be broken.  You have been warned.
  */
-[scriptable, uuid(9fb2a161-d075-4bf2-b07a-26bac650cc81)]
+[scriptable, uuid(44e35ead-6656-4f9e-b51d-ebaf1bee6e2e)]
 interface nsIHttpChannelInternal : nsISupports
 {
     /**
@@ -93,4 +99,9 @@ interface nsIHttpChannelInternal : nsISupports
      * Lets externalhandler tell the channel it is open on behalf of a download
      */
     attribute boolean channelIsForDownload;
+
+    /**
+     * Transfer chain of redirected cache-keys 
+     */
+    [noscript] void setCacheKeysRedirectChain(in StringArray cacheKeys);
 };

netwerk/test/unit/test_bug561276.js

@@ -0,0 +1,74 @@
+//
+// Verify that we hit the net if we discover a cycle of redirects
+// coming from cache.
+//
+
+do_load_httpd_js();
+
+var httpserver = new nsHttpServer();
+var iteration = 0;
+
+function getCacheService()
+{
+    return Components.classes["@mozilla.org/network/cache-service;1"]
+           .getService(Components.interfaces.nsICacheService);
+}
+
+function setupChannel(suffix)
+{
+    var ios =
+        Components.classes["@mozilla.org/network/io-service;1"]
+        .getService(Ci.nsIIOService);
+    var chan = ios.newChannel("http://localhost:4444" + suffix, "", null);
+    var httpChan = chan.QueryInterface(Components.interfaces.nsIHttpChannel);
+    httpChan.requestMethod = "GET";
+    return httpChan;
+}
+
+function checkValueAndTrigger(request, data, ctx)
+{
+    do_check_eq("Ok", data);
+    httpserver.stop(do_test_finished);
+}
+
+function run_test()
+{
+    httpserver.registerPathHandler("/redirect1", redirectHandler1);
+    httpserver.registerPathHandler("/redirect2", redirectHandler2);
+    httpserver.start(4444);
+
+    // clear cache
+    getCacheService().
+        evictEntries(Components.interfaces.nsICache.STORE_ANYWHERE);
+
+    // load first time
+    var channel = setupChannel("/redirect1");
+    channel.asyncOpen(new ChannelListener(checkValueAndTrigger, null), null);
+
+    do_test_pending();
+}
+
+function redirectHandler1(metadata, response)
+{
+    // first time we return a cacheable 302 pointing to next redirect
+	if (iteration < 1) {
+	    response.setStatusLine(metadata.httpVersion, 302, "Found");
+	    response.setHeader("Cache-Control", "max-age=600", false);
+	    response.setHeader("Location", "/redirect2", false);
+
+	// next time called we return 200
+    } else {
+	    response.setStatusLine(metadata.httpVersion, 200, "Ok");
+	    response.setHeader("Cache-Control", "max-age=600", false);
+        response.setHeader("Content-Type", "text/plain");
+        response.bodyOutputStream.write("Ok", "Ok".length);
+    }
+	iteration += 1;
+}
+
+function redirectHandler2(metadata, response)
+{
+    response.setStatusLine(metadata.httpVersion, 302, "Found");
+    response.setHeader("Cache-Control", "max-age=600", false);
+    response.setHeader("Location", "/redirect1", false);
+}