Simplify ImageContainerChild memory management. (bug 1299621 part 2, r=nical)
authorDavid Anderson <danderson@mozilla.com>
Tue, 06 Sep 2016 15:20:41 -0700
changeset 312921 9d2271c836ce127fbc343cbf253e0caf78ccca07
parent 312920 3972080f18990d102f1c74319dc5b2c89fabb541
child 312922 d94e8ab0f01ef09739bbe670f5efe14abb567952
push id30665
push usercbook@mozilla.com
push dateWed, 07 Sep 2016 15:20:43 +0000
treeherdermozilla-central@95acb9299faf [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnical
bugs1299621
milestone51.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
Simplify ImageContainerChild memory management. (bug 1299621 part 2, r=nical)
gfx/layers/ImageContainer.cpp
gfx/layers/ImageContainer.h
gfx/layers/ipc/ImageBridgeChild.cpp
gfx/layers/ipc/ImageBridgeChild.h
gfx/layers/ipc/ImageContainerChild.cpp
gfx/layers/ipc/ImageContainerChild.h
--- a/gfx/layers/ImageContainer.cpp
+++ b/gfx/layers/ImageContainer.cpp
@@ -98,72 +98,35 @@ BufferRecycleBin::ClearRecycledBuffers()
 {
   MutexAutoLock lock(mLock);
   if (!mRecycledBuffers.IsEmpty()) {
     mRecycledBuffers.Clear();
   }
   mRecycledBufferSize = 0;
 }
 
-// static
-void
-ImageContainer::DeallocActor(PImageContainerChild* aActor)
-{
-  MOZ_ASSERT(aActor);
-  MOZ_ASSERT(InImageBridgeChildThread());
-
-  auto actor = static_cast<ImageContainerChild*>(aActor);
-  if (actor->mImageContainerReleased) {
-    delete actor;
-  } else {
-    actor->mIPCOpen = false;
-  }
-}
-
-// static
-void
-ImageContainer::AsyncDestroyActor(PImageContainerChild* aActor)
-{
-  MOZ_ASSERT(aActor);
-  MOZ_ASSERT(InImageBridgeChildThread());
-
-  auto actor = static_cast<ImageContainerChild*>(aActor);
-
-  // Setting mImageContainerReleased to true means next time DeallocActor is
-  // called, the actor will be deleted.
-  actor->mImageContainerReleased = true;
-
-  if (actor->mIPCOpen && ImageBridgeChild::IsCreated() && !ImageBridgeChild::IsShutDown()) {
-    actor->SendAsyncDelete();
-  } else {
-    // The actor is already dead as far as IPDL is concerned, probably because
-    // of a channel error. We can deallocate it now.
-    DeallocActor(actor);
-  }
-}
-
 ImageContainer::ImageContainer(Mode flag)
 : mReentrantMonitor("ImageContainer.mReentrantMonitor"),
   mGenerationCounter(++sGenerationCounter),
   mPaintCount(0),
   mDroppedImageCount(0),
   mImageFactory(new ImageFactory()),
   mRecycleBin(new BufferRecycleBin()),
-  mCurrentProducerID(-1),
-  mIPDLChild(nullptr)
+  mCurrentProducerID(-1)
 {
-  if (ImageBridgeChild::IsCreated()) {
+  RefPtr<ImageBridgeChild> imageBridge = ImageBridgeChild::GetSingleton();
+  if (imageBridge) {
     // the refcount of this ImageClient is 1. we don't use a RefPtr here because the refcount
     // of this class must be done on the ImageBridge thread.
     switch (flag) {
       case SYNCHRONOUS:
         break;
       case ASYNCHRONOUS:
         mIPDLChild = new ImageContainerChild(this);
-        mImageClient = ImageBridgeChild::GetSingleton()->CreateImageClient(CompositableType::IMAGE, this);
+        mImageClient = imageBridge->CreateImageClient(CompositableType::IMAGE, this, mIPDLChild);
         MOZ_ASSERT(mImageClient);
         break;
       default:
         MOZ_ASSERT(false, "This flag is invalid.");
         break;
     }
   }
   mAsyncContainerID = mImageClient ? mImageClient->GetAsyncID()
@@ -173,18 +136,17 @@ ImageContainer::ImageContainer(Mode flag
 ImageContainer::ImageContainer(uint64_t aAsyncContainerID)
   : mReentrantMonitor("ImageContainer.mReentrantMonitor"),
   mGenerationCounter(++sGenerationCounter),
   mPaintCount(0),
   mDroppedImageCount(0),
   mImageFactory(nullptr),
   mRecycleBin(nullptr),
   mAsyncContainerID(aAsyncContainerID),
-  mCurrentProducerID(-1),
-  mIPDLChild(nullptr)
+  mCurrentProducerID(-1)
 {
   MOZ_ASSERT(mAsyncContainerID != sInvalidAsyncContainerId);
 }
 
 ImageContainer::~ImageContainer()
 {
   if (mIPDLChild) {
     mIPDLChild->ForgetImageContainer();
--- a/gfx/layers/ImageContainer.h
+++ b/gfx/layers/ImageContainer.h
@@ -576,22 +576,16 @@ public:
 
   PImageContainerChild* GetPImageContainerChild();
 
   /**
    * Main thread only.
    */
   static ProducerID AllocateProducerID();
 
-  /// ImageBridgeChild thread only.
-  static void AsyncDestroyActor(PImageContainerChild* aActor);
-
-  /// ImageBridgeChild thread only.
-  static void DeallocActor(PImageContainerChild* aActor);
-
 private:
   typedef mozilla::ReentrantMonitor ReentrantMonitor;
 
   // Private destructor, to discourage deletion outside of Release():
   ~ImageContainer();
 
   void SetCurrentImageInternal(const nsTArray<NonOwningImage>& aImages);
 
@@ -645,17 +639,17 @@ private:
 
   nsTArray<FrameID> mFrameIDsNotYetComposited;
   // ProducerID for last current image(s), including the frames in
   // mFrameIDsNotYetComposited
   ProducerID mCurrentProducerID;
 
   // Object must be released on the ImageBridge thread. Field is immutable
   // after creation of the ImageContainer.
-  ImageContainerChild* mIPDLChild;
+  RefPtr<ImageContainerChild> mIPDLChild;
 
   static mozilla::Atomic<uint32_t> sGenerationCounter;
 };
 
 class AutoLockImage
 {
 public:
   explicit AutoLockImage(ImageContainer *aContainer)
--- a/gfx/layers/ipc/ImageBridgeChild.cpp
+++ b/gfx/layers/ipc/ImageBridgeChild.cpp
@@ -450,26 +450,27 @@ static void ImageBridgeShutdownStep2(Ree
              "Should be in ImageBridgeChild thread.");
 
   sImageBridgeChildSingleton->Close();
 
   *aDone = true;
   aBarrier->NotifyAll();
 }
 
-// dispatched function
-static void CreateImageClientSync(RefPtr<ImageClient>* result,
-                                  ReentrantMonitor* barrier,
-                                  CompositableType aType,
-                                  ImageContainer* aImageContainer,
-                                  bool *aDone)
+/* static */ void
+CreateImageClientSync(RefPtr<ImageBridgeChild> aChild,
+                      RefPtr<ImageClient>* result,
+                      ReentrantMonitor* barrier,
+                      CompositableType aType,
+                      ImageContainer* aImageContainer,
+                      ImageContainerChild* aContainerChild,
+                      bool *aDone)
 {
   ReentrantMonitorAutoEnter autoMon(*barrier);
-  *result = sImageBridgeChildSingleton->CreateImageClientNow(
-      aType, aImageContainer);
+  *result = aChild->CreateImageClientNow(aType, aImageContainer, aContainerChild);
   *aDone = true;
   barrier->NotifyAll();
 }
 
 // dispatched function
 static void CreateCanvasClientSync(ReentrantMonitor* aBarrier,
                                    CanvasClient::CanvasClientType aType,
                                    TextureFlags aFlags,
@@ -563,39 +564,39 @@ ImageBridgeChild* ImageBridgeChild::GetS
   return sImageBridgeChildSingleton;
 }
 
 bool ImageBridgeChild::IsCreated()
 {
   return GetSingleton() != nullptr;
 }
 
-static void ReleaseImageContainerNow(PImageContainerChild* aChild)
+static void
+ReleaseImageContainerNow(RefPtr<ImageBridgeChild> aBridge, RefPtr<ImageContainerChild> aChild)
 {
   MOZ_ASSERT(InImageBridgeChildThread());
 
-  if (aChild) {
-    ImageContainer::AsyncDestroyActor(aChild);
-  }
+  aChild->SendAsyncDelete();
 }
 
 // static
-void ImageBridgeChild::DispatchReleaseImageContainer(PImageContainerChild* aChild)
+void ImageBridgeChild::DispatchReleaseImageContainer(ImageContainerChild* aChild)
 {
   if (!aChild) {
     return;
   }
 
-  if (!IsCreated()) {
-    delete aChild;
+  RefPtr<ImageBridgeChild> bridge = GetSingleton();
+  if (!bridge) {
     return;
   }
 
-  sImageBridgeChildSingleton->GetMessageLoop()->PostTask(
-    NewRunnableFunction(&ReleaseImageContainerNow, aChild));
+  RefPtr<ImageContainerChild> child(aChild);
+  bridge->GetMessageLoop()->PostTask(
+    NewRunnableFunction(&ReleaseImageContainerNow, bridge, child));
 }
 
 static void ReleaseTextureClientNow(TextureClient* aClient)
 {
   MOZ_ASSERT(InImageBridgeChildThread());
   RELEASE_MANUALLY(aClient);
 }
 
@@ -991,54 +992,61 @@ void ImageBridgeChild::ConnectAsync(Imag
 void
 ImageBridgeChild::IdentifyCompositorTextureHost(const TextureFactoryIdentifier& aIdentifier)
 {
   if (sImageBridgeChildSingleton) {
     sImageBridgeChildSingleton->IdentifyTextureHost(aIdentifier);
   }
 }
 
-already_AddRefed<ImageClient>
+RefPtr<ImageClient>
 ImageBridgeChild::CreateImageClient(CompositableType aType,
-                                    ImageContainer* aImageContainer)
+                                    ImageContainer* aImageContainer,
+                                    ImageContainerChild* aContainerChild)
 {
   if (InImageBridgeChildThread()) {
-    return CreateImageClientNow(aType, aImageContainer);
+    return CreateImageClientNow(aType, aImageContainer, aContainerChild);
   }
+
   ReentrantMonitor barrier("CreateImageClient Lock");
   ReentrantMonitorAutoEnter autoMon(barrier);
   bool done = false;
 
   RefPtr<ImageClient> result = nullptr;
   GetMessageLoop()->PostTask(
-      NewRunnableFunction(&CreateImageClientSync, &result, &barrier, aType,
-                          aImageContainer, &done));
+      NewRunnableFunction(&CreateImageClientSync, this, &result, &barrier, aType,
+                          aImageContainer, aContainerChild, &done));
   // should stop the thread until the ImageClient has been created on
   // the other thread
   while (!done) {
     barrier.Wait();
   }
-  return result.forget();
+
+  return result;
 }
 
-already_AddRefed<ImageClient>
+RefPtr<ImageClient>
 ImageBridgeChild::CreateImageClientNow(CompositableType aType,
-                                       ImageContainer* aImageContainer)
+                                       ImageContainer* aImageContainer,
+                                       ImageContainerChild* aContainerChild)
 {
-  MOZ_ASSERT(!sImageBridgeChildSingleton->mShuttingDown);
+  MOZ_ASSERT(!mShuttingDown);
+  MOZ_ASSERT(InImageBridgeChildThread());
+
   if (aImageContainer) {
-    SendPImageContainerConstructor(aImageContainer->GetPImageContainerChild());
+    SendPImageContainerConstructor(aContainerChild);
+    aContainerChild->RegisterWithIPDL();
   }
-  RefPtr<ImageClient> client
-    = ImageClient::CreateImageClient(aType, this, TextureFlags::NO_FLAGS);
+
+  RefPtr<ImageClient> client = ImageClient::CreateImageClient(aType, this, TextureFlags::NO_FLAGS);
   MOZ_ASSERT(client, "failed to create ImageClient");
   if (client) {
     client->Connect(aImageContainer);
   }
-  return client.forget();
+  return client;
 }
 
 already_AddRefed<CanvasClient>
 ImageBridgeChild::CreateCanvasClient(CanvasClient::CanvasClientType aType,
                                      TextureFlags aFlag)
 {
   if (InImageBridgeChildThread()) {
     return CreateCanvasClientNow(aType, aFlag);
@@ -1231,17 +1239,17 @@ ImageBridgeChild::AllocPImageContainerCh
   // we always use the "power-user" ctor
   NS_RUNTIMEABORT("not reached");
   return nullptr;
 }
 
 bool
 ImageBridgeChild::DeallocPImageContainerChild(PImageContainerChild* actor)
 {
-  ImageContainer::DeallocActor(actor);
+  static_cast<ImageContainerChild*>(actor)->UnregisterFromIPDL();
   return true;
 }
 
 bool
 ImageBridgeChild::RecvParentAsyncMessages(InfallibleTArray<AsyncParentMessageData>&& aMessages)
 {
   for (AsyncParentMessageArray::index_type i = 0; i < aMessages.Length(); ++i) {
     const AsyncParentMessageData& message = aMessages[i];
--- a/gfx/layers/ipc/ImageBridgeChild.h
+++ b/gfx/layers/ipc/ImageBridgeChild.h
@@ -34,16 +34,17 @@ class Shmem;
 } // namespace ipc
 
 namespace layers {
 
 class AsyncCanvasRenderer;
 class AsyncTransactionTracker;
 class ImageClient;
 class ImageContainer;
+class ImageContainerChild;
 class ImageBridgeParent;
 class CompositableClient;
 struct CompositableTransaction;
 class Image;
 class TextureClient;
 
 /**
  * Returns true if the current thread is the ImageBrdigeChild's thread.
@@ -213,26 +214,34 @@ public:
   DeallocPImageContainerChild(PImageContainerChild* actor) override;
 
   virtual bool
   RecvParentAsyncMessages(InfallibleTArray<AsyncParentMessageData>&& aMessages) override;
 
   virtual bool
   RecvDidComposite(InfallibleTArray<ImageCompositeNotification>&& aNotifications) override;
 
-  already_AddRefed<ImageClient> CreateImageClient(CompositableType aType,
-                                                  ImageContainer* aImageContainer);
-  already_AddRefed<ImageClient> CreateImageClientNow(CompositableType aType,
-                                                     ImageContainer* aImageContainer);
+  // Create an ImageClient from any thread.
+  RefPtr<ImageClient> CreateImageClient(
+    CompositableType aType,
+    ImageContainer* aImageContainer,
+    ImageContainerChild* aContainerChild);
+
+  // Create an ImageClient from the ImageBridge thread.
+  RefPtr<ImageClient> CreateImageClientNow(
+    CompositableType aType,
+    ImageContainer* aImageContainer,
+    ImageContainerChild* aContainerChild);
+
   already_AddRefed<CanvasClient> CreateCanvasClient(CanvasClient::CanvasClientType aType,
                                                     TextureFlags aFlag);
   already_AddRefed<CanvasClient> CreateCanvasClientNow(CanvasClient::CanvasClientType aType,
                                                        TextureFlags aFlag);
 
-  static void DispatchReleaseImageContainer(PImageContainerChild* aChild = nullptr);
+  static void DispatchReleaseImageContainer(ImageContainerChild* aChild);
   static void DispatchReleaseTextureClient(TextureClient* aClient);
   static void DispatchImageClientUpdate(ImageClient* aClient, ImageContainer* aContainer);
 
   static void UpdateAsyncCanvasRenderer(AsyncCanvasRenderer* aClient);
   static void UpdateAsyncCanvasRendererNow(AsyncCanvasRenderer* aClient);
 
   /**
    * Flush all Images sent to CompositableHost.
--- a/gfx/layers/ipc/ImageContainerChild.cpp
+++ b/gfx/layers/ipc/ImageContainerChild.cpp
@@ -9,18 +9,17 @@
 #include "mozilla/layers/ImageBridgeChild.h"
 
 namespace mozilla {
 namespace layers {
 
 ImageContainerChild::ImageContainerChild(ImageContainer* aImageContainer)
   : mLock("ImageContainerChild")
   , mImageContainer(aImageContainer)
-  , mImageContainerReleased(false)
-  , mIPCOpen(true)
+  , mIPCOpen(false)
 {
 }
 
 void
 ImageContainerChild::ForgetImageContainer()
 {
   MutexAutoLock lock(mLock);
   mImageContainer = nullptr;
@@ -32,10 +31,40 @@ ImageContainerChild::NotifyComposite(con
   MOZ_ASSERT(InImageBridgeChildThread());
 
   MutexAutoLock lock(mLock);
   if (mImageContainer) {
     mImageContainer->NotifyCompositeInternal(aNotification);
   }
 }
 
+void
+ImageContainerChild::RegisterWithIPDL()
+{
+  MOZ_ASSERT(!mIPCOpen);
+  MOZ_ASSERT(InImageBridgeChildThread());
+
+  AddRef();
+  mIPCOpen = true;
+}
+
+void
+ImageContainerChild::UnregisterFromIPDL()
+{
+  MOZ_ASSERT(mIPCOpen);
+  MOZ_ASSERT(InImageBridgeChildThread());
+
+  mIPCOpen = false;
+  Release();
+}
+
+void
+ImageContainerChild::SendAsyncDelete()
+{
+  MOZ_ASSERT(InImageBridgeChildThread());
+
+  if (mIPCOpen) {
+    PImageContainerChild::SendAsyncDelete();
+  }
+}
+
 } // namespace layers
 } // namespace mozilla
--- a/gfx/layers/ipc/ImageContainerChild.h
+++ b/gfx/layers/ipc/ImageContainerChild.h
@@ -24,35 +24,38 @@ class ImageCompositeNotification;
  * An ImageContainer owns one of these; we have a weak reference to our
  * ImageContainer.
  */
 class ImageContainerChild final : public PImageContainerChild
 {
 public:
   explicit ImageContainerChild(ImageContainer* aImageContainer);
 
-  void ForgetImageContainer();
+  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(ImageContainerChild)
+
+  void RegisterWithIPDL();
+  void UnregisterFromIPDL();
+  void SendAsyncDelete();
 
   void NotifyComposite(const ImageCompositeNotification& aNotification);
+  void ForgetImageContainer();
 
-public:
-  // If mImageContainerReleased is false when we try to deallocate this actor,
-  // it means the ImageContainer is still holding a pointer to this.
-  // mImageContainerReleased must not be accessed off the ImageBridgeChild thread.
-  bool mImageContainerReleased;
+private:
+  ~ImageContainerChild()
+  {}
+
+private:
+  Mutex mLock;
+  ImageContainer* mImageContainer;
 
   // If mIPCOpen is false, it means the IPDL code tried to deallocate the actor
   // before the ImageContainer released it. When this happens we don't actually
   // delete the actor right away because the ImageContainer has a reference to
   // it. In this case the actor will be deleted when the ImageContainer lets go
   // of it.
   // mIPCOpen must not be accessed off the ImageBridgeChild thread.
   bool mIPCOpen;
-
-private:
-  Mutex mLock;
-  ImageContainer* mImageContainer;
 };
 
 } // namespace layers
 } // namespace mozilla
 
 #endif // mozilla_gfx_layers_ImageContainerChild_h