From f0b8964459b3eef6f3fed91ac74427d1a168dc12 Mon Sep 17 00:00:00 2001 From: Benoit Jacob Date: Thu, 3 Jul 2014 14:53:27 -0400 Subject: [PATCH] Bug 774388 - Patch 5: Wait for [CrossProcess]CompositorParent's to be gone before we tear down the compositor thread - r=mattwoodrow --- gfx/layers/ipc/CompositorParent.cpp | 226 +++++++++++++++++----------- gfx/layers/ipc/CompositorParent.h | 37 +---- 2 files changed, 145 insertions(+), 118 deletions(-) diff --git a/gfx/layers/ipc/CompositorParent.cpp b/gfx/layers/ipc/CompositorParent.cpp index 11b8435c406..70e4ea16952 100644 --- a/gfx/layers/ipc/CompositorParent.cpp +++ b/gfx/layers/ipc/CompositorParent.cpp @@ -53,6 +53,7 @@ #include "mozilla/unused.h" #include "mozilla/Hal.h" #include "mozilla/HalTypes.h" +#include "mozilla/StaticPtr.h" namespace mozilla { namespace layers { @@ -73,64 +74,80 @@ CompositorParent::LayerTreeState::LayerTreeState() typedef map LayerTreeMap; static LayerTreeMap sIndirectLayerTrees; -// FIXME/bug 774386: we're assuming that there's only one -// CompositorParent, but that's not always true. This assumption only -// affects CrossProcessCompositorParent below. -static Thread* sCompositorThread = nullptr; -// manual reference count of the compositor thread. -static int sCompositorThreadRefCount = 0; -static MessageLoop* sMainLoop = nullptr; +/** + * A global map referencing each compositor by ID. + * + * This map is used by the ImageBridge protocol to trigger + * compositions without having to keep references to the + * compositor + */ +typedef map CompositorMap; +static CompositorMap* sCompositorMap; + +static void CreateCompositorMap() +{ + MOZ_ASSERT(!sCompositorMap); + sCompositorMap = new CompositorMap; +} + +static void DestroyCompositorMap() +{ + MOZ_ASSERT(sCompositorMap); + MOZ_ASSERT(sCompositorMap->empty()); + delete sCompositorMap; + sCompositorMap = nullptr; +} // See ImageBridgeChild.cpp void ReleaseImageBridgeParentSingleton(); -static void DeferredDeleteCompositorParent(CompositorParent* aNowReadyToDie) +class CompositorThreadHolder MOZ_FINAL { - aNowReadyToDie->Release(); -} + NS_INLINE_DECL_THREADSAFE_REFCOUNTING(CompositorThreadHolder) -static void DeleteCompositorThread() -{ - if (NS_IsMainThread()){ - ReleaseImageBridgeParentSingleton(); - delete sCompositorThread; - sCompositorThread = nullptr; - } else { - sMainLoop->PostTask(FROM_HERE, NewRunnableFunction(&DeleteCompositorThread)); +public: + CompositorThreadHolder() + : mCompositorThread(CreateCompositorThread()) + { + MOZ_ASSERT(NS_IsMainThread()); + MOZ_COUNT_CTOR(CompositorThreadHolder); } -} -static void ReleaseCompositorThread() -{ - if(--sCompositorThreadRefCount == 0) { - DeleteCompositorThread(); + Thread* GetCompositorThread() const { + return mCompositorThread; } -} -static void SetThreadPriority() -{ - hal::SetCurrentThreadPriority(hal::THREAD_PRIORITY_COMPOSITOR); -} +private: + ~CompositorThreadHolder() + { + MOZ_ASSERT(NS_IsMainThread()); -void CompositorParent::StartUp() + MOZ_COUNT_DTOR(CompositorThreadHolder); + + DestroyCompositorThread(mCompositorThread); + } + + Thread* const mCompositorThread; + + static Thread* CreateCompositorThread(); + static void DestroyCompositorThread(Thread* aCompositorThread); + + friend class CompositorParent; +}; + +static StaticRefPtr sCompositorThreadHolder; + +static MessageLoop* sMainLoop = nullptr; + +/* static */ Thread* +CompositorThreadHolder::CreateCompositorThread() { - CreateCompositorMap(); - CreateThread(); + MOZ_ASSERT(NS_IsMainThread()); sMainLoop = MessageLoop::current(); -} -void CompositorParent::ShutDown() -{ - DestroyThread(); - DestroyCompositorMap(); -} + MOZ_ASSERT(!sCompositorThreadHolder, "The compositor thread has already been started!"); -bool CompositorParent::CreateThread() -{ - NS_ASSERTION(NS_IsMainThread(), "Should be on the main Thread!"); - MOZ_ASSERT(!sCompositorThread); - sCompositorThreadRefCount = 1; - sCompositorThread = new Thread("Compositor"); + Thread* compositorThread = new Thread("Compositor"); Thread::Options options; /* Timeout values are powers-of-two to enable us get better data. @@ -141,24 +158,56 @@ bool CompositorParent::CreateThread() than the default hang timeout on major platforms (about 5 seconds). */ options.permanent_hang_timeout = 8192; // milliseconds - if (!sCompositorThread->StartWithOptions(options)) { - delete sCompositorThread; - sCompositorThread = nullptr; - return false; + if (!compositorThread->StartWithOptions(options)) { + delete compositorThread; + return nullptr; } - return true; + CreateCompositorMap(); + + return compositorThread; } -void CompositorParent::DestroyThread() +/* static */ void +CompositorThreadHolder::DestroyCompositorThread(Thread* aCompositorThread) { - NS_ASSERTION(NS_IsMainThread(), "Should be on the main Thread!"); - ReleaseCompositorThread(); + MOZ_ASSERT(NS_IsMainThread()); + + MOZ_ASSERT(!sCompositorThreadHolder, "We shouldn't be destroying the compositor thread yet."); + + DestroyCompositorMap(); + ReleaseImageBridgeParentSingleton(); + delete aCompositorThread; +} + +static Thread* CompositorThread() { + return sCompositorThreadHolder ? sCompositorThreadHolder->GetCompositorThread() : nullptr; +} + +static void SetThreadPriority() +{ + hal::SetCurrentThreadPriority(hal::THREAD_PRIORITY_COMPOSITOR); +} + +void CompositorParent::StartUp() +{ + MOZ_ASSERT(NS_IsMainThread(), "Should be on the main Thread!"); + MOZ_ASSERT(!sCompositorThreadHolder, "The compositor thread has already been started!"); + + sCompositorThreadHolder = new CompositorThreadHolder(); +} + +void CompositorParent::ShutDown() +{ + MOZ_ASSERT(NS_IsMainThread(), "Should be on the main Thread!"); + MOZ_ASSERT(sCompositorThreadHolder, "The compositor thread has already been shut down!"); + + sCompositorThreadHolder = nullptr; } MessageLoop* CompositorParent::CompositorLoop() { - return sCompositorThread ? sCompositorThread->message_loop() : nullptr; + return CompositorThread() ? CompositorThread()->message_loop() : nullptr; } CompositorParent::CompositorParent(nsIWidget* aWidget, @@ -175,9 +224,11 @@ CompositorParent::CompositorParent(nsIWidget* aWidget, , mResumeCompositionMonitor("ResumeCompositionMonitor") , mOverrideComposeReadiness(false) , mForceCompositionTask(nullptr) + , mCompositorThreadHolder(sCompositorThreadHolder) { - MOZ_ASSERT(sCompositorThread != nullptr, - "The compositor thread must be Initialized before instanciating a CmpositorParent."); + MOZ_ASSERT(NS_IsMainThread()); + MOZ_ASSERT(CompositorThread(), + "The compositor thread must be Initialized before instanciating a CompositorParent."); MOZ_COUNT_CTOR(CompositorParent); mCompositorID = 0; // FIXME: This holds on the the fact that right now the only thing that @@ -192,13 +243,12 @@ CompositorParent::CompositorParent(nsIWidget* aWidget, sIndirectLayerTrees[mRootLayerTreeID].mParent = this; mApzcTreeManager = new APZCTreeManager(); - ++sCompositorThreadRefCount; } bool CompositorParent::IsInCompositorThread() { - return sCompositorThread && sCompositorThread->thread_id() == PlatformThread::CurrentId(); + return CompositorThread() && CompositorThread()->thread_id() == PlatformThread::CurrentId(); } uint64_t @@ -209,9 +259,8 @@ CompositorParent::RootLayerTreeId() CompositorParent::~CompositorParent() { + MOZ_ASSERT(NS_IsMainThread()); MOZ_COUNT_DTOR(CompositorParent); - - ReleaseCompositorThread(); } void @@ -264,6 +313,21 @@ CompositorParent::RecvWillStop() return true; } +static void DeferredDeleteCompositorParentOnMainThread(CompositorParent* aNowReadyToDie) +{ + MOZ_ASSERT(NS_IsMainThread()); + aNowReadyToDie->Release(); +} + +static void DeferredDeleteCompositorParentOnIOThread(CompositorParent* aToBeDeletedOnMainThread) +{ + MOZ_ASSERT(!NS_IsMainThread()); + MOZ_ASSERT(sMainLoop); + sMainLoop->PostTask(FROM_HERE, + NewRunnableFunction(&DeferredDeleteCompositorParentOnMainThread, + aToBeDeletedOnMainThread)); +} + bool CompositorParent::RecvStop() { @@ -273,10 +337,10 @@ CompositorParent::RecvStop() // this thread. // We must keep the compositor parent alive untill the code handling message // reception is finished on this thread. - this->AddRef(); // Corresponds to DeferredDeleteCompositorParent's Release - CompositorLoop()->PostTask(FROM_HERE, - NewRunnableFunction(&DeferredDeleteCompositorParent, - this)); + this->AddRef(); // Corresponds to DeferredDeleteCompositorParentOnMainThread's Release + MessageLoop::current()->PostTask(FROM_HERE, + NewRunnableFunction(&DeferredDeleteCompositorParentOnIOThread, + this)); return true; } @@ -921,27 +985,6 @@ CompositorParent::DeallocPLayerTransactionParent(PLayerTransactionParent* actor) return true; } - -typedef map CompositorMap; -static CompositorMap* sCompositorMap; - -void CompositorParent::CreateCompositorMap() -{ - if (sCompositorMap == nullptr) { - sCompositorMap = new CompositorMap; - } -} - -void CompositorParent::DestroyCompositorMap() -{ - if (sCompositorMap != nullptr) { - NS_ASSERTION(sCompositorMap->empty(), - "The Compositor map should be empty when destroyed>"); - delete sCompositorMap; - sCompositorMap = nullptr; - } -} - CompositorParent* CompositorParent::GetCompositor(uint64_t id) { CompositorMap::iterator it = sCompositorMap->find(id); @@ -1082,7 +1125,10 @@ public: CrossProcessCompositorParent(Transport* aTransport, ProcessId aOtherProcess) : mTransport(aTransport) , mChildProcessId(aOtherProcess) - {} + , mCompositorThreadHolder(sCompositorThreadHolder) + { + MOZ_ASSERT(NS_IsMainThread()); + } // IToplevelProtocol::CloneToplevel() virtual IToplevelProtocol* @@ -1145,6 +1191,8 @@ private: Transport* mTransport; // Child side's process Id. base::ProcessId mChildProcessId; + + const nsRefPtr mCompositorThreadHolder; }; void @@ -1176,6 +1224,8 @@ OpenCompositor(CrossProcessCompositorParent* aCompositor, /*static*/ PCompositorParent* CompositorParent::Create(Transport* aTransport, ProcessId aOtherProcess) { + gfxPlatform::InitLayersIPC(); + nsRefPtr cpcp = new CrossProcessCompositorParent(aTransport, aOtherProcess); ProcessHandle handle; @@ -1407,13 +1457,15 @@ CrossProcessCompositorParent::DeferredDestroy() CrossProcessCompositorParent* self; mSelfRef.forget(&self); - nsCOMPtr runnable = - NS_NewNonOwningRunnableMethod(self, &CrossProcessCompositorParent::Release); - MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_DispatchToMainThread(runnable))); + MOZ_ASSERT(sMainLoop); + sMainLoop->PostTask(FROM_HERE, + NewRunnableMethod(self, &CrossProcessCompositorParent::Release)); } CrossProcessCompositorParent::~CrossProcessCompositorParent() { + MOZ_ASSERT(NS_IsMainThread()); + MOZ_ASSERT(XRE_GetIOMessageLoop()); XRE_GetIOMessageLoop()->PostTask(FROM_HERE, new DeleteTask(mTransport)); } diff --git a/gfx/layers/ipc/CompositorParent.h b/gfx/layers/ipc/CompositorParent.h index de41299ef88..0bc947b24cd 100644 --- a/gfx/layers/ipc/CompositorParent.h +++ b/gfx/layers/ipc/CompositorParent.h @@ -63,6 +63,8 @@ private: uint64_t mLayersId; }; +class CompositorThreadHolder; + class CompositorParent : public PCompositorParent, public ShadowLayersManager { @@ -166,7 +168,8 @@ public: static void StartUp(); /** - * Destroys the compositor thread and the global compositor map. + * Waits for all [CrossProcess]CompositorParent's to be gone, + * and destroys the compositor thread and global compositor map. */ static void ShutDown(); @@ -259,36 +262,6 @@ protected: void ForceComposition(); void CancelCurrentCompositeTask(); - /** - * Creates a global map referencing each compositor by ID. - * - * This map is used by the ImageBridge protocol to trigger - * compositions without having to keep references to the - * compositor - */ - static void CreateCompositorMap(); - static void DestroyCompositorMap(); - - /** - * Creates the compositor thread. - * - * All compositors live on the same thread. - * The thread is not lazily created on first access to avoid dealing with - * thread safety. Therefore it's best to create and destroy the thread when - * we know we areb't using it (So creating/destroying along with gfxPlatform - * looks like a good place). - */ - static bool CreateThread(); - - /** - * Destroys the compositor thread. - * - * It is safe to call this fucntion more than once, although the second call - * will have no effect. - * This function is not thread-safe. - */ - static void DestroyThread(); - /** * Add a compositor to the global compositor map. */ @@ -336,6 +309,8 @@ protected: nsRefPtr mApzcTreeManager; + const nsRefPtr mCompositorThreadHolder; + DISALLOW_EVIL_CONSTRUCTORS(CompositorParent); };