Start BindStateBase ref count from 1 instead of 0

base/memory/ref_counted.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef BASE_MEMORY_REF_COUNTED_H_
 #define BASE_MEMORY_REF_COUNTED_H_
 
 #include <stddef.h>
 
 #include <cassert>
 #include <iosfwd>
 #include <type_traits>
 
 #include "base/atomic_ref_count.h"
 #include "base/base_export.h"
 #include "base/compiler_specific.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #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() {
 #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"
     // without release it.
     // 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_);
 
   DISALLOW_COPY_AND_ASSIGN(RefCountedBase);
 };
 
 class BASE_EXPORT RefCountedThreadSafeBase {
  public:
   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;
 
   // Returns true if the object should self-delete.
   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
 
   DISALLOW_COPY_AND_ASSIGN(RefCountedThreadSafeBase);
 };
 
 }  // namespace subtle
 
 //
 // 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
 // class from it like so:
 //
 //   class MyFoo : public base::RefCounted<MyFoo> {
 //    ...
 //    private:
 //     friend class base::RefCounted<MyFoo>;
 //     ~MyFoo();
 //   };
 //
 // You should always make your destructor non-public, to avoid any code deleting
 // the object accidently while there are references to it.
 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();
   }
 
   void Release() const {
     if (subtle::RefCountedBase::Release()) {
       delete static_cast<const T*>(this);
     }
   }
@@ skipping 29 matching lines @@
 //   };
 //
 // If you're using the default trait, then you should add compile time
 // asserts that no one else is deleting your object.  i.e.
 //    private:
 //     friend class base::RefCountedThreadSafe<MyFoo>;
 //     ~MyFoo();
 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();
   }
 
   void Release() const {
     if (subtle::RefCountedThreadSafeBase::Release()) {
       Traits::Destruct(static_cast<const T*>(this));
     }
   }
@@ skipping 19 matching lines @@
   RefCountedData() : data() {}
   RefCountedData(const T& in_value) : data(in_value) {}
 
   T data;
 
  private:
   friend class base::RefCountedThreadSafe<base::RefCountedData<T> >;
   ~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
 
 //
 // A smart pointer class for reference counted objects.  Use this class instead
 // of calling AddRef and Release manually on a reference counted object to
 // avoid common memory leaks caused by forgetting to Release an object
 // reference.  Sample usage:
 //
 //   class MyFoo : public RefCounted<MyFoo> {
 //    ...
@@ skipping 147 matching lines @@
 
   template <typename U>
   bool operator<(const scoped_refptr<U>& rhs) const {
     return ptr_ < rhs.get();
   }
 
  protected:
   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;
 
   // Non-inline helpers to allow:
   //     class Opaque;
   //     extern template class scoped_refptr<Opaque>;
   // Otherwise the compiler will complain that Opaque is an incomplete type.
   static void AddRef(T* ptr);
   static void Release(T* ptr);
@@ skipping 57 matching lines @@
 bool operator!=(std::nullptr_t null, const scoped_refptr<T>& rhs) {
   return !operator==(null, rhs);
 }
 
 template <typename T>
 std::ostream& operator<<(std::ostream& out, const scoped_refptr<T>& p) {
   return out << p.get();
 }
 
 #endif  // BASE_MEMORY_REF_COUNTED_H_