Fix ImageBridgeChild memory tracking errors on shutdown. (bug 1323957 part 6, r=mattwoodrow)
authorDavid Anderson <danderson@mozilla.com>
Tue, 17 Jan 2017 18:47:07 -0800
changeset 374774 e3b518e5d081f754e1a19bf1710b390589a1f404
parent 374773 4704ac96489ac410b8be58336ebb6225d1f19afb
child 374775 3acd11541ea10db468f3b7e6bafd8b136ea8fd00
push id6996
push userjlorenzo@mozilla.com
push dateMon, 06 Mar 2017 20:48:21 +0000
treeherdermozilla-beta@d89512dab048 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmattwoodrow
bugs1323957
milestone53.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Fix ImageBridgeChild memory tracking errors on shutdown. (bug 1323957 part 6, r=mattwoodrow)
gfx/layers/composite/CompositableHost.h
gfx/layers/ipc/ImageBridgeChild.cpp
gfx/layers/ipc/ImageBridgeChild.h
gfx/layers/ipc/ImageBridgeParent.cpp
gfx/layers/ipc/LayerTransactionParent.cpp
--- a/gfx/layers/composite/CompositableHost.h
+++ b/gfx/layers/composite/CompositableHost.h
@@ -73,17 +73,17 @@ struct AsyncCompositableRef
  * rendering.
  */
 class CompositableHost
 {
 protected:
   virtual ~CompositableHost();
 
 public:
-  NS_INLINE_DECL_REFCOUNTING(CompositableHost)
+  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(CompositableHost)
   explicit CompositableHost(const TextureInfo& aTextureInfo);
 
   static already_AddRefed<CompositableHost> Create(const TextureInfo& aTextureInfo);
 
   virtual CompositableType GetType() = 0;
 
   // If base class overrides, it should still call the parent implementation
   virtual void SetCompositor(Compositor* aCompositor);
--- a/gfx/layers/ipc/ImageBridgeChild.cpp
+++ b/gfx/layers/ipc/ImageBridgeChild.cpp
@@ -290,28 +290,27 @@ ImageBridgeChild::ShutdownStep1(Synchron
 // dispatched function
 void
 ImageBridgeChild::ShutdownStep2(SynchronousTask* aTask)
 {
   AutoCompleteTask complete(aTask);
 
   MOZ_ASSERT(InImageBridgeChildThread(),
              "Should be in ImageBridgeChild thread.");
-
-  if (!mCalledClose) {
-    Close();
-    mCalledClose = true;
-  }
+  Close();
 }
 
 void
 ImageBridgeChild::ActorDestroy(ActorDestroyReason aWhy)
 {
   mCanSend = false;
-  mCalledClose = true;
+  {
+    MutexAutoLock lock(mContainerMapLock);
+    mImageContainers.Clear();
+  }
 }
 
 void
 ImageBridgeChild::DeallocPImageBridgeChild()
 {
   this->Release();
 }
 
@@ -333,17 +332,17 @@ ImageBridgeChild::CreateCanvasClientSync
                                          RefPtr<CanvasClient>* const outResult)
 {
   AutoCompleteTask complete(aTask);
   *outResult = CreateCanvasClientNow(aType, aFlags);
 }
 
 ImageBridgeChild::ImageBridgeChild()
   : mCanSend(false)
-  , mCalledClose(false)
+  , mDestroyed(false)
   , mFwdTransactionId(0)
   , mContainerMapLock("ImageBridgeChild.mContainerMapLock")
 {
   MOZ_ASSERT(NS_IsMainThread());
 
   mTxn = new CompositableTransaction();
 }
 
@@ -731,16 +730,18 @@ ImageBridgeChild::WillShutdown()
     RefPtr<Runnable> runnable = WrapRunnable(
       RefPtr<ImageBridgeChild>(this),
       &ImageBridgeChild::ShutdownStep2,
       &task);
     GetMessageLoop()->PostTask(runnable.forget());
 
     task.Wait();
   }
+
+  mDestroyed = true;
 }
 
 void
 ImageBridgeChild::InitSameProcess()
 {
   NS_ASSERTION(NS_IsMainThread(), "Should be on the main Thread!");
 
   MOZ_ASSERT(!sImageBridgeChildSingleton);
@@ -993,16 +994,22 @@ ImageBridgeChild::DeallocShmem(ipc::Shme
 {
   if (InImageBridgeChildThread()) {
     if (!CanSend()) {
       return false;
     }
     return PImageBridgeChild::DeallocShmem(aShmem);
   }
 
+  // If we can't post a task, then we definitely cannot send, so there's
+  // no reason to queue up this send.
+  if (!CanPostTask()) {
+    return false;
+  }
+
   SynchronousTask task("AllocatorProxy Dealloc");
   bool result = false;
 
   RefPtr<Runnable> runnable = WrapRunnable(
     RefPtr<ImageBridgeChild>(this),
     &ImageBridgeChild::ProxyDeallocShmemNow,
     &task,
     &aShmem,
@@ -1145,20 +1152,42 @@ ImageBridgeChild::RemoveTextureFromCompo
   }
 }
 
 bool ImageBridgeChild::IsSameProcess() const
 {
   return OtherPid() == base::GetCurrentProcId();
 }
 
+bool
+ImageBridgeChild::CanPostTask() const
+{
+  // During shutdown, the cycle collector may free objects that are holding a
+  // reference to ImageBridgeChild. Since this happens on the main thread,
+  // ImageBridgeChild will attempt to post a task to the ImageBridge thread.
+  // However the thread manager has already been shut down, so the task cannot
+  // post.
+  //
+  // It's okay if this races. We only care about the shutdown case where
+  // everything's happening on the main thread. Even if it races outside of
+  // shutdown, it's still harmless to post the task, since the task must
+  // check CanSend().
+  return !mDestroyed;
+}
+
 void
 ImageBridgeChild::ReleaseCompositable(const CompositableHandle& aHandle)
 {
   if (!InImageBridgeChildThread()) {
+    // If we can't post a task, then we definitely cannot send, so there's
+    // no reason to queue up this send.
+    if (!CanPostTask()) {
+      return;
+    }
+
     RefPtr<Runnable> runnable = WrapRunnable(
       RefPtr<ImageBridgeChild>(this),
       &ImageBridgeChild::ReleaseCompositable,
       aHandle);
     GetMessageLoop()->PostTask(runnable.forget());
     return;
   }
 
--- a/gfx/layers/ipc/ImageBridgeChild.h
+++ b/gfx/layers/ipc/ImageBridgeChild.h
@@ -355,24 +355,25 @@ protected:
   void ShutdownStep1(SynchronousTask* aTask);
   void ShutdownStep2(SynchronousTask* aTask);
   void MarkShutDown();
 
   void ActorDestroy(ActorDestroyReason aWhy) override;
   void DeallocPImageBridgeChild() override;
 
   bool CanSend() const;
+  bool CanPostTask() const;
 
   static void ShutdownSingleton();
 
 private:
   CompositableTransaction* mTxn;
 
   bool mCanSend;
-  bool mCalledClose;
+  mozilla::Atomic<bool> mDestroyed;
 
   /**
    * Transaction id of CompositableForwarder.
    * It is incrementaed by UpdateFwdTransactionId() in each BeginTransaction() call.
    */
   uint64_t mFwdTransactionId;
 
   /**
--- a/gfx/layers/ipc/ImageBridgeParent.cpp
+++ b/gfx/layers/ipc/ImageBridgeParent.cpp
@@ -99,16 +99,17 @@ ImageBridgeParent::CreateForGPUProcess(E
   return true;
 }
 
 void
 ImageBridgeParent::ActorDestroy(ActorDestroyReason aWhy)
 {
   // Can't alloc/dealloc shmems from now on.
   mClosed = true;
+  mCompositables.clear();
 
   MessageLoop::current()->PostTask(NewRunnableMethod(this, &ImageBridgeParent::DeferredDestroy));
 
   // It is very important that this method gets called at shutdown (be it a clean
   // or an abnormal shutdown), because DeferredDestroy is what clears mSelfRef.
   // If mSelfRef is not null and ActorDestroy is not called, the ImageBridgeParent
   // is leaked which causes the CompositorThreadHolder to be leaked and
   // CompsoitorParent's shutdown ends up spinning the event loop forever, waiting
--- a/gfx/layers/ipc/LayerTransactionParent.cpp
+++ b/gfx/layers/ipc/LayerTransactionParent.cpp
@@ -89,16 +89,17 @@ LayerTransactionParent::RecvShutdown()
   }
   return IPC_OK();
 }
 
 void
 LayerTransactionParent::Destroy()
 {
   mDestroyed = true;
+  mCompositables.clear();
 }
 
 mozilla::ipc::IPCResult
 LayerTransactionParent::RecvUpdateNoSwap(const TransactionInfo& txn)
 {
   return RecvUpdate(txn, nullptr);
 }