Bug 1131463 - Report AtomicRefCounterWithFinalize doing the wrong thing with AddRef and Release in release build as well. r=sotaro
authorMilan Sreckovic <milan@mozilla.com>
Fri, 29 May 2015 16:41:28 -0400
changeset 269655 d790f81f9f24f83762f888ea52850a9144d8e52d
parent 269654 524a9ccacb05e7bd68256aa4c5902d83a6ff5de6
child 269656 7f533d42cf01eef4a282ec3dea7dd5e4c3d318ee
push id2540
push userwcosta@mozilla.com
push dateWed, 03 Jun 2015 20:55:41 +0000
reviewerssotaro
bugs1131463
milestone41.0a1
Bug 1131463 - Report AtomicRefCounterWithFinalize doing the wrong thing with AddRef and Release in release build as well. r=sotaro
dom/media/omx/MediaCodecReader.cpp
dom/media/omx/OmxDecoder.cpp
dom/media/platforms/gonk/GonkVideoDecoderManager.cpp
gfx/layers/AtomicRefCountedWithFinalize.h
gfx/layers/client/TextureClientRecycleAllocator.cpp
gfx/layers/composite/TextureHost.cpp
mfbt/RefPtr.h
widget/gonk/nativewindow/GonkNativeWindowICS.cpp
widget/gonk/nativewindow/GonkNativeWindowJB.cpp
widget/gonk/nativewindow/GonkNativeWindowKK.cpp
widget/gonk/nativewindow/GonkNativeWindowLL.cpp
--- a/dom/media/omx/MediaCodecReader.cpp
+++ b/dom/media/omx/MediaCodecReader.cpp
@@ -763,17 +763,17 @@ MediaCodecReader::TextureClientRecycleCa
 
   reader->TextureClientRecycleCallback(aClient);
 }
 
 void
 MediaCodecReader::TextureClientRecycleCallback(TextureClient* aClient)
 {
   MOZ_ASSERT(aClient, "aClient should not be nullptr in RecycleCallback()");
-
+  MOZ_ASSERT(!aClient->IsDead());
   size_t index = 0;
 
   {
     MutexAutoLock al(mTextureClientIndexesLock);
 
     aClient->ClearRecycleCallback();
 
     // aClient has been removed from mTextureClientIndexes by
--- a/dom/media/omx/OmxDecoder.cpp
+++ b/dom/media/omx/OmxDecoder.cpp
@@ -910,11 +910,12 @@ void OmxDecoder::RecycleCallbackImp(Text
             new AMessage(kNotifyPostReleaseVideoBuffer, mReflector->id());
   // post AMessage to OmxDecoder via ALooper.
   notify->post();
 }
 
 /* static */ void
 OmxDecoder::RecycleCallback(TextureClient* aClient, void* aClosure)
 {
+  MOZ_ASSERT(aClient && !aClient->IsDead());
   OmxDecoder* decoder = static_cast<OmxDecoder*>(aClosure);
   decoder->RecycleCallbackImp(aClient);
 }
--- a/dom/media/platforms/gonk/GonkVideoDecoderManager.cpp
+++ b/dom/media/platforms/gonk/GonkVideoDecoderManager.cpp
@@ -564,16 +564,17 @@ GonkVideoDecoderManager::GetColorConvert
   }
   return mColorConverterBuffer.get();
 }
 
 /* static */
 void
 GonkVideoDecoderManager::RecycleCallback(TextureClient* aClient, void* aClosure)
 {
+  MOZ_ASSERT(aClient && !aClient->IsDead());
   GonkVideoDecoderManager* videoManager = static_cast<GonkVideoDecoderManager*>(aClosure);
   GrallocTextureClientOGL* client = static_cast<GrallocTextureClientOGL*>(aClient);
   aClient->ClearRecycleCallback();
   videoManager->PostReleaseVideoBuffer(client->GetMediaBuffer());
 }
 
 void GonkVideoDecoderManager::PostReleaseVideoBuffer(
                                 android::MediaBuffer *aBuffer)
--- a/gfx/layers/AtomicRefCountedWithFinalize.h
+++ b/gfx/layers/AtomicRefCountedWithFinalize.h
@@ -6,96 +6,126 @@
 #ifndef MOZILLA_ATOMICREFCOUNTEDWITHFINALIZE_H_
 #define MOZILLA_ATOMICREFCOUNTEDWITHFINALIZE_H_
 
 #include "mozilla/RefPtr.h"
 #include "mozilla/Likely.h"
 #include "MainThreadUtils.h"
 #include "base/message_loop.h"
 #include "base/task.h"
+#include "mozilla/gfx/Logging.h"
 
 namespace mozilla {
 
 template<typename T>
 class AtomicRefCountedWithFinalize
 {
   protected:
     AtomicRefCountedWithFinalize()
       : mRecycleCallback(nullptr)
       , mRefCount(0)
       , mMessageLoopToPostDestructionTo(nullptr)
     {}
 
-    ~AtomicRefCountedWithFinalize() {}
+    ~AtomicRefCountedWithFinalize() {
+      if (mRefCount >= 0) {
+        gfxCriticalError() << "Deleting referenced object? " << mRefCount;
+      }
+    }
 
     void SetMessageLoopToPostDestructionTo(MessageLoop* l) {
       MOZ_ASSERT(NS_IsMainThread());
       mMessageLoopToPostDestructionTo = l;
     }
 
     static void DestroyToBeCalledOnMainThread(T* ptr) {
       MOZ_ASSERT(NS_IsMainThread());
       delete ptr;
     }
 
   public:
     void AddRef() {
-      MOZ_ASSERT(mRefCount >= 0);
       ++mRefCount;
     }
 
     void Release() {
-      MOZ_ASSERT(mRefCount > 0);
       // Read mRecycleCallback early so that it does not get set to
-      // deleted memory, if the object is goes away.
+      // deleted memory, if the object is goes away.  See bug 994903.
+      // This saves us in the case where there is no callback, so that
+      // we can do the "else if" below.
       RecycleCallback recycleCallback = mRecycleCallback;
       int currCount = --mRefCount;
+      if (currCount < 0) {
+        gfxCriticalError() << "Invalid reference count release" << currCount;
+        ++mRefCount;
+        return;
+      }
+
       if (0 == currCount) {
+        mRefCount = detail::DEAD;
+        MOZ_ASSERT(IsDead());
+
         // Recycle listeners must call ClearRecycleCallback
         // before releasing their strong reference.
-        MOZ_ASSERT(mRecycleCallback == nullptr);
-#ifdef DEBUG
-        mRefCount = detail::DEAD;
-#endif
+        if (mRecycleCallback) {
+          gfxCriticalError() << "About to release with valid callback";
+          mRecycleCallback = nullptr;
+        }
+
         T* derived = static_cast<T*>(this);
         derived->Finalize();
         if (MOZ_LIKELY(!mMessageLoopToPostDestructionTo)) {
           delete derived;
         } else {
           if (MOZ_LIKELY(NS_IsMainThread())) {
             delete derived;
           } else {
             mMessageLoopToPostDestructionTo->PostTask(
               FROM_HERE,
               NewRunnableFunction(&DestroyToBeCalledOnMainThread, derived));
           }
         }
       } else if (1 == currCount && recycleCallback) {
+        // There is nothing enforcing this in the code, except how the callers
+        // are being careful to never let the reference count go down if there
+        // is a callback.
+        MOZ_ASSERT(!IsDead());
         T* derived = static_cast<T*>(this);
         recycleCallback(derived, mClosure);
       }
     }
 
     typedef void (*RecycleCallback)(T* aObject, void* aClosure);
     /**
      * Set a callback responsible for recycling this object
      * before it is finalized.
      */
     void SetRecycleCallback(RecycleCallback aCallback, void* aClosure)
     {
+      MOZ_ASSERT(!IsDead());
       mRecycleCallback = aCallback;
       mClosure = aClosure;
     }
-    void ClearRecycleCallback() { SetRecycleCallback(nullptr, nullptr); }
+    void ClearRecycleCallback()
+    {
+      MOZ_ASSERT(!IsDead());
+      SetRecycleCallback(nullptr, nullptr);
+    }
 
     bool HasRecycleCallback() const
     {
+      MOZ_ASSERT(!IsDead());
       return !!mRecycleCallback;
     }
 
+    bool IsDead() const
+    {
+      return mRefCount < 0;
+    }
+
 private:
     RecycleCallback mRecycleCallback;
     void *mClosure;
     Atomic<int> mRefCount;
     MessageLoop *mMessageLoopToPostDestructionTo;
 };
 
 }
--- a/gfx/layers/client/TextureClientRecycleAllocator.cpp
+++ b/gfx/layers/client/TextureClientRecycleAllocator.cpp
@@ -230,16 +230,17 @@ TextureClientRecycleAllocatorImp::Recycl
       mInUseClients.erase(aClient);
     }
   }
 }
 
 /* static */ void
 TextureClientRecycleAllocatorImp::RecycleCallback(TextureClient* aClient, void* aClosure)
 {
+  MOZ_ASSERT(aClient && !aClient->IsDead());
   TextureClientRecycleAllocatorImp* recycleAllocator = static_cast<TextureClientRecycleAllocatorImp*>(aClosure);
   recycleAllocator->RecycleCallbackImp(aClient);
 }
 
 TextureClientRecycleAllocator::TextureClientRecycleAllocator(ISurfaceAllocator *aAllocator)
 {
   mAllocator = new TextureClientRecycleAllocatorImp(aAllocator);
 }
--- a/gfx/layers/composite/TextureHost.cpp
+++ b/gfx/layers/composite/TextureHost.cpp
@@ -759,17 +759,17 @@ TextureParent::TextureParent(Compositabl
 TextureParent::~TextureParent()
 {
   MOZ_COUNT_DTOR(TextureParent);
   if (mTextureHost) {
     mTextureHost->ClearRecycleCallback();
   }
 }
 
-static void RecycleCallback(TextureHost* textureHost, void* aClosure) {
+static void RecycleCallback(TextureHost*, void* aClosure) {
   TextureParent* tp = reinterpret_cast<TextureParent*>(aClosure);
   tp->CompositorRecycle();
 }
 
 void
 TextureParent::CompositorRecycle()
 {
   mTextureHost->ClearRecycleCallback();
--- a/mfbt/RefPtr.h
+++ b/mfbt/RefPtr.h
@@ -55,19 +55,17 @@ template<typename T> OutParamRef<T> byRe
  * state distinguishes use-before-ref (refcount==0) from
  * use-after-destroy (refcount==0xffffdead).
  *
  * Note that when deriving from RefCounted or AtomicRefCounted, you
  * should add MOZ_DECLARE_REFCOUNTED_TYPENAME(ClassName) to the public
  * section of your class, where ClassName is the name of your class.
  */
 namespace detail {
-#ifdef DEBUG
 const MozRefCountType DEAD = 0xffffdead;
-#endif
 
 // When building code that gets compiled into Gecko, try to use the
 // trace-refcount leak logging facilities.
 #ifdef MOZ_REFCOUNTED_LEAK_CHECKING
 class RefCountLogger
 {
 public:
   static void logAddRef(const void* aPointer, MozRefCountType aRefCount,
--- a/widget/gonk/nativewindow/GonkNativeWindowICS.cpp
+++ b/widget/gonk/nativewindow/GonkNativeWindowICS.cpp
@@ -494,16 +494,17 @@ GonkNativeWindow::getCurrentBuffer() {
 }
 
 
 /* static */ void
 GonkNativeWindow::RecycleCallback(TextureClient* client, void* closure) {
   GonkNativeWindow* nativeWindow =
     static_cast<GonkNativeWindow*>(closure);
 
+  MOZ_ASSERT(client && !client->IsDead());
   client->ClearRecycleCallback();
   nativeWindow->returnBuffer(client);
 }
 
 void GonkNativeWindow::returnBuffer(TextureClient* client) {
   CNW_LOGD("GonkNativeWindow::returnBuffer");
   Mutex::Autolock lock(mMutex);
 
--- a/widget/gonk/nativewindow/GonkNativeWindowJB.cpp
+++ b/widget/gonk/nativewindow/GonkNativeWindowJB.cpp
@@ -129,16 +129,17 @@ GonkNativeWindow::getCurrentBuffer() {
     return textureClient.forget();
 }
 
 /* static */ void
 GonkNativeWindow::RecycleCallback(TextureClient* client, void* closure) {
   GonkNativeWindow* nativeWindow =
     static_cast<GonkNativeWindow*>(closure);
 
+  MOZ_ASSERT(client && !client->IsDead());
   client->ClearRecycleCallback();
   nativeWindow->returnBuffer(client);
 }
 
 void GonkNativeWindow::returnBuffer(TextureClient* client) {
     BI_LOGD("GonkNativeWindow::returnBuffer");
     Mutex::Autolock lock(mMutex);
 
--- a/widget/gonk/nativewindow/GonkNativeWindowKK.cpp
+++ b/widget/gonk/nativewindow/GonkNativeWindowKK.cpp
@@ -129,16 +129,17 @@ GonkNativeWindow::getCurrentBuffer() {
     return textureClient.forget();
 }
 
 /* static */ void
 GonkNativeWindow::RecycleCallback(TextureClient* client, void* closure) {
   GonkNativeWindow* nativeWindow =
     static_cast<GonkNativeWindow*>(closure);
 
+  MOZ_ASSERT(client && !client->IsDead());
   client->ClearRecycleCallback();
   nativeWindow->returnBuffer(client);
 }
 
 void GonkNativeWindow::returnBuffer(TextureClient* client) {
     ALOGD("GonkNativeWindow::returnBuffer");
     Mutex::Autolock lock(mMutex);
 
--- a/widget/gonk/nativewindow/GonkNativeWindowLL.cpp
+++ b/widget/gonk/nativewindow/GonkNativeWindowLL.cpp
@@ -140,16 +140,17 @@ GonkNativeWindow::getCurrentBuffer() {
     return textureClient.forget();
 }
 
 /* static */ void
 GonkNativeWindow::RecycleCallback(TextureClient* client, void* closure) {
     GonkNativeWindow* nativeWindow =
         static_cast<GonkNativeWindow*>(closure);
 
+    MOZ_ASSERT(client && !client->IsDead());
     client->ClearRecycleCallback();
     nativeWindow->returnBuffer(client);
 }
 
 void GonkNativeWindow::returnBuffer(TextureClient* client) {
     ALOGD("GonkNativeWindow::returnBuffer");
     Mutex::Autolock lock(mMutex);