bug 708415 spdy code review of nshttp* r=honzab
authorPatrick McManus <mcmanus@ducksong.com>
Thu, 26 Jan 2012 00:15:26 -0500
changeset 85430 50912fc183283fd8996760341f3e82990b36d273
parent 85429 c0e2f40662cc40631e4bc44650d4737b3838e7e9
child 85431 f583132674d5ab623bde7aa46c8e6d8db9c721b1
push id21921
push usermak77@bonardo.net
push dateThu, 26 Jan 2012 11:59:12 +0000
treeherdermozilla-central@f20f2b7c93cb [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershonzab
bugs708415
milestone12.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 708415 spdy code review of nshttp* r=honzab
netwerk/protocol/http/nsHttpChannel.cpp
netwerk/protocol/http/nsHttpConnection.cpp
netwerk/protocol/http/nsHttpConnectionMgr.cpp
netwerk/protocol/http/nsHttpConnectionMgr.h
--- a/netwerk/protocol/http/nsHttpChannel.cpp
+++ b/netwerk/protocol/http/nsHttpChannel.cpp
@@ -4126,17 +4126,17 @@ nsHttpChannel::OnStartRequest(nsIRequest
                  "If we have both pumps, the cache content must be partial");
 
     if (!mSecurityInfo && !mCachePump && mTransaction) {
         // grab the security info from the connection object; the transaction
         // is guaranteed to own a reference to the connection.
         mSecurityInfo = mTransaction->SecurityInfo();
     }
 
-    if (gHttpHandler->IsSpdyEnabled() && !mCachePump && NS_FAILED(mStatus) &&
+    if (!mCachePump && NS_FAILED(mStatus) &&
         (mLoadFlags & LOAD_REPLACE) && mOriginalURI && mAllowSpdy) {
         // For sanity's sake we may want to cancel an alternate protocol
         // redirection involving the original host name
 
         nsCAutoString hostPort;
         if (NS_SUCCEEDED(mOriginalURI->GetHostPort(hostPort)))
             gHttpHandler->ConnMgr()->RemoveSpdyAlternateProtocol(hostPort);
     }
--- a/netwerk/protocol/http/nsHttpConnection.cpp
+++ b/netwerk/protocol/http/nsHttpConnection.cpp
@@ -173,18 +173,23 @@ nsHttpConnection::Init(nsHttpConnectionI
 bool
 nsHttpConnection::EnsureNPNComplete()
 {
     // NPN is only used by SPDY right now.
     //
     // If for some reason the components to check on NPN aren't available,
     // this function will just return true to continue on and disable SPDY
 
-    NS_ABORT_IF_FALSE(mSocketTransport, "EnsureNPNComplete "
-                      "socket transport precondition");
+    if (!mSocketTransport) {
+        // this cannot happen
+        NS_ABORT_IF_FALSE(false,
+                          "EnsureNPNComplete socket transport precondition");
+        mNPNComplete = true;
+        return true;
+    }
 
     if (mNPNComplete)
         return true;
     
     nsresult rv;
 
     nsCOMPtr<nsISupports> securityInfo;
     nsCOMPtr<nsISSLSocketControl> ssl;
@@ -215,17 +220,22 @@ nsHttpConnection::EnsureNPNComplete()
         goto npnComplete;
 
     LOG(("nsHttpConnection::EnsureNPNComplete %p negotiated to '%s'",
          this, negotiatedNPN.get()));
     
     if (negotiatedNPN.Equals(NS_LITERAL_CSTRING("spdy/2"))) {
         mUsingSpdy = true;
         mEverUsedSpdy = true;
-        mIsReused = true;    /* all spdy streams are reused */
+
+        // Setting the connection as reused allows some transactions that fail
+        // with NS_ERROR_NET_RESET to be restarted and SPDY uses that code
+        // to handle clean rejections (such as those that arrived after
+        // a server goaway was generated).
+        mIsReused = true;
 
         // Wrap the old http transaction into the new spdy session
         // as the first stream
         mSpdySession = new SpdySession(mTransaction,
                                        mSocketTransport,
                                        mPriority);
         mTransaction = mSpdySession;
         mIdleTimeout = gHttpHandler->SpdyTimeout();
@@ -435,17 +445,17 @@ nsHttpConnection::ProxyStartSSL()
 }
 
 void
 nsHttpConnection::DontReuse()
 {
     mKeepAliveMask = false;
     mKeepAlive = false;
     mIdleTimeout = 0;
-    if (mUsingSpdy)
+    if (mSpdySession)
         mSpdySession->DontReuse();
 }
 
 bool
 nsHttpConnection::CanReuse()
 {
     bool canReuse;
     
@@ -969,20 +979,19 @@ nsHttpConnection::OnSocketWritable()
             // see the results of the handshake to know what bytes to send, so
             // we cannot proceed with the request headers.
 
             rv = NS_OK;
             mSocketOutCondition = NS_BASE_STREAM_WOULD_BLOCK;
             n = 0;
         }
         else {
-            if (gHttpHandler->IsSpdyEnabled() && !mReportedSpdy) {
+            if (!mReportedSpdy) {
                 mReportedSpdy = true;
-                gHttpHandler->ConnMgr()->
-                    ReportSpdyConnection(this, mUsingSpdy);
+                gHttpHandler->ConnMgr()->ReportSpdyConnection(this, mUsingSpdy);
             }
 
             LOG(("  writing transaction request stream\n"));
             rv = mTransaction->ReadSegments(this, nsIOService::gDefaultSegmentSize, &n);
         }
 
         LOG(("  ReadSegments returned [rv=%x read=%u sock-cond=%x]\n",
             rv, n, mSocketOutCondition));
--- a/netwerk/protocol/http/nsHttpConnectionMgr.cpp
+++ b/netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ -94,16 +94,17 @@ nsHttpConnectionMgr::nsHttpConnectionMgr
     , mIsShuttingDown(false)
     , mNumActiveConns(0)
     , mNumIdleConns(0)
     , mTimeOfNextWakeUp(LL_MAXUINT)
 {
     LOG(("Creating nsHttpConnectionMgr @%x\n", this));
     mCT.Init();
     mAlternateProtocolHash.Init(16);
+    mSpdyPreferredHash.Init();
 }
 
 nsHttpConnectionMgr::~nsHttpConnectionMgr()
 {
     LOG(("Destroying nsHttpConnectionMgr @%x\n", this));
 }
 
 nsresult
@@ -140,17 +141,16 @@ nsHttpConnectionMgr::Init(PRUint16 maxCo
                           PRUint16 maxPersistConnsPerProxy,
                           PRUint16 maxRequestDelay,
                           PRUint16 maxPipelinedRequests)
 {
     LOG(("nsHttpConnectionMgr::Init\n"));
 
     {
         ReentrantMonitorAutoEnter mon(mReentrantMonitor);
-        mSpdyPreferredHash.Init();
 
         mMaxConns = maxConns;
         mMaxConnsPerHost = maxConnsPerHost;
         mMaxConnsPerProxy = maxConnsPerProxy;
         mMaxPersistConnsPerHost = maxPersistConnsPerHost;
         mMaxPersistConnsPerProxy = maxPersistConnsPerProxy;
         mMaxRequestDelay = maxRequestDelay;
         mMaxPipelinedRequests = maxPipelinedRequests;
@@ -412,18 +412,17 @@ nsHttpConnectionMgr::LookupConnectionEnt
 {
     if (!ci)
         return nsnull;
 
     nsConnectionEntry *ent = mCT.Get(ci->HashKey());
     
     // If there is no sign of coalescing (or it is disabled) then just
     // return the primary hash lookup
-    if (!gHttpHandler->IsSpdyEnabled() || !gHttpHandler->CoalesceSpdy() ||
-        !ent || !ent->mUsingSpdy || ent->mCoalescingKey.IsEmpty())
+    if (!ent || !ent->mUsingSpdy || ent->mCoalescingKey.IsEmpty())
         return ent;
 
     // If there is no preferred coalescing entry for this host (or the
     // preferred entry is the one that matched the mCT hash lookup) then
     // there is only option
     nsConnectionEntry *preferred = mSpdyPreferredHash.Get(ent->mCoalescingKey);
     if (!preferred || (preferred == ent))
         return ent;
@@ -462,74 +461,90 @@ nsHttpConnectionMgr::CloseIdleConnection
 
     conn->Close(NS_ERROR_ABORT);
     NS_RELEASE(conn);
     mNumIdleConns--;
     ConditionallyStopPruneDeadConnectionsTimer();
     return NS_OK;
 }
 
+// This function lets a connection, after completing the NPN phase,
+// report whether or not it is using spdy through the usingSpdy
+// argument. It would not be necessary if NPN were driven out of
+// the connection manager. The connection entry associated with the
+// connection is then updated to indicate whether or not we want to use
+// spdy with that host and update the preliminary preferred host
+// entries used for de-sharding hostsnames.
 void
 nsHttpConnectionMgr::ReportSpdyConnection(nsHttpConnection *conn,
                                           bool usingSpdy)
 {
     NS_ABORT_IF_FALSE(PR_GetCurrentThread() == gSocketThread, "wrong thread");
     
     nsConnectionEntry *ent = LookupConnectionEntry(conn->ConnectionInfo(),
                                                    conn, nsnull);
 
     NS_ABORT_IF_FALSE(ent, "no connection entry");
     if (!ent)
         return;
 
     ent->mTestedSpdy = true;
 
-    if (!usingSpdy) {
-        if (ent->mUsingSpdy)
-            conn->DontReuse();
+    if (!usingSpdy)
         return;
-    }
     
     ent->mUsingSpdy = true;
 
     PRUint32 ttl = conn->TimeToLive();
     PRUint64 timeOfExpire = NowInSeconds() + ttl;
     if (!mTimer || timeOfExpire < mTimeOfNextWakeUp)
         PruneDeadConnectionsAfter(ttl);
 
     // Lookup preferred directly from the hash instead of using
-    // GetSpdyPreferred() because we want to avoid the cert compatibility
+    // GetSpdyPreferredEnt() because we want to avoid the cert compatibility
     // check at this point because the cert is never part of the hash
     // lookup. Filtering on that has to be done at the time of use
     // rather than the time of registration (i.e. now).
     nsConnectionEntry *preferred =
         mSpdyPreferredHash.Get(ent->mCoalescingKey);
 
-    LOG(("ReportSpdyConnection %s %s ent=%p ispreferred=%d\n",
+    LOG(("ReportSpdyConnection %s %s ent=%p preferred=%p\n",
          ent->mConnInfo->Host(), ent->mCoalescingKey.get(),
          ent, preferred));
     
     if (!preferred) {
-        ent->mSpdyPreferred = true;
-        SetSpdyPreferred(ent);
-        preferred = ent;
+        if (!ent->mCoalescingKey.IsEmpty()) {
+            mSpdyPreferredHash.Put(ent->mCoalescingKey, ent);
+            ent->mSpdyPreferred = true;
+            preferred = ent;
+        }
     }
     else if (preferred != ent) {
         // A different hostname is the preferred spdy host for this
-        // IP address.
-        ent->mUsingSpdy = true;
+        // IP address. That preferred mapping must have been setup while
+        // this connection was negotiating NPN.
+
+        // Call don't reuse on the current connection to shut it down as soon
+        // as possible without causing any errors.
+        // i.e. the current transaction(s) on this connection will be processed
+        // normally, but then it will go away and future connections will be
+        // coalesced through the preferred entry.
+
         conn->DontReuse();
     }
 
-    ProcessSpdyPendingQ();
+    ProcessAllSpdyPendingQ();
 }
 
 bool
 nsHttpConnectionMgr::GetSpdyAlternateProtocol(nsACString &hostPortKey)
 {
+    if (!gHttpHandler->UseAlternateProtocol())
+        return false;
+
     // The Alternate Protocol hash is protected under the monitor because
     // it is read from both the main and the network thread.
     ReentrantMonitorAutoEnter mon(mReentrantMonitor);
 
     return mAlternateProtocolHash.Contains(hostPortKey);
 }
 
 void
@@ -584,17 +599,17 @@ nsHttpConnectionMgr::TrimAlternateProtoc
     nsHttpConnectionMgr *self = (nsHttpConnectionMgr *) closure;
     
     if (self->mAlternateProtocolHash.mHashTable.entryCount > 2000)
         return PL_DHASH_REMOVE;
     return PL_DHASH_STOP;
 }
 
 nsHttpConnectionMgr::nsConnectionEntry *
-nsHttpConnectionMgr::GetSpdyPreferred(nsConnectionEntry *aOriginalEntry)
+nsHttpConnectionMgr::GetSpdyPreferredEnt(nsConnectionEntry *aOriginalEntry)
 {
     if (!gHttpHandler->IsSpdyEnabled() ||
         !gHttpHandler->CoalesceSpdy() ||
         aOriginalEntry->mCoalescingKey.IsEmpty())
         return nsnull;
 
     nsConnectionEntry *preferred =
         mSpdyPreferredHash.Get(aOriginalEntry->mCoalescingKey);
@@ -621,17 +636,17 @@ nsHttpConnectionMgr::GetSpdyPreferred(ns
             break;
         }
     }
 
     if (!activeSpdy) {
         // remove the preferred status of this entry if it cannot be
         // used for pooling.
         preferred->mSpdyPreferred = false;
-        RemoveSpdyPreferred(preferred->mCoalescingKey);
+        RemoveSpdyPreferredEnt(preferred->mCoalescingKey);
         LOG(("nsHttpConnectionMgr::GetSpdyPreferredConnection "
              "preferred host mapping %s to %s removed due to inactivity.\n",
              aOriginalEntry->mConnInfo->Host(),
              preferred->mConnInfo->Host()));
 
         return nsnull;
     }
 
@@ -639,64 +654,56 @@ nsHttpConnectionMgr::GetSpdyPreferred(ns
     nsresult rv;
     bool isJoined = false;
 
     nsCOMPtr<nsISupports> securityInfo;
     nsCOMPtr<nsISSLSocketControl> sslSocketControl;
     nsCAutoString negotiatedNPN;
     
     activeSpdy->GetSecurityInfo(getter_AddRefs(securityInfo));
-    if (!securityInfo)
+    if (!securityInfo) {
+        NS_WARNING("cannot obtain spdy security info");
         return nsnull;
+    }
 
     sslSocketControl = do_QueryInterface(securityInfo, &rv);
-    if (NS_FAILED(rv))
+    if (NS_FAILED(rv)) {
+        NS_WARNING("sslSocketControl QI Failed");
         return nsnull;
+    }
 
     rv = sslSocketControl->JoinConnection(NS_LITERAL_CSTRING("spdy/2"),
                                           aOriginalEntry->mConnInfo->GetHost(),
                                           aOriginalEntry->mConnInfo->Port(),
                                           &isJoined);
 
     if (NS_FAILED(rv) || !isJoined) {
         LOG(("nsHttpConnectionMgr::GetSpdyPreferredConnection "
              "Host %s cannot be confirmed to be joined "
-             "with %s connections",
-             preferred->mConnInfo->Host(), aOriginalEntry->mConnInfo->Host()));
+             "with %s connections. rv=%x isJoined=%d",
+             preferred->mConnInfo->Host(), aOriginalEntry->mConnInfo->Host(),
+             rv, isJoined));
         mozilla::Telemetry::Accumulate(mozilla::Telemetry::SPDY_NPN_JOIN,
                                        false);
         return nsnull;
     }
 
     // IP pooling confirmed
     LOG(("nsHttpConnectionMgr::GetSpdyPreferredConnection "
-         "Host %s has cert valid for %s connections",
-         preferred->mConnInfo->Host(), aOriginalEntry->mConnInfo->Host()));
+         "Host %s has cert valid for %s connections, "
+         "so %s will be coalesced with %s",
+         preferred->mConnInfo->Host(), aOriginalEntry->mConnInfo->Host(),
+         aOriginalEntry->mConnInfo->Host(), preferred->mConnInfo->Host()));
     mozilla::Telemetry::Accumulate(mozilla::Telemetry::SPDY_NPN_JOIN, true);
     return preferred;
 }
 
 void
-nsHttpConnectionMgr::SetSpdyPreferred(nsConnectionEntry *ent)
+nsHttpConnectionMgr::RemoveSpdyPreferredEnt(nsACString &aHashKey)
 {
-    if (!gHttpHandler->CoalesceSpdy())
-        return;
-
-    if (ent->mCoalescingKey.IsEmpty())
-        return;
-    
-    mSpdyPreferredHash.Put(ent->mCoalescingKey, ent);
-}
-
-void
-nsHttpConnectionMgr::RemoveSpdyPreferred(nsACString &aHashKey)
-{
-    if (!gHttpHandler->CoalesceSpdy())
-        return;
-
     if (aHashKey.IsEmpty())
         return;
     
     mSpdyPreferredHash.Remove(aHashKey);
 }
 
 //-----------------------------------------------------------------------------
 // enumeration callbacks
@@ -745,55 +752,52 @@ nsHttpConnectionMgr::PruneDeadConnection
                                             void *closure)
 {
     nsHttpConnectionMgr *self = (nsHttpConnectionMgr *) closure;
 
     LOG(("  pruning [ci=%s]\n", ent->mConnInfo->HashKey().get()));
 
     // Find out how long it will take for next idle connection to not be reusable
     // anymore.
-    bool liveConnections = false;
     PRUint32 timeToNextExpire = PR_UINT32_MAX;
     PRInt32 count = ent->mIdleConns.Length();
     if (count > 0) {
         for (PRInt32 i=count-1; i>=0; --i) {
             nsHttpConnection *conn = ent->mIdleConns[i];
             if (!conn->CanReuse()) {
                 ent->mIdleConns.RemoveElementAt(i);
                 conn->Close(NS_ERROR_ABORT);
                 NS_RELEASE(conn);
                 self->mNumIdleConns--;
             } else {
                 timeToNextExpire = NS_MIN(timeToNextExpire, conn->TimeToLive());
-                liveConnections = true;
             }
         }
     }
 
     if (ent->mUsingSpdy) {
         for (PRUint32 index = 0; index < ent->mActiveConns.Length(); ++index) {
             nsHttpConnection *conn = ent->mActiveConns[index];
             if (conn->UsingSpdy()) {
                 if (!conn->CanReuse()) {
                     // marking it dont reuse will create an active tear down if
                     // the spdy session is idle.
                     conn->DontReuse();
                 }
                 else {
                     timeToNextExpire = NS_MIN(timeToNextExpire,
                                               conn->TimeToLive());
-                    liveConnections = true;
                 }
             }
         }
     }
     
     // If time to next expire found is shorter than time to next wake-up, we need to
     // change the time for next wake-up.
-    if (liveConnections) {
+    if (timeToNextExpire != PR_UINT32_MAX) {
         PRUint32 now = NowInSeconds();
         PRUint64 timeOfNextExpire = now + timeToNextExpire;
         // If pruning of dead connections is not already scheduled to happen
         // or time found for next connection to expire is is before
         // mTimeOfNextWakeUp, we need to schedule the pruning to happen
         // after timeToNextExpire.
         if (!self->mTimer || timeOfNextExpire < self->mTimeOfNextWakeUp) {
             self->PruneDeadConnectionsAfter(timeToNextExpire);
@@ -887,18 +891,17 @@ nsHttpConnectionMgr::ShutdownPassCB(cons
 
 bool
 nsHttpConnectionMgr::ProcessPendingQForEntry(nsConnectionEntry *ent)
 {
     NS_ABORT_IF_FALSE(PR_GetCurrentThread() == gSocketThread, "wrong thread");
     LOG(("nsHttpConnectionMgr::ProcessPendingQForEntry [ci=%s]\n",
         ent->mConnInfo->HashKey().get()));
 
-    if (gHttpHandler->IsSpdyEnabled())
-        ProcessSpdyPendingQ(ent);
+    ProcessSpdyPendingQ(ent);
 
     PRUint32 i, count = ent->mPendingQ.Length();
     if (count > 0) {
         LOG(("  pending-count=%u\n", count));
         nsHttpTransaction *trans = nsnull;
         nsHttpConnection *conn = nsnull;
         for (i = 0; i < count; ++i) {
             trans = ent->mPendingQ[i];
@@ -915,22 +918,19 @@ nsHttpConnectionMgr::ProcessPendingQForE
                     break;
                 }
             }
 
             GetConnection(ent, trans, alreadyHalfOpen, &conn);
             if (conn)
                 break;
 
-            // Check to see if a pending transaction was dispatched with the
-            // coalesce logic
-            if (count != ent->mPendingQ.Length()) {
-                count = ent->mPendingQ.Length();
-                i = 0;
-            }
+            NS_ABORT_IF_FALSE(count == ent->mPendingQ.Length(),
+                              "something mutated pending queue from "
+                              "GetConnection()");
         }
         if (conn) {
             LOG(("  dispatching pending transaction...\n"));
 
             // remove pending transaction
             ent->mPendingQ.RemoveElementAt(i);
 
             nsresult rv = DispatchTransaction(ent, trans, trans->Caps(), conn);
@@ -1106,19 +1106,19 @@ nsHttpConnectionMgr::GetConnection(nsCon
         // does not create new transports under any circumstances.
         if (onlyReusedConnection)
             return;
         
         if (gHttpHandler->IsSpdyEnabled() &&
             ent->mConnInfo->UsingSSL() &&
             !ent->mConnInfo->UsingHttpProxy())
         {
-            // If this is a possible Spdy connection we need to limit the number
-            // of connections outstanding to 1 while we wait for the spdy/https
-            // ReportSpdyConnection()
+            // If this host is trying to negotiate a SPDY session right now,
+            // don't create any new connections until the result of the
+            // negotiation is known.
     
             if ((!ent->mTestedSpdy || ent->mUsingSpdy) &&
                 (ent->mHalfOpens.Length() || ent->mActiveConns.Length()))
                 return;
         }
         
         // Check if we need to purge an idle connection. Note that we may have
         // removed one above; if so, this will be a no-op. We do this before
@@ -1326,17 +1326,17 @@ nsHttpConnectionMgr::ProcessNewTransacti
         ent = new nsConnectionEntry(clone);
         if (!ent)
             return NS_ERROR_OUT_OF_MEMORY;
         mCT.Put(ci->HashKey(), ent);
     }
 
     // SPDY coalescing of hostnames means we might redirect from this
     // connection entry onto the preferred one.
-    nsConnectionEntry *preferredEntry = GetSpdyPreferred(ent);
+    nsConnectionEntry *preferredEntry = GetSpdyPreferredEnt(ent);
     if (preferredEntry && (preferredEntry != ent)) {
         LOG(("nsHttpConnectionMgr::ProcessNewTransaction trans=%p "
              "redirected via coalescing from %s to %s\n", trans,
              ent->mConnInfo->Host(), preferredEntry->mConnInfo->Host()));
 
         ent = preferredEntry;
     }
 
@@ -1398,48 +1398,56 @@ nsHttpConnectionMgr::ProcessSpdyPendingQ
         nsHttpTransaction *trans = ent->mPendingQ[index];
 
         if (!(trans->Caps() & NS_HTTP_ALLOW_KEEPALIVE) ||
             trans->Caps() & NS_HTTP_DISALLOW_SPDY)
             continue;
  
         ent->mPendingQ.RemoveElementAt(index);
 
-        nsresult rv2 = DispatchTransaction(ent, trans, trans->Caps(), conn);
-        NS_ABORT_IF_FALSE(NS_SUCCEEDED(rv2), "Dispatch SPDY Transaction");
+        nsresult rv = DispatchTransaction(ent, trans, trans->Caps(), conn);
+        if (NS_FAILED(rv)) {
+            // this cannot happen, but if due to some bug it does then
+            // close the transaction
+            NS_ABORT_IF_FALSE(false, "Dispatch SPDY Transaction");
+            LOG(("ProcessSpdyPendingQ Dispatch Transaction failed trans=%p\n",
+                    trans));
+            trans->Close(rv);
+        }
         NS_RELEASE(trans);
     }
 }
 
 PLDHashOperator
 nsHttpConnectionMgr::ProcessSpdyPendingQCB(const nsACString &key,
                                            nsAutoPtr<nsConnectionEntry> &ent,
                                            void *closure)
 {
     nsHttpConnectionMgr *self = (nsHttpConnectionMgr *) closure;
     self->ProcessSpdyPendingQ(ent);
     return PL_DHASH_NEXT;
 }
 
 void
-nsHttpConnectionMgr::ProcessSpdyPendingQ()
+nsHttpConnectionMgr::ProcessAllSpdyPendingQ()
 {
     mCT.Enumerate(ProcessSpdyPendingQCB, this);
 }
 
 nsHttpConnection *
 nsHttpConnectionMgr::GetSpdyPreferredConn(nsConnectionEntry *ent)
 {
     NS_ABORT_IF_FALSE(PR_GetCurrentThread() == gSocketThread, "wrong thread");
     NS_ABORT_IF_FALSE(ent, "no connection entry");
 
-    nsConnectionEntry *preferred = GetSpdyPreferred(ent);
+    nsConnectionEntry *preferred = GetSpdyPreferredEnt(ent);
 
     // this entry is spdy-enabled if it is involved in a redirect
     if (preferred)
+        // all new connections for this entry will use spdy too
         ent->mUsingSpdy = true;
     else
         preferred = ent;
     
     nsHttpConnection *conn = nsnull;
     
     if (preferred->mUsingSpdy) {
         for (PRUint32 index = 0;
@@ -1589,30 +1597,38 @@ nsHttpConnectionMgr::OnMsgReclaimConnect
     //
 
     nsConnectionEntry *ent = LookupConnectionEntry(conn->ConnectionInfo(),
                                                    conn, nsnull);
     nsHttpConnectionInfo *ci = nsnull;
 
     if (!ent) {
         // this should never happen
-        NS_ASSERTION(ent, "no connection entry");
+        LOG(("nsHttpConnectionMgr::OnMsgReclaimConnection ent == null\n"));
+        NS_ABORT_IF_FALSE(false, "no connection entry");
         NS_ADDREF(ci = conn->ConnectionInfo());
     }
     else {
         NS_ADDREF(ci = ent->mConnInfo);
 
         // If the connection is in the active list, remove that entry
         // and the reference held by the mActiveConns list.
         // This is never the final reference on conn as the event context
         // is also holding one that is released at the end of this function.
 
-        if (ent->mUsingSpdy)
+        if (ent->mUsingSpdy) {
+            // Spdy connections aren't reused in the traditional HTTP way in
+            // the idleconns list, they are actively multplexed as active
+            // conns. Even when they have 0 transactions on them they are
+            // considered active connections. So when one is reclaimed it
+            // is really complete and is meant to be shut down and not
+            // reused.
             conn->DontReuse();
-
+        }
+        
         if (ent->mActiveConns.RemoveElement(conn)) {
             nsHttpConnection *temp = conn;
             NS_RELEASE(temp);
             mNumActiveConns--;
         }
 
         if (conn->CanReuse()) {
             LOG(("  adding connection to idle list\n"));
@@ -1685,17 +1701,17 @@ nsHttpConnectionMgr::OnMsgUpdateParam(PR
         NS_NOTREACHED("unexpected parameter name");
     }
 }
 
 // nsHttpConnectionMgr::nsConnectionEntry
 nsHttpConnectionMgr::nsConnectionEntry::~nsConnectionEntry()
 {
     if (mSpdyPreferred)
-        gHttpHandler->ConnMgr()->RemoveSpdyPreferred(mCoalescingKey);
+        gHttpHandler->ConnMgr()->RemoveSpdyPreferredEnt(mCoalescingKey);
 
     NS_RELEASE(mConnInfo);
 }
 
 //-----------------------------------------------------------------------------
 // nsHttpConnectionMgr::nsConnectionHandle
 
 nsHttpConnectionMgr::nsConnectionHandle::~nsConnectionHandle()
--- a/netwerk/protocol/http/nsHttpConnectionMgr.h
+++ b/netwerk/protocol/http/nsHttpConnectionMgr.h
@@ -195,22 +195,27 @@ private:
         // to build the hash key for hosts in the same ip pool.
         //
         // When a set of hosts are coalesced together one of them is marked
         // mSpdyPreferred. The mapping is maintained in the connection mananger
         // mSpdyPreferred hash.
         //
         nsCString mCoalescingKey;
 
-        // To have the UsingSpdy flag means some host with the same hash information
-        // has done NPN=spdy/2 at some point. It does not mean every connection
-        // is currently using spdy.
+        // To have the UsingSpdy flag means some host with the same connection
+        // entry has done NPN=spdy/2 at some point. It does not mean every
+        // connection is currently using spdy.
         bool mUsingSpdy;
 
+        // mTestedSpdy is set after NPN negotiation has occurred and we know
+        // with confidence whether a host speaks spdy or not (which is reflected
+        // in mUsingSpdy). Before mTestedSpdy is set, handshake parallelism is
+        // minimized so that we can multiplex on a single spdy connection.
         bool mTestedSpdy;
+
         bool mSpdyPreferred;
     };
 
     // nsConnectionHandle
     //
     // thin wrapper around a real connection, used to keep track of references
     // to the connection to determine when the connection may be reused.  the
     // transaction (or pipeline) owns a reference to this handle.  this extra
@@ -316,27 +321,26 @@ private:
     nsresult EnsureSocketThreadTargetIfOnline();
     void     ClosePersistentConnections(nsConnectionEntry *ent);
     nsresult CreateTransport(nsConnectionEntry *, nsHttpTransaction *);
     void     AddActiveConn(nsHttpConnection *, nsConnectionEntry *);
     void     StartedConnect();
     void     RecvdConnect();
 
     // Manage the preferred spdy connection entry for this address
-    nsConnectionEntry *GetSpdyPreferred(nsConnectionEntry *aOriginalEntry);
-    void               SetSpdyPreferred(nsConnectionEntry *ent);
-    void               RemoveSpdyPreferred(nsACString &aDottedDecimal);
+    nsConnectionEntry *GetSpdyPreferredEnt(nsConnectionEntry *aOriginalEntry);
+    void               RemoveSpdyPreferredEnt(nsACString &aDottedDecimal);
     nsHttpConnection  *GetSpdyPreferredConn(nsConnectionEntry *ent);
     nsDataHashtable<nsCStringHashKey, nsConnectionEntry *>   mSpdyPreferredHash;
     nsConnectionEntry *LookupConnectionEntry(nsHttpConnectionInfo *ci,
                                              nsHttpConnection *conn,
                                              nsHttpTransaction *trans);
 
     void               ProcessSpdyPendingQ(nsConnectionEntry *ent);
-    void               ProcessSpdyPendingQ();
+    void               ProcessAllSpdyPendingQ();
     static PLDHashOperator ProcessSpdyPendingQCB(
         const nsACString &key, nsAutoPtr<nsConnectionEntry> &ent,
         void *closure);
 
     // message handlers have this signature
     typedef void (nsHttpConnectionMgr:: *nsConnEventHandler)(PRInt32, void *);
 
     // nsConnEvent
@@ -409,17 +413,17 @@ private:
     //
     // the connection table
     //
     // this table is indexed by connection key.  each entry is a
     // nsConnectionEntry object.
     //
     nsClassHashtable<nsCStringHashKey, nsConnectionEntry> mCT;
 
-    // this table is protected by the monitor
-    nsCStringHashSet mAlternateProtocolHash;
+    // mAlternateProtocolHash is used only for spdy/2 upgrades for now
+    nsCStringHashSet mAlternateProtocolHash; // protected by the monitor
     static PLDHashOperator TrimAlternateProtocolHash(PLDHashTable *table,
                                                      PLDHashEntryHdr *hdr,
                                                      PRUint32 number,
                                                      void *closure);
 };
 
 #endif // !nsHttpConnectionMgr_h__