Bug 1367861 - enable throttling for http/2. r?mayhemer draft
authorNicholas Hurley <hurley@mozilla.com>
Wed, 10 Jan 2018 13:35:02 -0800
changeset 719281 a441fe7b7e5fca0f9fc7540e31857bc40193c253
parent 719102 c4e4613dbe32bb218957a140e5d0bd4fe7d1e98c
child 745740 c976f4c91c2c85e05f8cd475838c7cc985e5e47e
push id95195
push userbmo:hurley@mozilla.com
push dateThu, 11 Jan 2018 19:00:18 +0000
reviewersmayhemer
bugs1367861
milestone59.0a1
Bug 1367861 - enable throttling for http/2. r?mayhemer This also fixes the bug that prevented throttled http/2 streams from ever re-starting by calling TransactionHasDataToRecv. MozReview-Commit-ID: 5dFotZGhQk9
netwerk/protocol/http/Http2Session.cpp
netwerk/protocol/http/nsAHttpTransaction.h
netwerk/protocol/http/nsHttpConnection.cpp
netwerk/protocol/http/nsHttpTransaction.cpp
netwerk/protocol/http/nsHttpTransaction.h
--- a/netwerk/protocol/http/Http2Session.cpp
+++ b/netwerk/protocol/http/Http2Session.cpp
@@ -440,17 +440,17 @@ Http2Session::AddStream(nsAHttpTransacti
               "transaction (%08x).\n", this, aHttpTransaction, trans,
               static_cast<uint32_t>(rv)));
       }
       return true;
     }
   }
 
   aHttpTransaction->SetConnection(this);
-  aHttpTransaction->OnActivated(true);
+  aHttpTransaction->OnActivated();
 
   if (aUseTunnel) {
     LOG3(("Http2Session::AddStream session=%p trans=%p OnTunnel",
           this, aHttpTransaction));
     DispatchOnTunnel(aHttpTransaction, aCallbacks);
     return true;
   }
 
--- a/netwerk/protocol/http/nsAHttpTransaction.h
+++ b/netwerk/protocol/http/nsAHttpTransaction.h
@@ -42,17 +42,17 @@ class nsAHttpTransaction : public nsSupp
 public:
     NS_DECLARE_STATIC_IID_ACCESSOR(NS_AHTTPTRANSACTION_IID)
 
     // called by the connection when it takes ownership of the transaction.
     virtual void SetConnection(nsAHttpConnection *) = 0;
 
     // called by the connection after a successfull activation of this transaction
     // in other words, tells the transaction it transitioned to the "active" state.
-    virtual void OnActivated(bool h2) {}
+    virtual void OnActivated() {}
 
     // used to obtain the connection associated with this transaction
     virtual nsAHttpConnection *Connection() = 0;
 
     // called by the connection to get security callbacks to set on the
     // socket transport.
     virtual void GetSecurityCallbacks(nsIInterfaceRequestor **) = 0;
 
--- a/netwerk/protocol/http/nsHttpConnection.cpp
+++ b/netwerk/protocol/http/nsHttpConnection.cpp
@@ -725,17 +725,17 @@ nsHttpConnection::Activate(nsAHttpTransa
     }
 
     if (mTLSFilter) {
         rv = mTLSFilter->SetProxiedTransaction(trans);
         NS_ENSURE_SUCCESS(rv, rv);
         mTransaction = mTLSFilter;
     }
 
-    trans->OnActivated(false);
+    trans->OnActivated();
 
     rv = OnOutputStreamReady(mSocketOut);
 
 failed_activation:
     if (NS_FAILED(rv)) {
         mTransaction = nullptr;
     }
 
--- a/netwerk/protocol/http/nsHttpTransaction.cpp
+++ b/netwerk/protocol/http/nsHttpTransaction.cpp
@@ -106,17 +106,16 @@ nsHttpTransaction::nsHttpTransaction()
     , mCurrentHttpResponseHeaderSize(0)
     , mThrottlingReadAllowance(THROTTLE_NO_LIMIT)
     , mCapsToClear(0)
     , mResponseIsComplete(false)
     , mReadingStopped(false)
     , mClosed(false)
     , mConnected(false)
     , mActivated(false)
-    , mActivatedAsH2(false)
     , mHaveStatusLine(false)
     , mHaveAllHeaders(false)
     , mTransactionDone(false)
     , mDidContentStart(false)
     , mNoContent(false)
     , mSentData(false)
     , mReceivedData(false)
     , mStatusEventPending(false)
@@ -170,16 +169,17 @@ void nsHttpTransaction::ResumeReading()
 
     mReadingStopped = false;
 
     // This with either reengage the limit when still throttled in WriteSegments or
     // simply reset to allow unlimeted reading again.
     mThrottlingReadAllowance = THROTTLE_NO_LIMIT;
 
     if (mConnection) {
+        mConnection->TransactionHasDataToRecv(this);
         nsresult rv = mConnection->ResumeRecv();
         if (NS_FAILED(rv)) {
             LOG(("  resume failed with rv=%" PRIx32, static_cast<uint32_t>(rv)));
         }
     }
 }
 
 bool nsHttpTransaction::EligibleForThrottling() const
@@ -534,21 +534,20 @@ nsHttpTransaction::SetConnection(nsAHttp
 {
     {
         MutexAutoLock lock(mLock);
         mConnection = conn;
     }
 }
 
 void
-nsHttpTransaction::OnActivated(bool h2)
+nsHttpTransaction::OnActivated()
 {
     MOZ_ASSERT(OnSocketThread());
 
-    mActivatedAsH2 = h2;
     if (mActivated) {
         return;
     }
 
     mActivated = true;
     gHttpHandler->ConnMgr()->AddActiveTransaction(this);
 }
 
@@ -870,27 +869,16 @@ nsHttpTransaction::WritePipeSegment(nsIO
     if (NS_FAILED(rv))
         trans->Close(rv);
 
     return rv; // failure code only stops WriteSegments; it is not propagated.
 }
 
 bool nsHttpTransaction::ShouldThrottle()
 {
-    if (mActivatedAsH2) {
-        // Throttling feature is now disabled for http/2 transactions
-        // because of bug 1367861.  The logic around mActivatedAsH2
-        // will be removed when that is fixed.
-        // 
-        // Calling ShouldThrottle on the manager just to make sure
-        // the throttling time window is correctly updated by this transaction.
-        Unused << gHttpHandler->ConnMgr()->ShouldThrottle(this);
-        return false;
-    }
-
     if (mClassOfService & nsIClassOfService::DontThrottle) {
         // We deliberately don't touch the throttling window here since
         // DontThrottle requests are expected to be long-standing media
         // streams and would just unnecessarily block running downloads.
         // If we want to ballance bandwidth for media responses against
         // running downloads, we need to find something smarter like 
         // changing the suspend/resume throttling intervals at-runtime.
         return false;
--- a/netwerk/protocol/http/nsHttpTransaction.h
+++ b/netwerk/protocol/http/nsHttpTransaction.h
@@ -85,17 +85,17 @@ public:
                                uint64_t               reqContentLength,
                                bool                   reqBodyIncludesHeaders,
                                nsIEventTarget        *consumerTarget,
                                nsIInterfaceRequestor *callbacks,
                                nsITransportEventSink *eventsink,
                                uint64_t               topLevelOuterContentWindowId,
                                nsIAsyncInputStream  **responseBody);
 
-    void OnActivated(bool h2) override;
+    void OnActivated() override;
 
     // attributes
     nsHttpResponseHead    *ResponseHead()   { return mHaveAllHeaders ? mResponseHead : nullptr; }
     nsISupports           *SecurityInfo()   { return mSecurityInfo; }
 
     nsIEventTarget        *ConsumerTarget() { return mConsumerTarget; }
     nsISupports           *HttpChannel()    { return mChannel; }
 
@@ -344,17 +344,16 @@ private:
     // to resume reading.
     bool                            mReadingStopped;
 
     // state flags, all logically boolean, but not packed together into a
     // bitfield so as to avoid bitfield-induced races.  See bug 560579.
     bool                            mClosed;
     bool                            mConnected;
     bool                            mActivated;
-    bool                            mActivatedAsH2;
     bool                            mHaveStatusLine;
     bool                            mHaveAllHeaders;
     bool                            mTransactionDone;
     bool                            mDidContentStart;
     bool                            mNoContent; // expecting an empty entity body
     bool                            mSentData;
     bool                            mReceivedData;
     bool                            mStatusEventPending;