Prepare to fail in RefBase destructor if count is untouched

Move towards crashing if a normally configured RefBase object is
destroyed without ever incrementing the reference count. We've been
threatening to do this for a long time. The previously last known
violation had been fixed.

This also fixes stack trace printing from RefBase, which had previously
been broken, and which we found necessary to track down further
violations of this rule.

Unfortunately, we found several more violations with the aid of
that fix. After existing CLs are submitted, there are
still some failures, but they are no longer numerous. Thus this CL
doesn't actually crash in the event of a violation, but does log a
verbose stack trace if it encounters one.

Bugs have been filed against the remaining known RefBase client offenders.
We plan to enable crashing on usage violations once those are fixed.

The fix for the stack trace printing breakage unfortunately requires
the use of weak symbols in order to avoid a circular build dependency.
We expect to eventually replace this with execinfo.h functionality.

Some random reformatting, driven by consistency with current formatting
requirements.

Add missing include to BacktraceMap.h.

Bug: 79112958
Bug: 30292291
Test: Boot AOSP, Master
Change-Id: I8151c54560c3b6f75ffc4c48229f0388a2066958

4 files changed, 121 insertions, 30 deletions

diff --git a/libbacktrace/include/backtrace/BacktraceMap.h b/libbacktrace/include/backtrace/BacktraceMap.h
index a9cfce4cb..c564271b6 100644
--- a/libbacktrace/include/backtrace/BacktraceMap.h
+++ b/libbacktrace/include/backtrace/BacktraceMap.h
@@ -31,6 +31,7 @@
 
 #include <deque>
 #include <iterator>
+#include <memory>
 #include <string>
 #include <vector>
 
diff --git a/libutils/CallStack.cpp b/libutils/CallStack.cpp
index bd6015e79..2a83bd9d1 100644
--- a/libutils/CallStack.cpp
+++ b/libutils/CallStack.cpp
@@ -16,16 +16,15 @@
 
 #define LOG_TAG "CallStack"
 
-#include <utils/CallStack.h>
-
-#include <memory>
-
 #include <utils/Printer.h>
 #include <utils/Errors.h>
 #include <utils/Log.h>
 
 #include <backtrace/Backtrace.h>
 
+#define CALLSTACK_WEAK  // Don't generate weak definitions.
+#include <utils/CallStack.h>
+
 namespace android {
 
 CallStack::CallStack() {
@@ -76,4 +75,26 @@ void CallStack::print(Printer& printer) const {
     }
 }
 
+// The following four functions may be used via weak symbol references from libutils.
+// Clients assume that if any of these symbols are available, then deleteStack() is.
+
+CallStack::CallStackUPtr CallStack::getCurrentInternal(int ignoreDepth) {
+    CallStack::CallStackUPtr stack(new CallStack());
+    stack->update(ignoreDepth + 1);
+    return stack;
+}
+
+void CallStack::logStackInternal(const char* logtag, const CallStack* stack,
+                                 android_LogPriority priority) {
+    stack->log(logtag, priority);
+}
+
+String8 CallStack::stackToStringInternal(const char* prefix, const CallStack* stack) {
+    return stack->toString(prefix);
+}
+
+void CallStack::deleteStack(CallStack* stack) {
+    delete stack;
+}
+
 }; // namespace android
diff --git a/libutils/RefBase.cpp b/libutils/RefBase.cpp
index 90748501d..3f1e79a04 100644
--- a/libutils/RefBase.cpp
+++ b/libutils/RefBase.cpp
@@ -17,30 +17,41 @@
 #define LOG_TAG "RefBase"
 // #define LOG_NDEBUG 0
 
+#include <memory>
+
 #include <utils/RefBase.h>
 
 #include <utils/CallStack.h>
 
+#include <utils/Mutex.h>
+
 #ifndef __unused
 #define __unused __attribute__((__unused__))
 #endif
 
-// compile with refcounting debugging enabled
-#define DEBUG_REFS                      0
+// Compile with refcounting debugging enabled.
+#define DEBUG_REFS 0
+
+// The following three are ignored unless DEBUG_REFS is set.
 
 // whether ref-tracking is enabled by default, if not, trackMe(true, false)
 // needs to be called explicitly
-#define DEBUG_REFS_ENABLED_BY_DEFAULT   0
+#define DEBUG_REFS_ENABLED_BY_DEFAULT 0
 
 // whether callstack are collected (significantly slows things down)
-#define DEBUG_REFS_CALLSTACK_ENABLED    1
+#define DEBUG_REFS_CALLSTACK_ENABLED 1
 
 // folder where stack traces are saved when DEBUG_REFS is enabled
 // this folder needs to exist and be writable
-#define DEBUG_REFS_CALLSTACK_PATH       "/data/debug"
+#define DEBUG_REFS_CALLSTACK_PATH "/data/debug"
 
 // log all reference counting operations
-#define PRINT_REFS                      0
+#define PRINT_REFS 0
+
+// Continue after logging a stack trace if ~RefBase discovers that reference
+// count has never been incremented. Normally we conspicuously crash in that
+// case.
+#define DEBUG_REFBASE_DESTRUCTION 1
 
 // ---------------------------------------------------------------------------
 
@@ -184,7 +195,7 @@ public:
                 char inc = refs->ref >= 0 ? '+' : '-';
                 ALOGD("\t%c ID %p (ref %d):", inc, refs->id, refs->ref);
 #if DEBUG_REFS_CALLSTACK_ENABLED
-                refs->stack.log(LOG_TAG);
+                CallStack::logStack(LOG_TAG, refs->stack.get());
 #endif
                 refs = refs->next;
             }
@@ -198,14 +209,14 @@ public:
                 char inc = refs->ref >= 0 ? '+' : '-';
                 ALOGD("\t%c ID %p (ref %d):", inc, refs->id, refs->ref);
 #if DEBUG_REFS_CALLSTACK_ENABLED
-                refs->stack.log(LOG_TAG);
+                CallStack::logStack(LOG_TAG, refs->stack.get());
 #endif
                 refs = refs->next;
             }
         }
         if (dumpStack) {
             ALOGE("above errors at:");
-            CallStack stack(LOG_TAG);
+            CallStack::logStack(LOG_TAG);
         }
     }
 
@@ -279,7 +290,7 @@ public:
                      this);
             int rc = open(name, O_RDWR | O_CREAT | O_APPEND, 644);
             if (rc >= 0) {
-                write(rc, text.string(), text.length());
+                (void)write(rc, text.string(), text.length());
                 close(rc);
                 ALOGD("STACK TRACE for %p saved in %s", this, name);
             }
@@ -294,7 +305,7 @@ private:
         ref_entry* next;
         const void* id;
 #if DEBUG_REFS_CALLSTACK_ENABLED
-        CallStack stack;
+        CallStack::CallStackUPtr stack;
 #endif
         int32_t ref;
     };
@@ -311,7 +322,7 @@ private:
             ref->ref = mRef;
             ref->id = id;
 #if DEBUG_REFS_CALLSTACK_ENABLED
-            ref->stack.update(2);
+            ref->stack = CallStack::getCurrent(2);
 #endif
             ref->next = *refs;
             *refs = ref;
@@ -346,7 +357,7 @@ private:
                 ref = ref->next;
             }
 
-            CallStack stack(LOG_TAG);
+            CallStack::logStack(LOG_TAG);
         }
     }
 
@@ -373,7 +384,7 @@ private:
                      inc, refs->id, refs->ref);
             out->append(buf);
 #if DEBUG_REFS_CALLSTACK_ENABLED
-            out->append(refs->stack.toString("\t\t"));
+            out->append(CallStack::stackToString("\t\t", refs->stack.get()));
 #else
             out->append("\t\t(call stacks disabled)");
 #endif
@@ -700,16 +711,16 @@ RefBase::~RefBase()
         if (mRefs->mWeak.load(std::memory_order_relaxed) == 0) {
             delete mRefs;
         }
-    } else if (mRefs->mStrong.load(std::memory_order_relaxed)
-            == INITIAL_STRONG_VALUE) {
+    } else if (mRefs->mStrong.load(std::memory_order_relaxed) == INITIAL_STRONG_VALUE) {
         // We never acquired a strong reference on this object.
-        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;
+#if DEBUG_REFBASE_DESTRUCTION
+        // Treating this as fatal is prone to causing boot loops. For debugging, it's
+        // better to treat as non-fatal.
+        ALOGD("RefBase: Explicit destruction, weak count = %d (in %p)", mRefs->mWeak.load(), this);
+        CallStack::logStack(LOG_TAG);
+#else
+        LOG_ALWAYS_FATAL("RefBase: Explicit destruction, weak count = %d", mRefs->mWeak.load());
+#endif
     }
     // For debugging purposes, clear mRefs.  Ineffective against outstanding wp's.
     const_cast<weakref_impl*&>(mRefs) = nullptr;
diff --git a/libutils/include/utils/CallStack.h b/libutils/include/utils/CallStack.h
index 0c1b87522..a7b1d1091 100644
--- a/libutils/include/utils/CallStack.h
+++ b/libutils/include/utils/CallStack.h
@@ -17,6 +17,8 @@
 #ifndef ANDROID_CALLSTACK_H
 #define ANDROID_CALLSTACK_H
 
+#include <memory>
+
 #include <android/log.h>
 #include <backtrace/backtrace_constants.h>
 #include <utils/String8.h>
@@ -25,6 +27,11 @@
 #include <stdint.h>
 #include <sys/types.h>
 
+#ifndef CALLSTACK_WEAK
+#define CALLSTACK_WEAK __attribute__((weak))
+#endif
+#define ALWAYS_INLINE __attribute__((always_inline))
+
 namespace android {
 
 class Printer;
@@ -36,7 +43,7 @@ public:
     CallStack();
     // Create a callstack with the current thread's stack trace.
     // Immediately dump it to logcat using the given logtag.
-    CallStack(const char* logtag, int32_t ignoreDepth=1);
+    CallStack(const char* logtag, int32_t ignoreDepth = 1);
     ~CallStack();
 
     // Reset the stack frames (same as creating an empty call stack).
@@ -44,7 +51,7 @@ public:
 
     // Immediately collect the stack traces for the specified thread.
     // The default is to dump the stack of the current call.
-    void update(int32_t ignoreDepth=1, pid_t tid=BACKTRACE_CURRENT_THREAD);
+    void update(int32_t ignoreDepth = 1, pid_t tid = BACKTRACE_CURRENT_THREAD);
 
     // Dump a stack trace to the log using the supplied logtag.
     void log(const char* logtag,
@@ -63,7 +70,58 @@ public:
     // Get the count of stack frames that are in this call stack.
     size_t size() const { return mFrameLines.size(); }
 
-private:
+    // DO NOT USE ANYTHING BELOW HERE. The following public members are expected
+    // to disappear again shortly, once a better replacement facility exists.
+    // The replacement facility will be incompatible!
+
+    // Debugging accesses to some basic functionality. These use weak symbols to
+    // avoid introducing a dependency on libutilscallstack. Such a dependency from
+    // libutils results in a cyclic build dependency. These routines can be called
+    // from within libutils. But if the actual library is unavailable, they do
+    // nothing.
+    //
+    // DO NOT USE THESE. They will disappear.
+    struct StackDeleter {
+        void operator()(CallStack* stack) { deleteStack(stack); }
+    };
+
+    typedef std::unique_ptr<CallStack, StackDeleter> CallStackUPtr;
+
+    // Return current call stack if possible, nullptr otherwise.
+    static CallStackUPtr ALWAYS_INLINE getCurrent(int32_t ignoreDepth = 1) {
+        if (reinterpret_cast<uintptr_t>(getCurrentInternal) == 0) {
+            ALOGW("CallStack::getCurrentInternal not linked, returning null");
+            return CallStackUPtr(nullptr);
+        } else {
+            return getCurrentInternal(ignoreDepth);
+        }
+    }
+    static void ALWAYS_INLINE logStack(const char* logtag, CallStack* stack = getCurrent().get(),
+                                       android_LogPriority priority = ANDROID_LOG_DEBUG) {
+        if (reinterpret_cast<uintptr_t>(logStackInternal) != 0 && stack != nullptr) {
+            logStackInternal(logtag, stack, priority);
+        } else {
+            ALOGW("CallStack::logStackInternal not linked");
+        }
+    }
+    static String8 ALWAYS_INLINE stackToString(const char* prefix = nullptr,
+                                               const CallStack* stack = getCurrent().get()) {
+        if (reinterpret_cast<uintptr_t>(stackToStringInternal) != 0 && stack != nullptr) {
+            return stackToStringInternal(prefix, stack);
+        } else {
+            return String8("<CallStack package not linked>");
+        }
+    }
+
+  private:
+    static CallStackUPtr CALLSTACK_WEAK getCurrentInternal(int32_t ignoreDepth);
+    static void CALLSTACK_WEAK logStackInternal(const char* logtag, const CallStack* stack,
+                                                android_LogPriority priority);
+    static String8 CALLSTACK_WEAK stackToStringInternal(const char* prefix, const CallStack* stack);
+    // The deleter is only invoked on non-null pointers. Hence it will never be
+    // invoked if CallStack is not linked.
+    static void CALLSTACK_WEAK deleteStack(CallStack* stack);
+
     Vector<String8> mFrameLines;
 };