Bug 1402592 - Ensure that ImageBridgeParent instances are closed by the parent during shutdown. r=dvander, a=sledru
authorAndrew Osmond <aosmond@mozilla.com>
Tue, 26 Sep 2017 14:03:29 -0400
changeset 431961 94e44c1f0875288e2612bcdff3f93f2b663b7909
parent 431960 e73836222b5775c7f81a46da0aef49db642e644f
child 431962 c7091b22d32efde7e224cb7b7671c121905a52bf
push id7846
push userryanvm@gmail.com
push dateThu, 28 Sep 2017 16:35:23 +0000
treeherdermozilla-beta@2429ebbd68a9 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdvander, sledru
bugs1402592
milestone57.0
Bug 1402592 - Ensure that ImageBridgeParent instances are closed by the parent during shutdown. r=dvander, a=sledru We currently allow the content process to shutdown the IPDL objects on behalf the parent, and we wait for all of these instances to be freed before we complete shutdown. This is undesirable because it requires the parent to trust the child rather than the other way around; the child can hold shutdown hostage by simply not releasing its instances. The child should already support the parent closing its graphics IPDL objects because the GPU process itself can die abruptly and be restored at a later time.
gfx/layers/ipc/CompositorThread.cpp
gfx/layers/ipc/ImageBridgeChild.cpp
gfx/layers/ipc/ImageBridgeParent.cpp
gfx/layers/ipc/ImageBridgeParent.h
--- a/gfx/layers/ipc/CompositorThread.cpp
+++ b/gfx/layers/ipc/CompositorThread.cpp
@@ -17,19 +17,16 @@ namespace gfx {
 void ReleaseVRManagerParentSingleton();
 } // namespace gfx
 
 namespace layers {
 
 static StaticRefPtr<CompositorThreadHolder> sCompositorThreadHolder;
 static bool sFinishedCompositorShutDown = false;
 
-// See ImageBridgeChild.cpp
-void ReleaseImageBridgeParentSingleton();
-
 CompositorThreadHolder* GetCompositorThreadHolder()
 {
   return sCompositorThreadHolder;
 }
 
 base::Thread*
 CompositorThread()
 {
@@ -121,17 +118,17 @@ CompositorThreadHolder::Start()
 }
 
 void
 CompositorThreadHolder::Shutdown()
 {
   MOZ_ASSERT(NS_IsMainThread(), "Should be on the main Thread!");
   MOZ_ASSERT(sCompositorThreadHolder, "The compositor thread has already been shut down!");
 
-  ReleaseImageBridgeParentSingleton();
+  ImageBridgeParent::Shutdown();
   gfx::ReleaseVRManagerParentSingleton();
   MediaSystemResourceService::Shutdown();
   CompositorManagerParent::Shutdown();
 
   sCompositorThreadHolder = nullptr;
 
   // No locking is needed around sFinishedCompositorShutDown because it is only
   // ever accessed on the main thread.
--- a/gfx/layers/ipc/ImageBridgeChild.cpp
+++ b/gfx/layers/ipc/ImageBridgeChild.cpp
@@ -235,23 +235,26 @@ ImageBridgeChild::ShutdownStep1(Synchron
 // dispatched function
 void
 ImageBridgeChild::ShutdownStep2(SynchronousTask* aTask)
 {
   AutoCompleteTask complete(aTask);
 
   MOZ_ASSERT(InImageBridgeChildThread(),
              "Should be in ImageBridgeChild thread.");
-  Close();
+  if (!mDestroyed) {
+    Close();
+  }
 }
 
 void
 ImageBridgeChild::ActorDestroy(ActorDestroyReason aWhy)
 {
   mCanSend = false;
+  mDestroyed = true;
   {
     MutexAutoLock lock(mContainerMapLock);
     mImageContainers.Clear();
   }
 }
 
 void
 ImageBridgeChild::DeallocPImageBridgeChild()
@@ -645,18 +648,16 @@ ImageBridgeChild::WillShutdown()
     RefPtr<Runnable> runnable = WrapRunnable(
       RefPtr<ImageBridgeChild>(this),
       &ImageBridgeChild::ShutdownStep2,
       &task);
     GetMessageLoop()->PostTask(runnable.forget());
 
     task.Wait();
   }
-
-  mDestroyed = true;
 }
 
 void
 ImageBridgeChild::InitSameProcess(uint32_t aNamespace)
 {
   NS_ASSERTION(NS_IsMainThread(), "Should be on the main Thread!");
 
   MOZ_ASSERT(!sImageBridgeChildSingleton);
--- a/gfx/layers/ipc/ImageBridgeParent.cpp
+++ b/gfx/layers/ipc/ImageBridgeParent.cpp
@@ -41,16 +41,18 @@ namespace layers {
 using namespace mozilla::ipc;
 using namespace mozilla::gfx;
 using namespace mozilla::media;
 
 std::map<base::ProcessId, ImageBridgeParent*> ImageBridgeParent::sImageBridges;
 
 StaticAutoPtr<mozilla::Monitor> sImageBridgesLock;
 
+static StaticRefPtr<ImageBridgeParent> sImageBridgeParentSingleton;
+
 // defined in CompositorBridgeParent.cpp
 CompositorThreadHolder* GetCompositorThreadHolder();
 
 /* static */ void
 ImageBridgeParent::Setup()
 {
   MOZ_ASSERT(NS_IsMainThread());
   if (!sImageBridgesLock) {
@@ -76,22 +78,16 @@ ImageBridgeParent::ImageBridgeParent(Mes
   }
   SetOtherProcessId(aChildProcessId);
 }
 
 ImageBridgeParent::~ImageBridgeParent()
 {
 }
 
-static StaticRefPtr<ImageBridgeParent> sImageBridgeParentSingleton;
-
-void ReleaseImageBridgeParentSingleton() {
-  sImageBridgeParentSingleton = nullptr;
-}
-
 /* static */ ImageBridgeParent*
 ImageBridgeParent::CreateSameProcess()
 {
   RefPtr<ImageBridgeParent> parent =
     new ImageBridgeParent(CompositorThreadHolder::Loop(), base::GetCurrentProcId());
   parent->mSelfRef = parent;
 
   sImageBridgeParentSingleton = parent;
@@ -111,16 +107,46 @@ ImageBridgeParent::CreateForGPUProcess(E
     parent,
     &ImageBridgeParent::Bind,
     Move(aEndpoint)));
 
   sImageBridgeParentSingleton = parent;
   return true;
 }
 
+/* static */ void
+ImageBridgeParent::ShutdownInternal()
+{
+  // We make a copy because we don't want to hold the lock while closing and we
+  // don't want the object to get freed underneath us.
+  nsTArray<RefPtr<ImageBridgeParent>> actors;
+  {
+    MonitorAutoLock lock(*sImageBridgesLock);
+    for (const auto& iter : sImageBridges) {
+      actors.AppendElement(iter.second);
+    }
+  }
+
+  for (auto const& actor : actors) {
+    MOZ_RELEASE_ASSERT(!actor->mClosed);
+    actor->Close();
+  }
+
+  sImageBridgeParentSingleton = nullptr;
+}
+
+/* static */ void
+ImageBridgeParent::Shutdown()
+{
+  CompositorThreadHolder::Loop()->PostTask(
+    NS_NewRunnableFunction("ImageBridgeParent::Shutdown", []() -> void {
+      ImageBridgeParent::ShutdownInternal();
+  }));
+}
+
 void
 ImageBridgeParent::ActorDestroy(ActorDestroyReason aWhy)
 {
   // Can't alloc/dealloc shmems from now on.
   mClosed = true;
   mCompositables.clear();
   {
     MonitorAutoLock lock(*sImageBridgesLock);
--- a/gfx/layers/ipc/ImageBridgeParent.h
+++ b/gfx/layers/ipc/ImageBridgeParent.h
@@ -53,16 +53,17 @@ public:
   /**
    * Creates the globals of ImageBridgeParent.
    */
   static void Setup();
 
   static ImageBridgeParent* CreateSameProcess();
   static bool CreateForGPUProcess(Endpoint<PImageBridgeParent>&& aEndpoint);
   static bool CreateForContent(Endpoint<PImageBridgeParent>&& aEndpoint);
+  static void Shutdown();
 
   virtual ShmemAllocator* AsShmemAllocator() override { return this; }
 
   virtual void ActorDestroy(ActorDestroyReason aWhy) override;
 
   // CompositableParentManager
   virtual void SendAsyncMessage(const InfallibleTArray<AsyncParentMessageData>& aMessage) override;
 
@@ -120,16 +121,18 @@ public:
   virtual bool UsesImageBridge() const override { return true; }
 
   virtual bool IPCOpen() const override { return !mClosed; }
 
 protected:
   void Bind(Endpoint<PImageBridgeParent>&& aEndpoint);
 
 private:
+  static void ShutdownInternal();
+
   void DeferredDestroy();
   MessageLoop* mMessageLoop;
   // This keeps us alive until ActorDestroy(), at which point we do a
   // deferred destruction of ourselves.
   RefPtr<ImageBridgeParent> mSelfRef;
 
   bool mSetChildThreadPriority;
   bool mClosed;