fix a couple race-conditions in RefBase::promote()

Bug: 8390295
Change-Id: I7a48e3bf5b213cc1da2b8e844c6bb37ee24cb047

1 files changed, 67 insertions, 26 deletions

diff --git a/libs/utils/RefBase.cpp b/libs/utils/RefBase.cpp
index 3dac89e..0623f46 100644
--- a/libs/utils/RefBase.cpp
+++ b/libs/utils/RefBase.cpp
@@ -441,39 +441,68 @@ bool RefBase::weakref_type::attemptIncStrong(const void* id)
     incWeak(id);
     
     weakref_impl* const impl = static_cast<weakref_impl*>(this);
-    
     int32_t curCount = impl->mStrong;
-    ALOG_ASSERT(curCount >= 0, "attemptIncStrong called on %p after underflow",
-               this);
+
+    ALOG_ASSERT(curCount >= 0,
+            "attemptIncStrong called on %p after underflow", this);
+
     while (curCount > 0 && curCount != INITIAL_STRONG_VALUE) {
+        // we're in the easy/common case of promoting a weak-reference
+        // from an existing strong reference.
         if (android_atomic_cmpxchg(curCount, curCount+1, &impl->mStrong) == 0) {
             break;
         }
+        // the strong count has changed on us, we need to re-assert our
+        // situation.
         curCount = impl->mStrong;
     }
     
     if (curCount <= 0 || curCount == INITIAL_STRONG_VALUE) {
-        bool allow;
-        if (curCount == INITIAL_STRONG_VALUE) {
-            // Attempting to acquire first strong reference...  this is allowed
-            // if the object does NOT have a longer lifetime (meaning the
-            // implementation doesn't need to see this), or if the implementation
-            // allows it to happen.
-            allow = (impl->mFlags&OBJECT_LIFETIME_WEAK) != OBJECT_LIFETIME_WEAK
-                  || impl->mBase->onIncStrongAttempted(FIRST_INC_STRONG, id);
+        // we're now in the harder case of either:
+        // - there never was a strong reference on us
+        // - or, all strong references have been released
+        if ((impl->mFlags&OBJECT_LIFETIME_WEAK) == OBJECT_LIFETIME_STRONG) {
+            // this object has a "normal" life-time, i.e.: it gets destroyed
+            // when the last strong reference goes away
+            if (curCount <= 0) {
+                // the last strong-reference got released, the object cannot
+                // be revived.
+                decWeak(id);
+                return false;
+            }
+
+            // here, curCount == INITIAL_STRONG_VALUE, which means
+            // there never was a strong-reference, so we can try to
+            // promote this object; we need to do that atomically.
+            while (curCount > 0) {
+                if (android_atomic_cmpxchg(curCount, curCount + 1,
+                        &impl->mStrong) == 0) {
+                    break;
+                }
+                // the strong count has changed on us, we need to re-assert our
+                // situation (e.g.: another thread has inc/decStrong'ed us)
+                curCount = impl->mStrong;
+            }
+
+            if (curCount <= 0) {
+                // promote() failed, some other thread destroyed us in the
+                // meantime (i.e.: strong count reached zero).
+                decWeak(id);
+                return false;
+            }
         } else {
-            // Attempting to revive the object...  this is allowed
-            // if the object DOES have a longer lifetime (so we can safely
-            // call the object with only a weak ref) and the implementation
-            // allows it to happen.
-            allow = (impl->mFlags&OBJECT_LIFETIME_WEAK) == OBJECT_LIFETIME_WEAK
-                  && impl->mBase->onIncStrongAttempted(FIRST_INC_STRONG, id);
-        }
-        if (!allow) {
-            decWeak(id);
-            return false;
+            // this object has an "extended" life-time, i.e.: it can be
+            // revived from a weak-reference only.
+            // Ask the object's implementation if it agrees to be revived
+            if (!impl->mBase->onIncStrongAttempted(FIRST_INC_STRONG, id)) {
+                // it didn't so give-up.
+                decWeak(id);
+                return false;
+            }
+            // grab a strong-reference, which is always safe due to the
+            // extended life-time.
+            curCount = android_atomic_inc(&impl->mStrong);
         }
-        curCount = android_atomic_inc(&impl->mStrong);
 
         // If the strong reference count has already been incremented by
         // someone else, the implementor of onIncStrongAttempted() is holding
@@ -491,11 +520,23 @@ bool RefBase::weakref_type::attemptIncStrong(const void* id)
     ALOGD("attemptIncStrong of %p from %p: cnt=%d\n", this, id, curCount);
 #endif
 
-    if (curCount == INITIAL_STRONG_VALUE) {
-        android_atomic_add(-INITIAL_STRONG_VALUE, &impl->mStrong);
-        impl->mBase->onFirstRef();
+    // now we need to fix-up the count if it was INITIAL_STRONG_VALUE
+    // this must be done safely, i.e.: handle the case where several threads
+    // were here in attemptIncStrong().
+    curCount = impl->mStrong;
+    while (curCount >= INITIAL_STRONG_VALUE) {
+        ALOG_ASSERT(curCount > INITIAL_STRONG_VALUE,
+                "attemptIncStrong in %p underflowed to INITIAL_STRONG_VALUE",
+                this);
+        if (android_atomic_cmpxchg(curCount, curCount-INITIAL_STRONG_VALUE,
+                &impl->mStrong) == 0) {
+            break;
+        }
+        // the strong-count changed on us, we need to re-assert the situation,
+        // for e.g.: it's possible the fix-up happened in another thread.
+        curCount = impl->mStrong;
     }
-    
+
     return true;
 }