bug 742827 - fix problem with landing of bug 671591 r=honzab
authorPatrick McManus <mcmanus@ducksong.com>
Thu, 05 Apr 2012 18:37:57 -0400
changeset 94437 33319eea57cacd791292b45f2918f5eb541760a5
parent 94436 6284f89a20696fdad4a3f12cf527222d25e65622
child 94438 76032e5e6cccdf3ccbf33eba88ecb1bd833eb76e
child 94445 f6cb35021951093c6c8ed93dd29f2ee6ee8105fe
push id886
push userlsblakk@mozilla.com
push dateMon, 04 Jun 2012 19:57:52 +0000
treeherdermozilla-beta@bbd8d5efd6d1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershonzab
bugs742827, 671591
milestone14.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
bug 742827 - fix problem with landing of bug 671591 r=honzab
netwerk/protocol/http/nsHttpTransaction.cpp
netwerk/protocol/http/nsHttpTransaction.h
--- a/netwerk/protocol/http/nsHttpTransaction.cpp
+++ b/netwerk/protocol/http/nsHttpTransaction.cpp
@@ -133,21 +133,20 @@ nsHttpTransaction::nsHttpTransaction()
     , mNoContent(false)
     , mSentData(false)
     , mReceivedData(false)
     , mStatusEventPending(false)
     , mHasRequestBody(false)
     , mSSLConnectFailed(false)
     , mHttpResponseMatched(false)
     , mPreserveStream(false)
-    , mToReadBeforeRestart(0)
     , mReportedStart(false)
     , mReportedResponseHeader(false)
     , mForTakeResponseHead(nsnull)
-    , mTakenResponseHeader(false)
+    , mResponseHeadTaken(false)
 {
     LOG(("Creating nsHttpTransaction @%x\n", this));
     gHttpHandler->GetMaxPipelineObjectSize(mMaxPipelineObjectSize);
 }
 
 nsHttpTransaction::~nsHttpTransaction()
 {
     LOG(("Destroying nsHttpTransaction @%x\n", this));
@@ -355,22 +354,22 @@ nsAHttpConnection *
 nsHttpTransaction::Connection()
 {
     return mConnection;
 }
 
 nsHttpResponseHead *
 nsHttpTransaction::TakeResponseHead()
 {
-    NS_ABORT_IF_FALSE(!mTakenResponseHeader, "TakeResponseHead called 2x");
+    NS_ABORT_IF_FALSE(!mResponseHeadTaken, "TakeResponseHead called 2x");
 
     // Lock RestartInProgress() and TakeResponseHead() against main thread
     MutexAutoLock lock(*nsHttp::GetLock());
 
-    mTakenResponseHeader = true;
+    mResponseHeadTaken = true;
 
     // Prefer mForTakeResponseHead over mResponseHead. It is always a complete
     // set of headers.
     nsHttpResponseHead *head;
     if (mForTakeResponseHead) {
         head = mForTakeResponseHead;
         mForTakeResponseHead = nsnull;
         return head;
@@ -465,17 +464,17 @@ nsHttpTransaction::OnTransportStatus(nsI
             (status == nsISocketTransport::STATUS_WAITING_FOR))
             mActivityDistributor->ObserveActivity(
                 mChannel,
                 NS_HTTP_ACTIVITY_TYPE_HTTP_TRANSACTION,
                 NS_HTTP_ACTIVITY_SUBTYPE_REQUEST_BODY_SENT,
                 PR_Now(), LL_ZERO, EmptyCString());
 
         // report the status and progress
-        if (!mRestartInProgressVerifier.Active())
+        if (!mRestartInProgressVerifier.IsDiscardingContent())
             mActivityDistributor->ObserveActivity(
                 mChannel,
                 NS_HTTP_ACTIVITY_TYPE_SOCKET_TRANSPORT,
                 static_cast<PRUint32>(status),
                 PR_Now(),
                 progress,
                 EmptyCString());
     }
@@ -836,40 +835,50 @@ nsHttpTransaction::PipelinePosition()
 // nsHttpTransaction <private>
 //-----------------------------------------------------------------------------
 
 nsresult
 nsHttpTransaction::RestartInProgress()
 {
     NS_ASSERTION(PR_GetCurrentThread() == gSocketThread, "wrong thread");
     
+    if ((mRestartCount + 1) >= gHttpHandler->MaxRequestAttempts()) {
+        LOG(("nsHttpTransaction::RestartInProgress() "
+             "reached max request attempts, failing transaction %p\n", this));
+        return NS_ERROR_NET_RESET;
+    }
+
     // Lock RestartInProgress() and TakeResponseHead() against main thread
     MutexAutoLock lock(*nsHttp::GetLock());
 
+    // Don't try and RestartInProgress() things that haven't gotten a response
+    // header yet. Those should be handled under the normal restart() path if
+    // they are eligible.
+    if (!mHaveAllHeaders)
+        return NS_ERROR_NET_RESET;
+
     // don't try and restart 0.9 or non 200/Get HTTP/1
-    if (mHaveAllHeaders && !mRestartInProgressVerifier.IsSetup())
+    if (!mRestartInProgressVerifier.IsSetup())
         return NS_ERROR_NET_RESET;
 
     LOG(("Will restart transaction %p and skip first %lld bytes, "
          "old Content-Length %lld",
          this, mContentRead, mContentLength));
 
-    if (mHaveAllHeaders) {
-        mRestartInProgressVerifier.SetAlreadyProcessed(
-            PR_MAX(mRestartInProgressVerifier.AlreadyProcessed(), mContentRead));
-        mToReadBeforeRestart = mRestartInProgressVerifier.AlreadyProcessed();
-        mRestartInProgressVerifier.SetActive(true);
+    mRestartInProgressVerifier.SetAlreadyProcessed(
+        PR_MAX(mRestartInProgressVerifier.AlreadyProcessed(), mContentRead));
 
-        if (!mTakenResponseHeader && !mForTakeResponseHead) {
-            // TakeResponseHeader() has not been called yet and this
-            // is the first restart. Store the resp headers exclusively
-            // for TakeResponseHeader()
-            mForTakeResponseHead = mResponseHead;
-            mResponseHead = nsnull;
-        }
+    if (!mResponseHeadTaken && !mForTakeResponseHead) {
+        // TakeResponseHeader() has not been called yet and this
+        // is the first restart. Store the resp headers exclusively
+        // for TakeResponseHead() which is called from the main thread and
+        // could happen at any time - so we can't continue to modify those
+        // headers (which restarting will effectively do)
+        mForTakeResponseHead = mResponseHead;
+        mResponseHead = nsnull;
     }
 
     if (mResponseHead) {
         mResponseHead->Reset();
     }
 
     mContentRead = 0;
     mContentLength = -1;
@@ -1248,26 +1257,30 @@ nsHttpTransaction::HandleContentStart()
                 // Ignore server specified Content-Length.
                 mContentLength = -1;
             }
 #if defined(PR_LOGGING)
             else if (mContentLength == PRInt64(-1))
                 LOG(("waiting for the server to close the connection.\n"));
 #endif
         }
-        if (mRestartInProgressVerifier.Active() &&
+        if (mRestartInProgressVerifier.IsSetup() &&
             !mRestartInProgressVerifier.Verify(mContentLength, mResponseHead)) {
             LOG(("Restart in progress subsequent transaction failed to match"));
             return NS_ERROR_ABORT;
         }
     }
 
     mDidContentStart = true;
+
+    // The verifier only initializes itself once (from the first iteration of
+    // a transaction that gets far enough to have response headers)
     if (mRequestHead->Method() == nsHttp::Get)
         mRestartInProgressVerifier.Set(mContentLength, mResponseHead);
+
     return NS_OK;
 }
 
 // called on the socket thread
 nsresult
 nsHttpTransaction::HandleContent(char *buf,
                                  PRUint32 count,
                                  PRUint32 *contentRead,
@@ -1317,27 +1330,29 @@ nsHttpTransaction::HandleContent(char *b
         }
     }
     else {
         // when we are just waiting for the server to close the connection...
         // (no explicit content-length given)
         *contentRead = count;
     }
     
-    if (mRestartInProgressVerifier.Active() &&
-        mToReadBeforeRestart && *contentRead) {
-        PRUint32 ignore = PR_MIN(*contentRead, PRUint32(mToReadBeforeRestart));
+    PRInt64 toReadBeforeRestart =
+        mRestartInProgressVerifier.ToReadBeforeRestart();
+
+    if (toReadBeforeRestart && *contentRead) {
+        PRUint32 ignore =
+            PR_MIN(toReadBeforeRestart, PR_UINT32_MAX);
+        ignore = PR_MIN(*contentRead, ignore);
         LOG(("Due To Restart ignoring %d of remaining %ld",
-             ignore, mToReadBeforeRestart));
+             ignore, toReadBeforeRestart));
         *contentRead -= ignore;
         mContentRead += ignore;
-        mToReadBeforeRestart -= ignore;
+        mRestartInProgressVerifier.HaveReadBeforeRestart(ignore);
         memmove(buf, buf + ignore, *contentRead + *contentRemaining);
-        if (!mToReadBeforeRestart)
-            mRestartInProgressVerifier.SetActive(false);
     }
 
     if (*contentRead) {
         // update count of content bytes read and report progress...
         mContentRead += *contentRead;
         /* when uncommenting, take care of 64-bit integers w/ NS_MAX...
         if (mProgressSink)
             mProgressSink->OnProgress(nsnull, nsnull, mContentRead, NS_MAX(0, mContentLength));
@@ -1642,11 +1657,17 @@ nsHttpTransaction::RestartVerifier::Set(
         if (val)
             mContentRange.Assign(val);
         val = head->PeekHeader(nsHttp::Content_Encoding);
         if (val)
             mContentEncoding.Assign(val);
         val = head->PeekHeader(nsHttp::Transfer_Encoding);
         if (val)
             mTransferEncoding.Assign(val);
+
+        // We can only restart with any confidence if we have a stored etag or
+        // last-modified header
+        if (mETag.IsEmpty() && mLastModified.IsEmpty())
+            return;
+
         mSetup = true;
     }
 }
--- a/netwerk/protocol/http/nsHttpTransaction.h
+++ b/netwerk/protocol/http/nsHttpTransaction.h
@@ -226,51 +226,59 @@ private:
     bool                            mHttpResponseMatched;
     bool                            mPreserveStream;
 
     // mClosed           := transaction has been explicitly closed
     // mTransactionDone  := transaction ran to completion or was interrupted
     // mResponseComplete := transaction ran to completion
 
     // For Restart-In-Progress Functionality
-    PRInt64                         mToReadBeforeRestart;
     bool                            mReportedStart;
     bool                            mReportedResponseHeader;
 
     // protected by nsHttp::GetLock()
     nsHttpResponseHead             *mForTakeResponseHead;
-    bool                            mTakenResponseHeader;
+    bool                            mResponseHeadTaken;
 
     class RestartVerifier 
     {
 
         // When a idemptotent transaction has received part of its response body
         // and incurs an error it can be restarted. To do this we mark the place
         // where we stopped feeding the body to the consumer and start the
         // network call over again. If everything we track (headers, length, etc..)
         // matches up to the place where we left off then the consumer starts being
         // fed data again with the new information. This can be done N times up
         // to the normal restart (i.e. with no response info) limit.
 
     public:
         RestartVerifier()
             : mContentLength(-1)
             , mAlreadyProcessed(0)
-            , mActive(false)
+            , mToReadBeforeRestart(0)
             , mSetup(false)
         {}
         ~RestartVerifier() {}
         
         void Set(PRInt64 contentLength, nsHttpResponseHead *head);
         bool Verify(PRInt64 contentLength, nsHttpResponseHead *head);
-        bool Active() { return mActive; }
-        void SetActive(bool val) { mActive = val; }
+        bool IsDiscardingContent() { return mToReadBeforeRestart != 0; }
         bool IsSetup() { return mSetup; }
         PRInt64 AlreadyProcessed() { return mAlreadyProcessed; }
-        void SetAlreadyProcessed(PRInt64 val) { mAlreadyProcessed = val; }
+        void SetAlreadyProcessed(PRInt64 val) {
+            mAlreadyProcessed = val;
+            mToReadBeforeRestart = val;
+        }
+        PRInt64 ToReadBeforeRestart() { return mToReadBeforeRestart; }
+        void HaveReadBeforeRestart(PRUint32 amt)
+        {
+            NS_ABORT_IF_FALSE(amt <= mToReadBeforeRestart,
+                              "too large of a HaveReadBeforeRestart deduction");
+            mToReadBeforeRestart -= amt;
+        }
 
     private:
         // This is the data from the first complete response header
         // used to make sure that all subsequent response headers match
 
         PRInt64                         mContentLength;
         nsCString                       mETag;
         nsCString                       mLastModified;
@@ -278,17 +286,19 @@ private:
         nsCString                       mContentEncoding;
         nsCString                       mTransferEncoding;
 
         // This is the amount of data that has been passed to the channel
         // from previous iterations of the transaction and must therefore
         // be skipped in the new one.
         PRInt64                         mAlreadyProcessed;
 
-        // true when iteration > 0 has started
-        bool                            mActive;
+        // The amount of data that must be discarded in the current iteration
+        // (where iteration > 0) to reach the mAlreadyProcessed high water
+        // mark.
+        PRInt64                         mToReadBeforeRestart;
 
         // true when ::Set has been called with a response header
         bool                            mSetup;
     } mRestartInProgressVerifier;
 };
 
 #endif // nsHttpTransaction_h__