Bug 1243466 - Don't crash if DeallocPImageContainerChild is called prematurely. r=sotaro a=ritu CLOSED TREE
authorNicolas Silva <nsilva@mozilla.com>
Mon, 04 Apr 2016 10:03:37 +0200
changeset 326230 39fce55feada79cade0a7f7b19f2f8cd75e69efb
parent 326229 866d5e9827e4d06299fe4a5ff922fc72168bff05
child 326231 a32e6eb7b93f3553cb81f65a4272a635fd944b4a
push id1128
push userjlund@mozilla.com
push dateWed, 01 Jun 2016 01:31:59 +0000
treeherdermozilla-release@fe0d30de989d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssotaro, ritu
bugs1243466
milestone47.0
Bug 1243466 - Don't crash if DeallocPImageContainerChild is called prematurely. r=sotaro a=ritu CLOSED TREE
gfx/layers/ImageContainer.cpp
gfx/layers/ImageContainer.h
gfx/layers/ipc/ImageBridgeChild.cpp
--- a/gfx/layers/ImageContainer.cpp
+++ b/gfx/layers/ImageContainer.cpp
@@ -98,29 +98,82 @@ BufferRecycleBin::GetBuffer(uint32_t aSi
  * destroyed on) the ImageBridge thread, except when we need to destroy it
  * during shutdown.
  * An ImageContainer owns one of these; we have a weak reference to our
  * ImageContainer.
  */
 class ImageContainerChild : public PImageContainerChild {
 public:
   explicit ImageContainerChild(ImageContainer* aImageContainer)
-    : mLock("ImageContainerChild"), mImageContainer(aImageContainer) {}
+    : mLock("ImageContainerChild")
+    , mImageContainer(aImageContainer)
+    , mImageContainerReleased(false)
+    , mIPCOpen(true)
+  {}
+
   void ForgetImageContainer()
   {
     MutexAutoLock lock(mLock);
     mImageContainer = nullptr;
   }
 
   // This protects mImageContainer. This is always taken before the
   // mImageContainer's monitor (when both need to be held).
   Mutex mLock;
   ImageContainer* mImageContainer;
+  // 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;
+  // 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;
 };
 
+// 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()),
   mImageClient(nullptr),
--- a/gfx/layers/ImageContainer.h
+++ b/gfx/layers/ImageContainer.h
@@ -549,16 +549,22 @@ public:
   PImageContainerChild* GetPImageContainerChild();
   static void NotifyComposite(const ImageCompositeNotification& aNotification);
 
   /**
    * 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():
   B2G_ACL_EXPORT ~ImageContainer();
 
   void SetCurrentImageInternal(const nsTArray<NonOwningImage>& aImages);
 
--- a/gfx/layers/ipc/ImageBridgeChild.cpp
+++ b/gfx/layers/ipc/ImageBridgeChild.cpp
@@ -442,18 +442,19 @@ ConnectImageBridgeInChildProcess(Transpo
 
 static void ReleaseImageClientNow(ImageClient* aClient,
                                   PImageContainerChild* aChild)
 {
   MOZ_ASSERT(InImageBridgeChildThread());
   if (aClient) {
     aClient->Release();
   }
-  if (aChild && ImageBridgeChild::IsCreated() && !ImageBridgeChild::IsShutDown()) {
-    aChild->SendAsyncDelete();
+
+  if (aChild) {
+    ImageContainer::AsyncDestroyActor(aChild);
   }
 }
 
 // static
 void ImageBridgeChild::DispatchReleaseImageClient(ImageClient* aClient,
                                                   PImageContainerChild* aChild)
 {
   if (!aClient && !aChild) {
@@ -1087,17 +1088,17 @@ ImageBridgeChild::AllocPImageContainerCh
   // we always use the "power-user" ctor
   NS_RUNTIMEABORT("not reached");
   return nullptr;
 }
 
 bool
 ImageBridgeChild::DeallocPImageContainerChild(PImageContainerChild* actor)
 {
-  delete actor;
+  ImageContainer::DeallocActor(actor);
   return true;
 }
 
 bool
 ImageBridgeChild::RecvParentAsyncMessages(InfallibleTArray<AsyncParentMessageData>&& aMessages)
 {
   for (AsyncParentMessageArray::index_type i = 0; i < aMessages.Length(); ++i) {
     const AsyncParentMessageData& message = aMessages[i];