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 279353 d790f81f9f24f83762f888ea52850a9144d8e52d
parent 279352 524a9ccacb05e7bd68256aa4c5902d83a6ff5de6
child 279354 7f533d42cf01eef4a282ec3dea7dd5e4c3d318ee
push id897
push userjlund@mozilla.com
push dateMon, 14 Sep 2015 18:56:12 +0000
treeherdermozilla-release@9411e2d2b214 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssotaro
bugs1131463
milestone41.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
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);