bug 603512 - large objects block pipelines r=honzab

the type and state patch tries hard not to form pipelines behind resources that
could become head of line blockers. But of course that requires the ability to
predict the future, and won't be perfect.

This patch reacts to a transaction that has a large response body (defined by
either a large content-length header or actually reading a large number of
chunked bytes) by cancelling any transactions that have been pipelined down the
same connection and rescheduling them elsewhere. It also changes the type of
the connection to "solo", which prevents new transactions from being pipelined
onto this one and provides class-specific negative feedback to the pipeline
manager so that near-future requests to the same host of the same type (e.g.
general) will not be pipelined but other types (e.g. img or js/css) can still
do that.

Content-Length is ideal, because it allows us to identify the problem so early.
But even actually reading the document for a fairly long time gives it a fairly
high probability of not ending soon. (i.e. long document sizes are spread over
a larger range than small ones. duh.)

The pref network.http.pipelining.maxsize controls the threshold. I set the
default at 300KB, which is roughly the bandwidth delay product of a 2mbit 120ms
rtt connection and 1 rtt is mostly what you are giving up by canceling it on
one connection and sending it on another. (modulo maybe needing a handshake).

--HG--
extra : rebase_source : ebc44f8dfc3fa7bcee613ff3b74269d908ceac51

modules/libpref/src/init/all.js

@@ -813,6 +813,7 @@ pref("network.http.pipelining.maxrequests" , 32);
 pref("network.http.pipelining.max-optimistic-requests" , 4);
 
 pref("network.http.pipelining.aggressive", false);
+pref("network.http.pipelining.maxsize" , 300000);
 
 // Prompt for 307 redirects
 pref("network.http.prompt-temp-redirect", true);

netwerk/protocol/http/SpdySession.cpp

@@ -2034,6 +2034,30 @@ SpdySession::Transport()
   return mConnection->Transport();
 }
 
+PRUint32
+SpdySession::CancelPipeline(nsresult reason)
+{
+  // we don't pipeline inside spdy, so this isn't an issue
+  return 0;
+}
+
+nsAHttpTransaction::Classifier
+SpdySession::Classification()
+{
+  if (!mConnection)
+    return nsAHttpTransaction::CLASS_GENERAL;
+  return mConnection->Classification();
+}
+
+void
+SpdySession::Classify(nsAHttpTransaction::Classifier newclass)
+{
+  if (!mConnection)
+    return;
+  
+  mConnection->Classify(newclass);
+}
+
 //-----------------------------------------------------------------------------
 // unused methods of nsAHttpTransaction
 // We can be sure of this because SpdySession is only constructed in

netwerk/protocol/http/SpdySession.h

@@ -75,7 +75,6 @@ public:
 
   bool AddStream(nsAHttpTransaction *, PRInt32);
   bool CanReuse() { return !mShouldGoAway && !mClosed; }
-  void DontReuse();
   bool RoomForMoreStreams();
 
   // When the connection is active this is called every 15 seconds

netwerk/protocol/http/nsAHttpConnection.h

@@ -39,8 +39,8 @@
 #define nsAHttpConnection_h__
 
 #include "nsISupports.h"
+#include "nsAHttpTransaction.h"
 
-class nsAHttpTransaction;
 class nsHttpRequestHead;
 class nsHttpResponseHead;
 class nsHttpConnectionInfo;
@@ -120,9 +120,10 @@ public:
     // persistent... important in determining the end of a response.
     virtual bool IsPersistent() = 0;
 
-    // called to determine if a connection has been reused.
+    // called to determine or set if a connection has been reused.
     virtual bool IsReused() = 0;
-    
+    virtual void   DontReuse() = 0;
+
     // called by a transaction when the transaction reads more from the socket
     // than it should have (eg. containing part of the next pipelined response).
     virtual nsresult PushBack(const char *data, PRUint32 length) = 0;
@@ -144,6 +145,14 @@ public:
     // Get the nsISocketTransport used by the connection without changing
     //  references or ownership.
     virtual nsISocketTransport *Transport() = 0;
+
+    // Cancel and reschedule transactions deeper than the current response.
+    // Returns the number of canceled transactions.
+    virtual PRUint32 CancelPipeline(nsresult originalReason) = 0;
+
+    // Read and write class of transaction that is carried on this connection
+    virtual nsAHttpTransaction::Classifier Classification() = 0;
+    virtual void Classify(nsAHttpTransaction::Classifier newclass) = 0;
 };
 
 #define NS_DECL_NSAHTTPCONNECTION \
@@ -158,11 +167,15 @@ public:
     void GetSecurityInfo(nsISupports **); \
     bool IsPersistent(); \
     bool IsReused(); \
+    void DontReuse();  \
     nsresult PushBack(const char *, PRUint32); \
     bool IsProxyConnectInProgress(); \
     bool LastTransactionExpectedNoContent(); \
-    void   SetLastTransactionExpectedNoContent(bool); \
+    void SetLastTransactionExpectedNoContent(bool); \
     nsHttpConnection *TakeHttpConnection(); \
-    nsISocketTransport *Transport();
+    nsISocketTransport *Transport();        \
+    PRUint32 CancelPipeline(nsresult originalReason);   \
+    nsAHttpTransaction::Classifier Classification();    \
+    void Classify(nsAHttpTransaction::Classifier);
 
 #endif // nsAHttpConnection_h__

netwerk/protocol/http/nsHttpConnection.h

@@ -42,6 +42,7 @@
 #include "nsHttp.h"
 #include "nsHttpConnectionInfo.h"
 #include "nsAHttpTransaction.h"
+#include "nsHttpPipeline.h"
 #include "nsXPIDLString.h"
 #include "nsCOMPtr.h"
 #include "nsAutoPtr.h"

netwerk/protocol/http/nsHttpConnectionMgr.cpp

@@ -2073,6 +2073,12 @@ nsHttpConnectionMgr::nsConnectionHandle::IsReused()
     return mConn->IsReused();
 }
 
+void
+nsHttpConnectionMgr::nsConnectionHandle::DontReuse()
+{
+    mConn->DontReuse();
+}
+
 nsresult
 nsHttpConnectionMgr::nsConnectionHandle::PushBack(const char *buf, PRUint32 bufLen)
 {
@@ -2517,6 +2523,32 @@ nsHttpConnectionMgr::nsConnectionHandle::IsProxyConnectInProgress()
     return mConn->IsProxyConnectInProgress();
 }
 
+PRUint32
+nsHttpConnectionMgr::nsConnectionHandle::CancelPipeline(nsresult reason)
+{
+    // no pipeline to cancel
+    return 0;
+}
+
+nsAHttpTransaction::Classifier
+nsHttpConnectionMgr::nsConnectionHandle::Classification()
+{
+    if (mConn)
+        return mConn->Classification();
+
+    LOG(("nsConnectionHandle::Classification this=%p "
+         "has null mConn using CLASS_SOLO default", this));
+    return nsAHttpTransaction::CLASS_SOLO;
+}
+
+void
+nsHttpConnectionMgr::
+nsConnectionHandle::Classify(nsAHttpTransaction::Classifier newclass)
+{
+    if (mConn)
+        mConn->Classify(newclass);
+}
+
 // nsConnectionEntry
 
 nsHttpConnectionMgr::
@@ -2581,7 +2613,8 @@ nsConnectionEntry::OnPipelineFeedbackInfo(
     nsAHttpTransaction::Classifier classification;
     if (conn)
         classification = conn->Classification();
-    else if (info == BadInsufficientFraming)
+    else if (info == BadInsufficientFraming ||
+             info == BadUnexpectedLarge)
         classification = (nsAHttpTransaction::Classifier) data;
     else
         classification = nsAHttpTransaction::CLASS_SOLO;
@@ -2640,6 +2673,9 @@ nsConnectionEntry::OnPipelineFeedbackInfo(
         case BadInsufficientFraming:
             mPipeliningClassPenalty[classification] += 7000;
             break;
+        case BadUnexpectedLarge:
+            mPipeliningClassPenalty[classification] += 120;
+            break;
 
         default:
             NS_ABORT_IF_FALSE(0, "Unknown Bad/Red Pipeline Feedback Event");

netwerk/protocol/http/nsHttpConnectionMgr.h

@@ -55,7 +55,7 @@
 #include "nsITimer.h"
 #include "nsIX509Cert3.h"
 
-class nsHttpPipeline;
+#include "nsHttpPipeline.h"
 
 //-----------------------------------------------------------------------------
 
@@ -161,7 +161,7 @@ public:
         RedBannedServer = kPipelineInfoTypeRed | kPipelineInfoTypeBad | 0x0002,
     
         // Used when a response is terminated early, or when it fails an
-        // integrity check such as assoc-req or md5
+        // integrity check such as assoc-req
         RedCorruptedContent = kPipelineInfoTypeRed | kPipelineInfoTypeBad | 0x0004,
 
         // Used when a pipeline is only partly satisfied - for instance if the
@@ -184,6 +184,10 @@ public:
         // Used when a response is received that is not framed with either chunked
         // encoding or a complete content length.
         BadInsufficientFraming = kPipelineInfoTypeBad | 0x0008,
+        
+        // Used when a very large response is recevied in a potential pipelining
+        // context. Large responses cause head of line blocking.
+        BadUnexpectedLarge = kPipelineInfoTypeBad | 0x000B,
 
         // Used when a response is received that has headers that appear to support
         // pipelining.

netwerk/protocol/http/nsHttpHandler.cpp

@@ -188,6 +188,7 @@ nsHttpHandler::nsHttpHandler()
     , mMaxPipelinedRequests(32)
     , mMaxOptimisticPipelinedRequests(4)
     , mPipelineAggressive(false)
+    , mMaxPipelineObjectSize(300000)
     , mRedirectionLimit(10)
     , mPhishyUserPassLength(1)
     , mQoSBits(0x00)
@@ -1044,6 +1045,14 @@ nsHttpHandler::PrefsChanged(nsIPrefBranch *prefs, const char *pref)
             mPipelineAggressive = cVar;
     }
 
+    if (PREF_CHANGED(HTTP_PREF("pipelining.maxsize"))) {
+        rv = prefs->GetIntPref(HTTP_PREF("pipelining.maxsize"), &val);
+        if (NS_SUCCEEDED(rv)) {
+            mMaxPipelineObjectSize =
+                static_cast<PRInt64>(clamped(val, 1000, 100000000));
+        }
+    }
+
     if (PREF_CHANGED(HTTP_PREF("pipelining.ssl"))) {
         rv = prefs->GetBoolPref(HTTP_PREF("pipelining.ssl"), &cVar);
         if (NS_SUCCEEDED(rv))

netwerk/protocol/http/nsHttpHandler.h

@@ -233,6 +233,10 @@ public:
                                      nsCString& hostLine);
 
     bool GetPipelineAggressive()     { return mPipelineAggressive; }
+    void GetMaxPipelineObjectSize(PRInt64 &outVal)
+    {
+        outVal = mMaxPipelineObjectSize;
+    }
 
 private:
 
@@ -293,6 +297,7 @@ private:
     PRUint16 mMaxPipelinedRequests;
     PRUint16 mMaxOptimisticPipelinedRequests;
     bool     mPipelineAggressive;
+    PRInt64  mMaxPipelineObjectSize;
 
     PRUint8  mRedirectionLimit;
 

netwerk/protocol/http/nsHttpPipeline.cpp

@@ -274,15 +274,17 @@ nsHttpPipeline::CloseTransaction(nsAHttpTransaction *trans, nsresult reason)
         killPipeline = true;
     }
 
+    // Marking this connection as non-reusable prevents other items from being
+    // added to it and causes it to be torn down soon. Don't tear it down yet
+    // as that would prevent Response(0) from being processed.
+    DontReuse();
+
     trans->Close(reason);
     NS_RELEASE(trans);
 
-    if (killPipeline) {
-        if (mConnection)
-            mConnection->CloseTransaction(this, reason);
-        else
-            Close(reason);
-    }
+    if (killPipeline)
+        // reschedule anything from this pipeline onto a different connection
+        CancelPipeline(reason);
 }
 
 void
@@ -325,6 +327,13 @@ nsHttpPipeline::IsReused()
     return true;
 }
 
+void
+nsHttpPipeline::DontReuse()
+{
+    if (mConnection)
+        mConnection->DontReuse();
+}
+
 nsresult
 nsHttpPipeline::PushBack(const char *data, PRUint32 length)
 {
@@ -407,6 +416,24 @@ nsHttpPipeline::Transport()
     return mConnection->Transport();
 }
 
+nsAHttpTransaction::Classifier
+nsHttpPipeline::Classification()
+{
+    if (mConnection)
+        return mConnection->Classification();
+
+    LOG(("nsHttpPipeline::Classification this=%p "
+         "has null mConnection using CLASS_SOLO default", this));
+    return nsAHttpTransaction::CLASS_SOLO;
+}
+
+void
+nsHttpPipeline::Classify(nsAHttpTransaction::Classifier newclass)
+{
+    if (mConnection)
+        mConnection->Classify(newclass);
+}
+
 void
 nsHttpPipeline::SetSSLConnectFailed()
 {
@@ -776,6 +803,52 @@ nsHttpPipeline::WriteSegments(nsAHttpSegmentWriter *writer,
     return rv;
 }
 
+PRUint32
+nsHttpPipeline::CancelPipeline(nsresult originalReason)
+{
+    PRUint32 i, reqLen, respLen, total;
+    nsAHttpTransaction *trans;
+
+    reqLen = mRequestQ.Length();
+    respLen = mResponseQ.Length();
+    total = reqLen + respLen;
+
+    // don't count the first response, if presnet
+    if (respLen)
+        total--;
+
+    if (!total)
+        return 0;
+
+    // any pending requests can ignore this error and be restarted
+    // unless it is during a CONNECT tunnel request
+    for (i = 0; i < reqLen; ++i) {
+        trans = Request(i);
+        if (mConnection && mConnection->IsProxyConnectInProgress())
+            trans->Close(originalReason);
+        else
+            trans->Close(NS_ERROR_NET_RESET);
+        NS_RELEASE(trans);
+    }
+    mRequestQ.Clear();
+
+    // any pending responses can be restarted except for the first one,
+    // that we might want to finish on this pipeline or cancel individually
+    for (i = 1; i < respLen; ++i) {
+        trans = Response(i);
+        trans->Close(NS_ERROR_NET_RESET);
+        NS_RELEASE(trans);
+    }
+
+    if (respLen > 1)
+        mResponseQ.TruncateLength(1);
+
+    DontReuse();
+    Classify(nsAHttpTransaction::CLASS_SOLO);
+
+    return total;
+}
+
 void
 nsHttpPipeline::Close(nsresult reason)
 {
@@ -790,55 +863,37 @@ nsHttpPipeline::Close(nsresult reason)
     mStatus = reason;
     mClosed = true;
 
-    PRUint32 i, count;
-    nsAHttpTransaction *trans;
-
-    // any pending requests can ignore this error and be restarted
-    // unless it is during a CONNECT tunnel request
-    count = mRequestQ.Length();
-    for (i = 0; i < count; ++i) {
-        trans = Request(i);
-        if (mConnection && mConnection->IsProxyConnectInProgress())
-            trans->Close(reason);
-        else
-            trans->Close(NS_ERROR_NET_RESET);
-        NS_RELEASE(trans);
-    }
-    mRequestQ.Clear();
-
-    trans = Response(0);
-    
     nsRefPtr<nsHttpConnectionInfo> ci;
     GetConnectionInfo(getter_AddRefs(ci));
-    if (ci && (trans || count))
+    PRUint32 numRescheduled = CancelPipeline(reason);
+
+    // numRescheduled can be 0 if there is just a single response in the
+    // pipeline object. That isn't really a meaningful pipeline that
+    // has been forced to be rescheduled so it does not need to generate
+    // negative feedback.
+    if (ci && numRescheduled)
         gHttpHandler->ConnMgr()->PipelineFeedbackInfo(
             ci, nsHttpConnectionMgr::RedCanceledPipeline, nsnull, 0);
 
-    if (trans) {
-        // The current transaction can be restarted via reset
-        // if the response has not started to arrive and the reason
-        // for failure is innocuous (e.g. not an SSL error)
-        if (!mResponseIsPartial &&
-            (reason == NS_ERROR_NET_RESET ||
-             reason == NS_OK ||
-             reason == NS_BASE_STREAM_CLOSED)) {
-            trans->Close(NS_ERROR_NET_RESET);            
-        }
-        else {
-            trans->Close(reason);
-        }
-        NS_RELEASE(trans);
-        
-        // any remaining pending responses can be restarted
-        count = mResponseQ.Length();
-        for (i=1; i<count; ++i) {
-            trans = Response(i);
-            trans->Close(NS_ERROR_NET_RESET);
-            NS_RELEASE(trans);
-        }
-        mResponseQ.Clear();
+    nsAHttpTransaction *trans = Response(0);
+    if (!trans)
+        return;
+
+    // The current transaction can be restarted via reset
+    // if the response has not started to arrive and the reason
+    // for failure is innocuous (e.g. not an SSL error)
+    if (!mResponseIsPartial &&
+        (reason == NS_ERROR_NET_RESET ||
+         reason == NS_OK ||
+         reason == NS_BASE_STREAM_CLOSED)) {
+        trans->Close(NS_ERROR_NET_RESET);
+    }
+    else {
+        trans->Close(reason);
     }
 
+    NS_RELEASE(trans);
+    mResponseQ.Clear();
 }
 
 nsresult

netwerk/protocol/http/nsHttpTransaction.cpp

@@ -138,6 +138,7 @@ nsHttpTransaction::nsHttpTransaction()
     , mPreserveStream(false)
 {
     LOG(("Creating nsHttpTransaction @%x\n", this));
+    gHttpHandler->GetMaxPipelineObjectSize(mMaxPipelineObjectSize);
 }
 
 nsHttpTransaction::~nsHttpTransaction()
@@ -1137,6 +1138,10 @@ nsHttpTransaction::HandleContentStart()
             // grab the content-length from the response headers
             mContentLength = mResponseHead->ContentLength();
 
+            if ((mClassification != CLASS_SOLO) &&
+                (mContentLength > mMaxPipelineObjectSize))
+                CancelPipeline(nsHttpConnectionMgr::BadUnexpectedLarge);
+            
             // handle chunked encoding here, so we'll know immediately when
             // we're done with the socket.  please note that _all_ other
             // decoding is done when the channel receives the content data
@@ -1231,6 +1236,13 @@ nsHttpTransaction::HandleContent(char *buf,
     LOG(("nsHttpTransaction::HandleContent [this=%x count=%u read=%u mContentRead=%lld mContentLength=%lld]\n",
         this, count, *contentRead, mContentRead, mContentLength));
 
+    // Check the size of chunked responses. If we exceed the max pipeline size
+    // for this response reschedule the pipeline
+    if ((mClassification != CLASS_SOLO) &&
+        mChunkedDecoder &&
+        (mContentRead > mMaxPipelineObjectSize))
+        CancelPipeline(nsHttpConnectionMgr::BadUnexpectedLarge);
+
     // check for end-of-file
     if ((mContentRead == mContentLength) ||
         (mChunkedDecoder && mChunkedDecoder->ReachedEOF())) {
@@ -1330,6 +1342,23 @@ nsHttpTransaction::ProcessData(char *buf, PRUint32 count, PRUint32 *countRead)
     return NS_OK;
 }
 
+void
+nsHttpTransaction::CancelPipeline(PRUint32 reason)
+{
+    // reason is casted through a uint to avoid compiler header deps
+    gHttpHandler->ConnMgr()->PipelineFeedbackInfo(
+        mConnInfo,
+        static_cast<nsHttpConnectionMgr::PipelineFeedbackInfoType>(reason),
+        nsnull, mClassification);
+
+    mConnection->CancelPipeline(NS_ERROR_CORRUPTED_CONTENT);
+
+    // Avoid pipelining this transaction on restart by classifying it as solo.
+    // This also prevents BadUnexpectedLarge from being reported more
+    // than one time per transaction.
+    mClassification = CLASS_SOLO;
+}
+
 //-----------------------------------------------------------------------------
 // nsHttpTransaction deletion event
 //-----------------------------------------------------------------------------

netwerk/protocol/http/nsHttpTransaction.h

@@ -149,6 +149,7 @@ private:
     void     DeleteSelfOnConsumerThread();
 
     Classifier Classify();
+    void       CancelPipeline(PRUint32 reason);
 
     static NS_METHOD ReadRequestSegment(nsIInputStream *, void *, const char *,
                                         PRUint32, PRUint32, PRUint32 *);
@@ -204,6 +205,7 @@ private:
     PRUint8                         mCaps;
     enum Classifier                 mClassification;
     PRInt32                         mPipelinePosition;
+    PRInt64                         mMaxPipelineObjectSize;
 
     // state flags, all logically boolean, but not packed together into a
     // bitfield so as to avoid bitfield-induced races.  See bug 560579.