Bug 1037360 - Fix SharedBufferManagerParent's destruction r=jrmuizel,jbacob

gfx/layers/ipc/SharedBufferManagerParent.cpp

@@ -14,6 +14,7 @@
 #include "mozilla/ipc/MessageChannel.h" // for MessageChannel, etc
 #include "mozilla/ipc/ProtocolUtils.h"
 #include "mozilla/ipc/Transport.h"      // for Transport
+#include "mozilla/UniquePtr.h"          // for UniquePtr
 #include "nsIMemoryReporter.h"
 #ifdef MOZ_WIDGET_GONK
 #include "ui/PixelFormat.h"
@@ -51,7 +52,7 @@ public:
       base::ProcessId pid = it->first;
       SharedBufferManagerParent *mgr = it->second;
 
-      MutexAutoLock lock(mgr->mBuffersMutex);
+      MutexAutoLock lock(mgr->mLock);
       std::map<int64_t, android::sp<android::GraphicBuffer> >::iterator buf_it;
       for (buf_it = mgr->mBuffers.begin(); buf_it != mgr->mBuffers.end(); buf_it++) {
         nsresult rv;
@@ -101,13 +102,28 @@ void InitGralloc() {
 #endif
 }
 
+/**
+ * Task that deletes SharedBufferManagerParent on a specified thread.
+ */
+class DeleteSharedBufferManagerParentTask : public Task
+{
+public:
+    DeleteSharedBufferManagerParentTask(UniquePtr<SharedBufferManagerParent> aSharedBufferManager)
+        : mSharedBufferManager(Move(aSharedBufferManager)) {
+    }
+    virtual void Run() MOZ_OVERRIDE {}
+private:
+    UniquePtr<SharedBufferManagerParent> mSharedBufferManager;
+};
+
 SharedBufferManagerParent::SharedBufferManagerParent(Transport* aTransport, base::ProcessId aOwner, base::Thread* aThread)
   : mTransport(aTransport)
   , mThread(aThread)
-#ifdef MOZ_HAVE_SURFACEDESCRIPTORGRALLOC
-  , mBuffersMutex("BuffersMonitor")
-#endif
+  , mMainMessageLoop(MessageLoop::current())
+  , mDestroyed(false)
+  , mLock("SharedBufferManagerParent.mLock")
 {
+  MOZ_ASSERT(NS_IsMainThread());
   if (!sManagerMonitor) {
     sManagerMonitor = new Monitor("Manager Monitor");
   }
@@ -139,10 +155,14 @@ SharedBufferManagerParent::~SharedBufferManagerParent()
 void
 SharedBufferManagerParent::ActorDestroy(ActorDestroyReason aWhy)
 {
+  MutexAutoLock lock(mLock);
+  mDestroyed = true;
 #ifdef MOZ_HAVE_SURFACEDESCRIPTORGRALLOC
-  MutexAutoLock lock(mBuffersMutex);
   mBuffers.clear();
 #endif
+  DeleteSharedBufferManagerParentTask* task =
+    new DeleteSharedBufferManagerParentTask(UniquePtr<SharedBufferManagerParent>(this));
+  mMainMessageLoop->PostTask(FROM_HERE, task);
 }
 
 static void
@@ -211,7 +231,7 @@ bool SharedBufferManagerParent::RecvAllocateGrallocBuffer(const IntSize& aSize,
   *aHandle = MagicGrallocBufferHandle(outgoingBuffer, ref);
 
   {
-    MutexAutoLock lock(mBuffersMutex);
+    MutexAutoLock lock(mLock);
     mBuffers[bufferKey] = outgoingBuffer;
   }
 #endif
@@ -225,7 +245,7 @@ bool SharedBufferManagerParent::RecvDropGrallocBuffer(const mozilla::layers::May
   int64_t bufferKey = handle.get_GrallocBufferRef().mKey;
   sp<GraphicBuffer> buf = GetGraphicBuffer(bufferKey);
   MOZ_ASSERT(buf.get());
-  MutexAutoLock lock(mBuffersMutex);
+  MutexAutoLock lock(mLock);
   NS_ASSERTION(mBuffers.count(bufferKey) == 1, "No such buffer");
   mBuffers.erase(bufferKey);
 
@@ -238,30 +258,46 @@ bool SharedBufferManagerParent::RecvDropGrallocBuffer(const mozilla::layers::May
   return true;
 }
 
+/*static*/
 void SharedBufferManagerParent::DropGrallocBufferSync(SharedBufferManagerParent* mgr, mozilla::layers::SurfaceDescriptor aDesc)
 {
   mgr->DropGrallocBufferImpl(aDesc);
 }
 
-void SharedBufferManagerParent::DropGrallocBuffer(mozilla::layers::SurfaceDescriptor aDesc)
+/*static*/
+void SharedBufferManagerParent::DropGrallocBuffer(ProcessId id, mozilla::layers::SurfaceDescriptor aDesc)
 {
   if (aDesc.type() != SurfaceDescriptor::TNewSurfaceDescriptorGralloc) {
     return;
   }
 
-  if (PlatformThread::CurrentId() == mThread->thread_id()) {
-    DropGrallocBufferImpl(aDesc);
+  MonitorAutoLock lock(*sManagerMonitor.get());
+  SharedBufferManagerParent* mgr = SharedBufferManagerParent::GetInstance(id);
+  if (!mgr) {
+    return;
+  }
+
+  MutexAutoLock mgrlock(mgr->mLock);
+  if (mgr->mDestroyed) {
+    return;
+  }
+
+  if (PlatformThread::CurrentId() == mgr->mThread->thread_id()) {
+    MOZ_CRASH("SharedBufferManagerParent::DropGrallocBuffer should not be called on SharedBufferManagerParent thread");
   } else {
-    mThread->message_loop()->PostTask(FROM_HERE,
-                                      NewRunnableFunction(&DropGrallocBufferSync, this, aDesc));
+    mgr->mThread->message_loop()->PostTask(FROM_HERE,
+                                      NewRunnableFunction(&DropGrallocBufferSync, mgr, aDesc));
   }
   return;
 }
 
 void SharedBufferManagerParent::DropGrallocBufferImpl(mozilla::layers::SurfaceDescriptor aDesc)
 {
+  MutexAutoLock lock(mLock);
+  if (mDestroyed) {
+    return;
+  }
 #ifdef MOZ_HAVE_SURFACEDESCRIPTORGRALLOC
-  MutexAutoLock lock(mBuffersMutex);
   int64_t key = -1;
   MaybeMagicGrallocBufferHandle handle;
   if (aDesc.type() == SurfaceDescriptor::TNewSurfaceDescriptorGralloc) {
@@ -290,7 +326,6 @@ MessageLoop* SharedBufferManagerParent::GetMessageLoop()
 
 SharedBufferManagerParent* SharedBufferManagerParent::GetInstance(ProcessId id)
 {
-  MonitorAutoLock lock(*sManagerMonitor.get());
   NS_ASSERTION(sManagers.count(id) == 1, "No BufferManager for the process");
   return sManagers[id];
 }
@@ -299,7 +334,7 @@ SharedBufferManagerParent* SharedBufferManagerParent::GetInstance(ProcessId id)
 android::sp<android::GraphicBuffer>
 SharedBufferManagerParent::GetGraphicBuffer(int64_t key)
 {
-  MutexAutoLock lock(mBuffersMutex);
+  MutexAutoLock lock(mLock);
   if (mBuffers.count(key) == 1) {
     return mBuffers[key];
   } else {
@@ -312,6 +347,7 @@ SharedBufferManagerParent::GetGraphicBuffer(int64_t key)
 android::sp<android::GraphicBuffer>
 SharedBufferManagerParent::GetGraphicBuffer(GrallocBufferRef aRef)
 {
+  MonitorAutoLock lock(*sManagerMonitor.get());
   return GetInstance(aRef.mOwner)->GetGraphicBuffer(aRef.mKey);
 }
 #endif

gfx/layers/ipc/SharedBufferManagerParent.h

@@ -40,11 +40,6 @@ public:
    */
   static PSharedBufferManagerParent* Create(Transport* aTransport, ProcessId aOtherProcess);
 
-  /**
-   * Function for find the buffer owner, most buffer passing on IPC contains only owner/key pair.
-   * Use these function to access the real buffer.
-   */
-  static SharedBufferManagerParent* GetInstance(ProcessId id);
 #ifdef MOZ_HAVE_SURFACEDESCRIPTORGRALLOC
   android::sp<android::GraphicBuffer> GetGraphicBuffer(int64_t key);
   static android::sp<android::GraphicBuffer> GetGraphicBuffer(GrallocBufferRef aRef);
@@ -66,7 +61,7 @@ public:
   /**
    * Break the buffer's sharing state, decrease buffer reference for both side
    */
-  void DropGrallocBuffer(mozilla::layers::SurfaceDescriptor aDesc);
+  static void DropGrallocBuffer(ProcessId id, mozilla::layers::SurfaceDescriptor aDesc);
 
   // Overriden from IToplevelProtocol
   IToplevelProtocol*
@@ -76,10 +71,6 @@ public:
   MessageLoop* GetMessageLoop();
 
 protected:
-  /**
-   * All living SharedBufferManager instances used to find the buffer owner, and parent->child IPCs
-   */
-  static std::map<base::ProcessId, SharedBufferManagerParent*> sManagers;
 
   /**
    * Break the buffer's sharing state, decrease buffer reference for both side
@@ -91,19 +82,33 @@ protected:
   // dispatched function
   static void DropGrallocBufferSync(SharedBufferManagerParent* mgr, mozilla::layers::SurfaceDescriptor aDesc);
 
+  /**
+   * Function for find the buffer owner, most buffer passing on IPC contains only owner/key pair.
+   * Use these function to access the real buffer.
+   * Caller needs to hold sManagerMonitor.
+   */
+  static SharedBufferManagerParent* GetInstance(ProcessId id);
+
+  /**
+   * All living SharedBufferManager instances used to find the buffer owner, and parent->child IPCs
+   */
+  static std::map<base::ProcessId, SharedBufferManagerParent*> sManagers;
+
 #ifdef MOZ_HAVE_SURFACEDESCRIPTORGRALLOC
   /**
    * Buffers owned by this SharedBufferManager pair
    */
   std::map<int64_t, android::sp<android::GraphicBuffer> > mBuffers;
-  Mutex mBuffersMutex;
 #endif
   
   Transport* mTransport;
   base::ProcessId mOwner;
   base::Thread* mThread;
-  static uint64_t sBufferKey;
+  MessageLoop* mMainMessageLoop;
+  bool mDestroyed;
+  Mutex mLock;
 
+  static uint64_t sBufferKey;
   static StaticAutoPtr<Monitor> sManagerMonitor;
 };
 

gfx/layers/opengl/GrallocTextureHost.cpp

@@ -383,7 +383,7 @@ GrallocTextureHostOGL::DeallocateSharedData()
       owner = handle.get_MagicGrallocBufferHandle().mRef.mOwner;
     }
 
-    SharedBufferManagerParent::GetInstance(owner)->DropGrallocBuffer(mGrallocHandle);
+    SharedBufferManagerParent::DropGrallocBuffer(owner, mGrallocHandle);
   }
 }