Merge "Make RefBase more robust and debuggable"
diff --git a/include/utils/RefBase.h b/include/utils/RefBase.h
index a232a65..3c318c4 100644
--- a/include/utils/RefBase.h
+++ b/include/utils/RefBase.h
@@ -105,16 +105,14 @@
// Other more specific restrictions for wp<> and sp<>:
-// Constructing a strong or weak pointer to "this" in its constructors is almost
-// always wrong. In the case of strong pointers. it is always wrong with RefBase
-// because the onFirstRef() callback will be mode on an incompletely constructed
-// object. In either case, it is wrong if such a pointer does not outlive the
-// constructor, since destruction of the smart pointer will attempt to destroy the
-// object before construction is finished, normally resulting in a pointer to a
-// destroyed object being returned from a new expression.
-
-// In the case of weak pointers, this occurs because an object that has never been
-// referenced by a strong pointer is destroyed when the last weak pointer disappears.
+// Do not construct a strong pointer to "this" in an object's constructor.
+// The onFirstRef() callback would be made on an incompletely constructed
+// object.
+// Construction of a weak pointer to "this" in an object's constructor is also
+// discouraged. But the implementation was recently changed so that, in the
+// absence of extendObjectLifetime() calls, weak pointers no longer impact
+// object lifetime, and hence this no longer risks premature deallocation,
+// and hence usually works correctly.
// Such strong or weak pointers can be safely created in the RefBase onFirstRef()
// callback.
@@ -126,8 +124,23 @@
// is a longer-lived sp<>, why not use an sp<> directly?) A wp<> should only be
// dereferenced by using promote().
+// Any object inheriting from RefBase should always be destroyed as the result
+// of a reference count decrement, not via any other means. Such objects
+// should never be stack allocated, or appear directly as data members in other
+// objects. Objects inheriting from RefBase should have their strong reference
+// count incremented as soon as possible after construction. Usually this
+// will be done via construction of an sp<> to the object, but may instead
+// involve other means of calling RefBase::incStrong().
// Explicitly deleting or otherwise destroying a RefBase object with outstanding
-// wp<> or sp<> pointers to it will result in heap corruption.
+// wp<> or sp<> pointers to it will result in an abort or heap corruption.
+
+// It is particularly important not to mix sp<> and direct storage management
+// since the sp from raw pointer constructor is implicit. Thus if a RefBase-
+// -derived object of type T is managed without ever incrementing its strong
+// count, and accidentally passed to f(sp<T>), a strong pointer to the object
+// will be temporarily constructed and destroyed, prematurely deallocating the
+// object, and resulting in heap corruption. None of this would be easily
+// visible in the source.
// Extra Features:
@@ -144,7 +157,7 @@
// events, as well as some debugging facilities.
// Debugging support can be enabled by turning on DEBUG_REFS in RefBase.cpp.
-// Otherwise essentially no checking is provided.
+// Otherwise little checking is provided.
// Thread safety:
diff --git a/libutils/RefBase.cpp b/libutils/RefBase.cpp
index df49a2f..fee9984 100644
--- a/libutils/RefBase.cpp
+++ b/libutils/RefBase.cpp
@@ -84,15 +84,16 @@
//
// A weakref_impl is allocated as the value of mRefs in a RefBase object on
// construction.
-// In the OBJECT_LIFETIME_STRONG case, it is deallocated in the RefBase
-// destructor iff the strong reference count was never incremented. The
-// destructor can be invoked either from decStrong, or from decWeak if there
-// was never a strong reference. If the reference count had been incremented,
-// it is deallocated directly in decWeak, and hence still lives as long as
-// the last weak reference.
-// In the OBJECT_LIFETIME_WEAK case, it is always deallocated from the RefBase
-// destructor, which is always invoked by decWeak. DecStrong explicitly avoids
-// the deletion in this case.
+// In the OBJECT_LIFETIME_STRONG case, it is normally deallocated in decWeak,
+// and hence lives as long as the last weak reference. (It can also be
+// deallocated in the RefBase destructor iff the strong reference count was
+// never incremented and the weak count is zero, e.g. if the RefBase object is
+// explicitly destroyed without decrementing the strong count. This should be
+// avoided.) In this case, the RefBase destructor should be invoked from
+// decStrong.
+// In the OBJECT_LIFETIME_WEAK case, the weakref_impl is always deallocated in
+// the RefBase destructor, which is always invoked by decWeak. DecStrong
+// explicitly avoids the deletion in this case.
//
// Memory ordering:
// The client must ensure that every inc() call, together with all other
@@ -126,6 +127,19 @@
#define INITIAL_STRONG_VALUE (1<<28)
+#define MAX_COUNT 0xfffff
+
+// Test whether the argument is a clearly invalid strong reference count.
+// Used only for error checking on the value before an atomic decrement.
+// Intended to be very cheap.
+// Note that we cannot just check for excess decrements by comparing to zero
+// since the object would be deallocated before that.
+#define BAD_STRONG(c) \
+ ((c) == 0 || ((c) & (~(MAX_COUNT | INITIAL_STRONG_VALUE))) != 0)
+
+// Same for weak counts.
+#define BAD_WEAK(c) ((c) == 0 || ((c) & (~MAX_COUNT)) != 0)
+
// ---------------------------------------------------------------------------
class RefBase::weakref_impl : public RefBase::weakref_type
@@ -421,15 +435,15 @@
#if PRINT_REFS
ALOGD("decStrong of %p from %p: cnt=%d\n", this, id, c);
#endif
- ALOG_ASSERT(c >= 1, "decStrong() called on %p too many times", refs);
+ LOG_ALWAYS_FATAL_IF(BAD_STRONG(c), "decStrong() called on %p too many times",
+ refs);
if (c == 1) {
std::atomic_thread_fence(std::memory_order_acquire);
refs->mBase->onLastStrongRef(id);
int32_t flags = refs->mFlags.load(std::memory_order_relaxed);
if ((flags&OBJECT_LIFETIME_MASK) == OBJECT_LIFETIME_STRONG) {
delete this;
- // Since mStrong had been incremented, the destructor did not
- // delete refs.
+ // The destructor does not delete refs in this case.
}
}
// Note that even with only strong reference operations, the thread
@@ -492,7 +506,8 @@
weakref_impl* const impl = static_cast<weakref_impl*>(this);
impl->removeWeakRef(id);
const int32_t c = impl->mWeak.fetch_sub(1, std::memory_order_release);
- ALOG_ASSERT(c >= 1, "decWeak called on %p too many times", this);
+ LOG_ALWAYS_FATAL_IF(BAD_WEAK(c), "decWeak called on %p too many times",
+ this);
if (c != 1) return;
atomic_thread_fence(std::memory_order_acquire);
@@ -500,13 +515,19 @@
if ((flags&OBJECT_LIFETIME_MASK) == OBJECT_LIFETIME_STRONG) {
// This is the regular lifetime case. The object is destroyed
// when the last strong reference goes away. Since weakref_impl
- // outlive the object, it is not destroyed in the dtor, and
+ // outlives the object, it is not destroyed in the dtor, and
// we'll have to do it here.
if (impl->mStrong.load(std::memory_order_relaxed)
== INITIAL_STRONG_VALUE) {
- // Special case: we never had a strong reference, so we need to
- // destroy the object now.
- delete impl->mBase;
+ // Decrementing a weak count to zero when object never had a strong
+ // reference. We assume it acquired a weak reference early, e.g.
+ // in the constructor, and will eventually be properly destroyed,
+ // usually via incrementing and decrementing the strong count.
+ // Thus we no longer do anything here. We log this case, since it
+ // seems to be extremely rare, and should not normally occur. We
+ // used to deallocate mBase here, so this may now indicate a leak.
+ ALOGW("RefBase: Object at %p lost last weak reference "
+ "before it had a strong reference", impl->mBase);
} else {
// ALOGV("Freeing refs %p of old RefBase %p\n", this, impl->mBase);
delete impl;
@@ -675,25 +696,28 @@
RefBase::~RefBase()
{
- if (mRefs->mStrong.load(std::memory_order_relaxed)
+ int32_t flags = mRefs->mFlags.load(std::memory_order_relaxed);
+ // Life-time of this object is extended to WEAK, in
+ // which case weakref_impl doesn't out-live the object and we
+ // can free it now.
+ if ((flags & OBJECT_LIFETIME_MASK) == OBJECT_LIFETIME_WEAK) {
+ // It's possible that the weak count is not 0 if the object
+ // re-acquired a weak reference in its destructor
+ if (mRefs->mWeak.load(std::memory_order_relaxed) == 0) {
+ delete mRefs;
+ }
+ } else if (mRefs->mStrong.load(std::memory_order_relaxed)
== INITIAL_STRONG_VALUE) {
// We never acquired a strong reference on this object.
- // We assume there are no outstanding weak references.
+ LOG_ALWAYS_FATAL_IF(mRefs->mWeak.load() != 0,
+ "RefBase: Explicit destruction with non-zero weak "
+ "reference count");
+ // TODO: Always report if we get here. Currently MediaMetadataRetriever
+ // C++ objects are inconsistently managed and sometimes get here.
+ // There may be other cases, but we believe they should all be fixed.
delete mRefs;
- } else {
- // life-time of this object is extended to WEAK, in
- // which case weakref_impl doesn't out-live the object and we
- // can free it now.
- int32_t flags = mRefs->mFlags.load(std::memory_order_relaxed);
- if ((flags & OBJECT_LIFETIME_MASK) != OBJECT_LIFETIME_STRONG) {
- // It's possible that the weak count is not 0 if the object
- // re-acquired a weak reference in its destructor
- if (mRefs->mWeak.load(std::memory_order_relaxed) == 0) {
- delete mRefs;
- }
- }
}
- // for debugging purposes, clear this.
+ // For debugging purposes, clear mRefs. Ineffective against outstanding wp's.
const_cast<weakref_impl*&>(mRefs) = NULL;
}
diff --git a/libutils/tests/RefBase_test.cpp b/libutils/tests/RefBase_test.cpp
index 224c2ca..2e0cf6e 100644
--- a/libutils/tests/RefBase_test.cpp
+++ b/libutils/tests/RefBase_test.cpp
@@ -87,7 +87,7 @@
EXPECT_EQ(1, foo->getWeakRefs()->getWeakCount());
ASSERT_FALSE(isDeleted) << "deleted too early! still has a reference!";
wp1 = nullptr;
- ASSERT_TRUE(isDeleted) << "foo2 was leaked!";
+ ASSERT_FALSE(isDeleted) << "Deletion on wp destruction should no longer occur";
}
@@ -121,8 +121,33 @@
cpu_set_t otherCpus;
+// Divide the cpus we're allowed to run on into myCpus and otherCpus.
+// Set origCpus to the processors we were originally allowed to run on.
+// Return false if origCpus doesn't include at least processors 0 and 1.
+static bool setExclusiveCpus(cpu_set_t* origCpus /* out */,
+ cpu_set_t* myCpus /* out */, cpu_set_t* otherCpus) {
+ if (sched_getaffinity(0, sizeof(cpu_set_t), origCpus) != 0) {
+ return false;
+ }
+ if (!CPU_ISSET(0, origCpus) || !CPU_ISSET(1, origCpus)) {
+ return false;
+ }
+ CPU_ZERO(myCpus);
+ CPU_ZERO(otherCpus);
+ CPU_OR(myCpus, myCpus, origCpus);
+ CPU_OR(otherCpus, otherCpus, origCpus);
+ for (unsigned i = 0; i < CPU_SETSIZE; ++i) {
+ // I get the even cores, the other thread gets the odd ones.
+ if (i & 1) {
+ CPU_CLR(i, myCpus);
+ } else {
+ CPU_CLR(i, otherCpus);
+ }
+ }
+ return true;
+}
+
static void visit2AndRemove() {
- EXPECT_TRUE(CPU_ISSET(1, &otherCpus));
if (sched_setaffinity(0, sizeof(cpu_set_t), &otherCpus) != 0) {
FAIL() << "setaffinity returned:" << errno;
}
@@ -139,27 +164,10 @@
cpu_set_t myCpus;
// Restrict us and the helper thread to disjoint cpu sets.
// This prevents us from getting scheduled against each other,
- // which would be atrociously slow. We fail if that's impossible.
- if (sched_getaffinity(0, sizeof(cpu_set_t), &origCpus) != 0) {
- FAIL();
- }
- EXPECT_TRUE(CPU_ISSET(0, &origCpus));
- if (CPU_ISSET(1, &origCpus)) {
- CPU_ZERO(&myCpus);
- CPU_ZERO(&otherCpus);
- CPU_OR(&myCpus, &myCpus, &origCpus);
- CPU_OR(&otherCpus, &otherCpus, &origCpus);
- for (unsigned i = 0; i < CPU_SETSIZE; ++i) {
- // I get the even cores, the other thread gets the odd ones.
- if (i & 1) {
- CPU_CLR(i, &myCpus);
- } else {
- CPU_CLR(i, &otherCpus);
- }
- }
+ // which would be atrociously slow.
+ if (setExclusiveCpus(&origCpus, &myCpus, &otherCpus)) {
std::thread t(visit2AndRemove);
std::atomic<int> deleteCount(0);
- EXPECT_TRUE(CPU_ISSET(0, &myCpus));
if (sched_setaffinity(0, sizeof(cpu_set_t), &myCpus) != 0) {
FAIL() << "setaffinity returned:" << errno;
}
@@ -182,3 +190,69 @@
ASSERT_EQ(NITERS, deleteCount) << "Deletions missed!";
} // Otherwise this is slow and probably pointless on a uniprocessor.
}
+
+static wp<Bar> wpBuffer;
+static std::atomic<bool> wpBufferFull(false);
+
+// Wait until wpBufferFull has value val.
+static inline void wpWaitFor(bool val) {
+ while (wpBufferFull != val) {}
+}
+
+static void visit3AndRemove() {
+ if (sched_setaffinity(0, sizeof(cpu_set_t), &otherCpus) != 0) {
+ FAIL() << "setaffinity returned:" << errno;
+ }
+ for (int i = 0; i < NITERS; ++i) {
+ wpWaitFor(true);
+ {
+ sp<Bar> sp1 = wpBuffer.promote();
+ // We implicitly check that sp1 != NULL
+ sp1->mVisited2 = true;
+ }
+ wpBuffer = nullptr;
+ wpBufferFull = false;
+ }
+}
+
+TEST(RefBase, RacingPromotions) {
+ cpu_set_t origCpus;
+ cpu_set_t myCpus;
+ // Restrict us and the helper thread to disjoint cpu sets.
+ // This prevents us from getting scheduled against each other,
+ // which would be atrociously slow.
+ if (setExclusiveCpus(&origCpus, &myCpus, &otherCpus)) {
+ std::thread t(visit3AndRemove);
+ std::atomic<int> deleteCount(0);
+ if (sched_setaffinity(0, sizeof(cpu_set_t), &myCpus) != 0) {
+ FAIL() << "setaffinity returned:" << errno;
+ }
+ for (int i = 0; i < NITERS; ++i) {
+ Bar* bar = new Bar(&deleteCount);
+ wp<Bar> wp1(bar);
+ bar->mVisited1 = true;
+ if (i % (NITERS / 10) == 0) {
+ // Do this rarely, since it generates a log message.
+ wp1 = nullptr; // No longer destroys the object.
+ wp1 = bar;
+ }
+ wpBuffer = wp1;
+ ASSERT_EQ(bar->getWeakRefs()->getWeakCount(), 2);
+ wpBufferFull = true;
+ // Promotion races with that in visit3AndRemove.
+ // This may or may not succeed, but it shouldn't interfere with
+ // the concurrent one.
+ sp<Bar> sp1 = wp1.promote();
+ wpWaitFor(false); // Waits for other thread to drop strong pointer.
+ sp1 = nullptr;
+ // No strong pointers here.
+ sp1 = wp1.promote();
+ ASSERT_EQ(sp1.get(), nullptr) << "Dead wp promotion succeeded!";
+ }
+ t.join();
+ if (sched_setaffinity(0, sizeof(cpu_set_t), &origCpus) != 0) {
+ FAIL();
+ }
+ ASSERT_EQ(NITERS, deleteCount) << "Deletions missed!";
+ } // Otherwise this is slow and probably pointless on a uniprocessor.
+}
