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

@@ -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<uint64_t, CompositorParent::LayerTreeState> 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
+  * A global map referencing each compositor by ID.
-// affects CrossProcessCompositorParent below.
+  *
-static Thread* sCompositorThread = nullptr;
+  * This map is used by the ImageBridge protocol to trigger
-// manual reference count of the compositor thread.
+  * compositions without having to keep references to the
-static int sCompositorThreadRefCount = 0;
+  * compositor
-static MessageLoop* sMainLoop = nullptr;
+  */
+typedef map<uint64_t,CompositorParent*> 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()
+public:
-{
+  CompositorThreadHolder()
-  if (NS_IsMainThread()){
+    : mCompositorThread(CreateCompositorThread())
-    ReleaseImageBridgeParentSingleton();
+  {
-    delete sCompositorThread;
+    MOZ_ASSERT(NS_IsMainThread());
-    sCompositorThread = nullptr;
+    MOZ_COUNT_CTOR(CompositorThreadHolder);
-  } else {
-    sMainLoop->PostTask(FROM_HERE, NewRunnableFunction(&DeleteCompositorThread));
   }
-}
 
-static void ReleaseCompositorThread()
+  Thread* GetCompositorThread() const {
-{
+    return mCompositorThread;
-  if(--sCompositorThreadRefCount == 0) {
-    DeleteCompositorThread();
   }
-}
 
-static void SetThreadPriority()
+private:
-{
+  ~CompositorThreadHolder()
-  hal::SetCurrentThreadPriority(hal::THREAD_PRIORITY_COMPOSITOR);
+  {
-}
+    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<CompositorThreadHolder> sCompositorThreadHolder;
+
+static MessageLoop* sMainLoop = nullptr;
+
+/* static */ Thread*
+CompositorThreadHolder::CreateCompositorThread()
 {
-  CreateCompositorMap();
+  MOZ_ASSERT(NS_IsMainThread());
-  CreateThread();
   sMainLoop = MessageLoop::current();
-}
 
-void CompositorParent::ShutDown()
+  MOZ_ASSERT(!sCompositorThreadHolder, "The compositor thread has already been started!");
-{
-  DestroyThread();
-  DestroyCompositorMap();
-}
 
-bool CompositorParent::CreateThread()
+  Thread* compositorThread = new Thread("Compositor");
-{
-  NS_ASSERTION(NS_IsMainThread(), "Should be on the main Thread!");
-  MOZ_ASSERT(!sCompositorThread);
-  sCompositorThreadRefCount = 1;
-  sCompositorThread = 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)) {
+  if (!compositorThread->StartWithOptions(options)) {
-    delete sCompositorThread;
+    delete compositorThread;
-    sCompositorThread = nullptr;
+    return nullptr;
-    return false;
   }
 
-  return true;
+  CreateCompositorMap();
+
+  return compositorThread;
 }
 
-void CompositorParent::DestroyThread()
+/* static */ void
+CompositorThreadHolder::DestroyCompositorThread(Thread* aCompositorThread)
 {
-  NS_ASSERTION(NS_IsMainThread(), "Should be on the main Thread!");
+  MOZ_ASSERT(NS_IsMainThread());
-  ReleaseCompositorThread();
+
+  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,
+  MOZ_ASSERT(NS_IsMainThread());
-             "The compositor thread must be Initialized before instanciating a CmpositorParent.");
+  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
+  this->AddRef(); // Corresponds to DeferredDeleteCompositorParentOnMainThread's Release
-  CompositorLoop()->PostTask(FROM_HERE,
+  MessageLoop::current()->PostTask(FROM_HERE,
-                           NewRunnableFunction(&DeferredDeleteCompositorParent,
+                                   NewRunnableFunction(&DeferredDeleteCompositorParentOnIOThread,
-                                               this));
+                                                       this));
   return true;
 }
 
@@ -921,27 +985,6 @@ CompositorParent::DeallocPLayerTransactionParent(PLayerTransactionParent* actor)
   return true;
 }
 
-
-typedef map<uint64_t,CompositorParent*> 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<CompositorThreadHolder> mCompositorThreadHolder;
 };
 
 void
@@ -1176,6 +1224,8 @@ OpenCompositor(CrossProcessCompositorParent* aCompositor,
 /*static*/ PCompositorParent*
 CompositorParent::Create(Transport* aTransport, ProcessId aOtherProcess)
 {
+  gfxPlatform::InitLayersIPC();
+
   nsRefPtr<CrossProcessCompositorParent> cpcp =
     new CrossProcessCompositorParent(aTransport, aOtherProcess);
   ProcessHandle handle;
@@ -1407,13 +1457,15 @@ CrossProcessCompositorParent::DeferredDestroy()
   CrossProcessCompositorParent* self;
   mSelfRef.forget(&self);
 
-  nsCOMPtr<nsIRunnable> runnable =
+  MOZ_ASSERT(sMainLoop);
-    NS_NewNonOwningRunnableMethod(self, &CrossProcessCompositorParent::Release);
+  sMainLoop->PostTask(FROM_HERE,
-  MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_DispatchToMainThread(runnable)));
+                      NewRunnableMethod(self, &CrossProcessCompositorParent::Release));
 }
 
 CrossProcessCompositorParent::~CrossProcessCompositorParent()
 {
+  MOZ_ASSERT(NS_IsMainThread());
+  MOZ_ASSERT(XRE_GetIOMessageLoop());
   XRE_GetIOMessageLoop()->PostTask(FROM_HERE,
                                    new DeleteTask<Transport>(mTransport));
 }

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<APZCTreeManager> mApzcTreeManager;
 
+  const nsRefPtr<CompositorThreadHolder> mCompositorThreadHolder;
+
   DISALLOW_EVIL_CONSTRUCTORS(CompositorParent);
 };