From 483a2def8dd26d0145e752250f3ddb382ea1f6f3 Mon Sep 17 00:00:00 2001
From: Steven Moreland <smoreland@google.com>
Date: Tue, 26 Jul 2022 17:57:39 +0000
Subject: [PATCH] libutils: RefBase: extra check for double own

Bug: 232557259
Test: libutils_test
Change-Id: Ibd400750e7973600ec3fa493838a3b52cafe3add
---
 libutils/RefBase.cpp      | 28 +++++++++++++++++-----------
 libutils/RefBase_test.cpp | 10 ++++++++++
 2 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/libutils/RefBase.cpp b/libutils/RefBase.cpp
index 4ddac3d2e..40b6bf07e 100644
--- a/libutils/RefBase.cpp
+++ b/libutils/RefBase.cpp
@@ -744,21 +744,27 @@ 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) {
-        // We never acquired a strong reference on this object.
+    } else {
+        int32_t strongs = mRefs->mStrong.load(std::memory_order_relaxed);
 
-        // TODO: make this fatal, but too much code in Android manages RefBase with
-        // new/delete manually (or using other mechanisms such as std::make_unique).
-        // However, this is dangerous because it's also common for code to use the
-        // sp<T>(T*) constructor, assuming that if the object is around, it is already
-        // owned by an sp<>.
-        ALOGW("RefBase: Explicit destruction, weak count = %d (in %p). Use sp<> to manage this "
-              "object.",
-              mRefs->mWeak.load(), this);
+        if (strongs == INITIAL_STRONG_VALUE) {
+            // We never acquired a strong reference on this object.
+
+            // It would be nice to make this fatal, but many places use RefBase on the stack.
+            // However, this is dangerous because it's also common for code to use the
+            // sp<T>(T*) constructor, assuming that if the object is around, it is already
+            // owned by an sp<>.
+            ALOGW("RefBase: Explicit destruction, weak count = %d (in %p). Use sp<> to manage this "
+                  "object.",
+                  mRefs->mWeak.load(), this);
 
 #if CALLSTACK_ENABLED
-        CallStack::logStack(LOG_TAG);
+            CallStack::logStack(LOG_TAG);
 #endif
+        } else if (strongs != 0) {
+            LOG_ALWAYS_FATAL("RefBase: object %p with strong count %d deleted. Double owned?", this,
+                             strongs);
+        }
     }
     // For debugging purposes, clear mRefs.  Ineffective against outstanding wp's.
     const_cast<weakref_impl*&>(mRefs) = nullptr;
diff --git a/libutils/RefBase_test.cpp b/libutils/RefBase_test.cpp
index 93f9654b1..b89779de6 100644
--- a/libutils/RefBase_test.cpp
+++ b/libutils/RefBase_test.cpp
@@ -265,6 +265,16 @@ TEST(RefBase, AssertWeakRefExistsDeath) {
     delete foo;
 }
 
+TEST(RefBase, DoubleOwnershipDeath) {
+    bool isDeleted;
+    auto foo = sp<Foo>::make(&isDeleted);
+
+    // if something else thinks it owns foo, should die
+    EXPECT_DEATH(delete foo.get(), "");
+
+    EXPECT_FALSE(isDeleted);
+}
+
 // Set up a situation in which we race with visit2AndRremove() to delete
 // 2 strong references.  Bar destructor checks that there are no early
 // deletions and prior updates are visible to destructor.