Merge "Make RefBase more robust and debuggable" am: daac359be7

am: 7bf3d21fde

Change-Id: I643c4bd38062b80d382c50ed161f5440f35c1dd9

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
+// Do not construct a strong pointer to "this" in an object's constructor.
-// always wrong.  In the case of strong pointers. it is always wrong with RefBase
+// The onFirstRef() callback would be made on an incompletely constructed
-// because the onFirstRef() callback will be mode on an incompletely constructed
+// object.
-// object. In either case, it is wrong if such a pointer does not outlive the
+// Construction of a weak pointer to "this" in an object's constructor is also
-// constructor, since destruction of the smart pointer will attempt to destroy the
+// discouraged. But the implementation was recently changed so that, in the
-// object before construction is finished, normally resulting in a pointer to a
+// absence of extendObjectLifetime() calls, weak pointers no longer impact
-// destroyed object being returned from a new expression.
+// object lifetime, and hence this no longer risks premature deallocation,
-
+// and hence usually works correctly.
-// 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.
 
 // 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:
 

libutils/RefBase.cpp

@@ -84,15 +84,16 @@ namespace android {
 //
 // 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
+// In the OBJECT_LIFETIME_STRONG case, it is normally deallocated in decWeak,
-// destructor iff the strong reference count was never incremented. The
+// and hence lives as long as the last weak reference. (It can also be
-// destructor can be invoked either from decStrong, or from decWeak if there
+// deallocated in the RefBase destructor iff the strong reference count was
-// was never a strong reference. If the reference count had been incremented,
+// never incremented and the weak count is zero, e.g.  if the RefBase object is
-// it is deallocated directly in decWeak, and hence still lives as long as
+// explicitly destroyed without decrementing the strong count.  This should be
-// the last weak reference.
+// avoided.) In this case, the RefBase destructor should be invoked from
-// In the OBJECT_LIFETIME_WEAK case, it is always deallocated from the RefBase
+// decStrong.
-// destructor, which is always invoked by decWeak. DecStrong explicitly avoids
+// In the OBJECT_LIFETIME_WEAK case, the weakref_impl is always deallocated in
-// the deletion in this case.
+// 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 @@ namespace android {
 
 #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 @@ void RefBase::decStrong(const void* id) const
 #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
+            // The destructor does not delete refs in this case.
-            // delete refs.
         }
     }
     // Note that even with only strong reference operations, the thread
@@ -492,7 +506,8 @@ void RefBase::weakref_type::decWeak(const void* id)
     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 @@ void RefBase::weakref_type::decWeak(const void* id)
     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
+            // Decrementing a weak count to zero when object never had a strong
-            // destroy the object now.
+            // reference.  We assume it acquired a weak reference early, e.g.
-            delete impl->mBase;
+            // 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()
 
 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;
 }
 

libutils/tests/RefBase_test.cpp

@@ -87,7 +87,7 @@ TEST(RefBase, WeakCopies) {
     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 @@ static inline void waitFor(bool val) {
 
 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 @@ TEST(RefBase, RacingDestructors) {
     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.
+    // which would be atrociously slow.
-    if (sched_getaffinity(0, sizeof(cpu_set_t), &origCpus) != 0) {
+    if (setExclusiveCpus(&origCpus, &myCpus, &otherCpus)) {
-        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);
-            }
-        }
         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 @@ TEST(RefBase, RacingDestructors) {
         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.
+}