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 784a1788a81a07808be26e82c830eeea59acc1b8..2c9e27720f844812eea4e8bd768b90c8c6815bc9 100644
--- a/base/memory/ref_counted.h
+++ b/base/memory/ref_counted.h
@@ -16,11 +16,11 @@
 #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 {
@@ -28,30 +28,21 @@ class BASE_EXPORT RefCountedBase {
   bool HasOneRef() const { return ref_count_ == 1; }
 
  protected:
-  RefCountedBase()
-      : ref_count_(0)
-#if DCHECK_IS_ON()
-        , in_dtor_(false)
-#endif
-  {
-  }
-
-  ~RefCountedBase() {
-#if DCHECK_IS_ON()
-    DCHECK(in_dtor_) << "RefCounted object deleted without calling Release()";
-#endif
-  }
-
+  RefCountedBase();
+  ~RefCountedBase();
 
   void AddRef() 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_);
+    if (ref_count_ >= 2)
+      DCHECK(CalledOnValidSequence());

[gab, 2017/02/16 21:38:47]

I found this whole comparing with "2" confusing (thought it should be 1) until I
realized you're comparing *after add* and *before release*.

I'd prefer this to be the other way around, check w.r.t to 1 and Add() after /
Release() before the check.

[tzik, 2017/02/17 11:36:08]

Done.

 #endif
-    ++ref_count_;
   }
 
   // Returns true if the object should self-delete.
@@ -62,6 +53,10 @@ class BASE_EXPORT RefCountedBase {
     // DFAKE_SCOPED_LOCK_THREAD_LOCKED(add_release_);
 #if DCHECK_IS_ON()
     DCHECK(!in_dtor_);
+    if (ref_count_ >= 2)
+      DCHECK(CalledOnValidSequence());
+    if (ref_count_ == 2)
+      DetachFromSequence();
 #endif
     if (--ref_count_ == 0) {
 #if DCHECK_IS_ON()
@@ -73,9 +68,13 @@ class BASE_EXPORT RefCountedBase {
   }
 
  private:
+  void DetachFromSequence() const;

[gab, 2017/02/16 21:38:46]

Invoked sequence_checker_.DetachFromSequence() directly and remove the
DetachFromSequence() private method.

[tzik, 2017/02/17 11:36:08]

Done.

+  bool CalledOnValidSequence() const;

[gab, 2017/02/16 21:38:47]

Wrap method itself in DCHECK_IS_ON() since it's already never called outside
those blocks.

[tzik, 2017/02/17 11:36:08]

Done.

+
   mutable size_t ref_count_;

[gab, 2017/02/16 21:38:47]

Tangential cleanup while you're here

= 0;

here and drop initialization list altogether in constructor :)

[tzik, 2017/02/17 11:36:09]

Done.

 #if DCHECK_IS_ON()
-  mutable bool in_dtor_;
+  mutable bool in_dtor_ = false;
+  mutable SequenceChecker sequence_checker_;
 #endif
 
   DFAKE_MUTEX(add_release_);
@@ -107,6 +106,17 @@ class BASE_EXPORT RefCountedThreadSafeBase {
 
 }  // namespace subtle
 
+class BASE_EXPORT ScopedAllowCrossThreadRefCountAccess final {

[gab, 2017/02/16 21:38:46]

// See RefCounted class for documentation.

[tzik, 2017/02/17 11:36:08]

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 +131,14 @@ 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

[gab, 2017/02/16 21:38:47]

rm comma

[tzik, 2017/02/17 11:36:08]

Done.

+// to trap unsafe cross thread usages. A subclass instance of RefCounted can be

[gab, 2017/02/16 21:38:47]

s/usages/usage/

[tzik, 2017/02/17 11:36:08]

Done.

+// passed to the other thread or sequence only when its ref count is 1. If the
+// ref count is more than 1, the ref count checks if it's on the same thread or

[gab, 2017/02/16 21:38:47]

s/checks if it's on the same thread or sequence to the previous ones/verifies
the ref updates are made on the same execution sequence as the previous ones/

[tzik, 2017/02/17 11:36:09]

Done.

+// sequence to the previous ones.

[gab, 2017/02/16 21:38:47]

s/thread or sequence/execution sequence/

[tzik, 2017/02/17 11:36:08]

Done.

+// If you want to temporarily opt-out from the check, use
+// ScopedAllowCrossThreadRefCountAccess to disable the DCHECK.

[gab, 2017/02/16 21:38:47]

For this last paragraph: 

// 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.

[tzik, 2017/02/17 11:36:09]

Done.

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