fix a couple race-conditions in RefBase::promote()
Bug: 8390295
Change-Id: I7a48e3bf5b213cc1da2b8e844c6bb37ee24cb047
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 @@
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);
+ // 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);
}
- if (!allow) {
- decWeak(id);
- return false;
- }
- 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 @@
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;
}