Revert "Prepare to fail in RefBase destructor if count is untouched"

This reverts commit 9d3146af22588e0c23e110be13a515f5347bf687.

Reason for revert: It appears that weak symbols don't work as expected on MacOS, breaking the MacOS aapt build.

Change-Id: Ica0955106485a7bf2e2c3f09ff7910e230eb4139

4 files changed, 30 insertions, 121 deletions

diff --git a/libbacktrace/include/backtrace/BacktraceMap.h b/libbacktrace/include/backtrace/BacktraceMap.h
index c564271b6..a9cfce4cb 100644
--- a/libbacktrace/include/backtrace/BacktraceMap.h
+++ b/libbacktrace/include/backtrace/BacktraceMap.h
@@ -31,7 +31,6 @@
 
 #include <deque>
 #include <iterator>
-#include <memory>
 #include <string>
 #include <vector>
 
diff --git a/libutils/CallStack.cpp b/libutils/CallStack.cpp
index 2a83bd9d1..bd6015e79 100644
--- a/libutils/CallStack.cpp
+++ b/libutils/CallStack.cpp
@@ -16,15 +16,16 @@
 
 #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() {
@@ -75,26 +76,4 @@ 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 3f1e79a04..90748501d 100644
--- a/libutils/RefBase.cpp
+++ b/libutils/RefBase.cpp
@@ -17,41 +17,30 @@
 #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
-
-// The following three are ignored unless DEBUG_REFS is set.
+// compile with refcounting debugging enabled
+#define DEBUG_REFS                      0
 
 // 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
-
-// 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
+#define PRINT_REFS                      0
 
 // ---------------------------------------------------------------------------
 
@@ -195,7 +184,7 @@ public:
                 char inc = refs->ref >= 0 ? '+' : '-';
                 ALOGD("\t%c ID %p (ref %d):", inc, refs->id, refs->ref);
 #if DEBUG_REFS_CALLSTACK_ENABLED
-                CallStack::logStack(LOG_TAG, refs->stack.get());
+                refs->stack.log(LOG_TAG);
 #endif
                 refs = refs->next;
             }
@@ -209,14 +198,14 @@ public:
                 char inc = refs->ref >= 0 ? '+' : '-';
                 ALOGD("\t%c ID %p (ref %d):", inc, refs->id, refs->ref);
 #if DEBUG_REFS_CALLSTACK_ENABLED
-                CallStack::logStack(LOG_TAG, refs->stack.get());
+                refs->stack.log(LOG_TAG);
 #endif
                 refs = refs->next;
             }
         }
         if (dumpStack) {
             ALOGE("above errors at:");
-            CallStack::logStack(LOG_TAG);
+            CallStack stack(LOG_TAG);
         }
     }
 
@@ -290,7 +279,7 @@ public:
                      this);
             int rc = open(name, O_RDWR | O_CREAT | O_APPEND, 644);
             if (rc >= 0) {
-                (void)write(rc, text.string(), text.length());
+                write(rc, text.string(), text.length());
                 close(rc);
                 ALOGD("STACK TRACE for %p saved in %s", this, name);
             }
@@ -305,7 +294,7 @@ private:
         ref_entry* next;
         const void* id;
 #if DEBUG_REFS_CALLSTACK_ENABLED
-        CallStack::CallStackUPtr stack;
+        CallStack stack;
 #endif
         int32_t ref;
     };
@@ -322,7 +311,7 @@ private:
             ref->ref = mRef;
             ref->id = id;
 #if DEBUG_REFS_CALLSTACK_ENABLED
-            ref->stack = CallStack::getCurrent(2);
+            ref->stack.update(2);
 #endif
             ref->next = *refs;
             *refs = ref;
@@ -357,7 +346,7 @@ private:
                 ref = ref->next;
             }
 
-            CallStack::logStack(LOG_TAG);
+            CallStack stack(LOG_TAG);
         }
     }
 
@@ -384,7 +373,7 @@ private:
                      inc, refs->id, refs->ref);
             out->append(buf);
 #if DEBUG_REFS_CALLSTACK_ENABLED
-            out->append(CallStack::stackToString("\t\t", refs->stack.get()));
+            out->append(refs->stack.toString("\t\t"));
 #else
             out->append("\t\t(call stacks disabled)");
 #endif
@@ -711,16 +700,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.
-#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
+        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;
     }
     // 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 a7b1d1091..0c1b87522 100644
--- a/libutils/include/utils/CallStack.h
+++ b/libutils/include/utils/CallStack.h
@@ -17,8 +17,6 @@
 #ifndef ANDROID_CALLSTACK_H
 #define ANDROID_CALLSTACK_H
 
-#include <memory>
-
 #include <android/log.h>
 #include <backtrace/backtrace_constants.h>
 #include <utils/String8.h>
@@ -27,11 +25,6 @@
 #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;
@@ -43,7 +36,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).
@@ -51,7 +44,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,
@@ -70,58 +63,7 @@ public:
     // Get the count of stack frames that are in this call stack.
     size_t size() const { return mFrameLines.size(); }
 
-    // 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);
-
+private:
     Vector<String8> mFrameLines;
 };