Bug 1649294 - Make RemoteDecoder use a background taskqueue. r=mattwoodrow

There's a small race that can happen when the remote decoder gets shutdown during xpcom shutdown; that would cause GetCurrentSerialEventTarget to return null. Leading to an assertion failure in ActorLifecycleProxy thread-safety check when PRemoteDecoderManagerParent gets destroyed.

So we use a background taskqueue instead and cleanup a bit the threading code in there allowed thanks to the TaskQueue ability to not require an explicit shutdown.

Differential Revision: https://phabricator.services.mozilla.com/D81287

build/clang-plugin/ThreadAllows.txt

@@ -67,7 +67,6 @@ RemoteLzyStream
 RWLockTester
 RacingServMan
 RemVidChild
-RemVidParent
 Sandbox Testing
 SaveScripts
 Socket Thread

dom/media/ipc/RemoteAudioDecoder.cpp

@@ -77,9 +77,9 @@ MediaResult RemoteAudioDecoderChild::InitIPDL(
 RemoteAudioDecoderParent::RemoteAudioDecoderParent(
     RemoteDecoderManagerParent* aParent, const AudioInfo& aAudioInfo,
     const CreateDecoderParams::OptionSet& aOptions,
-    TaskQueue* aManagerTaskQueue, TaskQueue* aDecodeTaskQueue, bool* aSuccess,
-    nsCString* aErrorDescription)
-    : RemoteDecoderParent(aParent, aManagerTaskQueue, aDecodeTaskQueue),
+    nsISerialEventTarget* aManagerThread, TaskQueue* aDecodeTaskQueue,
+    bool* aSuccess, nsCString* aErrorDescription)
+    : RemoteDecoderParent(aParent, aManagerThread, aDecodeTaskQueue),
       mAudioInfo(aAudioInfo) {
   CreateDecoderParams params(mAudioInfo);
   params.mTaskQueue = mDecodeTaskQueue;

dom/media/ipc/RemoteAudioDecoder.h

@@ -28,7 +28,7 @@ class RemoteAudioDecoderParent final : public RemoteDecoderParent {
   RemoteAudioDecoderParent(RemoteDecoderManagerParent* aParent,
                            const AudioInfo& aAudioInfo,
                            const CreateDecoderParams::OptionSet& aOptions,
-                           TaskQueue* aManagerTaskQueue,
+                           nsISerialEventTarget* aManagerThread,
                            TaskQueue* aDecodeTaskQueue, bool* aSuccess,
                            nsCString* aErrorDescription);
 

dom/media/ipc/RemoteDecoderManagerParent.cpp

@@ -29,8 +29,7 @@ using namespace ipc;
 using namespace layers;
 using namespace gfx;
 
-StaticRefPtr<nsIThread> sRemoteDecoderManagerParentThread;
-StaticRefPtr<TaskQueue> sRemoteDecoderManagerTaskQueue;
+StaticRefPtr<nsISerialEventTarget> sRemoteDecoderManagerParentThread;
 
 SurfaceDescriptorGPUVideo RemoteDecoderManagerParent::StoreImage(
     Image* aImage, TextureClient* aTexture) {
@@ -42,27 +41,6 @@ SurfaceDescriptorGPUVideo RemoteDecoderManagerParent::StoreImage(
   return ret;
 }
 
-class RemoteDecoderManagerThreadHolder {
-  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(RemoteDecoderManagerThreadHolder)
-
- public:
-  RemoteDecoderManagerThreadHolder() = default;
-
- private:
-  ~RemoteDecoderManagerThreadHolder() {
-    NS_DispatchToMainThread(
-        NS_NewRunnableFunction("dom::RemoteDecoderManagerThreadHolder::~"
-                               "RemoteDecoderManagerThreadHolder",
-                               []() {
-                                 sRemoteDecoderManagerParentThread->Shutdown();
-                                 sRemoteDecoderManagerParentThread = nullptr;
-                               }));
-  }
-};
-
-StaticRefPtr<RemoteDecoderManagerThreadHolder>
-    sRemoteDecoderManagerParentThreadHolder;
-
 class RemoteDecoderManagerThreadShutdownObserver : public nsIObserver {
   virtual ~RemoteDecoderManagerThreadShutdownObserver() = default;
 
@@ -94,25 +72,13 @@ bool RemoteDecoderManagerParent::StartupThreads() {
     return false;
   }
 
-  RefPtr<nsIThread> managerThread;
-  nsresult rv =
-      NS_NewNamedThread("RemVidParent", getter_AddRefs(managerThread));
+  nsCOMPtr<nsISerialEventTarget> managerThread;
+  nsresult rv = NS_CreateBackgroundTaskQueue("RemVidParent",
+                                             getter_AddRefs(managerThread));
   if (NS_FAILED(rv)) {
     return false;
   }
   sRemoteDecoderManagerParentThread = managerThread;
-  sRemoteDecoderManagerParentThreadHolder =
-      new RemoteDecoderManagerThreadHolder();
-#if XP_WIN
-  sRemoteDecoderManagerParentThread->Dispatch(
-      NS_NewRunnableFunction("RemoteDecoderManagerParent::StartupThreads",
-                             []() {
-                               DebugOnly<HRESULT> hr =
-                                   CoInitializeEx(0, COINIT_MULTITHREADED);
-                               MOZ_ASSERT(SUCCEEDED(hr));
-                             }),
-      NS_DISPATCH_NORMAL);
-#endif
   if (XRE_IsGPUProcess()) {
     sRemoteDecoderManagerParentThread->Dispatch(
         NS_NewRunnableFunction(
@@ -121,22 +87,13 @@ bool RemoteDecoderManagerParent::StartupThreads() {
         NS_DISPATCH_NORMAL);
   }
 
-  sRemoteDecoderManagerTaskQueue = new TaskQueue(
-      managerThread.forget(),
-      "RemoteDecoderManagerParent::sRemoteDecoderManagerTaskQueue");
-
   auto* obs = new RemoteDecoderManagerThreadShutdownObserver();
   observerService->AddObserver(obs, NS_XPCOM_SHUTDOWN_OBSERVER_ID, false);
   return true;
 }
 
 void RemoteDecoderManagerParent::ShutdownThreads() {
-  sRemoteDecoderManagerTaskQueue = nullptr;
-
-  sRemoteDecoderManagerParentThreadHolder = nullptr;
-  while (sRemoteDecoderManagerParentThread) {
-    NS_ProcessNextEvent(nullptr, true);
-  }
+  sRemoteDecoderManagerParentThread = nullptr;
 }
 
 void RemoteDecoderManagerParent::ShutdownVideoBridge() {
@@ -149,7 +106,7 @@ void RemoteDecoderManagerParent::ShutdownVideoBridge() {
 }
 
 bool RemoteDecoderManagerParent::OnManagerThread() {
-  return NS_GetCurrentThread() == sRemoteDecoderManagerParentThread;
+  return sRemoteDecoderManagerParentThread->IsOnCurrentThread();
 }
 
 bool RemoteDecoderManagerParent::CreateForContent(
@@ -163,7 +120,7 @@ bool RemoteDecoderManagerParent::CreateForContent(
   }
 
   RefPtr<RemoteDecoderManagerParent> parent =
-      new RemoteDecoderManagerParent(sRemoteDecoderManagerParentThreadHolder);
+      new RemoteDecoderManagerParent(sRemoteDecoderManagerParentThread);
 
   RefPtr<Runnable> task =
       NewRunnableMethod<Endpoint<PRemoteDecoderManagerParent>&&>(
@@ -194,8 +151,8 @@ bool RemoteDecoderManagerParent::CreateVideoBridgeToOtherProcess(
 }
 
 RemoteDecoderManagerParent::RemoteDecoderManagerParent(
-    RemoteDecoderManagerThreadHolder* aHolder)
-    : mThreadHolder(aHolder) {
+    nsISerialEventTarget* aThread)
+    : mThread(aThread) {
   MOZ_COUNT_CTOR(RemoteDecoderManagerParent);
 }
 
@@ -205,7 +162,7 @@ RemoteDecoderManagerParent::~RemoteDecoderManagerParent() {
 
 void RemoteDecoderManagerParent::ActorDestroy(
     mozilla::ipc::IProtocol::ActorDestroyReason) {
-  mThreadHolder = nullptr;
+  mThread = nullptr;
 }
 
 PRemoteDecoderParent* RemoteDecoderManagerParent::AllocPRemoteDecoderParent(
@@ -223,12 +180,12 @@ PRemoteDecoderParent* RemoteDecoderManagerParent::AllocPRemoteDecoderParent(
         aRemoteDecoderInfo.get_VideoDecoderInfoIPDL();
     return new RemoteVideoDecoderParent(
         this, decoderInfo.videoInfo(), decoderInfo.framerate(), aOptions,
-        aIdentifier, sRemoteDecoderManagerTaskQueue, decodeTaskQueue, aSuccess,
-        aErrorDescription);
+        aIdentifier, sRemoteDecoderManagerParentThread, decodeTaskQueue,
+        aSuccess, aErrorDescription);
   } else if (aRemoteDecoderInfo.type() == RemoteDecoderInfoIPDL::TAudioInfo) {
     return new RemoteAudioDecoderParent(
         this, aRemoteDecoderInfo.get_AudioInfo(), aOptions,
-        sRemoteDecoderManagerTaskQueue, decodeTaskQueue, aSuccess,
+        sRemoteDecoderManagerParentThread, decodeTaskQueue, aSuccess,
         aErrorDescription);
   }
 

dom/media/ipc/RemoteDecoderManagerParent.h

@@ -49,12 +49,10 @@ class RemoteDecoderManagerParent final : public PRemoteDecoderManagerParent {
       const SurfaceDescriptorGPUVideo& aSD);
 
   void ActorDestroy(mozilla::ipc::IProtocol::ActorDestroyReason) override;
-
   void ActorDealloc() override;
 
  private:
-  explicit RemoteDecoderManagerParent(
-      RemoteDecoderManagerThreadHolder* aThreadHolder);
+  explicit RemoteDecoderManagerParent(nsISerialEventTarget* aThread);
   ~RemoteDecoderManagerParent();
 
   void Open(Endpoint<PRemoteDecoderManagerParent>&& aEndpoint);
@@ -62,7 +60,7 @@ class RemoteDecoderManagerParent final : public PRemoteDecoderManagerParent {
   std::map<uint64_t, RefPtr<layers::Image>> mImageMap;
   std::map<uint64_t, RefPtr<layers::TextureClient>> mTextureMap;
 
-  RefPtr<RemoteDecoderManagerThreadHolder> mThreadHolder;
+  nsCOMPtr<nsISerialEventTarget> mThread;
 };
 
 }  // namespace mozilla

dom/media/ipc/RemoteDecoderParent.cpp

@@ -12,11 +12,11 @@
 namespace mozilla {
 
 RemoteDecoderParent::RemoteDecoderParent(RemoteDecoderManagerParent* aParent,
-                                         TaskQueue* aManagerTaskQueue,
+                                         nsISerialEventTarget* aManagerThread,
                                          TaskQueue* aDecodeTaskQueue)
     : mParent(aParent),
       mDecodeTaskQueue(aDecodeTaskQueue),
-      mManagerTaskQueue(aManagerTaskQueue),
+      mManagerThread(aManagerThread),
       mDecodedFramePool(1, ShmemPool::PoolType::DynamicPool) {
   MOZ_COUNT_CTOR(RemoteDecoderParent);
   MOZ_ASSERT(OnManagerThread());
@@ -41,7 +41,7 @@ mozilla::ipc::IPCResult RemoteDecoderParent::RecvInit(
   MOZ_ASSERT(OnManagerThread());
   RefPtr<RemoteDecoderParent> self = this;
   mDecoder->Init()->Then(
-      mManagerTaskQueue, __func__,
+      mManagerThread, __func__,
       [self, resolver = std::move(aResolver)](
           MediaDataDecoder::InitPromise::ResolveOrRejectValue&& aValue) {
         if (!self->CanRecv()) {
@@ -100,7 +100,7 @@ void RemoteDecoderParent::DecodeNextSample(nsTArray<MediaRawDataIPDL>&& aData,
 
   RefPtr<RemoteDecoderParent> self = this;
   mDecoder->Decode(data)->Then(
-      mManagerTaskQueue, __func__,
+      mManagerThread, __func__,
       [self, this, aData = std::move(aData), output = std::move(aOutput),
        resolver = std::move(aResolver)](
           MediaDataDecoder::DecodePromise::ResolveOrRejectValue&&
@@ -150,7 +150,7 @@ mozilla::ipc::IPCResult RemoteDecoderParent::RecvFlush(
   MOZ_ASSERT(OnManagerThread());
   RefPtr<RemoteDecoderParent> self = this;
   mDecoder->Flush()->Then(
-      mManagerTaskQueue, __func__,
+      mManagerThread, __func__,
       [self, resolver = std::move(aResolver)](
           MediaDataDecoder::FlushPromise::ResolveOrRejectValue&& aValue) {
         if (aValue.IsReject()) {
@@ -168,7 +168,7 @@ mozilla::ipc::IPCResult RemoteDecoderParent::RecvDrain(
   MOZ_ASSERT(OnManagerThread());
   RefPtr<RemoteDecoderParent> self = this;
   mDecoder->Drain()->Then(
-      mManagerTaskQueue, __func__,
+      mManagerThread, __func__,
       [self, this, resolver = std::move(aResolver)](
           MediaDataDecoder::DecodePromise::ResolveOrRejectValue&& aValue) {
         if (!self->CanRecv()) {
@@ -196,7 +196,7 @@ mozilla::ipc::IPCResult RemoteDecoderParent::RecvShutdown(
   if (mDecoder) {
     RefPtr<RemoteDecoderParent> self = this;
     mDecoder->Shutdown()->Then(
-        mManagerTaskQueue, __func__,
+        mManagerThread, __func__,
         [self, resolver = std::move(aResolver)](
             const ShutdownPromise::ResolveOrRejectValue& aValue) {
           MOZ_ASSERT(aValue.IsResolve());

dom/media/ipc/RemoteDecoderParent.h

@@ -23,7 +23,7 @@ class RemoteDecoderParent : public PRemoteDecoderParent {
   NS_INLINE_DECL_THREADSAFE_REFCOUNTING(RemoteDecoderParent)
 
   RemoteDecoderParent(RemoteDecoderManagerParent* aParent,
-                      TaskQueue* aManagerTaskQueue,
+                      nsISerialEventTarget* aManagerThread,
                       TaskQueue* aDecodeTaskQueue);
 
   void Destroy();
@@ -61,7 +61,7 @@ class RemoteDecoderParent : public PRemoteDecoderParent {
   void ReleaseBuffer(ShmemBuffer&& aBuffer);
   void ReleaseUsedShmems();
   RefPtr<RemoteDecoderParent> mIPDLSelfRef;
-  const RefPtr<TaskQueue> mManagerTaskQueue;
+  const RefPtr<nsISerialEventTarget> mManagerThread;
   ShmemPool mDecodedFramePool;
   AutoTArray<ShmemBuffer, 4> mUsedShmems;
 };

dom/media/ipc/RemoteVideoDecoder.cpp

@@ -251,9 +251,9 @@ RemoteVideoDecoderParent::RemoteVideoDecoderParent(
     RemoteDecoderManagerParent* aParent, const VideoInfo& aVideoInfo,
     float aFramerate, const CreateDecoderParams::OptionSet& aOptions,
     const Maybe<layers::TextureFactoryIdentifier>& aIdentifier,
-    TaskQueue* aManagerTaskQueue, TaskQueue* aDecodeTaskQueue, bool* aSuccess,
-    nsCString* aErrorDescription)
-    : RemoteDecoderParent(aParent, aManagerTaskQueue, aDecodeTaskQueue),
+    nsISerialEventTarget* aManagerThread, TaskQueue* aDecodeTaskQueue,
+    bool* aSuccess, nsCString* aErrorDescription)
+    : RemoteDecoderParent(aParent, aManagerThread, aDecodeTaskQueue),
       mVideoInfo(aVideoInfo) {
   if (aIdentifier) {
     // Check to see if we have a direct PVideoBridge connection to the

dom/media/ipc/RemoteVideoDecoder.h

@@ -53,8 +53,8 @@ class RemoteVideoDecoderParent final : public RemoteDecoderParent {
       RemoteDecoderManagerParent* aParent, const VideoInfo& aVideoInfo,
       float aFramerate, const CreateDecoderParams::OptionSet& aOptions,
       const Maybe<layers::TextureFactoryIdentifier>& aIdentifier,
-      TaskQueue* aManagerTaskQueue, TaskQueue* aDecodeTaskQueue, bool* aSuccess,
-      nsCString* aErrorDescription);
+      nsISerialEventTarget* aManagerThread, TaskQueue* aDecodeTaskQueue,
+      bool* aSuccess, nsCString* aErrorDescription);
 
  protected:
   MediaResult ProcessDecodedData(const MediaDataDecoder::DecodedData& aData,

gfx/layers/ipc/VideoBridgeChild.cpp

@@ -48,7 +48,9 @@ void VideoBridgeChild::Shutdown() {
 }
 
 VideoBridgeChild::VideoBridgeChild()
-    : mIPDLSelfRef(this), mThread(NS_GetCurrentThread()), mCanSend(true) {}
+    : mIPDLSelfRef(this),
+      mThread(GetCurrentSerialEventTarget()),
+      mCanSend(true) {}
 
 VideoBridgeChild::~VideoBridgeChild() = default;