Bug 1355591 - h2 coalescing creating unused sessions r=nwgh draft
authorPatrick McManus <mcmanus@ducksong.com>
Thu, 13 Apr 2017 10:09:10 -0400
changeset 562142 1625dfa7f08c36510ec46ef796abf64546e87235
parent 561911 819a666afddc804b6099ee1b3cff3a0fdf35ec15
child 624179 3a9cdee1452c42108921f0c15612d2ccbafbdb04
push id53962
push userbmo:mcmanus@ducksong.com
push dateThu, 13 Apr 2017 14:25:56 +0000
reviewersnwgh
bugs1355591
milestone55.0a1
Bug 1355591 - h2 coalescing creating unused sessions r=nwgh MozReview-Commit-ID: 5aMrdcBep5M
netwerk/protocol/http/nsHttpConnectionMgr.cpp
--- a/netwerk/protocol/http/nsHttpConnectionMgr.cpp
+++ b/netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ -638,22 +638,22 @@ nsHttpConnectionMgr::FindCoalescableConn
 
         bool couldJoin;
         if (justKidding) {
             couldJoin = potentialMatch->TestJoinConnection(ci->GetOrigin(), ci->OriginPort());
         } else {
             couldJoin = potentialMatch->JoinConnection(ci->GetOrigin(), ci->OriginPort());
         }
         if (couldJoin) {
-            LOG(("FindCoalescableConnectionByHashKey() found hashtable match %p for key %s of CI %s join ok\n",
-                 potentialMatch.get(), key.get(), ci->HashKey().get()));
+            LOG(("FindCoalescableConnectionByHashKey() found match conn=%p key=%s newCI=%s matchedCI=%s join ok\n",
+                 potentialMatch.get(), key.get(), ci->HashKey().get(), potentialMatch->ConnectionInfo()->HashKey().get()));
             return potentialMatch.get();
         } else {
-            LOG(("FindCoalescableConnectionByHashKey() found hashtable match %p for key %s of CI %s join failed\n",
-                 potentialMatch.get(), key.get(), ci->HashKey().get()));
+            LOG(("FindCoalescableConnectionByHashKey() found match conn=%p key=%s newCI=%s matchedCI=%s join failed\n",
+                 potentialMatch.get(), key.get(), ci->HashKey().get(), potentialMatch->ConnectionInfo()->HashKey().get()));
         }
         ++j; // bypassed by continue when weakptr fails
     }
 
     if (!listLen) { // shrunk to 0 while iterating
         LOG(("FindCoalescableConnectionByHashKey() removing empty list element\n"));
         mCoalescingHash.Remove(key);
     }
@@ -709,51 +709,51 @@ nsHttpConnectionMgr::FindCoalescableConn
         }
     }
 
     LOG(("FindCoalescableConnection(%s) no matching conn\n", ci->HashKey().get()));
     return nullptr;
 }
 
 void
-nsHttpConnectionMgr::UpdateCoalescingForNewConn(nsHttpConnection *conn,
+nsHttpConnectionMgr::UpdateCoalescingForNewConn(nsHttpConnection *newConn,
                                                 nsConnectionEntry *ent)
 {
     MOZ_ASSERT(PR_GetCurrentThread() == gSocketThread);
-    MOZ_ASSERT(conn);
-    MOZ_ASSERT(conn->ConnectionInfo());
+    MOZ_ASSERT(newConn);
+    MOZ_ASSERT(newConn->ConnectionInfo());
     MOZ_ASSERT(ent);
-    MOZ_ASSERT(mCT.Get(conn->ConnectionInfo()->HashKey()) == ent);
+    MOZ_ASSERT(mCT.Get(newConn->ConnectionInfo()->HashKey()) == ent);
 
     nsHttpConnection *existingConn = FindCoalescableConnection(ent, true);
     if (existingConn) {
-        LOG(("UpdateCoalescingForNewConn() found active conn that could have served newConn\n"
-             "graceful close of conn=%p to migrate to %p\n", conn, existingConn));
-        conn->DontReuse();
+        LOG(("UpdateCoalescingForNewConn() found existing active conn that could have served newConn "
+             "graceful close of newConn=%p to migrate to existingConn %p\n", newConn, existingConn));
+        newConn->DontReuse();
         return;
     }
 
     // This connection might go into the mCoalescingHash for new transactions to be coalesced onto
     // if it can accept new transactions
-    if (!conn->CanDirectlyActivate()) {
+    if (!newConn->CanDirectlyActivate()) {
         return;
     }
 
     uint32_t keyLen = ent->mCoalescingKeys.Length();
     for (uint32_t i = 0;i < keyLen; ++i) {
-        LOG(("UpdateCoalescingForNewConn() registering conn %p %s under key %s\n",
-             conn, conn->ConnectionInfo()->HashKey().get(), ent->mCoalescingKeys[i].get()));
+        LOG(("UpdateCoalescingForNewConn() registering newConn %p %s under key %s\n",
+             newConn, newConn->ConnectionInfo()->HashKey().get(), ent->mCoalescingKeys[i].get()));
         nsTArray<nsWeakPtr> *listOfWeakConns =  mCoalescingHash.Get(ent->mCoalescingKeys[i]);
         if (!listOfWeakConns) {
             LOG(("UpdateCoalescingForNewConn() need new list element\n"));
             listOfWeakConns = new nsTArray<nsWeakPtr>(1);
             mCoalescingHash.Put(ent->mCoalescingKeys[i], listOfWeakConns);
         }
         listOfWeakConns->AppendElement(
-            do_GetWeakReference(static_cast<nsISupportsWeakReference*>(conn)));
+            do_GetWeakReference(static_cast<nsISupportsWeakReference*>(newConn)));
     }
 
     // Cancel any other pending connections - their associated transactions
     // are in the pending queue and will be dispatched onto this new connection
     for (int32_t index = ent->mHalfOpens.Length() - 1; index >= 0; --index) {
         LOG(("UpdateCoalescingForNewConn() forcing halfopen abandon %p\n",
              ent->mHalfOpens[index]));
         ent->mHalfOpens[index]->Abandon();
@@ -763,19 +763,19 @@ nsHttpConnectionMgr::UpdateCoalescingFor
         // this is a new connection that can be coalesced onto. hooray!
         // if there are other connection to this entry (e.g.
         // some could still be handshaking, shutting down, etc..) then close
         // them down after any transactions that are on them are complete.
         // This probably happened due to the parallel connection algorithm
         // that is used only before the host is known to speak h2.
         for (uint32_t index = 0; index < ent->mActiveConns.Length(); ++index) {
             nsHttpConnection *otherConn = ent->mActiveConns[index];
-            if (otherConn != conn) {
-                LOG(("UpdateCoalescingForNewConn() shutting down connection (%p) because new "
-                     "spdy connection (%p) takes precedence\n", otherConn, conn));
+            if (otherConn != newConn) {
+                LOG(("UpdateCoalescingForNewConn() shutting down old connection (%p) because new "
+                     "spdy connection (%p) takes precedence\n", otherConn, newConn));
                 otherConn->DontReuse();
             }
         }
     }
 }
 
 // This function lets a connection, after completing the NPN phase,
 // report whether or not it is using spdy through the usingSpdy
@@ -949,23 +949,23 @@ nsHttpConnectionMgr::AvailableNewConnect
 }
 
 bool
 nsHttpConnectionMgr::ProcessPendingQForEntry(nsConnectionEntry *ent, bool considerAll)
 {
     MOZ_ASSERT(PR_GetCurrentThread() == gSocketThread);
 
     LOG(("nsHttpConnectionMgr::ProcessPendingQForEntry "
-         "[ci=%s ent=%p active=%" PRIuSIZE " idle=%" PRIuSIZE " urgent-start queue=%" PRIuSIZE
-         "queued=%" PRIuSIZE "]\n",
+         "[ci=%s ent=%p active=%" PRIuSIZE " idle=%" PRIuSIZE " urgent-start-queue=%" PRIuSIZE
+         " queued=%" PRIuSIZE "]\n",
          ent->mConnInfo->HashKey().get(), ent, ent->mActiveConns.Length(),
          ent->mIdleConns.Length(), ent->mUrgentStartQ.Length(),
          ent->PendingQLength()));
 
-    if (!ent->mUrgentStartQ.Length() && !ent->mPendingTransactionTable.Count()) {
+    if (!ent->mUrgentStartQ.Length() && !ent->PendingQLength()) {
         return false;
     }
     ProcessSpdyPendingQ(ent);
 
     bool dispatchedSuccessfully = false;
 
     if (!ent->mUrgentStartQ.IsEmpty()) {
         dispatchedSuccessfully = DispatchPendingQ(ent->mUrgentStartQ,
@@ -1144,16 +1144,25 @@ nsHttpConnectionMgr::ClosePersistentConn
         ent->mActiveConns[i]->DontReuse();
 }
 
 bool
 nsHttpConnectionMgr::RestrictConnections(nsConnectionEntry *ent)
 {
     MOZ_ASSERT(PR_GetCurrentThread() == gSocketThread);
 
+    if (ent->AvailableForDispatchNow()) {
+        // this might be a h2/spdy connection in this connection entry that
+        // is able to be immediately muxxed, or it might be one that
+        // was found in the same state through a coalescing hash
+        LOG(("nsHttpConnectionMgr::RestrictConnections %p %s restricted due to active >=h2\n",
+             ent, ent->mConnInfo->HashKey().get()));
+        return true;
+    }
+
     // If this host is trying to negotiate a SPDY session right now,
     // don't create any new ssl connections until the result of the
     // negotiation is known.
 
     bool doRestrict =
         ent->mConnInfo->FirstHopSSL() && gHttpHandler->IsSpdyEnabled() &&
         ent->mUsingSpdy && (ent->mHalfOpens.Length() || ent->mActiveConns.Length());