Bug 1179560 - Some 421 retries dont work. r=hurley, a=sledru
authorPatrick McManus <mcmanus@ducksong.com>
Thu, 02 Jul 2015 10:34:01 -0400
changeset 273793 2e5f12e03d3673295a92a903f7fd2dfb39be9dca
parent 273792 25e950d0b04102f8d37ce7f19b1bd752c4a29035
child 273794 d28c97be9b85e0495001ad63f172c7ab1ea11959
push id4867
push userryanvm@gmail.com
push dateMon, 13 Jul 2015 18:55:25 +0000
treeherdermozilla-beta@d23402a8262f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershurley, sledru
bugs1179560
milestone40.0
Bug 1179560 - Some 421 retries dont work. r=hurley, a=sledru
netwerk/protocol/http/nsHttpConnection.cpp
netwerk/protocol/http/nsHttpTransaction.cpp
netwerk/test/unit/test_421.js
netwerk/test/unit/xpcshell.ini
--- a/netwerk/protocol/http/nsHttpConnection.cpp
+++ b/netwerk/protocol/http/nsHttpConnection.cpp
@@ -1460,18 +1460,19 @@ nsHttpConnection::CloseTransaction(nsAHt
         mTransaction = nullptr;
     }
 
     {
         MutexAutoLock lock(mCallbacksLock);
         mCallbacks = nullptr;
     }
 
-    if (NS_FAILED(reason))
+    if (NS_FAILED(reason) && (reason != NS_BINDING_RETARGETED)) {
         Close(reason);
+    }
 
     // flag the connection as reused here for convenience sake.  certainly
     // it might be going away instead ;-)
     mIsReused = true;
 }
 
 NS_METHOD
 nsHttpConnection::ReadFromStream(nsIInputStream *input,
--- a/netwerk/protocol/http/nsHttpTransaction.cpp
+++ b/netwerk/protocol/http/nsHttpTransaction.cpp
@@ -685,16 +685,26 @@ nsHttpTransaction::ReadSegments(nsAHttpS
     }
 
     mReader = reader;
 
     nsresult rv = mRequestStream->ReadSegments(ReadRequestSegment, this, count, countRead);
 
     mReader = nullptr;
 
+    if (mForceRestart) {
+        // The forceRestart condition was dealt with on the stack, but it did not
+        // clear the flag because nsPipe in the readsegment stack clears out
+        // return codes, so we need to use the flag here as a cue to return ERETARGETED
+        if (NS_SUCCEEDED(rv)) {
+            rv = NS_BINDING_RETARGETED;
+        }
+        mForceRestart = false;
+    }
+
     // if read would block then we need to AsyncWait on the request stream.
     // have callback occur on socket thread so we stay synchronized.
     if (rv == NS_BASE_STREAM_WOULD_BLOCK) {
         nsCOMPtr<nsIAsyncInputStream> asyncIn =
                 do_QueryInterface(mRequestStream);
         if (asyncIn) {
             nsCOMPtr<nsIEventTarget> target;
             gHttpHandler->GetSocketThreadTarget(getter_AddRefs(target));
@@ -779,16 +789,26 @@ nsHttpTransaction::WriteSegments(nsAHttp
     if (!mPipeOut) {
         return NS_ERROR_UNEXPECTED;
     }
 
     nsresult rv = mPipeOut->WriteSegments(WritePipeSegment, this, count, countWritten);
 
     mWriter = nullptr;
 
+    if (mForceRestart) {
+        // The forceRestart condition was dealt with on the stack, but it did not
+        // clear the flag because nsPipe in the writesegment stack clears out
+        // return codes, so we need to use the flag here as a cue to return ERETARGETED
+        if (NS_SUCCEEDED(rv)) {
+            rv = NS_BINDING_RETARGETED;
+        }
+        mForceRestart = false;
+    }
+
     // if pipe would block then we need to AsyncWait on it.  have callback
     // occur on socket thread so we stay synchronized.
     if (rv == NS_BASE_STREAM_WOULD_BLOCK) {
         nsCOMPtr<nsIEventTarget> target;
         gHttpHandler->GetSocketThreadTarget(getter_AddRefs(target));
         if (target)
             mPipeOut->AsyncWait(this, 0, 0, target);
         else {
@@ -840,16 +860,20 @@ nsHttpTransaction::SaveNetworkStats(bool
 }
 
 void
 nsHttpTransaction::Close(nsresult reason)
 {
     LOG(("nsHttpTransaction::Close [this=%p reason=%x]\n", this, reason));
 
     MOZ_ASSERT(PR_GetCurrentThread() == gSocketThread);
+    if (reason == NS_BINDING_RETARGETED) {
+        LOG(("  close %p skipped due to ERETARGETED\n", this));
+        return;
+    }
 
     if (mClosed) {
         LOG(("  already closed\n"));
         return;
     }
 
     if (mTokenBucketCancel) {
         mTokenBucketCancel->Cancel(reason);
@@ -898,16 +922,31 @@ nsHttpTransaction::Close(nsresult reason
     // NOTE: because of the way SSL proxy CONNECT is implemented, it is
     // possible that the transaction may have received data without having
     // sent any data.  for this reason, mSendData == FALSE does not imply
     // mReceivedData == FALSE.  (see bug 203057 for more info.)
     //
     if (reason == NS_ERROR_NET_RESET || reason == NS_OK) {
 
         if (mForceRestart && NS_SUCCEEDED(Restart())) {
+            if (mResponseHead) {
+                mResponseHead->Reset();
+            }
+            mContentRead = 0;
+            mContentLength = -1;
+            delete mChunkedDecoder;
+            mChunkedDecoder = nullptr;
+            mHaveStatusLine = false;
+            mHaveAllHeaders = false;
+            mHttpResponseMatched = false;
+            mResponseIsComplete = false;
+            mDidContentStart = false;
+            mNoContent = false;
+            mSentData = false;
+            mReceivedData = false;
             LOG(("transaction force restarted\n"));
             return;
         }
 
         // reallySentData is meant to separate the instances where data has
         // been sent by this transaction but buffered at a higher level while
         // a TLS session (perhaps via a tunnel) is setup.
         bool reallySentData =
@@ -1197,17 +1236,16 @@ nsHttpTransaction::Restart()
         MutexAutoLock lock(*nsHttp::GetLock());
         nsRefPtr<nsHttpConnectionInfo> ci;
          mConnInfo->CloneAsDirectRoute(getter_AddRefs(ci));
          mConnInfo = ci;
         if (mRequestHead) {
             mRequestHead->SetHeader(nsHttp::Alternate_Service_Used, NS_LITERAL_CSTRING("0"));
         }
     }
-    mForceRestart = false;
 
     return gHttpHandler->InitiateTransaction(this, mPriority);
 }
 
 char *
 nsHttpTransaction::LocateHttpStart(char *buf, uint32_t len,
                                    bool aAllowPartialMatch)
 {
@@ -1522,18 +1560,21 @@ nsHttpTransaction::HandleContentStart()
             mNoContent = true;
             LOG(("this response should not contain a body.\n"));
             break;
         case 421:
             LOG(("Misdirected Request.\n"));
             gHttpHandler->ConnMgr()->ClearHostMapping(mConnInfo);
 
             // retry on a new connection - just in case
-            mCaps &= ~NS_HTTP_ALLOW_KEEPALIVE;
-            mForceRestart = true; // force restart has built in loop protection
+            if (!mRestartCount) {
+                mCaps &= ~NS_HTTP_ALLOW_KEEPALIVE;
+                mForceRestart = true; // force restart has built in loop protection
+                return NS_ERROR_NET_RESET;
+            }
             break;
         }
 
         if (mResponseHead->Status() == 200 &&
             mConnection->IsProxyConnectInProgress()) {
             // successful CONNECTs do not have response bodies
             mNoContent = true;
         }
new file mode 100644
--- /dev/null
+++ b/netwerk/test/unit/test_421.js
@@ -0,0 +1,68 @@
+Cu.import("resource://testing-common/httpd.js");
+Cu.import("resource://gre/modules/Services.jsm");
+
+XPCOMUtils.defineLazyGetter(this, "URL", function() {
+  return "http://localhost:" + httpserver.identity.primaryPort;
+});
+
+var httpserver = new HttpServer();
+var testpath = "/421";
+var httpbody = "0123456789";
+var channel;
+var ios;
+
+function run_test() {
+  setup_test();
+  do_test_pending();
+}
+
+function setup_test() {
+  httpserver.registerPathHandler(testpath, serverHandler);
+  httpserver.start(-1);
+
+  channel = setupChannel(testpath);
+
+  channel.asyncOpen(new ChannelListener(checkRequestResponse, channel), null);
+}
+
+function setupChannel(path) {
+  ios = Cc["@mozilla.org/network/io-service;1"].getService(Ci.nsIIOService);
+  var chan = ios.newChannel2(URL + path,
+                             "",
+                             null,
+                             null,      // aLoadingNode
+                             Services.scriptSecurityManager.getSystemPrincipal(),
+                             null,      // aTriggeringPrincipal
+                             Ci.nsILoadInfo.SEC_NORMAL,
+                             Ci.nsIContentPolicy.TYPE_OTHER);
+  chan.QueryInterface(Ci.nsIHttpChannel);
+  chan.requestMethod = "GET";
+  return chan;
+}
+
+var iters = 0;
+
+function serverHandler(metadata, response) {
+  response.setHeader("Content-Type", "text/plain", false);
+
+  if (!iters) {
+    response.setStatusLine("1.1", 421, "Not Authoritative " + iters);
+  } else {
+    response.setStatusLine("1.1", 200, "OK");
+  }
+  ++iters;
+
+  response.bodyOutputStream.write(httpbody, httpbody.length);
+}
+
+function checkRequestResponse(request, data, context) {
+  do_check_eq(channel.responseStatus, 200);
+  do_check_eq(channel.responseStatusText, "OK");
+  do_check_true(channel.requestSucceeded);
+
+  do_check_eq(channel.contentType, "text/plain");
+  do_check_eq(channel.contentLength, httpbody.length);
+  do_check_eq(data, httpbody);
+
+  httpserver.stop(do_test_finished);
+}
--- a/netwerk/test/unit/xpcshell.ini
+++ b/netwerk/test/unit/xpcshell.ini
@@ -66,16 +66,17 @@ skip-if = os == "android"
 skip-if = true
 [test_cache2-28a-OPEN_SECRETLY.js]
 # This test will be fixed in bug 1067931
 skip-if = true
 [test_cache2-28-concurrent_read_resumable_entry_size_zero.js]
 [test_cache2-29-concurrent_read_non-resumable_entry_size_zero.js]
 [test_partial_response_entry_size_smart_shrink.js]
 [test_304_responses.js]
+[test_421.js]
 [test_cacheForOfflineUse_no-store.js]
 [test_307_redirect.js]
 [test_NetUtil.js]
 [test_URIs.js]
 [test_URIs2.js]
 [test_aboutblank.js]
 [test_assoc.js]
 [test_auth_jar.js]