Add support for threadsafe completion callback factory.

ppapi/utility/completion_callback_factory.h

Index: ppapi/utility/completion_callback_factory.h
diff --git a/ppapi/utility/completion_callback_factory.h b/ppapi/utility/completion_callback_factory.h
index d521f4b5d92bb38b45b78ce3684217d68f3cab07..68289338de9119fa4fffa0a8ad53f280812ea847 100644
--- a/ppapi/utility/completion_callback_factory.h
+++ b/ppapi/utility/completion_callback_factory.h
@@ -6,7 +6,7 @@
 #define PPAPI_UTILITY_COMPLETION_CALLBACK_FACTORY_H_
 
 #include "ppapi/cpp/completion_callback.h"
-#include "ppapi/utility/non_thread_safe_ref_count.h"
+#include "ppapi/utility/completion_callback_factory_thread_traits.h"
 
 /// @file
 /// This file defines the API to create CompletionCallback objects that are
@@ -43,8 +43,8 @@ template <typename T> struct TypeUnwrapper<const T&> {
 /// factory.
 ///
 /// <strong>Note: </strong><code>CompletionCallbackFactory<T></code> isn't
-/// thread safe, but you can make it more thread-friendly by passing a
-/// thread-safe refcounting class as the second template element. However, it
+/// thread safe, but it is somewhat thread-friendly when used with a
+/// thread-safe traits class as the second template element. However, it
 /// only guarantees safety for creating a callback from another thread, the
 /// callback itself needs to execute on the same thread as the thread that
 /// creates/destroys the factory. With this restriction, it is safe to create
@@ -185,7 +185,7 @@ template <typename T> struct TypeUnwrapper<const T&> {
 /// is to accept your output argument as a non-const reference and to swap()
 /// the argument with a vector of your own to store it. This means you don't
 /// have to copy the buffer to consume it.
-template <typename T, typename RefCount = NonThreadSafeRefCount>
+template <typename T, typename ThreadTraits = ThreadSafeThreadTraits>
 class CompletionCallbackFactory {
  public:
 
@@ -200,20 +200,27 @@ class CompletionCallbackFactory {
   /// parameter is <code>NULL</code>.
   explicit CompletionCallbackFactory(T* object = NULL)
       : object_(object) {
+    // Assume that we don't need to lock since construction should be complete
+    // before the pointer is used on another thread.
     InitBackPointer();
   }
 
   /// Destructor.
   ~CompletionCallbackFactory() {
+    // Assume that we don't need to lock since this object should not be used
+    // from multiple threads during destruction.
     ResetBackPointer();
   }
 
   /// CancelAll() cancels all <code>CompletionCallbacks</code> allocated from
   /// this factory.
   void CancelAll() {
+    ThreadTraits::AutoLock lock(lock_);
+
     ResetBackPointer();
     InitBackPointer();
   }
+
   /// Initialize() binds the <code>CallbackFactory</code> to a particular
   /// object. Use this when the object is not available at
   /// <code>CallbackFactory</code> creation, and the <code>NULL</code> default
@@ -522,7 +529,7 @@ class CompletionCallbackFactory {
  private:
   class BackPointer {
    public:
-    typedef CompletionCallbackFactory<T, RefCount> FactoryType;
+    typedef CompletionCallbackFactory<T, ThreadTraits> FactoryType;
 
     explicit BackPointer(FactoryType* factory)
         : factory_(factory) {
@@ -546,7 +553,7 @@ class CompletionCallbackFactory {
     }
 
    private:
-    RefCount ref_;
+    typename ThreadTraits::RefCount ref_;
     FactoryType* factory_;
   };
 
@@ -817,19 +824,26 @@ class CompletionCallbackFactory {
     typename Traits::StorageType output_;
   };
 
+  // Creates the back pointer object and takes a reference to it. This assumes
+  // either that the lock is held or that it is not needed.
   void InitBackPointer() {
     back_pointer_ = new BackPointer(this);
     back_pointer_->AddRef();
   }
 
+  // Releases our reference to the back pointer object and clears the pointer.
+  // This assumes either that the lock is held or that it is not needed.
   void ResetBackPointer() {
     back_pointer_->DropFactory();
     back_pointer_->Release();
+    back_pointer_ = NULL;
   }
 
   // Takes ownership of the dispatcher pointer, which should be heap allocated.
   template <typename Dispatcher>
   CompletionCallback NewCallbackHelper(Dispatcher* dispatcher) {
+    ThreadTraits::AutoLock lock(lock_);
+
     PP_DCHECK(object_);  // Expects a non-null object!
     return CompletionCallback(
         &CallbackData<Dispatcher>::Thunk,
@@ -841,6 +855,8 @@ class CompletionCallbackFactory {
       typename internal::TypeUnwrapper<
           typename Dispatcher::OutputType>::StorageType>
   NewCallbackWithOutputHelper(Dispatcher* dispatcher) {
+    ThreadTraits::AutoLock lock(lock_);
+
     PP_DCHECK(object_);  // Expects a non-null object!
     CallbackData<Dispatcher>* data =
         new CallbackData<Dispatcher>(back_pointer_, dispatcher);
@@ -856,6 +872,9 @@ class CompletionCallbackFactory {
   CompletionCallbackFactory& operator=(const CompletionCallbackFactory&);
 
   T* object_;

[viettrungluu, 2012/07/10 22:47:58]

Maybe this should have a comment that this is only initialized once (and then
never changed).

+
+  typename ThreadTraits::Lock lock_;

[viettrungluu, 2012/07/10 22:47:58]

Probably should indicate that this protects back_pointer_.

+
   BackPointer* back_pointer_;
 };