Make reference counted assignment check for races.

If SkRefCnt_SafeAssign is erroneously invoked simultaneously by two
threads on the same destination, a likely outcome is that the
reference count of an object other than the replaced dst will
be decremented. This results in an extra reference count decrement,
which usually means that the final reference count decrement will
be applied to a random location in an object that has already been
reallocated.  This is exceedingly hard to debug.

We add a hack to detect such data races with sufficiently high
probability, so that such a data race bug should sometimes actually
generate bug reports that lend themselves to diagnosis.

We detect changes within a relatively larger range that normally
includes several (typically slow) memory fences.  Not all such changes
would provoke a crash. Even if we decrement the wrong reference count,
there's a chance we would decrement a dead location. Thus, to
avoid potentially adding instability, we currently only log.

This change tries to minimize additional runtime overhead.
The macro is only expanded a few dozen times, so we do not worry too
much about code size.

Bug: b/31227650
Change-Id: Ia40c9ed2c4d0fa578ea682fbec4b71a2ef22a5d1
Reviewed-on: https://skia-review.googlesource.com/9994
Commit-Queue: Derek Sollenberger <djsollen@google.com>
Reviewed-by: Mike Klein <mtklein@chromium.org>

1 files changed, 24 insertions, 0 deletions

diff --git a/include/core/SkRefCnt.h b/include/core/SkRefCnt.h
index aca6c2b785..04670e8e12 100644
--- a/include/core/SkRefCnt.h
+++ b/include/core/SkRefCnt.h
@@ -137,6 +137,28 @@ class SK_API SkRefCnt : public SkRefCntBase {
     null in on each side of the assignment, and ensuring that ref() is called
     before unref(), in case the two pointers point to the same object.
  */
+
+#if defined(SK_BUILD_FOR_ANDROID_FRAMEWORK) || defined(SK_DEBUG)
+// This version heuristically detects data races, since those otherwise result
+// in redundant reference count decrements, which are exceedingly
+// difficult to debug.
+
+#define SkRefCnt_SafeAssign(dst, src)   \
+    do {                                \
+        typedef typename std::remove_reference<decltype(dst)>::type \
+                SkRefCntPtrT;  \
+        SkRefCntPtrT old_dst = *const_cast<SkRefCntPtrT volatile *>(&dst); \
+        if (src) src->ref();            \
+        if (old_dst) old_dst->unref();          \
+        if (old_dst != *const_cast<SkRefCntPtrT volatile *>(&dst)) { \
+            SkDebugf("Detected racing Skia calls at %s:%d\n", \
+                    __FILE__, __LINE__); \
+        } \
+        dst = src;                      \
+    } while (0)
+
+#else /* !(SK_BUILD_FOR_ANDROID_FRAMEWORK || SK_DEBUG) */
+
 #define SkRefCnt_SafeAssign(dst, src)   \
     do {                                \
         if (src) src->ref();            \
@@ -144,6 +166,8 @@ class SK_API SkRefCnt : public SkRefCntBase {
         dst = src;                      \
     } while (0)
 
+#endif
+
 
 /** Call obj->ref() and return obj. The obj must not be nullptr.
  */