Use base::RefCountedThreadSafe on BindStateBase

base/callback_internal.h

Index: base/callback_internal.h
diff --git a/base/callback_internal.h b/base/callback_internal.h
index d6dcfeb3c02862cbc5b801047dfe2b6b0e553cf9..8ae606749e1e170c8a5bbd192f6ff3c28c8bdf6b 100644
--- a/base/callback_internal.h
+++ b/base/callback_internal.h
@@ -8,17 +8,29 @@
 #ifndef BASE_CALLBACK_INTERNAL_H_
 #define BASE_CALLBACK_INTERNAL_H_
 
-#include "base/atomic_ref_count.h"
 #include "base/base_export.h"
 #include "base/callback_forward.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 
 namespace base {
+
+struct FakeBindState;
+
 namespace internal {
+
 template <CopyMode copy_mode>
 class CallbackBase;
 
+class BindStateBase;
+
+template <typename Functor, typename... BoundArgs>
+struct BindState;
+
+struct BindStateBaseRefCountTraits {
+  static void Destruct(const 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,38 +42,41 @@ 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 {
+class BASE_EXPORT BindStateBase
+    : public RefCountedThreadSafe<BindStateBase, BindStateBaseRefCountTraits> {
  public:
   using InvokeFuncStorage = void(*)();
 
- protected:
+ private:
   BindStateBase(InvokeFuncStorage polymorphic_invoke,
                 void (*destructor)(const BindStateBase*));
   BindStateBase(InvokeFuncStorage polymorphic_invoke,
                 void (*destructor)(const BindStateBase*),
                 bool (*is_cancelled)(const BindStateBase*));
+
   ~BindStateBase() = default;
 
- private:
-  friend class scoped_refptr<BindStateBase>;
+  friend struct BindStateBaseRefCountTraits;
+  friend class RefCountedThreadSafe<BindStateBase, BindStateBaseRefCountTraits>;
+
   template <CopyMode copy_mode>
   friend class CallbackBase;
 
+  // Whitelist subclasses that access the destructor of BindStateBase.

[dcheng, 2017/03/22 00:26:17]

Hmm, wouldn't it be easier just to keep the destructor protected? What is the
disadvantage of doing that?

[tzik, 2017/03/22 01:50:05]

That hits our clang plugin check. It requires the protected destructor of
RefCounted type should be virtual.

[dcheng, 2017/03/27 07:15:26]

Hmm... so this seems pretty dangerous (though it's safe at the moment). Is there
any way to make this safer? Or note that subclasses must not add any additional
fields?

[tzik, 2017/03/29 08:29:47]

Do we have a pitfall where we may easily fall into?
It's broken if a subclass destroys BindStateBase through a BindStateBase pointer
rather than the pointer of the subclass, but I see no other error prone part.
Also, it looks safe to me to add a field to the subclass.

[dcheng, 2017/03/29 17:38:00]

Right, this is the thing I'm concerned about. I don't think it's likely to
happen, but it also seems like it's not quite what typical C++ best practices
ought to be.
Unless I'm missing something, it's always unsafe to add a field to a subclass:
CallbackBase holds on to the bind state as a BindStateBase.

[tzik, 2017/03/29 18:25:26]

CallbackBase holds it as a scoped_refptr of BindStateBase, but it is deleted as
the subclass rather than BindStateBase, since it has the custom deleter that
deletes BindStateBase after casting it back to the right subclass.

[dcheng, 2017/03/30 04:11:02]

Ah... right. This is a bit subtle, but I guess it's my fault for not reading the
comments more carefully =)

+  template <typename Functor, typename... BoundArgs>
+  friend struct BindState;
+  friend struct ::base::FakeBindState;
+
   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*);
@@ -86,7 +101,7 @@ class BASE_EXPORT CallbackBase<CopyMode::MoveOnly> {
   CallbackBase& operator=(CallbackBase<CopyMode::Copyable>&& c);
 
   // Returns true if Callback is null (doesn't refer to anything).
-  bool is_null() const { return bind_state_.get() == NULL; }
+  bool is_null() const { return !bind_state_; }
   explicit operator bool() const { return !is_null(); }
 
   // Returns true if the callback invocation will be nop due to an cancellation.