Bug 770243: Close cache input stream only when we're sure we don't need it in nsHttpChannel, r=bsmith for Honza's code, r=honzab for bsmith's changes
authorBrian Smith <bsmith@mozilla.com>
Mon, 24 Sep 2012 10:20:11 -0700
changeset 111771 cbfe6a468a1202143916a4805736b9b73f51b335
parent 111770 e2da1a9124b654d16fa087e61440f5e7bb54be67
child 111772 1a989993c992618fe16f0339c37554ee68142075
push idunknown
push userunknown
push dateunknown
reviewersbsmith, honzab
bugs770243
milestone18.0a1
Bug 770243: Close cache input stream only when we're sure we don't need it in nsHttpChannel, r=bsmith for Honza's code, r=honzab for bsmith's changes
netwerk/protocol/http/nsHttpChannel.cpp
netwerk/test/unit/head_channels.js
netwerk/test/unit/test_bug770243.js
netwerk/test/unit/xpcshell.ini
--- a/netwerk/protocol/http/nsHttpChannel.cpp
+++ b/netwerk/protocol/http/nsHttpChannel.cpp
@@ -1182,19 +1182,16 @@ nsHttpChannel::ProcessResponse()
         // for Strict-Transport-Security.
     } else {
         // Given a successful connection, process any STS data that's relevant.
         rv = ProcessSTSHeader();
         NS_ASSERTION(NS_SUCCEEDED(rv), "ProcessSTSHeader failed, continuing load.");
     }
 
     MOZ_ASSERT(!mCachedContentIsValid);
-    if (httpStatus != 304 && httpStatus != 206) {
-        mCacheInputStream.CloseAndRelease();
-    }
 
     // notify "http-on-examine-response" observers
     gHttpHandler->OnExamineResponse(this);
 
     SetCookie(mResponseHead->PeekHeader(nsHttp::Set_Cookie));
 
     // handle unused username and password in url (see bug 232567)
     if (httpStatus != 401 && httpStatus != 407) {
@@ -3854,17 +3851,24 @@ nsHttpChannel::InstallCacheListener(uint
          mResponseHead->ContentType().EqualsLiteral(APPLICATION_ECMASCRIPT) ||
          mResponseHead->ContentType().EqualsLiteral(APPLICATION_XJAVASCRIPT) ||
          mResponseHead->ContentType().EqualsLiteral(APPLICATION_XHTML_XML))) {
         rv = mCacheEntry->SetMetaDataElement("uncompressed-len", "0"); 
         if (NS_FAILED(rv)) {
             LOG(("unable to mark cache entry for compression"));
         }
     } 
-      
+
+    LOG(("Trading cache input stream for output stream [channel=%p]", this));
+
+    // We must close the input stream first because cache entries do not
+    // correctly handle having an output stream and input streams open at
+    // the same time.
+    mCacheInputStream.CloseAndRelease();
+
     nsCOMPtr<nsIOutputStream> out;
     rv = mCacheEntry->OpenOutputStream(offset, getter_AddRefs(out));
     if (NS_FAILED(rv)) return rv;
 
     // XXX disk cache does not support overlapped i/o yet
 #if 0
     // Mark entry valid inorder to allow simultaneous reading...
     rv = mCacheEntry->MarkValid();
--- a/netwerk/test/unit/head_channels.js
+++ b/netwerk/test/unit/head_channels.js
@@ -22,16 +22,18 @@ function read_stream(stream, count) {
 }
 
 const CL_EXPECT_FAILURE = 0x1;
 const CL_EXPECT_GZIP = 0x2;
 const CL_EXPECT_3S_DELAY = 0x4;
 const CL_SUSPEND = 0x8;
 const CL_ALLOW_UNKNOWN_CL = 0x10;
 const CL_EXPECT_LATE_FAILURE = 0x20;
+const CL_FROM_CACHE = 0x40; // Response must be from the cache
+const CL_NOT_FROM_CACHE = 0x80; // Response must NOT be from the cache
 
 const SUSPEND_DELAY = 3000;
 
 /**
  * A stream listener that calls a callback function with a specified
  * context and the received data when the channel is loaded.
  *
  * Signature of the closure:
@@ -80,16 +82,29 @@ ChannelListener.prototype = {
       }
       catch (ex) {
         if (!(this._flags & (CL_EXPECT_FAILURE | CL_ALLOW_UNKNOWN_CL)))
           do_throw("Could not get contentLength");
       }
       if (this._contentLen == -1 && !(this._flags & (CL_EXPECT_FAILURE | CL_ALLOW_UNKNOWN_CL)))
         do_throw("Content length is unknown in onStartRequest!");
 
+      if ((this._flags & CL_FROM_CACHE)) {
+        request.QueryInterface(Ci.nsICachingChannel);
+        if (!request.isFromCache()) {
+          do_throw("Response is not from the cache (CL_FROM_CACHE)");
+        }
+      }
+      if ((this._flags & CL_NOT_FROM_CACHE)) {
+        request.QueryInterface(Ci.nsICachingChannel);
+        if (request.isFromCache()) {
+          do_throw("Response is from the cache (CL_NOT_FROM_CACHE)");
+        }
+      }
+
       if (this._flags & CL_SUSPEND) {
         request.suspend();
         do_timeout(SUSPEND_DELAY, function() { request.resume(); });
       }
 
     } catch (ex) {
       do_throw("Error in onStartRequest: " + ex);
     }
new file mode 100644
--- /dev/null
+++ b/netwerk/test/unit/test_bug770243.js
@@ -0,0 +1,211 @@
+/* this test does the following:
+ Always requests the same resource, while for each request getting:
+ 1. 200 + ETag: "one"
+ 2. 401 followed by 200 + ETag: "two"
+ 3. 401 followed by 304
+ 4. 407 followed by 200 + ETag: "three"
+ 5. 407 followed by 304
+*/
+
+const Cc = Components.classes;
+const Ci = Components.interfaces;
+const Cu = Components.utils;
+const Cr = Components.results;
+
+Cu.import("resource://testing-common/httpd.js");
+
+var httpserv;
+ 
+function addCreds(scheme, host)
+{
+  var authMgr = Components.classes['@mozilla.org/network/http-auth-manager;1']
+                          .getService(Ci.nsIHttpAuthManager);
+  authMgr.setAuthIdentity(scheme, host, 4444, "basic", "secret", "/", "", "user", "pass");
+}
+
+function clearCreds()
+{
+  var authMgr = Components.classes['@mozilla.org/network/http-auth-manager;1']
+                          .getService(Ci.nsIHttpAuthManager);
+  authMgr.clearAll();
+}
+
+function makeChan() {
+  var ios = Cc["@mozilla.org/network/io-service;1"]
+                      .getService(Ci.nsIIOService);
+  var chan = ios.newChannel("http://localhost:4444/", null, null)
+                .QueryInterface(Ci.nsIHttpChannel);
+  return chan;
+}
+
+// Array of handlers that are called one by one in response to expected requests
+
+var handlers = [
+  // Test 1
+  function(metadata, response) {
+    do_check_eq(metadata.hasHeader("Authorization"), false);
+    response.setStatusLine(metadata.httpVersion, 200, "OK");
+    response.setHeader("ETag", '"one"', false);
+    response.setHeader("Cache-control", 'no-cache', false);
+    response.setHeader("Content-type", 'text/plain', false);
+    var body = "Response body 1";
+    response.bodyOutputStream.write(body, body.length);
+  },
+
+  // Test 2
+  function(metadata, response) {
+    do_check_eq(metadata.hasHeader("Authorization"), false);
+    do_check_eq(metadata.getHeader("If-None-Match"), '"one"');
+    response.setStatusLine(metadata.httpVersion, 401, "Authenticate");
+    response.setHeader("WWW-Authenticate", 'Basic realm="secret"', false);
+    addCreds("http", "localhost");
+  },
+  function(metadata, response) {
+    do_check_eq(metadata.hasHeader("Authorization"), true);
+    response.setStatusLine(metadata.httpVersion, 200, "OK");
+    response.setHeader("ETag", '"two"', false);
+    response.setHeader("Cache-control", 'no-cache', false);
+    response.setHeader("Content-type", 'text/plain', false);
+    var body = "Response body 2";
+    response.bodyOutputStream.write(body, body.length);
+    clearCreds();
+  },
+  
+  // Test 3
+  function(metadata, response) {
+    do_check_eq(metadata.hasHeader("Authorization"), false);
+    do_check_eq(metadata.getHeader("If-None-Match"), '"two"');
+    response.setStatusLine(metadata.httpVersion, 401, "Authenticate");
+    response.setHeader("WWW-Authenticate", 'Basic realm="secret"', false);
+    addCreds("http", "localhost");
+  },
+  function(metadata, response) {
+    do_check_eq(metadata.hasHeader("Authorization"), true);
+    do_check_eq(metadata.getHeader("If-None-Match"), '"two"');
+    response.setStatusLine(metadata.httpVersion, 304, "OK");
+    response.setHeader("ETag", '"two"', false);
+    clearCreds();
+  },
+  
+  // Test 4
+  function(metadata, response) {
+    do_check_eq(metadata.hasHeader("Authorization"), false);
+    do_check_eq(metadata.getHeader("If-None-Match"), '"two"');
+    response.setStatusLine(metadata.httpVersion, 407, "Proxy Authenticate");
+    response.setHeader("Proxy-Authenticate", 'Basic realm="secret"', false);
+    addCreds("http", "localhost");
+  },
+  function(metadata, response) {
+    do_check_eq(metadata.hasHeader("Proxy-Authorization"), true);
+    do_check_eq(metadata.getHeader("If-None-Match"), '"two"');
+    response.setStatusLine(metadata.httpVersion, 200, "OK");
+    response.setHeader("ETag", '"three"', false);
+    response.setHeader("Cache-control", 'no-cache', false);
+    response.setHeader("Content-type", 'text/plain', false);
+    var body = "Response body 3";
+    response.bodyOutputStream.write(body, body.length);
+    clearCreds();
+  }, 
+  
+  // Test 5
+  function(metadata, response) {
+    do_check_eq(metadata.hasHeader("Proxy-Authorization"), false);
+    do_check_eq(metadata.getHeader("If-None-Match"), '"three"');
+    response.setStatusLine(metadata.httpVersion, 407, "Proxy Authenticate");
+    response.setHeader("Proxy-Authenticate", 'Basic realm="secret"', false);
+    addCreds("http", "localhost");
+  },
+  function(metadata, response) {
+    do_check_eq(metadata.hasHeader("Proxy-Authorization"), true);
+    do_check_eq(metadata.getHeader("If-None-Match"), '"three"');
+    response.setStatusLine(metadata.httpVersion, 304, "OK");
+    response.setHeader("ETag", '"three"', false);
+    response.setHeader("Cache-control", 'no-cache', false);
+    clearCreds();
+  }
+];
+
+function handler(metadata, response) 
+{
+  handlers.shift()(metadata, response); 
+}
+
+// Array of tests to run, self-driven
+
+function sync_and_run_next_test()
+{
+  syncWithCacheIOThread(function() {
+    tests.shift()();
+  });
+}
+
+var tests = [
+  // Test 1: 200 (cacheable)
+  function() {
+    var ch = makeChan();
+    ch.asyncOpen(new ChannelListener(function(req, body) {
+      do_check_eq(body, "Response body 1");
+      sync_and_run_next_test();
+    }, null, CL_NOT_FROM_CACHE), null);
+  },
+
+  // Test 2: 401 and 200 + new content
+  function() {
+    var ch = makeChan();
+    ch.asyncOpen(new ChannelListener(function(req, body) {
+      do_check_eq(body, "Response body 2");
+      sync_and_run_next_test();
+    }, null, CL_NOT_FROM_CACHE), null);
+  },
+  
+  // Test 3: 401 and 304
+  function() {
+    var ch = makeChan();
+    ch.asyncOpen(new ChannelListener(function(req, body) {
+      do_check_eq(body, "Response body 2");
+      sync_and_run_next_test();
+    }, null, CL_FROM_CACHE), null);
+  },
+  
+  // Test 4: 407 and 200 + new content
+  function() {
+    var ch = makeChan();
+    ch.asyncOpen(new ChannelListener(function(req, body) {
+      do_check_eq(body, "Response body 3");
+      sync_and_run_next_test();
+    }, null, CL_NOT_FROM_CACHE), null);
+  },
+  
+  // Test 5: 407 and 304
+  function() {
+    var ch = makeChan();
+    ch.asyncOpen(new ChannelListener(function(req, body) {
+      do_check_eq(body, "Response body 3");
+      sync_and_run_next_test();
+    }, null, CL_FROM_CACHE), null);
+  },
+
+  // End of test run
+  function() {
+    httpserv.stop(do_test_finished);
+  }
+];
+
+function run_test()
+{
+  do_get_profile();
+  
+  httpserv = new HttpServer();
+  httpserv.registerPathHandler("/", handler);
+  httpserv.start(4444);
+
+  const prefs = Cc["@mozilla.org/preferences-service;1"]
+                         .getService(Ci.nsIPrefBranch);
+  prefs.setCharPref("network.proxy.http", "localhost");
+  prefs.setIntPref("network.proxy.http_port", 4444);
+  prefs.setCharPref("network.proxy.no_proxies_on", "");
+  prefs.setIntPref("network.proxy.type", 1);
+
+  tests.shift()();
+  do_test_pending();
+}
--- a/netwerk/test/unit/xpcshell.ini
+++ b/netwerk/test/unit/xpcshell.ini
@@ -82,16 +82,17 @@ fail-if = os == "android"
 # Bug 675049: test fails consistently on Android
 fail-if = os == "android"
 [test_bug659569.js]
 [test_bug660066.js]
 [test_bug667907.js]
 [test_bug667818.js]
 [test_bug669001.js]
 [test_bug712914_secinfo_validation.js]
+[test_bug770243.js]
 [test_doomentry.js]
 [test_cacheflags.js]
 [test_channel_close.js]
 [test_compareURIs.js]
 [test_compressappend.js]
 [test_content_encoding_gzip.js]
 [test_content_sniffer.js]
 [test_cookie_header.js]