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 91129 33319eea57cacd791292b45f2918f5eb541760a5
parent 91128 6284f89a20696fdad4a3f12cf527222d25e65622
child 91130 76032e5e6cccdf3ccbf33eba88ecb1bd833eb76e
child 91137 f6cb35021951093c6c8ed93dd29f2ee6ee8105fe
push id667
push usertim.taubert@gmx.de
push dateTue, 10 Apr 2012 10:56:50 +0000
treeherderfx-team@6fe5b0271cd1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershonzab
bugs742827, 671591
milestone14.0a1
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__