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 37012de0a6bcd3a548db9c4f5c1fe19c26582fef..98003972f7026a8765f0fc20dc307142b3b0ab67 100644
--- a/base/memory/ref_counted.h
+++ b/base/memory/ref_counted.h
@@ -19,21 +19,29 @@
 #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 };
+
 class BASE_EXPORT RefCountedBase {
  public:
   bool HasOneRef() const { return ref_count_ == 1; }
 
  protected:
-  RefCountedBase()
-      : ref_count_(0)
+  explicit RefCountedBase(size_t initial_ref_count = 0)

[dcheng, 2017/03/30 04:26:15]

I think I would prefer that we have a default ctor which starts at 0 and then a
single-argument constructor which takes a special tag to tell the refcount to
start at 1.

(We DCHECK so this should be noticed fairly quickly, but I think it would be
better if subclasses couldn't specify the refcount directly)

+      : ref_count_(initial_ref_count) {
 #if DCHECK_IS_ON()
-        , in_dtor_(false)
+    DCHECK(initial_ref_count == 0 || initial_ref_count == 1);
+    needs_adopt_ref_ = ref_count_ == 1;
 #endif
-  {
   }
 
   ~RefCountedBase() {
@@ -42,7 +50,6 @@ class BASE_EXPORT RefCountedBase {
 #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 +57,49 @@ 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_;
   }
 
   // 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;
 #endif
-      return true;
-    }
-    return false;
+
+    return ref_count_ == 0;

[dcheng, 2017/03/30 04:26:15]

What is the purpose / necessity for these changes?

[tzik, 2017/03/30 13:16:27]

Oops, this is a contamination from another CL: http://crrev.com/2666423002.
Reverted from this CL.

   }
 
  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_;
+
 #if DCHECK_IS_ON()
-  mutable bool in_dtor_;
+  mutable bool needs_adopt_ref_;
+  mutable bool in_dtor_ = false;
 #endif
 
   DFAKE_MUTEX(add_release_);
@@ -88,7 +112,13 @@ class BASE_EXPORT RefCountedThreadSafeBase {
   bool HasOneRef() const;
 
  protected:
-  RefCountedThreadSafeBase();
+  explicit RefCountedThreadSafeBase(AtomicRefCount initial_ref_count = 0)
+      : ref_count_(initial_ref_count) {
+#if DCHECK_IS_ON()
+    needs_adopt_ref_ = initial_ref_count == 1;
+#endif
+  }
+
   ~RefCountedThreadSafeBase();
 
   void AddRef() const;
@@ -97,8 +127,19 @@ class BASE_EXPORT RefCountedThreadSafeBase {
   bool Release() const;
 
  private:
-  mutable AtomicRefCount ref_count_ = 0;
+  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_;
+#if DCHECK_IS_ON()
+  mutable bool needs_adopt_ref_;
   mutable bool in_dtor_ = false;
 #endif
 
@@ -124,7 +165,8 @@ class BASE_EXPORT RefCountedThreadSafeBase {
 template <class T>
 class RefCounted : public subtle::RefCountedBase {
  public:
-  RefCounted() = default;
+  explicit RefCounted(size_t initial_ref_count = 0)
+      : subtle::RefCountedBase(initial_ref_count) {}
 
   void AddRef() const {
     subtle::RefCountedBase::AddRef();
@@ -174,7 +216,8 @@ struct DefaultRefCountedThreadSafeTraits {
 template <class T, typename Traits = DefaultRefCountedThreadSafeTraits<T> >
 class RefCountedThreadSafe : public subtle::RefCountedThreadSafeBase {
  public:
-  RefCountedThreadSafe() = default;
+  explicit RefCountedThreadSafe(AtomicRefCount initial_ref_count = 0)
+      : subtle::RefCountedThreadSafeBase(initial_ref_count) {}
 
   void AddRef() const {
     subtle::RefCountedThreadSafeBase::AddRef();
@@ -214,6 +257,27 @@ 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* t) {
+  DCHECK(!t || t->HasOneRef());

[dcheng, 2017/03/30 04:26:16]

Hmm... why would we want to handle a null |t| here? In general, it seems like we
want this to always succeed.

[tzik, 2017/03/30 13:16:27]

OK, updated to force nun-null.

+  if (t)
+    t->Adopted();
+  return scoped_refptr<T>(t, subtle::kAdoptRefTag);
+}
+
+// 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)...);
+  if (obj->HasOneRef())
+    return AdoptRef(obj);
+  return scoped_refptr<T>(obj);

[dcheng, 2017/03/30 04:26:16]

Not sure if it's possible, but it would be nice if we didn't have to do runtime
selection. Is it possible to be clever here?

For example, maybe we could do something like what C++03 move support used to
do: it provided a macro that movable things would include in their class
declaration, and we would use the presence of a typedef on T to determine if it
should be treated as move-only or not. Perhaps we could do that here? Not sure
if we could make the constructors work with that too... but if that's possible,
that might be a nice safety improvement.

[tzik, 2017/03/30 13:16:27]

Trying that.

+}
+
 }  // namespace base
 
 //
@@ -381,6 +445,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;