Assert sequence validity on non-thread-safe RefCount manipulations (2)

base/memory/ref_counted.h

Index: base/memory/ref_counted.h
diff --git a/base/memory/ref_counted.h b/base/memory/ref_counted.h
index 37012de0a6bcd3a548db9c4f5c1fe19c26582fef..45b608c10c6c60187e4562af8699c8c33f8a5c44 100644
--- a/base/memory/ref_counted.h
+++ b/base/memory/ref_counted.h
@@ -16,33 +16,35 @@
 #include "base/compiler_specific.h"
 #include "base/logging.h"
 #include "base/macros.h"
+#include "base/sequence_checker.h"
 #include "base/threading/thread_collision_warner.h"
 #include "build/build_config.h"
 
 namespace base {
-
 namespace subtle {
 
 class BASE_EXPORT RefCountedBase {
  public:
   bool HasOneRef() const { return ref_count_ == 1; }
 
- protected:
-  RefCountedBase()
-      : ref_count_(0)
+  void DisableSequenceConsistencyAssertions() {
 #if DCHECK_IS_ON()
-        , in_dtor_(false)
+    sequence_consistency_assertion_enabled_ = false;
 #endif
-  {
   }
 
+ protected:
+  RefCountedBase() {
+#if DCHECK_IS_ON()
+    sequence_checker_.DetachFromSequence();
+#endif
+  }
   ~RefCountedBase() {
 #if DCHECK_IS_ON()
     DCHECK(in_dtor_) << "RefCounted object deleted without calling Release()";
 #endif
   }
 
-
   void AddRef() const {
     // TODO(maruel): Add back once it doesn't assert 500 times/sec.
     // Current thread books the critical section "AddRelease"
@@ -50,32 +52,50 @@ class BASE_EXPORT RefCountedBase {
     // DFAKE_SCOPED_LOCK_THREAD_LOCKED(add_release_);
 #if DCHECK_IS_ON()
     DCHECK(!in_dtor_);
+    if (ref_count_ >= 1) {
+      DCHECK(!sequence_consistency_assertion_enabled_ ||
+             CalledOnValidSequence());
+    }
 #endif
+
     ++ref_count_;
   }
 
   // Returns true if the object should self-delete.
   bool Release() const {
+    --ref_count_;
+
     // TODO(maruel): Add back once it doesn't assert 500 times/sec.
     // Current thread books the critical section "AddRelease"
     // without release it.
     // DFAKE_SCOPED_LOCK_THREAD_LOCKED(add_release_);
+
 #if DCHECK_IS_ON()
     DCHECK(!in_dtor_);
-#endif
-    if (--ref_count_ == 0) {
-#if DCHECK_IS_ON()
+    if (ref_count_ == 0)
       in_dtor_ = true;
+
+    if (ref_count_ >= 1)
+      DCHECK(!sequence_consistency_assertion_enabled_ ||
+             CalledOnValidSequence());
+    if (ref_count_ == 1)
+      sequence_checker_.DetachFromSequence();
 #endif
-      return true;
-    }
-    return false;
+
+    return ref_count_ == 0;
   }
 
  private:
-  mutable size_t ref_count_;
 #if DCHECK_IS_ON()
-  mutable bool in_dtor_;
+  bool CalledOnValidSequence() const;
+#endif
+
+  mutable size_t ref_count_ = 0;
+
+#if DCHECK_IS_ON()
+  mutable bool in_dtor_ = false;
+  mutable SequenceChecker sequence_checker_;
+  bool sequence_consistency_assertion_enabled_ = true;
 #endif
 
   DFAKE_MUTEX(add_release_);
@@ -107,6 +127,18 @@ class BASE_EXPORT RefCountedThreadSafeBase {
 
 }  // namespace subtle
 
+// See RefCounted class for documentation.
+class BASE_EXPORT ScopedAllowCrossThreadRefCountAccess final {

[gab, 2017/03/24 15:46:45]

// ScopedAllowCrossThreadRefCountAccess disables the check documented on
RefCounted below for rare pre-existing use cases where thread-safety was
guaranteed through other means (e.g. explicit sequencing of calls across
execution sequences when bouncing between threads in order). New callers should
refrain from using this (callsites handling thread-safety through locks should
use RefCountedThreadSafe per the overhead of its atomics being negligible
compared to locks anyways and callsites doing explicit sequencing should
properly std::move() the ref to avoid hitting this check).
// TODO(tzik): Cleanup existing use cases and remove
ScopedAllowCrossThreadRefCountAccess.

[tzik, 2017/03/28 07:11:28]

Done.

+ public:
+#if DCHECK_IS_ON()
+  ScopedAllowCrossThreadRefCountAccess();
+  ~ScopedAllowCrossThreadRefCountAccess();
+#else
+  ScopedAllowCrossThreadRefCountAccess() {}
+  ~ScopedAllowCrossThreadRefCountAccess() {}
+#endif
+};
+
 //
 // A base class for reference counted classes.  Otherwise, known as a cheap
 // knock-off of WebKit's RefCounted<T> class.  To use this, just extend your
@@ -121,6 +153,16 @@ class BASE_EXPORT RefCountedThreadSafeBase {
 //
 // You should always make your destructor non-public, to avoid any code deleting
 // the object accidently while there are references to it.
+//
+// The ref count manipulation to RefCounted is NOT thread safe and has DCHECKs
+// to trap unsafe cross thread usage. A subclass instance of RefCounted can be
+// passed to another execution sequence only when its ref count is 1. If the ref
+// count is more than 1, the RefCounted class verifies the ref updates are made
+// on the same execution sequence as the previous ones.
+// In rare cases where thread-safety is guaranteed through other means (e.g.
+// locks or explicit sequencing of calls across execution sequences):
+// ScopedAllowCrossThreadRefCountAccess can be used to explicitly indicate this
+// and disable the aforementioned check.

[gab, 2017/03/24 15:46:45]

Remove last sentence so it's not part of main API (no new users of
ScopedAllowCrossThreadRefCountAccess should be added) and move documentation
above with comment as suggested.

[tzik, 2017/03/28 07:11:28]

Done.

 template <class T>
 class RefCounted : public subtle::RefCountedBase {
  public: