Make RefBase more robust and debuggable
This prevents two different kinds of client errors from causing
undetected memory corruption, and helps with the detection of others:
1. We no longer deallocate objects when the weak count goes to zero
and there have been no strong references. This otherwise causes
us to return a garbage object from a constructor if the constructor
allocates and deallocates a weak pointer to this. And we do know
that clients allocate such weak pointers in constructors and their
lifetime is hard to trace.
2. We abort if a RefBase object is explicitly destroyed while
the weak count is nonzero. Otherwise a subsequent decrement
would cause a write to potentially reallocated memory.
3. We check counter values returned by atomic decrements for
plausibility, and fail immediately if they are not plausible.
We unconditionally log any cases in which 1 changes behavior
from before. We abort in cases in which 2 changes behavior, since
those reflect clear bugs.
In case 1, a log message now indicates a possible leak. We have
not seen such a message in practice.
The third point introduces a small amount of overhead into the
reference count decrement path. But this should be negligible
compared to the actual decrement cost.
Add a test for promote/attemptIncStrong that tries to check for
both (1) above and concurrent operation of attemptIncStrong.
Add some additional warnings and explanations to the RefBase
documentation.
Bug: 30503444
Bug: 30292291
Bug: 30292538
Change-Id: Ida92b9a2e247f543a948a75d221fbc0038dea66c
diff --git a/include/utils/RefBase.h b/include/utils/RefBase.h
index a232a65..3c318c4 100644
--- a/include/utils/RefBase.h
+++ b/include/utils/RefBase.h
@@ -105,16 +105,14 @@
// Other more specific restrictions for wp<> and sp<>:
-// Constructing a strong or weak pointer to "this" in its constructors is almost
-// always wrong. In the case of strong pointers. it is always wrong with RefBase
-// because the onFirstRef() callback will be mode on an incompletely constructed
-// object. In either case, it is wrong if such a pointer does not outlive the
-// constructor, since destruction of the smart pointer will attempt to destroy the
-// object before construction is finished, normally resulting in a pointer to a
-// destroyed object being returned from a new expression.
-
-// In the case of weak pointers, this occurs because an object that has never been
-// referenced by a strong pointer is destroyed when the last weak pointer disappears.
+// Do not construct a strong pointer to "this" in an object's constructor.
+// The onFirstRef() callback would be made on an incompletely constructed
+// object.
+// Construction of a weak pointer to "this" in an object's constructor is also
+// discouraged. But the implementation was recently changed so that, in the
+// absence of extendObjectLifetime() calls, weak pointers no longer impact
+// object lifetime, and hence this no longer risks premature deallocation,
+// and hence usually works correctly.
// Such strong or weak pointers can be safely created in the RefBase onFirstRef()
// callback.
@@ -126,8 +124,23 @@
// is a longer-lived sp<>, why not use an sp<> directly?) A wp<> should only be
// dereferenced by using promote().
+// Any object inheriting from RefBase should always be destroyed as the result
+// of a reference count decrement, not via any other means. Such objects
+// should never be stack allocated, or appear directly as data members in other
+// objects. Objects inheriting from RefBase should have their strong reference
+// count incremented as soon as possible after construction. Usually this
+// will be done via construction of an sp<> to the object, but may instead
+// involve other means of calling RefBase::incStrong().
// Explicitly deleting or otherwise destroying a RefBase object with outstanding
-// wp<> or sp<> pointers to it will result in heap corruption.
+// wp<> or sp<> pointers to it will result in an abort or heap corruption.
+
+// It is particularly important not to mix sp<> and direct storage management
+// since the sp from raw pointer constructor is implicit. Thus if a RefBase-
+// -derived object of type T is managed without ever incrementing its strong
+// count, and accidentally passed to f(sp<T>), a strong pointer to the object
+// will be temporarily constructed and destroyed, prematurely deallocating the
+// object, and resulting in heap corruption. None of this would be easily
+// visible in the source.
// Extra Features:
@@ -144,7 +157,7 @@
// events, as well as some debugging facilities.
// Debugging support can be enabled by turning on DEBUG_REFS in RefBase.cpp.
-// Otherwise essentially no checking is provided.
+// Otherwise little checking is provided.
// Thread safety: