Bug 1387090 - Properly handle DontThrottle flag in all places in nsHttpTransaction. r=mcmanus
authorHonza Bambas <honzab.moz@firemni.cz>
Thu, 03 Aug 2017 10:47:00 -0400
changeset 642166 c037eccf31b487c85eabd7348bfa5016f061f8db
parent 642039 44fec2ed960a03a1d25626ea1b8014e6a234d508
child 642167 eb96d03a53a2af0ebe762ce6ff4a37cc9035073c
push id72668
push userbmo:tchiovoloni@mozilla.com
push dateMon, 07 Aug 2017 20:01:43 +0000
reviewersmcmanus
bugs1387090
milestone57.0a1
Bug 1387090 - Properly handle DontThrottle flag in all places in nsHttpTransaction. r=mcmanus
netwerk/protocol/http/nsHttpConnectionMgr.cpp
netwerk/protocol/http/nsHttpConnectionMgr.h
netwerk/protocol/http/nsHttpTransaction.cpp
netwerk/protocol/http/nsHttpTransaction.h
--- a/netwerk/protocol/http/nsHttpConnectionMgr.cpp
+++ b/netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ -2956,85 +2956,88 @@ void nsHttpConnectionMgr::LogActiveTrans
     // - unthrottled transaction for the active tab
     // - throttled transaction for the active tab
     // - unthrottled transaction for background tabs
     // - throttled transaction for background tabs
     LOG(("Active transactions %c[%u,%u,%u,%u]", operation, au, at, bu, bt));
 }
 
 void
-nsHttpConnectionMgr::AddActiveTransaction(nsHttpTransaction * aTrans, bool aThrottled)
+nsHttpConnectionMgr::AddActiveTransaction(nsHttpTransaction * aTrans)
 {
     MOZ_ASSERT(OnSocketThread(), "not on socket thread");
 
     uint64_t tabId = aTrans->TopLevelOuterContentWindowId();
+    bool throttled = aTrans->EligibleForThrottling();
 
     nsTArray<RefPtr<nsHttpTransaction>> *transactions =
-        mActiveTransactions[aThrottled].LookupOrAdd(tabId);
+        mActiveTransactions[throttled].LookupOrAdd(tabId);
 
     MOZ_ASSERT(!transactions->Contains(aTrans));
 
     transactions->AppendElement(aTrans);
 
     LOG(("nsHttpConnectionMgr::AddActiveTransaction    t=%p tabid=%" PRIx64 "(%d) thr=%d",
-          aTrans, tabId, tabId == mCurrentTopLevelOuterContentWindowId, aThrottled));
+          aTrans, tabId, tabId == mCurrentTopLevelOuterContentWindowId, throttled));
     LogActiveTransactions('+');
 
     if (tabId == mCurrentTopLevelOuterContentWindowId) {
         mActiveTabTransactionsExist = true;
-        if (!aThrottled) {
+        if (!throttled) {
             mActiveTabUnthrottledTransactionsExist = true;
         }
     }
 
     if (!mThrottleEnabled) {
         return;
     }
 
     EnsureThrottleTickerIfNeeded();
 }
 
 void
-nsHttpConnectionMgr::RemoveActiveTransaction(nsHttpTransaction * aTrans, bool aThrottled)
+nsHttpConnectionMgr::RemoveActiveTransaction(nsHttpTransaction * aTrans,
+                                             Maybe<bool> const& aOverride)
 {
     MOZ_ASSERT(OnSocketThread(), "not on socket thread");
 
     uint64_t tabId = aTrans->TopLevelOuterContentWindowId();
     bool forActiveTab = tabId == mCurrentTopLevelOuterContentWindowId;
+    bool throttled = aOverride.valueOr(aTrans->EligibleForThrottling());
 
     nsTArray<RefPtr<nsHttpTransaction>> *transactions =
-        mActiveTransactions[aThrottled].Get(tabId);
+        mActiveTransactions[throttled].Get(tabId);
 
     if (!transactions || !transactions->RemoveElement(aTrans)) {
         // Was not tracked as active, probably just ignore.
         return;
     }
 
     LOG(("nsHttpConnectionMgr::RemoveActiveTransaction t=%p tabid=%" PRIx64 "(%d) thr=%d",
-          aTrans, tabId, forActiveTab, aThrottled));
+          aTrans, tabId, forActiveTab, throttled));
 
     if (!transactions->IsEmpty()) {
         // There are still transactions of the type, hence nothing in the throttling conditions
         // has changed and we don't need to update "Exists" caches nor we need to wake any now
         // throttled transactions.
         LogActiveTransactions('-');
         return;
     }
 
     // To optimize the following logic, always remove the entry when the array is empty.
-    mActiveTransactions[aThrottled].Remove(tabId);
+    mActiveTransactions[throttled].Remove(tabId);
     LogActiveTransactions('-');
 
     if (forActiveTab) {
         // Update caches of the active tab transaction existence, since it's now affected
-        if (!aThrottled) {
+        if (!throttled) {
             mActiveTabUnthrottledTransactionsExist = false;
         }
         if (mActiveTabTransactionsExist) {
-            mActiveTabTransactionsExist = mActiveTransactions[!aThrottled].Contains(tabId);
+            mActiveTabTransactionsExist = mActiveTransactions[!throttled].Contains(tabId);
         }
     }
 
     if (!mThrottleEnabled) {
         return;
     }
 
     bool unthrottledExist = !mActiveTransactions[false].IsEmpty();
@@ -3062,17 +3065,17 @@ nsHttpConnectionMgr::RemoveActiveTransac
         LOG(("  there are unthrottled for the active tab"));
         return;
     }
 
     if (mActiveTabTransactionsExist) {
         // There are only trottled transactions for the active tab.
         // If the last transaction we just removed was a non-throttled for the active tab
         // we can wake the throttled transactions for the active tab.
-        if (forActiveTab && !aThrottled) {
+        if (forActiveTab && !throttled) {
             LOG(("  resuming throttled for active tab"));
             ResumeReadOf(mActiveTransactions[true].Get(mCurrentTopLevelOuterContentWindowId));
         }
         return;
     }
 
     if (!unthrottledExist) {
         // There are no unthrottled transactions for any tab.  Resume all throttled,
@@ -3089,49 +3092,55 @@ nsHttpConnectionMgr::RemoveActiveTransac
         DelayedResumeBackgroundThrottledTransactions();
         return;
     }
 
     LOG(("  not resuming anything"));
 }
 
 void
-nsHttpConnectionMgr::MoveActiveTransaction(nsHttpTransaction * aTrans, bool aThrottled)
+nsHttpConnectionMgr::UpdateActiveTransaction(nsHttpTransaction * aTrans)
 {
-    LOG(("nsHttpConnectionMgr::MoveActiveTransaction ENTER t=%p", aTrans));
-    AddActiveTransaction(aTrans, aThrottled);
-    RemoveActiveTransaction(aTrans, !aThrottled);
-    LOG(("nsHttpConnectionMgr::MoveActiveTransaction EXIT t=%p", aTrans));
+    LOG(("nsHttpConnectionMgr::UpdateActiveTransaction ENTER t=%p", aTrans));
+
+    AddActiveTransaction(aTrans);
+
+    Maybe<bool> reversed;
+    reversed.emplace(!aTrans->EligibleForThrottling());
+    RemoveActiveTransaction(aTrans, reversed);
+
+    LOG(("nsHttpConnectionMgr::UpdateActiveTransaction EXIT t=%p", aTrans));
 }
 
 bool
-nsHttpConnectionMgr::ShouldStopReading(nsHttpTransaction * aTrans, bool aThrottled)
+nsHttpConnectionMgr::ShouldStopReading(nsHttpTransaction * aTrans)
 {
     MOZ_ASSERT(OnSocketThread(), "not on socket thread");
 
     if (!mThrottlingInhibitsReading || !mThrottleEnabled) {
         return false;
     }
 
     uint64_t tabId = aTrans->TopLevelOuterContentWindowId();
     bool forActiveTab = tabId == mCurrentTopLevelOuterContentWindowId;
+    bool throttled = aTrans->EligibleForThrottling();
 
     if (mActiveTabTransactionsExist) {
         if (!tabId) {
             // Chrome initiated and unidentified transactions just respect
             // their throttle flag, when something for the active tab is happening.
-            return aThrottled;
+            return throttled;
         }
         if (!forActiveTab) {
             // This is a background tab request, we want them to always throttle.
             return true;
         }
         if (mActiveTabUnthrottledTransactionsExist) {
             // Unthrottled transactions for the active tab take precedence
-            return aThrottled;
+            return throttled;
         }
         // This is a throttled transaction for the active tab and there are no
         // unthrottled for the active tab, just let go on full fuel.
         return false;
     }
 
     MOZ_ASSERT(!forActiveTab);
 
@@ -3140,17 +3149,17 @@ nsHttpConnectionMgr::ShouldStopReading(n
         // after a delay.
         return true;
     }
 
     if (!mActiveTransactions[false].IsEmpty()) {
         // This means there are unthrottled active transactions for background tabs.
         // If we are here, there can't be any transactions for the active tab.
         // (If there is no transaction for a tab id, there is no entry for it in the hashtable.)
-        return aThrottled;
+        return throttled;
     }
 
     // There are only unthrottled transactions for background tabs: don't throttle.
     return false;
 }
 
 bool nsHttpConnectionMgr::IsConnEntryUnderPressure(nsHttpConnectionInfo *connInfo)
 {
--- a/netwerk/protocol/http/nsHttpConnectionMgr.h
+++ b/netwerk/protocol/http/nsHttpConnectionMgr.h
@@ -212,25 +212,26 @@ public:
     uint16_t MaxRequestDelay() { return mMaxRequestDelay; }
 
     // public, so that the SPDY/http2 seesions can activate
     void ActivateTimeoutTick();
 
     nsresult UpdateCurrentTopLevelOuterContentWindowId(uint64_t aWindowId);
 
     // tracks and untracks active transactions according their throttle status
-    void AddActiveTransaction(nsHttpTransaction* aTrans, bool aThrottled);
-    void RemoveActiveTransaction(nsHttpTransaction* aTrans, bool aThrottled);
-    void MoveActiveTransaction(nsHttpTransaction* aTrans, bool aThrottled);
+    void AddActiveTransaction(nsHttpTransaction* aTrans);
+    void RemoveActiveTransaction(nsHttpTransaction* aTrans,
+                                 Maybe<bool> const& aOverride = Nothing());
+    void UpdateActiveTransaction(nsHttpTransaction* aTrans);
 
     // called by nsHttpTransaction::WriteSegments.  decides whether the transaction
     // should stop reading data based on: the throttling ticker status, overall
     // status of all active transactions regarding active tab and respective
     // throttling state.
-    bool ShouldStopReading(nsHttpTransaction* aTrans, bool aThrottled);
+    bool ShouldStopReading(nsHttpTransaction* aTrans);
 
     // return true iff the connection has pending transactions for the active tab.
     // it's mainly used to disallow throttling (stop reading) of a response
     // belonging to the same conn info to free up a connection ASAP.
     // NOTE: relatively expensive to call, there are two hashtable lookups.
     bool IsConnEntryUnderPressure(nsHttpConnectionInfo*);
 
 private:
--- a/netwerk/protocol/http/nsHttpTransaction.cpp
+++ b/netwerk/protocol/http/nsHttpTransaction.cpp
@@ -168,36 +168,36 @@ void nsHttpTransaction::ResumeReading()
     if (mConnection) {
         nsresult rv = mConnection->ResumeRecv();
         if (NS_FAILED(rv)) {
             LOG(("  resume failed with rv=%" PRIx32, static_cast<uint32_t>(rv)));
         }
     }
 }
 
-bool nsHttpTransaction::EligibleForThrottling()
+bool nsHttpTransaction::EligibleForThrottling() const
 {
-  return (mClassOfService & (nsIClassOfService::Throttleable |
-                             nsIClassOfService::DontThrottle |
-                             nsIClassOfService::Leader |
-                             nsIClassOfService::Unblocked)) ==
-    nsIClassOfService::Throttleable;
+    return (mClassOfService & (nsIClassOfService::Throttleable |
+                               nsIClassOfService::DontThrottle |
+                               nsIClassOfService::Leader |
+                               nsIClassOfService::Unblocked)) ==
+        nsIClassOfService::Throttleable;
 }
 
 void nsHttpTransaction::SetClassOfService(uint32_t cos)
 {
     bool wasThrottling = EligibleForThrottling();
     mClassOfService = cos;
     bool isThrottling = EligibleForThrottling();
 
     if (mConnection && wasThrottling != isThrottling) {
         // Do nothing until we are actually activated.  For now
-        // only remember the throttle flag.  Call to MoveActiveTransaction
+        // only remember the throttle flag.  Call to UpdateActiveTransaction
         // would add this transaction to the list too early.
-        gHttpHandler->ConnMgr()->MoveActiveTransaction(this, isThrottling);
+        gHttpHandler->ConnMgr()->UpdateActiveTransaction(this);
 
         if (mReadingStopped && !isThrottling) {
             ResumeReading();
         }
     }
 }
 
 nsHttpTransaction::~nsHttpTransaction()
@@ -521,18 +521,17 @@ nsHttpTransaction::OnActivated(bool h2)
     MOZ_ASSERT(OnSocketThread());
 
     mActivatedAsH2 = h2;
     if (mActivated) {
         return;
     }
 
     mActivated = true;
-    gHttpHandler->ConnMgr()->AddActiveTransaction(
-        this, mClassOfService & nsIClassOfService::Throttleable);
+    gHttpHandler->ConnMgr()->AddActiveTransaction(this);
 }
 
 void
 nsHttpTransaction::GetSecurityCallbacks(nsIInterfaceRequestor **cb)
 {
     MutexAutoLock lock(mLock);
     nsCOMPtr<nsIInterfaceRequestor> tmp(mCallbacks);
     tmp.forget(cb);
@@ -844,17 +843,17 @@ bool nsHttpTransaction::ShouldStopReadin
         // will be removed when that is fixed
         return false;
     }
 
     if (mClassOfService & nsIClassOfService::DontThrottle) {
         return false;
     }
 
-    if (!gHttpHandler->ConnMgr()->ShouldStopReading(this, EligibleForThrottling())) {
+    if (!gHttpHandler->ConnMgr()->ShouldStopReading(this)) {
         // We are not obligated to throttle
         return false;
     }
 
     if (mContentRead < 16000) {
         // Let the first bytes go, it may also well be all the content we get
         LOG(("nsHttpTransaction::ShouldStopReading too few content (%" PRIi64 ") this=%p", mContentRead, this));
         return false;
@@ -938,18 +937,17 @@ nsHttpTransaction::WriteSegments(nsAHttp
 
 void
 nsHttpTransaction::Close(nsresult reason)
 {
     LOG(("nsHttpTransaction::Close [this=%p reason=%" PRIx32 "]\n",
          this, static_cast<uint32_t>(reason)));
 
     if (!mClosed) {
-        gHttpHandler->ConnMgr()->RemoveActiveTransaction(
-            this, mClassOfService & nsIClassOfService::Throttleable);
+        gHttpHandler->ConnMgr()->RemoveActiveTransaction(this);
         mActivated = false;
     }
 
     MOZ_ASSERT(OnSocketThread(), "not on socket thread");
     if (reason == NS_BINDING_RETARGETED) {
         LOG(("  close %p skipped due to ERETARGETED\n", this));
         return;
     }
--- a/netwerk/protocol/http/nsHttpTransaction.h
+++ b/netwerk/protocol/http/nsHttpTransaction.h
@@ -389,18 +389,18 @@ public:
     // but later can be dispatched via spdy (not subject to rate pacing).
     void CancelPacing(nsresult reason);
 
     // Called by the connetion manager on the socket thread when reading for this
     // previously throttled transaction has to be resumed.
     void ResumeReading();
 
     // This examins classification of this transaction whether the Throttleable class
-    // has been set while Leader or Unblocked are not.
-    bool EligibleForThrottling();
+    // has been set while Leader, Unblocked, DontThrottle has not.
+    bool EligibleForThrottling() const;
 
 private:
     bool mSubmittedRatePacing;
     bool mPassedRatePacing;
     bool mSynchronousRatePaceRequest;
     nsCOMPtr<nsICancelable> mTokenBucketCancel;
 public:
     void     SetClassOfService(uint32_t cos);