Bug 1502055 - Make nsHttpChannel::ResumeInternal keep order of transaction pump OnStopRequest before cache pump OnStartRequest. r=dragana
☠☠ backed out by 9fe7380322a2 ☠ ☠
authorHonza Bambas <honzab.moz@firemni.cz>
Sun, 28 Oct 2018 07:58:00 -0400
changeset 499820 c0cf2301443d9c7adda82829aa2b7ace041983f4
parent 499819 bc4333f4fabb3a0dfdfd6475fbef339a4dee9039
child 499821 c5317de7cf018c2e00fc67d0db3f0c45fd6a1483
push id10290
push userffxbld-merge
push dateMon, 03 Dec 2018 16:23:23 +0000
treeherdermozilla-beta@700bed2445e6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdragana
bugs1502055
milestone65.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 1502055 - Make nsHttpChannel::ResumeInternal keep order of transaction pump OnStopRequest before cache pump OnStartRequest. r=dragana
netwerk/protocol/http/nsHttpChannel.cpp
netwerk/protocol/http/nsHttpChannel.h
--- a/netwerk/protocol/http/nsHttpChannel.cpp
+++ b/netwerk/protocol/http/nsHttpChannel.cpp
@@ -361,16 +361,17 @@ nsHttpChannel::nsHttpChannel()
     , mIsPartialRequest(0)
     , mHasAutoRedirectVetoNotifier(0)
     , mPinCacheContent(0)
     , mIsCorsPreflightDone(0)
     , mStronglyFramed(false)
     , mUsedNetwork(0)
     , mAuthConnectionRestartable(0)
     , mTrackingProtectionCancellationPending(0)
+    , mAsyncResumePending(0)
     , mPushedStream(nullptr)
     , mLocalBlocklist(false)
     , mOnTailUnblock(nullptr)
     , mWarningReporter(nullptr)
     , mIsReadingFromCache(false)
     , mFirstResponseSource(RESPONSE_PENDING)
     , mRaceCacheWithNetwork(false)
     , mRaceDelay(0)
@@ -965,18 +966,22 @@ nsHttpChannel::ContinueConnect()
 
     rv = gHttpHandler->InitiateTransaction(mTransaction, mPriority);
     if (NS_FAILED(rv)) return rv;
 
     rv = mTransactionPump->AsyncRead(this, nullptr);
     if (NS_FAILED(rv)) return rv;
 
     uint32_t suspendCount = mSuspendCount;
-    while (suspendCount--)
+    if (mAsyncResumePending) {
+        ++suspendCount;
+    }
+    while (suspendCount--) {
         mTransactionPump->Suspend();
+    }
 
     return NS_OK;
 }
 
 void
 nsHttpChannel::SpeculativeConnect()
 {
     // Before we take the latency hit of dealing with the cache, try and
@@ -5228,18 +5233,22 @@ nsHttpChannel::ReadFromCache(bool alread
 
     rv = mCachePump->AsyncRead(this, mListenerContext);
     if (NS_FAILED(rv)) return rv;
 
     if (mTimingEnabled)
         mCacheReadStart = TimeStamp::Now();
 
     uint32_t suspendCount = mSuspendCount;
-    while (suspendCount--)
+    if (mAsyncResumePending) {
+        ++suspendCount;
+    }
+    while (suspendCount--) {
         mCachePump->Suspend();
+    }
 
     return NS_OK;
 }
 
 void
 nsHttpChannel::CloseCacheEntry(bool doomOnFailure)
 {
     mCacheInputStream.CloseAndRelease();
@@ -8588,18 +8597,22 @@ nsHttpChannel::DoAuthRetry(nsAHttpConnec
 
     rv = gHttpHandler->InitiateTransaction(mTransaction, mPriority);
     if (NS_FAILED(rv)) return rv;
 
     rv = mTransactionPump->AsyncRead(this, nullptr);
     if (NS_FAILED(rv)) return rv;
 
     uint32_t suspendCount = mSuspendCount;
-    while (suspendCount--)
+    if (mAsyncResumePending) {
+        ++suspendCount;
+    }
+    while (suspendCount--) {
         mTransactionPump->Suspend();
+    }
 
     return NS_OK;
 }
 
 //-----------------------------------------------------------------------------
 // nsHttpChannel::nsIApplicationCacheChannel
 //-----------------------------------------------------------------------------
 
@@ -9205,38 +9218,59 @@ nsHttpChannel::ResumeInternal()
 
     if (--mSuspendCount == 0) {
         mSuspendTotalTime += (TimeStamp::NowLoRes() - mSuspendTimestamp).
                                ToMilliseconds();
 
         if (mCallOnResume) {
             // Resume the interrupted procedure first, then resume
             // the pump to continue process the input stream.
-            RefPtr<nsRunnableMethod<nsHttpChannel>> callOnResume=
-                NewRunnableMethod("CallOnResume", this, mCallOnResume);
-            // Should not resume pump that created after resumption.
+            // Any newly created pump MUST be suspended to prevent calling
+            // its OnStartRequest before OnStopRequest of any pre-existing
+            // pump.  mAsyncResumePending ensures that.
+            MOZ_ASSERT(!mAsyncResumePending);
+            mAsyncResumePending = 1;
+
+            auto const callOnResume = mCallOnResume;
+            mCallOnResume = nullptr;
+
+            RefPtr<nsHttpChannel> self(this);
             RefPtr<nsInputStreamPump> transactionPump = mTransactionPump;
             RefPtr<nsInputStreamPump> cachePump = mCachePump;
 
             nsresult rv =
                 NS_DispatchToCurrentThread(NS_NewRunnableFunction(
                     "nsHttpChannel::CallOnResume",
-                    [callOnResume, transactionPump, cachePump]() {
-                        callOnResume->Run();
-
+                    [callOnResume,
+                     self{std::move(self)},
+                     transactionPump{std::move(transactionPump)},
+                     cachePump{std::move(cachePump)}]() {
+                        MOZ_ASSERT(self->mAsyncResumePending);
+                        (self->*callOnResume)();
+                        MOZ_ASSERT(self->mAsyncResumePending);
+
+                        self->mAsyncResumePending = 0;
+
+                        // And now actually resume the previously existing pumps.
                         if (transactionPump) {
-                            transactionPump->Resume();
+                          transactionPump->Resume();
+                        }
+                        if (cachePump) {
+                          cachePump->Resume();
                         }
 
-                        if (cachePump) {
-                            cachePump->Resume();
+                        // Any newly created pumps were suspended once because of mAsyncResumePending.
+                        if (transactionPump != self->mTransactionPump && self->mTransactionPump) {
+                            self->mTransactionPump->Resume();
+                        }
+                        if (cachePump != self->mCachePump && self->mCachePump) {
+                            self->mCachePump->Resume();
                         }
                     })
                 );
-            mCallOnResume = nullptr;
             NS_ENSURE_SUCCESS(rv, rv);
             return rv;
         }
     }
 
     nsresult rvTransaction = NS_OK;
     if (mTransactionPump) {
         rvTransaction = mTransactionPump->Resume();
--- a/netwerk/protocol/http/nsHttpChannel.h
+++ b/netwerk/protocol/http/nsHttpChannel.h
@@ -643,16 +643,20 @@ private:
     // the next authentication request can be sent on a whole new connection
     uint32_t                          mAuthConnectionRestartable : 1;
 
     // True if the channel classifier has marked the channel to be cancelled
     // due to the tracking protection rules, but the asynchronous cancellation
     // process hasn't finished yet.
     uint32_t                          mTrackingProtectionCancellationPending : 1;
 
+    // True only when we are between Resume and async fire of mCallOnResume.
+    // Used to suspend any newly created pumps in mCallOnResume handler.
+    uint32_t                          mAsyncResumePending : 1;
+
     nsTArray<nsContinueRedirectionFunc> mRedirectFuncStack;
 
     // Needed for accurate DNS timing
     RefPtr<nsDNSPrefetch>           mDNSPrefetch;
 
     Http2PushedStream                 *mPushedStream;
     // True if the channel's principal was found on a phishing, malware, or
     // tracking (if tracking protection is enabled) blocklist