Backed out changeset 06acd829f970 (bug 865314) for causing new topcrashes. a=me
authorRyan VanderMeulen <ryanvm@gmail.com>
Mon, 27 Oct 2014 14:26:13 -0400
changeset 212450 da125623d9cb2840648ecdbe8af26cb634a25123
parent 212449 20408ad61ce5a530d7e563da564efb9bb6b8658f
child 212471 d0cd2665184a9e9c6e58f7f94c5a8e14e20e5c6f
push id27712
push userryanvm@gmail.com
push dateMon, 27 Oct 2014 18:27:36 +0000
treeherdermozilla-central@da125623d9cb [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersme
bugs865314
milestone36.0a1
backs out06acd829f970fec501f5c2052a5f4420fd9a5904
first release with
nightly linux32
da125623d9cb / 36.0a1 / 20141027113017 / files
nightly linux64
da125623d9cb / 36.0a1 / 20141027113017 / files
nightly mac
da125623d9cb / 36.0a1 / 20141027113017 / files
nightly win32
da125623d9cb / 36.0a1 / 20141027113017 / files
nightly win64
da125623d9cb / 36.0a1 / 20141027113017 / files
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
releases
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Backed out changeset 06acd829f970 (bug 865314) for causing new topcrashes. a=me
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,26 +30,33 @@ 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(1040ebe3-6ed1-45a6-8587-995e082518d7)]
+[builtinclass, uuid(f6a0d1e5-369f-4abc-81ae-d370d36e4006)]
 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,16 +369,23 @@ 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,16 +410,23 @@ 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, preferred = %d\n",
-                              ent->mUsingSpdy, ent->mSpdyPreferred);
+  self->mLogData.AppendPrintf("   Spdy using = %d, tested = %d, preferred = %d\n",
+                              ent->mUsingSpdy, ent->mTestedSpdy, 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,17 +154,16 @@ 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
@@ -205,16 +204,17 @@ 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,16 +375,17 @@ 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
@@ -431,16 +432,18 @@ 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();
@@ -647,16 +650,18 @@ 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;
@@ -667,89 +672,44 @@ 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 conn=%p %s %s ent=%p preferred=%p\n",conn,
-         ent->mConnInfo->Host(), ent->mCoalescingKey.get(), ent, preferred));
+    LOG(("ReportSpdyConnection %s %s ent=%p preferred=%p\n",
+         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 (desharding)\n", conn, ent));
+                 "migrate to preferred\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();
-
-            DebugOnly<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)
 {
@@ -1062,17 +1022,19 @@ 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->mUsingSpdy || self->mCT.Count() > 300)) {
+        ((!ent->mTestedSpdy && !ent->mUsingSpdy) ||
+         !gHttpHandler->IsSpdyEnabled() ||
+         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();
@@ -1442,38 +1404,39 @@ nsHttpConnectionMgr::ClosePersistentConn
                                                   void *closure)
 {
     nsHttpConnectionMgr *self = static_cast<nsHttpConnectionMgr *>(closure);
     self->ClosePersistentConnections(ent);
     return PL_DHASH_NEXT;
 }
 
 bool
-nsHttpConnectionMgr::RestrictConnections(nsConnectionEntry *ent)
+nsHttpConnectionMgr::RestrictConnections(nsConnectionEntry *ent,
+                                         bool ignorePossibleSpdyConnections)
 {
     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->mUsingSpdy &&
+        ((!ent->mTestedSpdy && !ignorePossibleSpdyConnections) ||
+         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()) {
+    if (ent->UnconnectedHalfOpens() && !ignorePossibleSpdyConnections)
         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];
@@ -2984,32 +2947,34 @@ 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)) &&
+        !(keepAlive && RestrictConnections(ent, ignorePossibleSpdyConnections)) &&
         !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"));
     }
 }
 
@@ -3363,19 +3328,18 @@ 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 {
-        MOZ_ASSERT(!mTransaction->IsNullTransaction(),
-                   "null transactions dont have backup timers");
+    }
+    else {
         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())));
 
@@ -3595,16 +3559,17 @@ 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;
@@ -3992,16 +3957,17 @@ 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,16 +371,22 @@ 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,
@@ -554,17 +560,17 @@ private:
     nsresult DispatchAbstractTransaction(nsConnectionEntry *,
                                          nsAHttpTransaction *,
                                          uint32_t,
                                          nsHttpConnection *,
                                          int32_t);
     nsresult BuildPipeline(nsConnectionEntry *,
                            nsAHttpTransaction *,
                            nsHttpPipeline **);
-    bool     RestrictConnections(nsConnectionEntry *);
+    bool     RestrictConnections(nsConnectionEntry *, bool = false);
     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 *);