Fix race bug in attemptIncStrong

The compensating onLastStrongRef call could be made even when there
was no onIncStrongAttempted call to compensate for.  This
happened in the OBJECT_LIFETIME_STRONG case when e.g. curCount
was initially zero, but was concurrently incremented by another
thread.

I believe the old code was also incorrect in the
curCount = INITIAL_STRONG_VALUE + 1 case,
which seems to be possible under unlikely conditions.
In that case, I believe the compensating call IS needed.
Thus the condition was also changed.

Bug: 30503444
Change-Id: I44bcbcbb1264e4b52b6d3750dc39b041c4140381

1 files changed, 9 insertions, 10 deletions

diff --git a/libutils/RefBase.cpp b/libutils/RefBase.cpp
index 085b314c1..d4d7d7e93 100644
--- a/libutils/RefBase.cpp
+++ b/libutils/RefBase.cpp
@@ -580,15 +580,14 @@ bool RefBase::weakref_type::attemptIncStrong(const void* id)
             // grab a strong-reference, which is always safe due to the
             // extended life-time.
             curCount = impl->mStrong.fetch_add(1, std::memory_order_relaxed);
-        }
-
-        // If the strong reference count has already been incremented by
-        // someone else, the implementor of onIncStrongAttempted() is holding
-        // an unneeded reference.  So call onLastStrongRef() here to remove it.
-        // (No, this is not pretty.)  Note that we MUST NOT do this if we
-        // are in fact acquiring the first reference.
-        if (curCount > 0 && curCount < INITIAL_STRONG_VALUE) {
-            impl->mBase->onLastStrongRef(id);
+            // If the strong reference count has already been incremented by
+            // someone else, the implementor of onIncStrongAttempted() is holding
+            // an unneeded reference.  So call onLastStrongRef() here to remove it.
+            // (No, this is not pretty.)  Note that we MUST NOT do this if we
+            // are in fact acquiring the first reference.
+            if (curCount != 0 && curCount != INITIAL_STRONG_VALUE) {
+                impl->mBase->onLastStrongRef(id);
+            }
         }
     }
     
@@ -598,7 +597,7 @@ bool RefBase::weakref_type::attemptIncStrong(const void* id)
     ALOGD("attemptIncStrong of %p from %p: cnt=%d\n", this, id, curCount);
 #endif
 
-    // curCount is the value of mStrong before we increment ed it.
+    // curCount is the value of mStrong before we incremented it.
     // 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().