Start BindStateBase ref count from 1 instead of 0

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 503a86a45f02bd5d65c7ad0a2c6912a923eba0d7..ef06436e34e655aff86b01eabb0c06abc31e1881 100644
--- a/base/memory/ref_counted.h
+++ b/base/memory/ref_counted.h
@@ -19,16 +19,31 @@
 #include "base/threading/thread_collision_warner.h"
 #include "build/build_config.h"
 
+template <class T>
+class scoped_refptr;
+
 namespace base {
 
+template <typename T>
+scoped_refptr<T> AdoptRef(T* t);
+
 namespace subtle {
 
+enum AdoptRefTag { kAdoptRefTag };
+enum StartRefCountFromZeroTag { kStartRefCountFromZeroTag };
+enum StartRefCountFromOneTag { kStartRefCountFromOneTag };
+
 class BASE_EXPORT RefCountedBase {
  public:
   bool HasOneRef() const { return ref_count_ == 1; }
 
  protected:
-  RefCountedBase() {}
+  explicit RefCountedBase(StartRefCountFromZeroTag) {}
+  explicit RefCountedBase(StartRefCountFromOneTag) : ref_count_(1) {
+#if DCHECK_IS_ON()
+    needs_adopt_ref_ = true;
+#endif
+  }
 
   ~RefCountedBase() {
 #if DCHECK_IS_ON()
@@ -43,6 +58,10 @@ class BASE_EXPORT RefCountedBase {
     // DFAKE_SCOPED_LOCK_THREAD_LOCKED(add_release_);
 #if DCHECK_IS_ON()
     DCHECK(!in_dtor_);
+    DCHECK(!needs_adopt_ref_)
+        << "This RefCounted object is created with non-zero reference count."
+        << " The first reference to such a object has to be made by AdoptRef or"
+        << " MakeShared.";
 #endif
 
     ++ref_count_;
@@ -67,8 +86,20 @@ class BASE_EXPORT RefCountedBase {
   }
 
  private:
+  template <typename U>
+  friend scoped_refptr<U> base::AdoptRef(U*);
+
+  void Adopted() const {
+#if DCHECK_IS_ON()
+    DCHECK(needs_adopt_ref_);
+    needs_adopt_ref_ = false;
+#endif
+  }
+
   mutable size_t ref_count_ = 0;
+
 #if DCHECK_IS_ON()
+  mutable bool needs_adopt_ref_ = false;
   mutable bool in_dtor_ = false;
 #endif
 
@@ -82,7 +113,13 @@ class BASE_EXPORT RefCountedThreadSafeBase {
   bool HasOneRef() const;
 
  protected:
-  RefCountedThreadSafeBase();
+  explicit RefCountedThreadSafeBase(StartRefCountFromZeroTag) {}
+  explicit RefCountedThreadSafeBase(StartRefCountFromOneTag) : ref_count_(1) {
+#if DCHECK_IS_ON()
+    needs_adopt_ref_ = true;
+#endif
+  }
+
   ~RefCountedThreadSafeBase();
 
   void AddRef() const;
@@ -91,8 +128,19 @@ class BASE_EXPORT RefCountedThreadSafeBase {
   bool Release() const;
 
  private:
+  template <typename U>
+  friend scoped_refptr<U> base::AdoptRef(U*);
+
+  void Adopted() const {
+#if DCHECK_IS_ON()
+    DCHECK(needs_adopt_ref_);
+    needs_adopt_ref_ = false;
+#endif
+  }
+
   mutable AtomicRefCount ref_count_ = 0;
 #if DCHECK_IS_ON()
+  mutable bool needs_adopt_ref_ = false;
   mutable bool in_dtor_ = false;
 #endif
 
@@ -115,10 +163,37 @@ 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 reference count starts from zero by default, and we intended to migrate
+// to start-from-one ref count. Put MAKE_REF_COUNT_START_FROM_ONE to the
+// ref counted class to opt-in.
+//
+// If an object has start-from-one ref count, the first scoped_refptr need to be
+// created by base::AdoptRef() or base::MakeShared(). We can use
+// base::MakeShared() to create create both type of ref counted object.
+//
+// The motivations to use start-from-one ref count are:
+//  - Start-from-one ref count doesn't need the ref count increment for the
+//    first reference.
+//  - It can detect an invalid object acquisition for a being-deleted object
+//    that has zero ref count. That tends to happen on custom deleter that
+//    delays the deletion.
+//    TODO(tzik): Implement invalid acquisition detection.
+//  - Behavior parity to Blink's WTF::RefCounted, whose count starts from one.
+//    And start-from-one ref count is a step to merge WTF::RefCounted into
+//    base::RefCounted.
+//
+#define MAKE_REF_COUNT_START_FROM_ONE                      \

[dcheng, 2017/04/03 00:07:28]

Two nits
1) Make this a function-style macro, so that a typo fails to compile.
2) Maybe name it something like START_REFCOUNT_AT_ONE() or
REQUIRE_ADOPTION_FOR_REFCOUNTED_TYPE().

[tzik, 2017/04/03 03:48:23]

Done. Updated its name to REQUIRE_ADOPTION_FOR_REFCOUNTED_TYPE.

+  static constexpr ::base::subtle::StartRefCountFromOneTag \
+      kRefCountPreference = ::base::subtle::kStartRefCountFromOneTag
+
 template <class T>
 class RefCounted : public subtle::RefCountedBase {
  public:
-  RefCounted() = default;
+  static constexpr subtle::StartRefCountFromZeroTag kRefCountPreference =
+      subtle::kStartRefCountFromZeroTag;
+
+  RefCounted() : subtle::RefCountedBase(T::kRefCountPreference) {}
 
   void AddRef() const {
     subtle::RefCountedBase::AddRef();
@@ -134,7 +209,7 @@ class RefCounted : public subtle::RefCountedBase {
   ~RefCounted() = default;
 
  private:
-  DISALLOW_COPY_AND_ASSIGN(RefCounted<T>);
+  DISALLOW_COPY_AND_ASSIGN(RefCounted);
 };
 
 // Forward declaration.
@@ -165,10 +240,17 @@ struct DefaultRefCountedThreadSafeTraits {
 //    private:
 //     friend class base::RefCountedThreadSafe<MyFoo>;
 //     ~MyFoo();
+//
+// We can use MAKE_REF_COUNT_START_FROM_ONE with RefCountedThreadSafe too.
+// See the comment above the RefCounted definition for details.
 template <class T, typename Traits = DefaultRefCountedThreadSafeTraits<T> >
 class RefCountedThreadSafe : public subtle::RefCountedThreadSafeBase {
  public:
-  RefCountedThreadSafe() = default;
+  static constexpr subtle::StartRefCountFromZeroTag kRefCountPreference =
+      subtle::kStartRefCountFromZeroTag;
+
+  explicit RefCountedThreadSafe()
+      : subtle::RefCountedThreadSafeBase(T::kRefCountPreference) {}
 
   void AddRef() const {
     subtle::RefCountedThreadSafeBase::AddRef();
@@ -208,6 +290,43 @@ class RefCountedData
   ~RefCountedData() = default;
 };
 
+// Creates a scoped_refptr from a raw pointer without incrementing the reference
+// count. Use this only for a newly created object whose reference count starts
+// from 1 instead of 0.
+template <typename T>
+scoped_refptr<T> AdoptRef(T* obj) {
+  using Tag = typename std::decay<decltype(T::kRefCountPreference)>::type;
+  static_assert(std::is_same<subtle::StartRefCountFromOneTag, Tag>::value,
+                "Use AdoptRef only for the reference count starts from one.");
+
+  DCHECK(obj);
+  DCHECK(obj->HasOneRef());
+  obj->Adopted();
+  return scoped_refptr<T>(obj, subtle::kAdoptRefTag);
+}
+
+namespace subtle {
+
+template <typename T>
+scoped_refptr<T> AdoptRefIfNeeded(T* obj, StartRefCountFromZeroTag) {
+  return scoped_refptr<T>(obj);
+}
+
+template <typename T>
+scoped_refptr<T> AdoptRefIfNeeded(T* obj, StartRefCountFromOneTag) {
+  return AdoptRef(obj);
+}
+
+}  // namespace subtle
+
+// Constructs an instance of T, which is a ref counted type, and wraps the
+// object into a scoped_refptr.
+template <typename T, typename... Args>
+scoped_refptr<T> MakeShared(Args&&... args) {
+  T* obj = new T(std::forward<Args>(args)...);
+  return subtle::AdoptRefIfNeeded(obj, T::kRefCountPreference);
+}
+
 }  // namespace base
 
 //
@@ -375,6 +494,11 @@ class scoped_refptr {
   T* ptr_ = nullptr;
 
  private:
+  template <typename U>
+  friend scoped_refptr<U> base::AdoptRef(U*);
+
+  scoped_refptr(T* p, base::subtle::AdoptRefTag) : ptr_(p) {}
+
   // Friend required for move constructors that set r.ptr_ to null.
   template <typename U>
   friend class scoped_refptr;