bug 865314 - ssl parallelism to new host should not be 1 r=hurley
authorPatrick McManus <mcmanus@ducksong.com>
Thu, 30 Jan 2014 03:56:36 -0500
changeset 212250 d4845054ea1823f0a3be3ca6f741bee59706f1d8
parent 212249 e5d757477ec15b623b072a56f986e1b0fee516a9
child 212251 e981fd5453dc71703d19408bbf78d1ad8685e6da
push id27702
push userkwierso@gmail.com
push dateFri, 24 Oct 2014 22:05:50 +0000
treeherdermozilla-central@c70f62375f7d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershurley
bugs865314
milestone36.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 865314 - ssl parallelism to new host should not be 1 r=hurley
netwerk/base/public/nsISpeculativeConnect.idl
netwerk/base/src/Predictor.cpp
netwerk/protocol/http/AlternateServices.cpp
netwerk/protocol/http/ConnectionDiagnostics.cpp
netwerk/protocol/http/nsHttpConnection.cpp
netwerk/protocol/http/nsHttpConnectionMgr.cpp
netwerk/protocol/http/nsHttpConnectionMgr.h
--- a/netwerk/base/public/nsISpeculativeConnect.idl
+++ b/netwerk/base/public/nsISpeculativeConnect.idl
@@ -30,33 +30,26 @@ interface nsISpeculativeConnect : nsISup
 
 };
 
 /**
  * This is used to override the default values for various values (documented
  * inline) to determine whether or not to actually make a speculative
  * connection.
  */
-[builtinclass, uuid(f6a0d1e5-369f-4abc-81ae-d370d36e4006)]
+[builtinclass, uuid(1040ebe3-6ed1-45a6-8587-995e082518d7)]
 interface nsISpeculativeConnectionOverrider : nsISupports
 {
     /**
      * Used to determine the maximum number of unused speculative connections
      * we will have open for a host at any one time
      */
     [infallible] readonly attribute unsigned long parallelSpeculativeConnectLimit;
 
     /**
-     * Used to loosen the restrictions nsHttpConnectionMgr::RestrictConnections
-     * to allow more speculative connections when we're unsure if a host will
-     * connect via SPDY or not.
-     */
-    [infallible] readonly attribute boolean ignorePossibleSpdyConnections;
-
-    /**
      * Used to determine if we will ignore the existence of any currently idle
      * connections when we decide whether or not to make a speculative
      * connection.
      */
     [infallible] readonly attribute boolean ignoreIdle;
 
     /*
      * Used by the Predictor to gather telemetry data on speculative connection
--- a/netwerk/base/src/Predictor.cpp
+++ b/netwerk/base/src/Predictor.cpp
@@ -369,23 +369,16 @@ Predictor::Observe(nsISupports *subject,
 NS_IMETHODIMP
 Predictor::GetIgnoreIdle(bool *ignoreIdle)
 {
   *ignoreIdle = true;
   return NS_OK;
 }
 
 NS_IMETHODIMP
-Predictor::GetIgnorePossibleSpdyConnections(bool *ignorePossibleSpdyConnections)
-{
-  *ignorePossibleSpdyConnections = true;
-  return NS_OK;
-}
-
-NS_IMETHODIMP
 Predictor::GetParallelSpeculativeConnectLimit(
     uint32_t *parallelSpeculativeConnectLimit)
 {
   *parallelSpeculativeConnectLimit = 6;
   return NS_OK;
 }
 
 NS_IMETHODIMP
--- a/netwerk/protocol/http/AlternateServices.cpp
+++ b/netwerk/protocol/http/AlternateServices.cpp
@@ -410,23 +410,16 @@ AltSvcOverride::GetInterface(const nsIID
 NS_IMETHODIMP
 AltSvcOverride::GetIgnoreIdle(bool *ignoreIdle)
 {
   *ignoreIdle = true;
   return NS_OK;
 }
 
 NS_IMETHODIMP
-AltSvcOverride::GetIgnorePossibleSpdyConnections(bool *ignorePossibleSpdyConnections)
-{
-  *ignorePossibleSpdyConnections = true;
-  return NS_OK;
-}
-
-NS_IMETHODIMP
 AltSvcOverride::GetParallelSpeculativeConnectLimit(
   uint32_t *parallelSpeculativeConnectLimit)
 {
   *parallelSpeculativeConnectLimit = 32;
   return NS_OK;
 }
 
 NS_IMETHODIMP
--- a/netwerk/protocol/http/ConnectionDiagnostics.cpp
+++ b/netwerk/protocol/http/ConnectionDiagnostics.cpp
@@ -68,18 +68,18 @@ nsHttpConnectionMgr::PrintDiagnosticsCB(
   self->mLogData.AppendPrintf("   Active Conns Length = %u\n",
                               ent->mActiveConns.Length());
   self->mLogData.AppendPrintf("   Idle Conns Length = %u\n",
                               ent->mIdleConns.Length());
   self->mLogData.AppendPrintf("   Half Opens Length = %u\n",
                               ent->mHalfOpens.Length());
   self->mLogData.AppendPrintf("   Coalescing Key = %s\n",
                               ent->mCoalescingKey.get());
-  self->mLogData.AppendPrintf("   Spdy using = %d, tested = %d, preferred = %d\n",
-                              ent->mUsingSpdy, ent->mTestedSpdy, ent->mSpdyPreferred);
+  self->mLogData.AppendPrintf("   Spdy using = %d, preferred = %d\n",
+                              ent->mUsingSpdy, ent->mSpdyPreferred);
   self->mLogData.AppendPrintf("   pipelinestate = %d penalty = %d\n",
                               ent->mPipelineState, ent->mPipeliningPenalty);
   for (i = 0; i < nsAHttpTransaction::CLASS_MAX; ++i) {
     self->mLogData.AppendPrintf("   pipeline per class penalty 0x%x %d\n",
                                 i, ent->mPipeliningClassPenalty[i]);
   }
   for (i = 0; i < ent->mActiveConns.Length(); ++i) {
     self->mLogData.AppendPrintf("   :: Active Connection #%u\n", i);
--- a/netwerk/protocol/http/nsHttpConnection.cpp
+++ b/netwerk/protocol/http/nsHttpConnection.cpp
@@ -154,16 +154,17 @@ void
 nsHttpConnection::StartSpdy(uint8_t spdyVersion)
 {
     LOG(("nsHttpConnection::StartSpdy [this=%p]\n", this));
 
     MOZ_ASSERT(!mSpdySession);
 
     mUsingSpdyVersion = spdyVersion;
     mEverUsedSpdy = true;
+    mSpdySession = ASpdySession::NewSpdySession(spdyVersion, mSocketTransport);
 
     if (!mReportedSpdy) {
         mReportedSpdy = true;
         gHttpHandler->ConnMgr()->ReportSpdyConnection(this, true);
     }
 
     // Setting the connection as reused allows some transactions that fail
     // with NS_ERROR_NET_RESET to be restarted and SPDY uses that code
@@ -204,17 +205,16 @@ nsHttpConnection::StartSpdy(uint8_t spdy
               "Proxy and Need Connect", this));
         MOZ_ASSERT(mProxyConnectStream);
 
         mProxyConnectStream = nullptr;
         mCompletedProxyConnect = true;
         mProxyConnectInProgress = false;
     }
 
-    mSpdySession = ASpdySession::NewSpdySession(spdyVersion, mSocketTransport);
     bool spdyProxy = mConnInfo->UsingHttpsProxy() && !mTLSFilter;
     if (spdyProxy) {
         nsRefPtr<nsHttpConnectionInfo> wildCardProxyCi;
         mConnInfo->CreateWildCard(getter_AddRefs(wildCardProxyCi));
         gHttpHandler->ConnMgr()->MoveToWildCardConnEntry(mConnInfo,
                                                          wildCardProxyCi, this);
         mConnInfo = wildCardProxyCi;
     }
--- a/netwerk/protocol/http/nsHttpConnectionMgr.cpp
+++ b/netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ -375,17 +375,16 @@ public:
     NS_IMETHOD_(MozExternalRefCountType) Release(void);
 
 public: // intentional!
     nsRefPtr<NullHttpTransaction> mTrans;
 
     bool mOverridesOK;
     uint32_t mParallelSpeculativeConnectLimit;
     bool mIgnoreIdle;
-    bool mIgnorePossibleSpdyConnections;
     bool mIsFromPredictor;
     bool mAllow1918;
 
     // As above, added manually so we can use nsRefPtr without inheriting from
     // nsISupports
 protected:
     ThreadSafeAutoRefCnt mRefCnt;
     NS_DECL_OWNINGTHREAD
@@ -432,18 +431,16 @@ nsHttpConnectionMgr::SpeculativeConnect(
     args->mTrans =
         nullTransaction ? nullTransaction : new NullHttpTransaction(ci, wrappedCallbacks, caps);
 
     if (overrider) {
         args->mOverridesOK = true;
         overrider->GetParallelSpeculativeConnectLimit(
             &args->mParallelSpeculativeConnectLimit);
         overrider->GetIgnoreIdle(&args->mIgnoreIdle);
-        overrider->GetIgnorePossibleSpdyConnections(
-            &args->mIgnorePossibleSpdyConnections);
         overrider->GetIsFromPredictor(&args->mIsFromPredictor);
         overrider->GetAllow1918(&args->mAllow1918);
     }
 
     nsresult rv =
         PostEvent(&nsHttpConnectionMgr::OnMsgSpeculativeConnect, 0, args);
     if (NS_SUCCEEDED(rv))
         unused << args.forget();
@@ -650,18 +647,16 @@ nsHttpConnectionMgr::ReportSpdyConnectio
     MOZ_ASSERT(PR_GetCurrentThread() == gSocketThread);
 
     nsConnectionEntry *ent = LookupConnectionEntry(conn->ConnectionInfo(),
                                                    conn, nullptr);
 
     if (!ent)
         return;
 
-    ent->mTestedSpdy = true;
-
     if (!usingSpdy)
         return;
 
     ent->mUsingSpdy = true;
     mNumSpdyActiveConns++;
 
     uint32_t ttl = conn->TimeToLive();
     uint64_t timeOfExpire = NowInSeconds() + ttl;
@@ -672,44 +667,89 @@ nsHttpConnectionMgr::ReportSpdyConnectio
     // 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 *joinedConnection;
     nsConnectionEntry *preferred =
         mSpdyPreferredHash.Get(ent->mCoalescingKey);
 
-    LOG(("ReportSpdyConnection %s %s ent=%p preferred=%p\n",
-         ent->mConnInfo->Host(), ent->mCoalescingKey.get(),
-         ent, preferred));
+    LOG(("ReportSpdyConnection conn=%p %s %s ent=%p preferred=%p\n",conn,
+         ent->mConnInfo->Host(), ent->mCoalescingKey.get(), ent, preferred));
 
     if (!preferred) {
+        preferred = ent;
         if (!ent->mCoalescingKey.IsEmpty()) {
             mSpdyPreferredHash.Put(ent->mCoalescingKey, ent);
             ent->mSpdyPreferred = true;
         }
     } else if ((preferred != ent) &&
                (joinedConnection = GetSpdyPreferredEnt(ent)) &&
                (joinedConnection != ent)) {
         //
         // A connection entry (e.g. made with a different hostname) with
         // the same IP address is preferred for future transactions over this
         // connection entry. Gracefully close down the connection to help
         // new transactions migrate over.
 
         LOG(("ReportSpdyConnection graceful close of conn=%p ent=%p to "
-                 "migrate to preferred\n", conn, ent));
-
+                 "migrate to preferred (desharding)\n", conn, ent));
         conn->DontReuse();
     } else if (preferred != ent) {
         LOG (("ReportSpdyConnection preferred host may be in false start or "
               "may have insufficient cert. Leave mapping in place but do not "
               "abandon this connection yet."));
     }
 
+    if (preferred == ent) {
+        // this is a new spdy connection to the preferred entry
+        for (int32_t index = ent->mHalfOpens.Length() - 1;
+             (index >= 0) &&  conn->CanDirectlyActivate(); --index) {
+
+            // Abandon all other half-open sockets belonging to the given transaction.
+            // While activating their transactions on the new conn
+
+            nsHalfOpenSocket *half = ent->mHalfOpens[index];
+            nsAHttpTransaction *abstractTrans = half->Transaction();
+            nsRefPtr<nsHttpTransaction> concreteTrans = abstractTrans->QueryHttpTransaction();
+
+            if (!concreteTrans) {
+                continue;
+            }
+
+            LOG(("ReportSpdyConnection conn %p taking transaction %p from a "
+                 "half open that it will cancel\n", conn, concreteTrans.get()));
+
+            // concreteTrans is holding a ref to the transaction in half - so it
+            // is ok to destroy half
+            ent->RemoveHalfOpen(half);
+            half->Abandon();
+
+            nsresult rv = DispatchTransaction(ent, concreteTrans, conn);
+            MOZ_ASSERT(NS_SUCCEEDED(rv)); // this cannot fail
+        }
+
+        if (conn->CanDirectlyActivate() && ent->mActiveConns.Length() > 1) {
+            // this is a new connection to an established preferred spdy host.
+            // if there is more than 1 live and established spdy connection (e.g.
+            // some could still be handshaking, shutting down, etc..) then close
+            // this one down after any transactions that are on it are complete.
+            // This probably happened due to the parallel connection algorithm
+            // that is used only before the host is known to speak spdy.
+            for (uint32_t index = 0; index < ent->mActiveConns.Length(); ++index) {
+                nsHttpConnection *otherConn = ent->mActiveConns[index];
+                if (otherConn != conn) {
+                    LOG(("ReportSpdyConnection shutting down connection (%p) because new "
+                         "spdy connection (%p) takes precedence\n", otherConn, conn));
+                    otherConn->DontReuse();
+                }
+            }
+        }
+    }
+
     ProcessPendingQ(ent->mConnInfo);
     PostEvent(&nsHttpConnectionMgr::OnMsgProcessAllSpdyPendingQ);
 }
 
 void
 nsHttpConnectionMgr::ReportSpdyCWNDSetting(nsHttpConnectionInfo *ci,
                                            uint32_t cwndValue)
 {
@@ -1022,19 +1062,17 @@ nsHttpConnectionMgr::PruneDeadConnection
     // red condition, then we can clean it up and restart from
     // yellow
     if (ent->PipelineState()       != PS_RED &&
         self->mCT.Count()          >  125 &&
         ent->mIdleConns.Length()   == 0 &&
         ent->mActiveConns.Length() == 0 &&
         ent->mHalfOpens.Length()   == 0 &&
         ent->mPendingQ.Length()    == 0 &&
-        ((!ent->mTestedSpdy && !ent->mUsingSpdy) ||
-         !gHttpHandler->IsSpdyEnabled() ||
-         self->mCT.Count() > 300)) {
+        (!ent->mUsingSpdy || self->mCT.Count() > 300)) {
         LOG(("    removing empty connection entry\n"));
         return PL_DHASH_REMOVE;
     }
 
     // otherwise use this opportunity to compact our arrays...
     ent->mIdleConns.Compact();
     ent->mActiveConns.Compact();
     ent->mPendingQ.Compact();
@@ -1404,39 +1442,38 @@ nsHttpConnectionMgr::ClosePersistentConn
                                                   void *closure)
 {
     nsHttpConnectionMgr *self = static_cast<nsHttpConnectionMgr *>(closure);
     self->ClosePersistentConnections(ent);
     return PL_DHASH_NEXT;
 }
 
 bool
-nsHttpConnectionMgr::RestrictConnections(nsConnectionEntry *ent,
-                                         bool ignorePossibleSpdyConnections)
+nsHttpConnectionMgr::RestrictConnections(nsConnectionEntry *ent)
 {
     MOZ_ASSERT(PR_GetCurrentThread() == gSocketThread);
 
     // 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->mTestedSpdy && !ignorePossibleSpdyConnections) ||
-         ent->mUsingSpdy) &&
+        ent->mUsingSpdy &&
         (ent->mHalfOpens.Length() || ent->mActiveConns.Length());
 
     // If there are no restrictions, we are done
     if (!doRestrict)
         return false;
 
     // If the restriction is based on a tcp handshake in progress
     // let that connect and then see if it was SPDY or not
-    if (ent->UnconnectedHalfOpens() && !ignorePossibleSpdyConnections)
+    if (ent->UnconnectedHalfOpens()) {
         return true;
+    }
 
     // There is a concern that a host is using a mix of HTTP/1 and SPDY.
     // In that case we don't want to restrict connections just because
     // there is a single active HTTP/1 session in use.
     if (ent->mUsingSpdy && ent->mActiveConns.Length()) {
         bool confirmedRestrict = false;
         for (uint32_t index = 0; index < ent->mActiveConns.Length(); ++index) {
             nsHttpConnection *conn = ent->mActiveConns[index];
@@ -2940,34 +2977,32 @@ nsHttpConnectionMgr::OnMsgSpeculativeCon
     // the ip pooling rules. If so, connect to the preferred host instead of
     // the one directly passed in here.
     nsConnectionEntry *preferredEntry = GetSpdyPreferredEnt(ent);
     if (preferredEntry)
         ent = preferredEntry;
 
     uint32_t parallelSpeculativeConnectLimit =
         gHttpHandler->ParallelSpeculativeConnectLimit();
-    bool ignorePossibleSpdyConnections = false;
     bool ignoreIdle = false;
     bool isFromPredictor = false;
     bool allow1918 = false;
 
     if (args->mOverridesOK) {
         parallelSpeculativeConnectLimit = args->mParallelSpeculativeConnectLimit;
-        ignorePossibleSpdyConnections = args->mIgnorePossibleSpdyConnections;
         ignoreIdle = args->mIgnoreIdle;
         isFromPredictor = args->mIsFromPredictor;
         allow1918 = args->mAllow1918;
     }
 
     bool keepAlive = args->mTrans->Caps() & NS_HTTP_ALLOW_KEEPALIVE;
     if (mNumHalfOpenConns < parallelSpeculativeConnectLimit &&
         ((ignoreIdle && (ent->mIdleConns.Length() < parallelSpeculativeConnectLimit)) ||
          !ent->mIdleConns.Length()) &&
-        !(keepAlive && RestrictConnections(ent, ignorePossibleSpdyConnections)) &&
+        !(keepAlive && RestrictConnections(ent)) &&
         !AtActiveConnectionLimit(ent, args->mTrans->Caps())) {
         CreateTransport(ent, args->mTrans, args->mTrans->Caps(), true, isFromPredictor, allow1918);
     }
     else {
         LOG(("  Transport not created due to existing connection count\n"));
     }
 }
 
@@ -3321,18 +3356,19 @@ nsHalfOpenSocket::OnOutputStreamReady(ns
 
         if (NS_SUCCEEDED(mSocketTransport->GetPeerAddr(&peeraddr)))
             mEnt->RecordIPFamilyPreference(peeraddr.raw.family);
 
         // The nsHttpConnection object now owns these streams and sockets
         mStreamOut = nullptr;
         mStreamIn = nullptr;
         mSocketTransport = nullptr;
-    }
-    else {
+    } else {
+        MOZ_ASSERT(!mTransaction->IsNullTransaction(),
+                   "null transactions dont have backup timers");
         TimeDuration rtt = TimeStamp::Now() - mBackupSynStarted;
         rv = conn->Init(mEnt->mConnInfo,
                         gHttpHandler->ConnMgr()->mMaxRequestDelay,
                         mBackupTransport, mBackupStreamIn, mBackupStreamOut,
                         mBackupConnectedOK, callbacks,
                         PR_MillisecondsToInterval(
                           static_cast<uint32_t>(rtt.ToMilliseconds())));
 
@@ -3552,17 +3588,16 @@ nsConnectionEntry::nsConnectionEntry(nsH
     , mPipelineState(PS_YELLOW)
     , mYellowGoodEvents(0)
     , mYellowBadEvents(0)
     , mYellowConnection(nullptr)
     , mGreenDepth(kPipelineOpen)
     , mPipeliningPenalty(0)
     , mSpdyCWND(0)
     , mUsingSpdy(false)
-    , mTestedSpdy(false)
     , mSpdyPreferred(false)
     , mPreferIPv4(false)
     , mPreferIPv6(false)
 {
     MOZ_COUNT_CTOR(nsConnectionEntry);
     if (gHttpHandler->GetPipelineAggressive()) {
         mGreenDepth = kPipelineUnlimited;
         mPipelineState = PS_GREEN;
@@ -3950,17 +3985,16 @@ nsHttpConnectionMgr::MoveToWildCardConnE
     }
 
     nsConnectionEntry *wcEnt = GetOrCreateConnectionEntry(wildCardCI, true);
     if (wcEnt == ent) {
         // nothing to do!
         return;
     }
     wcEnt->mUsingSpdy = true;
-    wcEnt->mTestedSpdy = true;
 
     LOG(("nsHttpConnectionMgr::MakeConnEntryWildCard ent %p "
          "idle=%d active=%d half=%d pending=%d\n", ent,
          ent->mIdleConns.Length(), ent->mActiveConns.Length(),
          ent->mHalfOpens.Length(), ent->mPendingQ.Length()));
 
     LOG(("nsHttpConnectionMgr::MakeConnEntryWildCard wc-ent %p "
          "idle=%d active=%d half=%d pending=%d\n", wcEnt,
--- a/netwerk/protocol/http/nsHttpConnectionMgr.h
+++ b/netwerk/protocol/http/nsHttpConnectionMgr.h
@@ -371,22 +371,16 @@ private:
         uint32_t            mSpdyCWND;
         TimeStamp  mSpdyCWNDTimeStamp;
 
         // To have the UsingSpdy flag means some host with the same connection
         // entry has done NPN=spdy/* 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;
 
         // Flags to remember our happy-eyeballs decision.
         // Reset only by Ctrl-F5 reload.
         // True when we've first connected an IPv4 server for this host,
         // initially false.
         bool mPreferIPv4 : 1;
         // True when we've first connected an IPv6 server for this host,
@@ -560,17 +554,17 @@ private:
     nsresult DispatchAbstractTransaction(nsConnectionEntry *,
                                          nsAHttpTransaction *,
                                          uint32_t,
                                          nsHttpConnection *,
                                          int32_t);
     nsresult BuildPipeline(nsConnectionEntry *,
                            nsAHttpTransaction *,
                            nsHttpPipeline **);
-    bool     RestrictConnections(nsConnectionEntry *, bool = false);
+    bool     RestrictConnections(nsConnectionEntry *);
     nsresult ProcessNewTransaction(nsHttpTransaction *);
     nsresult EnsureSocketThreadTarget();
     void     ClosePersistentConnections(nsConnectionEntry *ent);
     void     ReportProxyTelemetry(nsConnectionEntry *ent);
     nsresult CreateTransport(nsConnectionEntry *, nsAHttpTransaction *,
                              uint32_t, bool, bool, bool);
     void     AddActiveConn(nsHttpConnection *, nsConnectionEntry *);
     void     DecrementActiveConnCount(nsHttpConnection *);