Bug 1826206 - Require nsISerialEventTarget for RetargetDeliveryTo, r=…

…necko-reviewers,valentin This avoids potential issues where multiple OnDataAvailable callbacks or similar could theoretically be called concurrently on different StreamTransportService threads when targeting the STS - these cases will now target a TaskQueue on the STS instead, structurally ensuring serial execution. Differential Revision: https://phabricator.services.mozilla.com/D179984
Thaodan · Jun 7, 2023 · 9f4d22f · 9f4d22f
1 parent c1f04d7
commit 9f4d22f
Show file tree

Hide file tree

Showing 41 changed files with 133 additions and 112 deletions.
diff --git a/dom/base/BodyConsumer.cpp b/dom/base/BodyConsumer.cpp
@@ -23,6 +23,7 @@
 #include "mozilla/dom/WorkerScope.h"
 #include "mozilla/ipc/PBackgroundSharedTypes.h"
 #include "mozilla/ScopeExit.h"
+#include "mozilla/TaskQueue.h"
 #include "nsComponentManagerUtils.h"
 #include "nsIFile.h"
 #include "nsIThreadRetargetableRequest.h"
@@ -272,7 +273,7 @@ NS_IMPL_ISUPPORTS(ConsumeBodyDoneObserver, nsIStreamLoaderObserver)
 }  // namespace
 
 /* static */ already_AddRefed<Promise> BodyConsumer::Create(
-    nsIGlobalObject* aGlobal, nsIEventTarget* aMainThreadEventTarget,
+    nsIGlobalObject* aGlobal, nsISerialEventTarget* aMainThreadEventTarget,
     nsIInputStream* aBodyStream, AbortSignalImpl* aSignalImpl,
     ConsumeType aType, const nsACString& aBodyBlobURISpec,
     const nsAString& aBodyLocalPath, const nsACString& aBodyMimeType,
@@ -359,10 +360,11 @@ void BodyConsumer::ReleaseObject() {
 }
 
 BodyConsumer::BodyConsumer(
-    nsIEventTarget* aMainThreadEventTarget, nsIGlobalObject* aGlobalObject,
-    nsIInputStream* aBodyStream, Promise* aPromise, ConsumeType aType,
-    const nsACString& aBodyBlobURISpec, const nsAString& aBodyLocalPath,
-    const nsACString& aBodyMimeType, const nsACString& aMixedCaseMimeType,
+    nsISerialEventTarget* aMainThreadEventTarget,
+    nsIGlobalObject* aGlobalObject, nsIInputStream* aBodyStream,
+    Promise* aPromise, ConsumeType aType, const nsACString& aBodyBlobURISpec,
+    const nsAString& aBodyLocalPath, const nsACString& aBodyMimeType,
+    const nsACString& aMixedCaseMimeType,
     MutableBlobStorage::MutableBlobStorageType aBlobStorageType)
     : mTargetThread(NS_GetCurrentThread()),
       mMainThreadEventTarget(aMainThreadEventTarget),
@@ -575,7 +577,9 @@ void BodyConsumer::BeginConsumeBodyMainThread(ThreadSafeWorkerRef* aWorkerRef) {
   if (rr) {
     nsCOMPtr<nsIEventTarget> sts =
         do_GetService(NS_STREAMTRANSPORTSERVICE_CONTRACTID);
-    rv = rr->RetargetDeliveryTo(sts);
+    RefPtr<TaskQueue> queue =
+        TaskQueue::Create(sts.forget(), "BodyConsumer STS Delivery Queue");
+    rv = rr->RetargetDeliveryTo(queue);
     if (NS_FAILED(rv)) {
       NS_WARNING("Retargeting failed");
     }

diff --git a/dom/base/BodyConsumer.h b/dom/base/BodyConsumer.h
@@ -60,7 +60,7 @@ class BodyConsumer final : public nsIObserver,
    * @param aRv An ErrorResult.
    */
   static already_AddRefed<Promise> Create(
-      nsIGlobalObject* aGlobal, nsIEventTarget* aMainThreadEventTarget,
+      nsIGlobalObject* aGlobal, nsISerialEventTarget* aMainThreadEventTarget,
       nsIInputStream* aBodyStream, AbortSignalImpl* aSignalImpl,
       ConsumeType aType, const nsACString& aBodyBlobURISpec,
       const nsAString& aBodyLocalPath, const nsACString& aBodyMimeType,
@@ -94,7 +94,7 @@ class BodyConsumer final : public nsIObserver,
   void RunAbortAlgorithm() override;
 
  private:
-  BodyConsumer(nsIEventTarget* aMainThreadEventTarget,
+  BodyConsumer(nsISerialEventTarget* aMainThreadEventTarget,
                nsIGlobalObject* aGlobalObject, nsIInputStream* aBodyStream,
                Promise* aPromise, ConsumeType aType,
                const nsACString& aBodyBlobURISpec,
@@ -109,7 +109,7 @@ class BodyConsumer final : public nsIObserver,
   void AssertIsOnTargetThread() const;
 
   nsCOMPtr<nsIThread> mTargetThread;
-  nsCOMPtr<nsIEventTarget> mMainThreadEventTarget;
+  nsCOMPtr<nsISerialEventTarget> mMainThreadEventTarget;
 
   // This is nullified when the consuming of the body starts.
   nsCOMPtr<nsIInputStream> mBodyStream;

diff --git a/dom/base/EventSource.cpp b/dom/base/EventSource.cpp
@@ -76,7 +76,7 @@ class EventSourceImpl final : public nsIObserver,
                               public nsIChannelEventSink,
                               public nsIInterfaceRequestor,
                               public nsSupportsWeakReference,
-                              public nsIEventTarget,
+                              public nsISerialEventTarget,
                               public nsITimerCallback,
                               public nsINamed,
                               public nsIThreadRetargetableStreamListener {
@@ -364,8 +364,9 @@ class EventSourceImpl final : public nsIObserver,
 NS_IMPL_ISUPPORTS(EventSourceImpl, nsIObserver, nsIStreamListener,
                   nsIRequestObserver, nsIChannelEventSink,
                   nsIInterfaceRequestor, nsISupportsWeakReference,
-                  nsIEventTarget, nsIThreadRetargetableStreamListener,
-                  nsITimerCallback, nsINamed)
+                  nsISerialEventTarget, nsIEventTarget,
+                  nsIThreadRetargetableStreamListener, nsITimerCallback,
+                  nsINamed)
 
 EventSourceImpl::EventSourceImpl(EventSource* aEventSource,
                                  nsICookieJarSettings* aCookieJarSettings)
@@ -1056,16 +1057,15 @@ nsresult EventSourceImpl::InitChannelAndRequestEventSource(
 
   MOZ_ASSERT_IF(mIsMainThread, aEventTargetAccessAllowed);
 
-  nsresult rv = aEventTargetAccessAllowed
-                    ? [this]() {
-                        // We can't call GetEventSource() because we're not
-                        // allowed to touch the refcount off the worker thread
-                        // due to an assertion, event if it would have otherwise
-                        // been safe.
-                        auto lock = mSharedData.Lock();
-                        return lock->mEventSource->CheckCurrentGlobalCorrectness();
-                      }()
-                    : NS_OK;
+  nsresult rv = aEventTargetAccessAllowed ? [this]() {
+    // We can't call GetEventSource() because we're not
+    // allowed to touch the refcount off the worker thread
+    // due to an assertion, event if it would have otherwise
+    // been safe.
+    auto lock = mSharedData.Lock();
+    return lock->mEventSource->CheckCurrentGlobalCorrectness();
+  }()
+                                          : NS_OK;
   if (NS_FAILED(rv) || !isValidScheme) {
     DispatchFailConnection();
     return NS_ERROR_DOM_SECURITY_ERR;

diff --git a/dom/fetch/Fetch.h b/dom/fetch/Fetch.h
@@ -251,7 +251,7 @@ class FetchBody : public FetchBodyBase, public AbortFollower {
   bool mBodyUsed;
 
   // The main-thread event target for runnable dispatching.
-  nsCOMPtr<nsIEventTarget> mMainThreadEventTarget;
+  nsCOMPtr<nsISerialEventTarget> mMainThreadEventTarget;
 };
 
 class EmptyBody final : public FetchBody<EmptyBody> {

diff --git a/dom/fetch/FetchDriver.cpp b/dom/fetch/FetchDriver.cpp
@@ -6,6 +6,7 @@
 
 #include "js/Value.h"
 #include "mozilla/DebugOnly.h"
+#include "mozilla/TaskQueue.h"
 #include "mozilla/dom/FetchDriver.h"
 
 #include "mozilla/dom/ReferrerInfo.h"
@@ -1236,7 +1237,9 @@ FetchDriver::OnStartRequest(nsIRequest* aRequest) {
 
   // Try to retarget off main thread.
   if (nsCOMPtr<nsIThreadRetargetableRequest> rr = do_QueryInterface(aRequest)) {
-    Unused << NS_WARN_IF(NS_FAILED(rr->RetargetDeliveryTo(sts)));
+    RefPtr<TaskQueue> queue =
+        TaskQueue::Create(sts.forget(), "FetchDriver STS Delivery Queue");
+    Unused << NS_WARN_IF(NS_FAILED(rr->RetargetDeliveryTo(queue)));
   }
   return NS_OK;
 }

diff --git a/dom/file/Blob.cpp b/dom/file/Blob.cpp
@@ -274,7 +274,7 @@ already_AddRefed<Promise> Blob::ConsumeBody(
     return nullptr;
   }
 
-  nsCOMPtr<nsIEventTarget> mainThreadEventTarget;
+  nsCOMPtr<nsISerialEventTarget> mainThreadEventTarget;
   if (!NS_IsMainThread()) {
     WorkerPrivate* workerPrivate = GetCurrentThreadWorkerPrivate();
     MOZ_ASSERT(workerPrivate);

diff --git a/dom/serviceworkers/ServiceWorkerScriptCache.cpp b/dom/serviceworkers/ServiceWorkerScriptCache.cpp
@@ -8,6 +8,7 @@
 
 #include "js/Array.h"               // JS::GetArrayLength
 #include "js/PropertyAndElement.h"  // JS_GetElement
+#include "mozilla/TaskQueue.h"
 #include "mozilla/Unused.h"
 #include "mozilla/dom/CacheBinding.h"
 #include "mozilla/dom/cache/CacheStorage.h"
@@ -1278,7 +1279,9 @@ void CompareCache::ManageValueResult(JSContext* aCx,
   if (rr) {
     nsCOMPtr<nsIEventTarget> sts =
         do_GetService(NS_STREAMTRANSPORTSERVICE_CONTRACTID);
-    rv = rr->RetargetDeliveryTo(sts);
+    RefPtr<TaskQueue> queue =
+        TaskQueue::Create(sts.forget(), "CompareCache STS Delivery Queue");
+    rv = rr->RetargetDeliveryTo(queue);
     if (NS_WARN_IF(NS_FAILED(rv))) {
       mPump = nullptr;
       Finish(rv, false);

diff --git a/dom/websocket/WebSocket.cpp b/dom/websocket/WebSocket.cpp
@@ -111,7 +111,7 @@ class WebSocketImpl final : public nsIInterfaceRequestor,
                             public nsIWebSocketListener,
                             public nsIObserver,
                             public nsIRequest,
-                            public nsIEventTarget,
+                            public nsISerialEventTarget,
                             public nsIWebSocketImpl {
  public:
   NS_DECL_NSIINTERFACEREQUESTOR
@@ -255,7 +255,7 @@ class WebSocketImpl final : public nsIInterfaceRequestor,
   nsCOMPtr<nsIPrincipal> mLoadingPrincipal;
 
   // For dispatching runnables to main thread.
-  nsCOMPtr<nsIEventTarget> mMainThreadEventTarget;
+  nsCOMPtr<nsISerialEventTarget> mMainThreadEventTarget;
 
   RefPtr<WebSocketImplProxy> mImplProxy;
 
@@ -294,7 +294,8 @@ WebSocketImplProxy::SendMessage(const nsAString& aMessage) {
 }
 
 NS_IMPL_ISUPPORTS(WebSocketImpl, nsIInterfaceRequestor, nsIWebSocketListener,
-                  nsIObserver, nsIRequest, nsIEventTarget, nsIWebSocketImpl)
+                  nsIObserver, nsIRequest, nsIEventTarget, nsISerialEventTarget,
+                  nsIWebSocketImpl)
 
 class CallDispatchConnectionCloseEvents final : public DiscardableRunnable {
  public:
@@ -2826,6 +2827,8 @@ NS_IMETHODIMP_(bool)
 WebSocketImpl::IsOnCurrentThreadInfallible() { return IsTargetThread(); }
 
 bool WebSocketImpl::IsTargetThread() const {
+  // FIXME: This should also check if we're on the worker thread. Code using
+  // `IsOnCurrentThread` could easily misbehave here!
   return NS_IsMainThread() == mIsMainThread;
 }
 

diff --git a/dom/workers/WorkerPrivate.cpp b/dom/workers/WorkerPrivate.cpp
@@ -3419,7 +3419,7 @@ void WorkerPrivate::AfterProcessNextEvent() {
   MOZ_ASSERT(GetEffectiveEventLoopRecursionDepth());
 }
 
-nsIEventTarget* WorkerPrivate::MainThreadEventTargetForMessaging() {
+nsISerialEventTarget* WorkerPrivate::MainThreadEventTargetForMessaging() {
   return mMainThreadEventTargetForMessaging;
 }
 
@@ -3435,7 +3435,7 @@ nsresult WorkerPrivate::DispatchToMainThreadForMessaging(
                                                       aFlags);
 }
 
-nsIEventTarget* WorkerPrivate::MainThreadEventTarget() {
+nsISerialEventTarget* WorkerPrivate::MainThreadEventTarget() {
   return mMainThreadEventTarget;
 }
 

diff --git a/dom/workers/WorkerPrivate.h b/dom/workers/WorkerPrivate.h
@@ -560,7 +560,7 @@ class WorkerPrivate final
   // Get the event target to use when dispatching to the main thread
   // from this Worker thread.  This may be the main thread itself or
   // a ThrottledEventQueue to the main thread.
-  nsIEventTarget* MainThreadEventTargetForMessaging();
+  nsISerialEventTarget* MainThreadEventTargetForMessaging();
 
   nsresult DispatchToMainThreadForMessaging(
       nsIRunnable* aRunnable, uint32_t aFlags = NS_DISPATCH_NORMAL);
@@ -569,7 +569,7 @@ class WorkerPrivate final
       already_AddRefed<nsIRunnable> aRunnable,
       uint32_t aFlags = NS_DISPATCH_NORMAL);
 
-  nsIEventTarget* MainThreadEventTarget();
+  nsISerialEventTarget* MainThreadEventTarget();
 
   nsresult DispatchToMainThread(nsIRunnable* aRunnable,
                                 uint32_t aFlags = NS_DISPATCH_NORMAL);

diff --git a/dom/workers/loader/CacheLoadHandler.cpp b/dom/workers/loader/CacheLoadHandler.cpp
@@ -23,6 +23,7 @@
 #include "mozilla/dom/Response.h"
 #include "mozilla/dom/ServiceWorkerBinding.h"  // ServiceWorkerState
 #include "mozilla/Result.h"
+#include "mozilla/TaskQueue.h"
 #include "mozilla/UniquePtr.h"
 #include "mozilla/dom/Document.h"
 #include "mozilla/dom/WorkerScope.h"
@@ -477,7 +478,9 @@ void CacheLoadHandler::ResolvedCallback(JSContext* aCx,
   if (rr) {
     nsCOMPtr<nsIEventTarget> sts =
         do_GetService(NS_STREAMTRANSPORTSERVICE_CONTRACTID);
-    rv = rr->RetargetDeliveryTo(sts);
+    RefPtr<TaskQueue> queue =
+        TaskQueue::Create(sts.forget(), "CacheLoadHandler STS Delivery Queue");
+    rv = rr->RetargetDeliveryTo(queue);
     if (NS_FAILED(rv)) {
       NS_WARNING("Failed to dispatch the nsIInputStreamPump to a IO thread.");
     }

diff --git a/dom/workers/loader/CacheLoadHandler.h b/dom/workers/loader/CacheLoadHandler.h
@@ -119,7 +119,7 @@ class CacheLoadHandler final : public PromiseNativeHandler,
   nsCString mCSPHeaderValue;
   nsCString mCSPReportOnlyHeaderValue;
   nsCString mReferrerPolicyHeaderValue;
-  nsCOMPtr<nsIEventTarget> mMainThreadEventTarget;
+  nsCOMPtr<nsISerialEventTarget> mMainThreadEventTarget;
 };
 
 /*

diff --git a/dom/worklet/WorkletFetchHandler.cpp b/dom/worklet/WorkletFetchHandler.cpp
@@ -21,6 +21,7 @@
 #include "mozilla/dom/worklet/WorkletModuleLoader.h"
 #include "mozilla/CycleCollectedJSContext.h"
 #include "mozilla/ScopeExit.h"
+#include "mozilla/TaskQueue.h"
 #include "nsIInputStreamPump.h"
 #include "nsIThreadRetargetableRequest.h"
 
@@ -573,7 +574,9 @@ void WorkletScriptHandler::ResolvedCallback(JSContext* aCx,
   if (rr) {
     nsCOMPtr<nsIEventTarget> sts =
         do_GetService(NS_STREAMTRANSPORTSERVICE_CONTRACTID);
-    rv = rr->RetargetDeliveryTo(sts);
+    RefPtr<TaskQueue> queue = TaskQueue::Create(
+        sts.forget(), "WorkletScriptHandler STS Delivery Queue");
+    rv = rr->RetargetDeliveryTo(queue);
     if (NS_FAILED(rv)) {
       NS_WARNING("Failed to dispatch the nsIInputStreamPump to a IO thread.");
     }

diff --git a/image/DecodePool.cpp b/image/DecodePool.cpp
@@ -193,9 +193,9 @@ void DecodePool::SyncRunIfPossible(IDecodingTask* aTask,
   aTask->Run();
 }
 
-already_AddRefed<nsIEventTarget> DecodePool::GetIOEventTarget() {
+already_AddRefed<nsISerialEventTarget> DecodePool::GetIOEventTarget() {
   MutexAutoLock threadPoolLock(mMutex);
-  nsCOMPtr<nsIEventTarget> target = mIOThread;
+  nsCOMPtr<nsISerialEventTarget> target = mIOThread;
   return target.forget();
 }
 

diff --git a/image/DecodePool.h b/image/DecodePool.h
@@ -83,9 +83,9 @@ class DecodePool final : public nsIObserver {
    * who want to deliver data to workers on the DecodePool can use this event
    * target.
    *
-   * @return An nsIEventTarget interface to the thread pool's I/O thread.
+   * @return An nsISerialEventTarget interface to the thread pool's I/O thread.
    */
-  already_AddRefed<nsIEventTarget> GetIOEventTarget();
+  already_AddRefed<nsISerialEventTarget> GetIOEventTarget();
 
  private:
   friend class DecodePoolWorker;

diff --git a/image/decoders/icon/mac/nsIconChannelCocoa.mm b/image/decoders/icon/mac/nsIconChannelCocoa.mm
@@ -217,7 +217,7 @@
   }
 
   // Init our stream pump
-  nsCOMPtr<nsIEventTarget> target =
+  nsCOMPtr<nsISerialEventTarget> target =
       nsContentUtils::GetEventTargetByLoadInfo(mLoadInfo, mozilla::TaskCategory::Other);
   rv = mPump->Init(inStream, 0, 0, false, target);
   if (NS_FAILED(rv)) {

diff --git a/image/decoders/icon/win/nsIconChannel.cpp b/image/decoders/icon/win/nsIconChannel.cpp
@@ -843,7 +843,7 @@ nsresult nsIconChannel::StartAsyncOpen() {
 
   // Use the main thread for the pumped events unless the load info
   // specifies otherwise
-  nsCOMPtr<nsIEventTarget> listenerTarget =
+  nsCOMPtr<nsISerialEventTarget> listenerTarget =
       nsContentUtils::GetEventTargetByLoadInfo(mLoadInfo,
                                                mozilla::TaskCategory::Other);
   if (!listenerTarget) {

diff --git a/image/imgRequest.cpp b/image/imgRequest.cpp
@@ -711,7 +711,7 @@ imgRequest::OnStartRequest(nsIRequest* aRequest) {
     nsresult rv = channel->GetContentType(mimeType);
     if (NS_SUCCEEDED(rv) && !mimeType.EqualsLiteral(IMAGE_SVG_XML)) {
       // Retarget OnDataAvailable to the DecodePool's IO thread.
-      nsCOMPtr<nsIEventTarget> target =
+      nsCOMPtr<nsISerialEventTarget> target =
           DecodePool::Singleton()->GetIOEventTarget();
       rv = retargetable->RetargetDeliveryTo(target);
     }

diff --git a/layout/style/nsFontFaceLoader.cpp b/layout/style/nsFontFaceLoader.cpp
@@ -15,6 +15,7 @@
 #include "mozilla/AutoRestore.h"
 #include "mozilla/Preferences.h"
 #include "mozilla/StaticPrefs_layout.h"
+#include "mozilla/TaskQueue.h"
 #include "mozilla/Telemetry.h"
 #include "mozilla/Unused.h"
 #include "FontFaceSet.h"
@@ -340,7 +341,9 @@ nsFontFaceLoader::OnStartRequest(nsIRequest* aRequest) {
   if (req) {
     nsCOMPtr<nsIEventTarget> sts =
         do_GetService(NS_STREAMTRANSPORTSERVICE_CONTRACTID);
-    Unused << NS_WARN_IF(NS_FAILED(req->RetargetDeliveryTo(sts)));
+    RefPtr<TaskQueue> queue =
+        TaskQueue::Create(sts.forget(), "nsFontFaceLoader STS Delivery Queue");
+    Unused << NS_WARN_IF(NS_FAILED(req->RetargetDeliveryTo(queue)));
   }
   return NS_OK;
 }

diff --git a/modules/libjar/nsJARChannel.cpp b/modules/libjar/nsJARChannel.cpp
@@ -1324,7 +1324,7 @@ nsJARChannel::OnDataAvailable(nsIRequest* req, nsIInputStream* stream,
 }
 
 NS_IMETHODIMP
-nsJARChannel::RetargetDeliveryTo(nsIEventTarget* aEventTarget) {
+nsJARChannel::RetargetDeliveryTo(nsISerialEventTarget* aEventTarget) {
   MOZ_ASSERT(NS_IsMainThread());
 
   nsCOMPtr<nsIThreadRetargetableRequest> request = do_QueryInterface(mRequest);
@@ -1336,7 +1336,7 @@ nsJARChannel::RetargetDeliveryTo(nsIEventTarget* aEventTarget) {
 }
 
 NS_IMETHODIMP
-nsJARChannel::GetDeliveryTarget(nsIEventTarget** aEventTarget) {
+nsJARChannel::GetDeliveryTarget(nsISerialEventTarget** aEventTarget) {
   MOZ_ASSERT(NS_IsMainThread());
 
   nsCOMPtr<nsIThreadRetargetableRequest> request = do_QueryInterface(mRequest);

diff --git a/netwerk/base/ProxyAutoConfig.cpp b/netwerk/base/ProxyAutoConfig.cpp
@@ -488,7 +488,7 @@ nsresult ProxyAutoConfig::ConfigurePAC(const nsACString& aPACURI,
                                        const nsACString& aPACScriptData,
                                        bool aIncludePath,
                                        uint32_t aExtraHeapSize,
-                                       nsIEventTarget* aEventTarget) {
+                                       nsISerialEventTarget* aEventTarget) {
   mShutdown = false;  // Shutdown needs to be called prior to destruction
 
   mPACURI = aPACURI;
@@ -904,7 +904,7 @@ nsresult RemoteProxyAutoConfig::ConfigurePAC(const nsACString& aPACURI,
                                              const nsACString& aPACScriptData,
                                              bool aIncludePath,
                                              uint32_t aExtraHeapSize,
-                                             nsIEventTarget*) {
+                                             nsISerialEventTarget*) {
   Unused << mProxyAutoConfigParent->SendConfigurePAC(
       aPACURI, aPACScriptData, aIncludePath, aExtraHeapSize);
   return NS_OK;