Bug 761228: Fix 304 response handling for custom conditional responses to prevent crash, r=jduell

--HG--
extra : rebase_source : 26494ad6bf23c3290f74fdd6ffc21a1db0b7f429

netwerk/protocol/http/nsHttpChannel.cpp

@@ -2140,8 +2140,15 @@ nsHttpChannel::ProcessNotModified()
         return NS_ERROR_FAILURE;
     }
 
-    NS_ENSURE_TRUE(mCachedResponseHead, NS_ERROR_NOT_INITIALIZED);
-    NS_ENSURE_TRUE(mCacheEntry, NS_ERROR_NOT_INITIALIZED);
+    if (!mDidReval) {
+        LOG(("Server returned a 304 response even though we did not send a "
+             "conditional request"));
+        return NS_ERROR_FAILURE;
+    }
+
+    MOZ_ASSERT(mCachedResponseHead);
+    MOZ_ASSERT(mCacheEntry);
+    NS_ENSURE_TRUE(mCachedResponseHead && mCacheEntry, NS_ERROR_UNEXPECTED);
 
     // If the 304 response contains a Last-Modified different than the
     // one in our cache that is pretty suspicious and is, in at least the
@@ -2915,7 +2922,16 @@ HttpCacheQuery::CheckCache()
 
     LOG(("HttpCacheQuery::CheckCache enter [channel=%p entry=%p access=%d]",
         mChannel.get(), mCacheEntry.get(), mCacheAccess));
-    
+
+    // Remember the request is a custom conditional request so that we can
+    // process any 304 response correctly.
+    mCustomConditionalRequest =
+        mRequestHead.PeekHeader(nsHttp::If_Modified_Since) ||
+        mRequestHead.PeekHeader(nsHttp::If_None_Match) ||
+        mRequestHead.PeekHeader(nsHttp::If_Unmodified_Since) ||
+        mRequestHead.PeekHeader(nsHttp::If_Match) ||
+        mRequestHead.PeekHeader(nsHttp::If_Range);
+
     // Be pessimistic: assume the cache entry has no useful data.
     mCachedContentIsValid = false;
 
@@ -2983,13 +2999,6 @@ HttpCacheQuery::CheckCache()
         return rv;
     }
 
-    mCustomConditionalRequest =
-        mRequestHead.PeekHeader(nsHttp::If_Modified_Since) ||
-        mRequestHead.PeekHeader(nsHttp::If_None_Match) ||
-        mRequestHead.PeekHeader(nsHttp::If_Unmodified_Since) ||
-        mRequestHead.PeekHeader(nsHttp::If_Match) ||
-        mRequestHead.PeekHeader(nsHttp::If_Range);
-
     if (method != nsHttp::Head && !isCachedRedirect) {
         // If the cached content-length is set and it does not match the data
         // size of the cached content, then the cached response is partial...

netwerk/test/unit/test_304_responses.js

@@ -0,0 +1,86 @@
+"use strict";
+// https://bugzilla.mozilla.org/show_bug.cgi?id=761228
+
+do_load_httpd_js();
+
+var httpServer = null;
+const testFileName = "test_customConditionalRequest_304";
+const basePath = "/" + testFileName + "/";
+const baseURI = "http://localhost:4444" + basePath;
+const unexpected304 = "unexpected304";
+const existingCached304 = "existingCached304";
+
+function make_uri(url) {
+  var ios = Cc["@mozilla.org/network/io-service;1"].
+            getService(Ci.nsIIOService);
+  return ios.newURI(url, null, null);
+}
+
+function make_channel(url) {
+  var ios = Cc["@mozilla.org/network/io-service;1"].getService(Ci.nsIIOService);
+  var chan = ios.newChannel(url, null, null).QueryInterface(Ci.nsIHttpChannel);
+  return chan;
+}
+
+function clearCache() {
+    var service = Components.classes["@mozilla.org/network/cache-service;1"]
+        .getService(Ci.nsICacheService);
+    service.evictEntries(Ci.nsICache.STORE_ANYWHERE);
+}
+
+function alwaysReturn304Handler(metadata, response) {
+  response.setStatusLine(metadata.httpVersion, 304, "Not Modified");
+  response.setHeader("Returned-From-Handler", "1");
+}
+
+function run_test() {
+  evict_cache_entries();
+
+  httpServer = new nsHttpServer();
+  httpServer.registerPathHandler(basePath + unexpected304,
+                                 alwaysReturn304Handler);
+  httpServer.registerPathHandler(basePath + existingCached304,
+                                 alwaysReturn304Handler);
+  httpServer.start(4444);
+  run_next_test();
+}
+
+function finish_test(request, buffer) {
+  httpServer.stop(do_test_finished);
+}
+
+function consume304(request, buffer) {
+  do_check_eq(request.responseStatus, 304);
+  do_check_eq(request.getResponseHeader("Returned-From-Handler"), "1");
+  run_next_test();
+}
+
+// Test that we return a 304 response to the caller when we are not expecting
+// a 304 response (i.e. when the server shouldn't have sent us one).
+add_test(function test_unexpected_304() {
+  var chan = make_channel(baseURI + unexpected304);
+  chan.asyncOpen(new ChannelListener(consume304, null), null);
+});
+
+// Test that we can cope with a 304 response that was (erroneously) stored in
+// the cache.
+add_test(function test_304_stored_in_cache() {
+  asyncOpenCacheEntry(
+    baseURI + existingCached304, "HTTP",
+    Ci.nsICache.STORE_ANYWHERE, Ci.nsICache.ACCESS_READ_WRITE,
+    function (entryStatus, cacheEntry) {
+      cacheEntry.setMetaDataElement("request-method", "GET");
+      cacheEntry.setMetaDataElement("response-head",
+                                    "HTTP/1.1 304 Not Modified\r\n" +
+                                    "\r\n");
+      cacheEntry.close();
+
+      var chan = make_channel(baseURI + existingCached304);
+
+      // make it a custom conditional request
+      chan.QueryInterface(Components.interfaces.nsIHttpChannel);
+      chan.setRequestHeader("If-None-Match", '"foo"', false);
+
+      chan.asyncOpen(new ChannelListener(consume304, null), null);
+    });
+});

netwerk/test/unit/xpcshell.ini

@@ -2,6 +2,7 @@
 head = head_channels.js head_cache.js
 tail =
 
+[test_304_responses.js]
 [test_cacheForOfflineUse_no-store.js]
 [test_307_redirect.js]
 [test_NetUtil.js]