Fix a leak in RefBase (DO NOT MERGE)
this bug was introduced recently. it caused RefBase's weakref_impl
structure to be leaked for every RefBase object (about 20 bytes).
Change-Id: Ia9b155fbfa643ef72cfb8129e96260a3b806a78c
diff --git a/libs/utils/RefBase.cpp b/libs/utils/RefBase.cpp
index dd0052a..1c870cf 100644
--- a/libs/utils/RefBase.cpp
+++ b/libs/utils/RefBase.cpp
@@ -20,9 +20,9 @@
#include <utils/Atomic.h>
#include <utils/CallStack.h>
+#include <utils/KeyedVector.h>
#include <utils/Log.h>
#include <utils/threads.h>
-#include <utils/TextOutput.h>
#include <stdlib.h>
#include <stdio.h>
@@ -34,7 +34,6 @@
// compile with refcounting debugging enabled
#define DEBUG_REFS 0
-#define DEBUG_REFS_FATAL_SANITY_CHECKS 0
#define DEBUG_REFS_ENABLED_BY_DEFAULT 1
#define DEBUG_REFS_CALLSTACK_ENABLED 1
@@ -70,10 +69,8 @@
void addStrongRef(const void* /*id*/) { }
void removeStrongRef(const void* /*id*/) { }
- void renameStrongRefId(const void* /*old_id*/, const void* /*new_id*/) { }
void addWeakRef(const void* /*id*/) { }
void removeWeakRef(const void* /*id*/) { }
- void renameWeakRefId(const void* /*old_id*/, const void* /*new_id*/) { }
void printRefs() const { }
void trackMe(bool, bool) { }
@@ -89,91 +86,39 @@
, mTrackEnabled(!!DEBUG_REFS_ENABLED_BY_DEFAULT)
, mRetain(false)
{
+ //LOGI("NEW weakref_impl %p for RefBase %p", this, base);
}
~weakref_impl()
{
- bool dumpStack = false;
- if (!mRetain && mStrongRefs != NULL) {
- dumpStack = true;
-#if DEBUG_REFS_FATAL_SANITY_CHECKS
- LOG_ALWAYS_FATAL("Strong references remain!");
-#else
- LOGE("Strong references remain:");
-#endif
- ref_entry* refs = mStrongRefs;
- while (refs) {
- char inc = refs->ref >= 0 ? '+' : '-';
- LOGD("\t%c ID %p (ref %d):", inc, refs->id, refs->ref);
-#if DEBUG_REFS_CALLSTACK_ENABLED
- refs->stack.dump();
-#endif;
- refs = refs->next;
- }
- }
-
- if (!mRetain && mWeakRefs != NULL) {
- dumpStack = true;
-#if DEBUG_REFS_FATAL_SANITY_CHECKS
- LOG_ALWAYS_FATAL("Weak references remain:");
-#else
- LOGE("Weak references remain!");
-#endif
- ref_entry* refs = mWeakRefs;
- while (refs) {
- char inc = refs->ref >= 0 ? '+' : '-';
- LOGD("\t%c ID %p (ref %d):", inc, refs->id, refs->ref);
-#if DEBUG_REFS_CALLSTACK_ENABLED
- refs->stack.dump();
-#endif;
- refs = refs->next;
- }
- }
- if (dumpStack) {
- LOGE("above errors at:");
- CallStack stack;
- stack.update();
- stack.dump();
- }
+ LOG_ALWAYS_FATAL_IF(!mRetain && mStrongRefs != NULL, "Strong references remain!");
+ LOG_ALWAYS_FATAL_IF(!mRetain && mWeakRefs != NULL, "Weak references remain!");
}
- void addStrongRef(const void* id) {
- //LOGD_IF(mTrackEnabled,
- // "addStrongRef: RefBase=%p, id=%p", mBase, id);
+ void addStrongRef(const void* id)
+ {
addRef(&mStrongRefs, id, mStrong);
}
- void removeStrongRef(const void* id) {
- //LOGD_IF(mTrackEnabled,
- // "removeStrongRef: RefBase=%p, id=%p", mBase, id);
- if (!mRetain) {
+ void removeStrongRef(const void* id)
+ {
+ if (!mRetain)
removeRef(&mStrongRefs, id);
- } else {
+ else
addRef(&mStrongRefs, id, -mStrong);
- }
}
- void renameStrongRefId(const void* old_id, const void* new_id) {
- //LOGD_IF(mTrackEnabled,
- // "renameStrongRefId: RefBase=%p, oid=%p, nid=%p",
- // mBase, old_id, new_id);
- renameRefsId(mStrongRefs, old_id, new_id);
- }
-
- void addWeakRef(const void* id) {
+ void addWeakRef(const void* id)
+ {
addRef(&mWeakRefs, id, mWeak);
}
- void removeWeakRef(const void* id) {
- if (!mRetain) {
+ void removeWeakRef(const void* id)
+ {
+ if (!mRetain)
removeRef(&mWeakRefs, id);
- } else {
+ else
addRef(&mWeakRefs, id, -mWeak);
- }
- }
-
- void renameWeakRefId(const void* old_id, const void* new_id) {
- renameRefsId(mWeakRefs, old_id, new_id);
}
void trackMe(bool track, bool retain)
@@ -187,7 +132,8 @@
String8 text;
{
- Mutex::Autolock _l(const_cast<weakref_impl*>(this)->mMutex);
+ AutoMutex _l(const_cast<weakref_impl*>(this)->mMutex);
+
char buf[128];
sprintf(buf, "Strong references on RefBase %p (weakref_type %p):\n", mBase, this);
text.append(buf);
@@ -226,7 +172,6 @@
{
if (mTrackEnabled) {
AutoMutex _l(mMutex);
-
ref_entry* ref = new ref_entry;
// Reference count at the time of the snapshot, but before the
// update. Positive value means we increment, negative--we
@@ -236,6 +181,7 @@
#if DEBUG_REFS_CALLSTACK_ENABLED
ref->stack.update(2);
#endif
+
ref->next = *refs;
*refs = ref;
}
@@ -246,52 +192,20 @@
if (mTrackEnabled) {
AutoMutex _l(mMutex);
- ref_entry* const head = *refs;
- ref_entry* ref = head;
+ ref_entry* ref = *refs;
while (ref != NULL) {
if (ref->id == id) {
*refs = ref->next;
delete ref;
return;
}
+
refs = &ref->next;
ref = *refs;
}
-
-#if DEBUG_REFS_FATAL_SANITY_CHECKS
- LOG_ALWAYS_FATAL("RefBase: removing id %p on RefBase %p"
- "(weakref_type %p) that doesn't exist!",
- id, mBase, this);
-#endif
-
- LOGE("RefBase: removing id %p on RefBase %p"
- "(weakref_type %p) that doesn't exist!",
- id, mBase, this);
-
- ref = head;
- while (ref) {
- char inc = ref->ref >= 0 ? '+' : '-';
- LOGD("\t%c ID %p (ref %d):", inc, ref->id, ref->ref);
- ref = ref->next;
- }
-
- CallStack stack;
- stack.update();
- stack.dump();
- }
- }
-
- void renameRefsId(ref_entry* r, const void* old_id, const void* new_id)
- {
- if (mTrackEnabled) {
- AutoMutex _l(mMutex);
- ref_entry* ref = r;
- while (ref != NULL) {
- if (ref->id == old_id) {
- ref->id = new_id;
- }
- ref = ref->next;
- }
+
+ LOG_ALWAYS_FATAL("RefBase: removing id %p on RefBase %p (weakref_type %p) that doesn't exist!",
+ id, mBase, this);
}
}
@@ -321,6 +235,44 @@
// on removeref that match the address ones.
bool mRetain;
+#if 0
+ void addRef(KeyedVector<const void*, int32_t>* refs, const void* id)
+ {
+ AutoMutex _l(mMutex);
+ ssize_t i = refs->indexOfKey(id);
+ if (i >= 0) {
+ ++(refs->editValueAt(i));
+ } else {
+ i = refs->add(id, 1);
+ }
+ }
+
+ void removeRef(KeyedVector<const void*, int32_t>* refs, const void* id)
+ {
+ AutoMutex _l(mMutex);
+ ssize_t i = refs->indexOfKey(id);
+ LOG_ALWAYS_FATAL_IF(i < 0, "RefBase: removing id %p that doesn't exist!", id);
+ if (i >= 0) {
+ int32_t val = --(refs->editValueAt(i));
+ if (val == 0) {
+ refs->removeItemsAt(i);
+ }
+ }
+ }
+
+ void printRefs(const KeyedVector<const void*, int32_t>& refs)
+ {
+ const size_t N=refs.size();
+ for (size_t i=0; i<N; i++) {
+ printf("\tID %p: %d remain\n", refs.keyAt(i), refs.valueAt(i));
+ }
+ }
+
+ mutable Mutex mMutex;
+ KeyedVector<const void*, int32_t> mStrongRefs;
+ KeyedVector<const void*, int32_t> mWeakRefs;
+#endif
+
#endif
};
@@ -329,6 +281,7 @@
void RefBase::incStrong(const void* id) const
{
weakref_impl* const refs = mRefs;
+ refs->addWeakRef(id);
refs->incWeak(id);
refs->addStrongRef(id);
@@ -364,12 +317,14 @@
destroy();
}
}
+ refs->removeWeakRef(id);
refs->decWeak(id);
}
void RefBase::forceIncStrong(const void* id) const
{
weakref_impl* const refs = mRefs;
+ refs->addWeakRef(id);
refs->incWeak(id);
refs->addStrongRef(id);
@@ -418,18 +373,20 @@
if (c != 1) return;
if ((impl->mFlags&OBJECT_LIFETIME_WEAK) != OBJECT_LIFETIME_WEAK) {
- if (impl->mStrong == INITIAL_STRONG_VALUE)
- if (impl->mBase)
+ if (impl->mStrong == INITIAL_STRONG_VALUE) {
+ if (impl->mBase) {
impl->mBase->destroy();
- else {
+ }
+ } else {
// LOGV("Freeing refs %p of old RefBase %p\n", this, impl->mBase);
delete impl;
}
} else {
impl->mBase->onLastWeakRef(id);
if ((impl->mFlags&OBJECT_LIFETIME_FOREVER) != OBJECT_LIFETIME_FOREVER) {
- if (impl->mBase)
+ if (impl->mBase) {
impl->mBase->destroy();
+ }
}
}
}
@@ -483,6 +440,7 @@
}
}
+ impl->addWeakRef(id);
impl->addStrongRef(id);
#if PRINT_REFS
@@ -500,7 +458,7 @@
bool RefBase::weakref_type::attemptIncWeak(const void* id)
{
weakref_impl* const impl = static_cast<weakref_impl*>(this);
-
+
int32_t curCount = impl->mWeak;
LOG_ASSERT(curCount >= 0, "attemptIncWeak called on %p after underflow",
this);
@@ -530,7 +488,7 @@
void RefBase::weakref_type::trackMe(bool enable, bool retain)
{
- static_cast<weakref_impl*>(this)->trackMe(enable, retain);
+ static_cast<const weakref_impl*>(this)->trackMe(enable, retain);
}
RefBase::weakref_type* RefBase::createWeak(const void* id) const
@@ -547,12 +505,15 @@
RefBase::RefBase()
: mRefs(new weakref_impl(this))
{
+// LOGV("Creating refs %p with RefBase %p\n", mRefs, this);
}
RefBase::~RefBase()
{
- if (mRefs->mWeak == 0) {
- delete mRefs;
+ if ((mRefs->mFlags & OBJECT_LIFETIME_WEAK) == OBJECT_LIFETIME_WEAK) {
+ if (mRefs->mWeak == 0) {
+ delete mRefs;
+ }
}
}
@@ -577,37 +538,5 @@
void RefBase::onLastWeakRef(const void* /*id*/)
{
}
-
-// ---------------------------------------------------------------------------
-
-void RefBase::moveReferences(void* dst, void const* src, size_t n,
- const ReferenceConverterBase& caster)
-{
-#if DEBUG_REFS
- const size_t itemSize = caster.getReferenceTypeSize();
- for (size_t i=0 ; i<n ; i++) {
- void* d = reinterpret_cast<void *>(intptr_t(dst) + i*itemSize);
- void const* s = reinterpret_cast<void const*>(intptr_t(src) + i*itemSize);
- RefBase* ref(reinterpret_cast<RefBase*>(caster.getReferenceBase(d)));
- ref->mRefs->renameStrongRefId(s, d);
- ref->mRefs->renameWeakRefId(s, d);
- }
-#endif
-}
-
-// ---------------------------------------------------------------------------
-
-TextOutput& printStrongPointer(TextOutput& to, const void* val)
-{
- to << "sp<>(" << val << ")";
- return to;
-}
-
-TextOutput& printWeakPointer(TextOutput& to, const void* val)
-{
- to << "wp<>(" << val << ")";
- return to;
-}
-
-
+
}; // namespace android