Start BindStateBase ref count from 1 instead of 0

The first atomic increments of ref counts are generally unneeded if the
ref count starts from 1 instead of 0, and we can detect an invalid AddRef
from 0 to 1 in that case.
Especially, the ref count in BindStateBase is incremented 300k times in
its constructor by the first page load from the browser boot, and also
it sometimes hits invalid state.
Note that 300k atomic increments are probably not so time consuming as
we can measure.

This CL adds `base::AdoptRef` to scoped_refptr for ref counted types whose
ref count start from 1, and converts BindStateBase to use it.

Review-Url: https://codereview.chromium.org/2723423002
Cr-Commit-Position: refs/heads/master@{#461372}

base/BUILD.gn

@@ -2408,6 +2408,7 @@ if (enable_nocompile_tests) {
       "bind_unittest.nc",
       "callback_list_unittest.nc",
       "callback_unittest.nc",
+      "memory/ref_counted_unittest.nc",
       "memory/weak_ptr_unittest.nc",
       "metrics/histogram_unittest.nc",
     ]

base/callback_internal.cc

@@ -74,9 +74,8 @@ bool CallbackBase<CopyMode::MoveOnly>::EqualsInternal(
   return bind_state_ == other.bind_state_;
 }
 
-CallbackBase<CopyMode::MoveOnly>::CallbackBase(
-    BindStateBase* bind_state)
-    : bind_state_(bind_state) {
+CallbackBase<CopyMode::MoveOnly>::CallbackBase(BindStateBase* bind_state)
+    : bind_state_(bind_state ? AdoptRef(bind_state) : nullptr) {
   DCHECK(!bind_state_.get() || bind_state_->HasOneRef());
 }
 

base/callback_internal.h

@@ -45,6 +45,8 @@ struct BindStateBaseRefCountTraits {
 class BASE_EXPORT BindStateBase
     : public RefCountedThreadSafe<BindStateBase, BindStateBaseRefCountTraits> {
  public:
+  REQUIRE_ADOPTION_FOR_REFCOUNTED_TYPE();
+
   using InvokeFuncStorage = void(*)();
 
  private:

base/memory/ref_counted.cc

@@ -21,8 +21,6 @@ bool RefCountedThreadSafeBase::HasOneRef() const {
   return AtomicRefCountIsOne(&ref_count_);
 }
 
-RefCountedThreadSafeBase::RefCountedThreadSafeBase() = default;
-
 RefCountedThreadSafeBase::~RefCountedThreadSafeBase() {
 #if DCHECK_IS_ON()
   DCHECK(in_dtor_) << "RefCountedThreadSafe object deleted without "
@@ -33,6 +31,10 @@ RefCountedThreadSafeBase::~RefCountedThreadSafeBase() {
 void RefCountedThreadSafeBase::AddRef() const {
 #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
   AtomicRefCountInc(&ref_count_);
 }

base/memory/ref_counted.h

@@ -20,17 +20,34 @@
 #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) {
+#if DCHECK_IS_ON()
+    sequence_checker_.DetachFromSequence();
+#endif
+  }
+
+  explicit RefCountedBase(StartRefCountFromOneTag) : ref_count_(1) {
 #if DCHECK_IS_ON()
+    needs_adopt_ref_ = true;
     sequence_checker_.DetachFromSequence();
 #endif
   }
@@ -48,6 +65,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.";
     if (ref_count_ >= 1) {
       DCHECK(CalledOnValidSequence());
     }
@@ -80,13 +101,24 @@ 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
+  }
+
 #if DCHECK_IS_ON()
   bool CalledOnValidSequence() const;
 #endif
 
   mutable size_t ref_count_ = 0;
 
 #if DCHECK_IS_ON()
+  mutable bool needs_adopt_ref_ = false;
   mutable bool in_dtor_ = false;
   mutable SequenceChecker sequence_checker_;
 #endif
@@ -101,7 +133,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;
@@ -110,8 +148,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
 
@@ -156,15 +205,44 @@ class BASE_EXPORT ScopedAllowCrossThreadRefCountAccess final {
 // 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.
+//
+//
+// The reference count starts from zero by default, and we intended to migrate
+// to start-from-one ref count. Put REQUIRE_ADOPTION_FOR_REFCOUNTED_TYPE() 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 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();
@@ -180,7 +258,7 @@ class RefCounted : public subtle::RefCountedBase {
   ~RefCounted() = default;
 
  private:
-  DISALLOW_COPY_AND_ASSIGN(RefCounted<T>);
+  DISALLOW_COPY_AND_ASSIGN(RefCounted);
 };
 
 // Forward declaration.
@@ -211,10 +289,17 @@ struct DefaultRefCountedThreadSafeTraits {
 //    private:
 //     friend class base::RefCountedThreadSafe<MyFoo>;
 //     ~MyFoo();
+//
+// We can use REQUIRE_ADOPTION_FOR_REFCOUNTED_TYPE() 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();
@@ -254,6 +339,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
 
 //
@@ -421,6 +543,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;

base/memory/ref_counted_delete_on_sequence.h

@@ -33,10 +33,14 @@ namespace base {
 template <class T>
 class RefCountedDeleteOnSequence : public subtle::RefCountedThreadSafeBase {
  public:
+  static constexpr subtle::StartRefCountFromZeroTag kRefCountPreference =
+      subtle::kStartRefCountFromZeroTag;
+
   // A SequencedTaskRunner for the current sequence can be acquired by calling
   // SequencedTaskRunnerHandle::Get().
   RefCountedDeleteOnSequence(scoped_refptr<SequencedTaskRunner> task_runner)
-      : task_runner_(std::move(task_runner)) {
+      : subtle::RefCountedThreadSafeBase(T::kRefCountPreference),
+        task_runner_(std::move(task_runner)) {
     DCHECK(task_runner_);
   }
 

base/memory/ref_counted_unittest.cc

@@ -6,6 +6,7 @@
 
 #include <utility>
 
+#include "base/test/gtest_util.h"
 #include "base/test/opaque_ref_counted.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -122,6 +123,16 @@ scoped_refptr<SelfAssign> Overloaded(scoped_refptr<SelfAssign> self_assign) {
   return self_assign;
 }
 
+class InitialRefCountIsOne : public base::RefCounted<InitialRefCountIsOne> {
+ public:
+  REQUIRE_ADOPTION_FOR_REFCOUNTED_TYPE();
+
+  InitialRefCountIsOne() {}
+
+ private:
+  friend class base::RefCounted<InitialRefCountIsOne>;
+  ~InitialRefCountIsOne() {}
+};
 
 }  // end namespace
 
@@ -528,3 +539,30 @@ TEST(RefCountedUnitTest, TestOverloadResolutionMove) {
   scoped_refptr<Other> other2(other);
   EXPECT_EQ(other2, Overloaded(std::move(other)));
 }
+
+TEST(RefCountedUnitTest, TestInitialRefCountIsOne) {
+  scoped_refptr<InitialRefCountIsOne> obj =
+      base::MakeShared<InitialRefCountIsOne>();
+  EXPECT_TRUE(obj->HasOneRef());
+  obj = nullptr;
+
+  scoped_refptr<InitialRefCountIsOne> obj2 =
+      base::AdoptRef(new InitialRefCountIsOne);
+  EXPECT_TRUE(obj2->HasOneRef());
+  obj2 = nullptr;
+
+  scoped_refptr<Other> obj3 = base::MakeShared<Other>();
+  EXPECT_TRUE(obj3->HasOneRef());
+  obj3 = nullptr;
+}
+
+TEST(RefCountedDeathTest, TestAdoptRef) {
+  EXPECT_DCHECK_DEATH(make_scoped_refptr(new InitialRefCountIsOne));
+
+  InitialRefCountIsOne* ptr = nullptr;
+  EXPECT_DCHECK_DEATH(base::AdoptRef(ptr));
+
+  scoped_refptr<InitialRefCountIsOne> obj =
+      base::MakeShared<InitialRefCountIsOne>();
+  EXPECT_DCHECK_DEATH(base::AdoptRef(obj.get()));
+}

base/memory/ref_counted_unittest.nc

@@ -0,0 +1,25 @@
+// Copyright 2017 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.
+
+#include "base/memory/ref_counted.h"
+
+namespace base {
+
+class InitialRefCountIsZero : public base::RefCounted<InitialRefCountIsZero> {
+ public:
+  InitialRefCountIsZero() {}
+ private:
+  friend class base::RefCounted<InitialRefCountIsZero>;
+  ~InitialRefCountIsZero() {}
+};
+
+#if defined(NCTEST_ADOPT_REF_TO_ZERO_START)  // [r"fatal error: static_assert failed \"Use AdoptRef only for the reference count starts from one\.\""]
+
+void WontCompile() {
+  AdoptRef(new InitialRefCountIsZero());
+}
+
+#endif
+
+}  // namespace base