From 0aff7d9a80d419bf4085d2fc2c1708b2ecba31c4 Mon Sep 17 00:00:00 2001 From: Randell Jesup Date: Wed, 23 Mar 2022 14:56:55 +0000 Subject: [PATCH] Bug 1207753 - InputStreamPump thread-safety annotations r=necko-reviewers,dragana Differential Revision: https://phabricator.services.mozilla.com/D141610 --- netwerk/base/nsInputStreamPump.cpp | 31 ++++++++++++------ netwerk/base/nsInputStreamPump.h | 50 ++++++++++++++++-------------- 2 files changed, 47 insertions(+), 34 deletions(-) diff --git a/netwerk/base/nsInputStreamPump.cpp b/netwerk/base/nsInputStreamPump.cpp index 935cff868d55..ce6a4d6dcaa3 100644 --- a/netwerk/base/nsInputStreamPump.cpp +++ b/netwerk/base/nsInputStreamPump.cpp @@ -154,7 +154,7 @@ NS_IMETHODIMP nsInputStreamPump::IsPending(bool* result) { RecursiveMutexAutoLock lock(mMutex); - *result = (mState != STATE_IDLE); + *result = (mState != STATE_IDLE && mState != STATE_DEAD); return NS_OK; } @@ -200,7 +200,8 @@ nsInputStreamPump::Suspend() { RecursiveMutexAutoLock lock(mMutex); LOG(("nsInputStreamPump::Suspend [this=%p]\n", this)); - NS_ENSURE_TRUE(mState != STATE_IDLE, NS_ERROR_UNEXPECTED); + NS_ENSURE_TRUE(mState != STATE_IDLE && mState != STATE_DEAD, + NS_ERROR_UNEXPECTED); ++mSuspendCount; return NS_OK; } @@ -211,12 +212,15 @@ nsInputStreamPump::Resume() { LOG(("nsInputStreamPump::Resume [this=%p]\n", this)); NS_ENSURE_TRUE(mSuspendCount > 0, NS_ERROR_UNEXPECTED); - NS_ENSURE_TRUE(mState != STATE_IDLE, NS_ERROR_UNEXPECTED); + NS_ENSURE_TRUE(mState != STATE_IDLE && mState != STATE_DEAD, + NS_ERROR_UNEXPECTED); // There is a brief in-between state when we null out mAsyncStream in // OnStateStop() before calling OnStopRequest, and only afterwards set - // STATE_IDLE, which we need to handle gracefully. - if (--mSuspendCount == 0 && mAsyncStream) EnsureWaiting(); + // STATE_DEAD, which we need to handle gracefully. + if (--mSuspendCount == 0 && mAsyncStream) { + EnsureWaiting(); + } return NS_OK; } @@ -359,7 +363,7 @@ nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream* stream) { break; } mProcessingCallbacks = true; - if (mSuspendCount || mState == STATE_IDLE) { + if (mSuspendCount || mState == STATE_IDLE || mState == STATE_DEAD) { mWaitingForInputStreamReady = false; mProcessingCallbacks = false; break; @@ -460,8 +464,10 @@ uint32_t nsInputStreamPump::OnStateStart() { // nsInputStreamPumps are needed (e.g. nsHttpChannel). RecursiveMutexAutoUnlock unlock(mMutex); // We're on the writing thread + PUSH_IGNORE_THREAD_SAFETY AssertOnThread(); rv = mListener->OnStartRequest(this); + POP_THREAD_SAFETY } // an error returned from OnStartRequest should cause us to abort; however, @@ -531,8 +537,10 @@ uint32_t nsInputStreamPump::OnStateTransfer() { // We're on the writing thread for mListener and mAsyncStream. // mStreamOffset is only touched in OnStateTransfer, and AsyncRead // shouldn't be called during OnDataAvailable() + // We may be called on non-MainThread even if mOffMainThread is // false, due to RetargetDeliveryTo(), so don't use AssertOnThread() + PUSH_IGNORE_THREAD_SAFETY if (mTargetThread) { MOZ_ASSERT(mTargetThread->IsOnCurrentThread()); } else { @@ -540,6 +548,7 @@ uint32_t nsInputStreamPump::OnStateTransfer() { } rv = mListener->OnDataAvailable(this, mAsyncStream, mStreamOffset, odaAvail); + POP_THREAD_SAFETY } // don't enter this code if ODA failed or called Cancel @@ -609,8 +618,8 @@ uint32_t nsInputStreamPump::OnStateStop() { nsresult rv = mLabeledMainThreadTarget->Dispatch( NewRunnableMethod("nsInputStreamPump::CallOnStateStop", this, &nsInputStreamPump::CallOnStateStop)); - NS_ENSURE_SUCCESS(rv, STATE_IDLE); - return STATE_IDLE; + NS_ENSURE_SUCCESS(rv, STATE_DEAD); + return STATE_DEAD; } AUTO_PROFILER_LABEL("nsInputStreamPump::OnStateStop", NETWORK); @@ -625,7 +634,7 @@ uint32_t nsInputStreamPump::OnStateStop() { if (!mAsyncStream || !mListener) { MOZ_ASSERT(mAsyncStream, "null mAsyncStream: OnStateStop called twice?"); MOZ_ASSERT(mListener, "null mListener: OnStateStop called twice?"); - return STATE_IDLE; + return STATE_DEAD; } if (NS_FAILED(mStatus)) { @@ -643,15 +652,17 @@ uint32_t nsInputStreamPump::OnStateStop() { RecursiveMutexAutoUnlock unlock(mMutex); // We're on the writing thread. // We believe that mStatus can't be changed on us here. + PUSH_IGNORE_THREAD_SAFETY AssertOnThread(); mListener->OnStopRequest(this, mStatus); + POP_THREAD_SAFETY } mTargetThread = nullptr; mListener = nullptr; if (mLoadGroup) mLoadGroup->RemoveRequest(this, nullptr, mStatus); - return STATE_IDLE; + return STATE_DEAD; } nsresult nsInputStreamPump::CreateBufferedStreamIfNeeded() { diff --git a/netwerk/base/nsInputStreamPump.h b/netwerk/base/nsInputStreamPump.h index e5571ecdbc3d..ef815c7f9555 100644 --- a/netwerk/base/nsInputStreamPump.h +++ b/netwerk/base/nsInputStreamPump.h @@ -71,56 +71,58 @@ class nsInputStreamPump final : public nsIInputStreamPump, nsresult CallOnStateStop(); protected: - enum { STATE_IDLE, STATE_START, STATE_TRANSFER, STATE_STOP }; + enum { STATE_IDLE, STATE_START, STATE_TRANSFER, STATE_STOP, STATE_DEAD }; nsresult EnsureWaiting(); uint32_t OnStateStart(); uint32_t OnStateTransfer(); uint32_t OnStateStop(); - nsresult CreateBufferedStreamIfNeeded(); + nsresult CreateBufferedStreamIfNeeded() REQUIRES(mMutex); // This should optimize away in non-DEBUG builds MOZ_ALWAYS_INLINE void AssertOnThread() const { + PUSH_IGNORE_THREAD_SAFETY if (mOffMainThread) { MOZ_ASSERT(mTargetThread->IsOnCurrentThread()); } else { MOZ_ASSERT(NS_IsMainThread()); } + POP_THREAD_SAFETY } - uint32_t mState{STATE_IDLE}; - nsCOMPtr mLoadGroup; - nsCOMPtr mListener; - nsCOMPtr mTargetThread; - nsCOMPtr mLabeledMainThreadTarget; - nsCOMPtr mStream; - nsCOMPtr mAsyncStream; - uint64_t mStreamOffset{0}; - uint64_t mStreamLength{0}; - uint32_t mSegSize{0}; - uint32_t mSegCount{0}; - nsresult mStatus{NS_OK}; - uint32_t mSuspendCount{0}; - uint32_t mLoadFlags{LOAD_NORMAL}; - bool mIsPending{false}; + uint32_t mState GUARDED_BY(mMutex){STATE_IDLE}; + nsCOMPtr mLoadGroup GUARDED_BY(mMutex); // mListener is written on a single thread (either MainThread or an // off-MainThread thread), read from that thread and perhaps others (in // RetargetDeliveryTo) + nsCOMPtr mListener GUARDED_BY(mMutex); + nsCOMPtr mTargetThread GUARDED_BY(mMutex); + nsCOMPtr mLabeledMainThreadTarget GUARDED_BY(mMutex); + nsCOMPtr mStream GUARDED_BY(mMutex); // mAsyncStream is written on a single thread (either MainThread or an // off-MainThread thread), and lives from AsyncRead() to OnStateStop(). + nsCOMPtr mAsyncStream GUARDED_BY(mMutex); + uint64_t mStreamOffset GUARDED_BY(mMutex){0}; + uint64_t mStreamLength GUARDED_BY(mMutex){0}; + uint32_t mSegSize GUARDED_BY(mMutex){0}; + uint32_t mSegCount GUARDED_BY(mMutex){0}; + nsresult mStatus GUARDED_BY(mMutex){NS_OK}; + uint32_t mSuspendCount GUARDED_BY(mMutex){0}; + uint32_t mLoadFlags GUARDED_BY(mMutex){LOAD_NORMAL}; + bool mIsPending GUARDED_BY(mMutex){false}; // True while in OnInputStreamReady, calling OnStateStart, OnStateTransfer // and OnStateStop. Used to prevent calls to AsyncWait during callbacks. - bool mProcessingCallbacks{false}; + bool mProcessingCallbacks GUARDED_BY(mMutex){false}; // True if waiting on the "input stream ready" callback. - bool mWaitingForInputStreamReady{false}; - bool mCloseWhenDone{false}; - bool mRetargeting{false}; - bool mAsyncStreamIsBuffered{false}; + bool mWaitingForInputStreamReady GUARDED_BY(mMutex){false}; + bool mCloseWhenDone GUARDED_BY(mMutex){false}; + bool mRetargeting GUARDED_BY(mMutex){false}; + bool mAsyncStreamIsBuffered GUARDED_BY(mMutex){false}; // Indicate whether nsInputStreamPump is used completely off main thread. - bool mOffMainThread; // If true, OnStateStop() is executed off main thread. Set at creation. + const bool mOffMainThread; // Protects state/member var accesses across multiple threads. - mozilla::RecursiveMutex mMutex MOZ_UNANNOTATED{"nsInputStreamPump"}; + mozilla::RecursiveMutex mMutex{"nsInputStreamPump"}; }; NS_DEFINE_STATIC_IID_ACCESSOR(nsInputStreamPump, NS_INPUT_STREAM_PUMP_IID)