From c251c5b2c7ffae5226747dbb5b000445c76b49ed Mon Sep 17 00:00:00 2001 From: Jean-Yves Avenard Date: Tue, 30 Jun 2020 02:50:38 +0000 Subject: [PATCH] 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 | 1 - dom/media/ipc/RemoteAudioDecoder.cpp | 6 +- dom/media/ipc/RemoteAudioDecoder.h | 2 +- dom/media/ipc/RemoteDecoderManagerParent.cpp | 69 ++++---------------- dom/media/ipc/RemoteDecoderManagerParent.h | 6 +- dom/media/ipc/RemoteDecoderParent.cpp | 14 ++-- dom/media/ipc/RemoteDecoderParent.h | 4 +- dom/media/ipc/RemoteVideoDecoder.cpp | 6 +- dom/media/ipc/RemoteVideoDecoder.h | 4 +- gfx/layers/ipc/VideoBridgeChild.cpp | 4 +- 10 files changed, 36 insertions(+), 80 deletions(-) diff --git a/build/clang-plugin/ThreadAllows.txt b/build/clang-plugin/ThreadAllows.txt index 9d732ea2ab7e..568a6669cfad 100644 --- a/build/clang-plugin/ThreadAllows.txt +++ b/build/clang-plugin/ThreadAllows.txt @@ -67,7 +67,6 @@ RemoteLzyStream RWLockTester RacingServMan RemVidChild -RemVidParent Sandbox Testing SaveScripts Socket Thread diff --git a/dom/media/ipc/RemoteAudioDecoder.cpp b/dom/media/ipc/RemoteAudioDecoder.cpp index e4b32fe1ed51..9e5d49776182 100644 --- a/dom/media/ipc/RemoteAudioDecoder.cpp +++ b/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; diff --git a/dom/media/ipc/RemoteAudioDecoder.h b/dom/media/ipc/RemoteAudioDecoder.h index 6a7bcf2aee2f..5b9834c544c0 100644 --- a/dom/media/ipc/RemoteAudioDecoder.h +++ b/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); diff --git a/dom/media/ipc/RemoteDecoderManagerParent.cpp b/dom/media/ipc/RemoteDecoderManagerParent.cpp index db3de7a64f43..cf8ed02289d0 100644 --- a/dom/media/ipc/RemoteDecoderManagerParent.cpp +++ b/dom/media/ipc/RemoteDecoderManagerParent.cpp @@ -29,8 +29,7 @@ using namespace ipc; using namespace layers; using namespace gfx; -StaticRefPtr sRemoteDecoderManagerParentThread; -StaticRefPtr sRemoteDecoderManagerTaskQueue; +StaticRefPtr 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 - sRemoteDecoderManagerParentThreadHolder; - class RemoteDecoderManagerThreadShutdownObserver : public nsIObserver { virtual ~RemoteDecoderManagerThreadShutdownObserver() = default; @@ -94,25 +72,13 @@ bool RemoteDecoderManagerParent::StartupThreads() { return false; } - RefPtr managerThread; - nsresult rv = - NS_NewNamedThread("RemVidParent", getter_AddRefs(managerThread)); + nsCOMPtr 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 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 parent = - new RemoteDecoderManagerParent(sRemoteDecoderManagerParentThreadHolder); + new RemoteDecoderManagerParent(sRemoteDecoderManagerParentThread); RefPtr task = NewRunnableMethod&&>( @@ -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); } diff --git a/dom/media/ipc/RemoteDecoderManagerParent.h b/dom/media/ipc/RemoteDecoderManagerParent.h index 11fd4be1ad3b..7c841cec3c8f 100644 --- a/dom/media/ipc/RemoteDecoderManagerParent.h +++ b/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&& aEndpoint); @@ -62,7 +60,7 @@ class RemoteDecoderManagerParent final : public PRemoteDecoderManagerParent { std::map> mImageMap; std::map> mTextureMap; - RefPtr mThreadHolder; + nsCOMPtr mThread; }; } // namespace mozilla diff --git a/dom/media/ipc/RemoteDecoderParent.cpp b/dom/media/ipc/RemoteDecoderParent.cpp index 8a242d9b5ca7..f6f1b4b0ddd1 100644 --- a/dom/media/ipc/RemoteDecoderParent.cpp +++ b/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 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&& aData, RefPtr 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 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 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 self = this; mDecoder->Shutdown()->Then( - mManagerTaskQueue, __func__, + mManagerThread, __func__, [self, resolver = std::move(aResolver)]( const ShutdownPromise::ResolveOrRejectValue& aValue) { MOZ_ASSERT(aValue.IsResolve()); diff --git a/dom/media/ipc/RemoteDecoderParent.h b/dom/media/ipc/RemoteDecoderParent.h index 5b155d2deda4..6d61ce625de2 100644 --- a/dom/media/ipc/RemoteDecoderParent.h +++ b/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 mIPDLSelfRef; - const RefPtr mManagerTaskQueue; + const RefPtr mManagerThread; ShmemPool mDecodedFramePool; AutoTArray mUsedShmems; }; diff --git a/dom/media/ipc/RemoteVideoDecoder.cpp b/dom/media/ipc/RemoteVideoDecoder.cpp index 25c345184c00..8822dc5bafa2 100644 --- a/dom/media/ipc/RemoteVideoDecoder.cpp +++ b/dom/media/ipc/RemoteVideoDecoder.cpp @@ -251,9 +251,9 @@ RemoteVideoDecoderParent::RemoteVideoDecoderParent( RemoteDecoderManagerParent* aParent, const VideoInfo& aVideoInfo, float aFramerate, const CreateDecoderParams::OptionSet& aOptions, const Maybe& 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 diff --git a/dom/media/ipc/RemoteVideoDecoder.h b/dom/media/ipc/RemoteVideoDecoder.h index 4c566270e1bc..6c4df8adbc2c 100644 --- a/dom/media/ipc/RemoteVideoDecoder.h +++ b/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& aIdentifier, - TaskQueue* aManagerTaskQueue, TaskQueue* aDecodeTaskQueue, bool* aSuccess, - nsCString* aErrorDescription); + nsISerialEventTarget* aManagerThread, TaskQueue* aDecodeTaskQueue, + bool* aSuccess, nsCString* aErrorDescription); protected: MediaResult ProcessDecodedData(const MediaDataDecoder::DecodedData& aData, diff --git a/gfx/layers/ipc/VideoBridgeChild.cpp b/gfx/layers/ipc/VideoBridgeChild.cpp index 53f4c90aef24..b4f145ec49a7 100644 --- a/gfx/layers/ipc/VideoBridgeChild.cpp +++ b/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;