Make BindStateBase RefCountedThreadSafe

base/callback_internal.h

Index: base/callback_internal.h
diff --git a/base/callback_internal.h b/base/callback_internal.h
index f7501f96c5d737f0a0be70590743c45e6b05329a..44e0952d1c5098d05141a319aeadc0ff50d936d7 100644
--- a/base/callback_internal.h
+++ b/base/callback_internal.h
@@ -15,10 +15,19 @@
 #include "base/memory/ref_counted.h"
 
 namespace base {
+
+struct FakeBindState1;
+struct FakeBindState2;
+
 namespace internal {
 template <CopyMode copy_mode>
 class CallbackBase;
 
+template <typename Functor, typename... Args>
+struct BindState;
+
+class BindStateBase;
+
 // BindStateBase is used to provide an opaque handle that the Callback
 // class can use to represent a function object with bound arguments.  It
 // behaves as an existential type that is used by a corresponding
@@ -30,7 +39,13 @@ class CallbackBase;
 // Creating a vtable for every BindState template instantiation results in a lot
 // of bloat. Its only task is to call the destructor which can be done with a
 // function pointer.
-class BASE_EXPORT BindStateBase {
+
+struct BASE_EXPORT BindStateBaseRefCountTraits {
+  static void Destruct(const BindStateBase* obj);

[dcheng, 2016/09/21 22:07:50]

Does it make sense to inline the definition in the header? I'm wondering if it
will have any binary size impact.

+};
+
+class BASE_EXPORT BindStateBase
+    : public RefCountedThreadSafe<BindStateBase, BindStateBaseRefCountTraits> {
  public:
   using InvokeFuncStorage = void(*)();
 
@@ -40,28 +55,32 @@ class BASE_EXPORT BindStateBase {
   BindStateBase(InvokeFuncStorage polymorphic_invoke,
                 void (*destructor)(const BindStateBase*),
                 bool (*is_cancelled)(const BindStateBase*));
-  ~BindStateBase() = default;
 
  private:
   friend class scoped_refptr<BindStateBase>;
   template <CopyMode copy_mode>
   friend class CallbackBase;
 
+  // Add known callers of the destructor as friend classes. Note that making the
+  // destructor protected hits a chromium style check done by the clang plugin.
+  friend struct BindStateBaseRefCountTraits;
+  template <typename Functor, typename... BoundArgs>
+  friend struct BindState;

[dcheng, 2016/09/21 22:07:50]

It feels a bit funny that we have to friend both the traits object and our
derived class.

+  friend struct base::FakeBindState1;
+  friend struct base::FakeBindState2;

[dcheng, 2016/09/21 22:07:50]

I'm a bit sad about having to friend these test only structs. I wonder... can
the tests just store this thing in a scoped_refptr?

+
+  ~BindStateBase() = default;
+
   bool IsCancelled() const {
     return is_cancelled_(this);
   }
 
-  void AddRef() const;
-  void Release() const;
-
   // In C++, it is safe to cast function pointers to function pointers of
   // another type. It is not okay to use void*. We create a InvokeFuncStorage
   // that that can store our function pointer, and then cast it back to
   // the original type on usage.
   InvokeFuncStorage polymorphic_invoke_;
 
-  mutable AtomicRefCount ref_count_;
-
   // Pointer to a function that will properly destroy |this|.
   void (*destructor_)(const BindStateBase*);
   bool (*is_cancelled_)(const BindStateBase*);