Add support for threadsafe completion callback factory.

This also makes the default be threadsafe. The old factory wasn't threadsafe even to the extent claimed in the header which was causing hangs in plugins

BUG=136284

Review URL: https://chromiumcodereview.appspot.com/10696157

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@146611 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 347 insertions, 70 deletions

diff --git a/ppapi/utility/completion_callback_factory.h b/ppapi/utility/completion_callback_factory.h
index d521f4b..885767b 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,26 +200,35 @@ 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() {
+    typename 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
   /// is passed to the constructor. The object may only be initialized once,
   /// either by the constructor, or by a call to Initialize().
   ///
+  /// This class may not be used on any thread until initialization is complete.
+  ///
   /// @param[in] object The object whose member functions are to be bound to
   /// the <code>CompletionCallback</code> created by this
   /// <code>CompletionCallbackFactory</code>.
@@ -247,7 +256,6 @@ class CompletionCallbackFactory {
   /// @return A <code>CompletionCallback</code>.
   template <typename Method>
   CompletionCallback NewCallback(Method method) {
-    PP_DCHECK(object_);
     return NewCallbackHelper(new Dispatcher0<Method>(method));
   }
 
@@ -302,7 +310,6 @@ class CompletionCallbackFactory {
   /// @return A <code>CompletionCallback</code>.
   template <typename Method, typename A>
   CompletionCallback NewCallback(Method method, const A& a) {
-    PP_DCHECK(object_);
     return NewCallbackHelper(new Dispatcher1<Method, A>(method, a));
   }
 
@@ -369,7 +376,6 @@ class CompletionCallbackFactory {
   /// @return A <code>CompletionCallback</code>.
   template <typename Method, typename A, typename B>
   CompletionCallback NewCallback(Method method, const A& a, const B& b) {
-    PP_DCHECK(object_);
     return NewCallbackHelper(new Dispatcher2<Method, A, B>(method, a, b));
   }
 
@@ -451,7 +457,6 @@ class CompletionCallbackFactory {
   template <typename Method, typename A, typename B, typename C>
   CompletionCallback NewCallback(Method method, const A& a, const B& b,
                                  const C& c) {
-    PP_DCHECK(object_);
     return NewCallbackHelper(new Dispatcher3<Method, A, B, C>(method, a, b, c));
   }
 
@@ -522,7 +527,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 +551,7 @@ class CompletionCallbackFactory {
     }
 
    private:
-    RefCount ref_;
+    typename ThreadTraits::RefCount ref_;
     FactoryType* factory_;
   };
 
@@ -817,19 +822,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) {
+    typename ThreadTraits::AutoLock lock(lock_);
+
     PP_DCHECK(object_);  // Expects a non-null object!
     return CompletionCallback(
         &CallbackData<Dispatcher>::Thunk,
@@ -841,6 +853,8 @@ class CompletionCallbackFactory {
       typename internal::TypeUnwrapper<
           typename Dispatcher::OutputType>::StorageType>
   NewCallbackWithOutputHelper(Dispatcher* dispatcher) {
+    typename ThreadTraits::AutoLock lock(lock_);
+
     PP_DCHECK(object_);  // Expects a non-null object!
     CallbackData<Dispatcher>* data =
         new CallbackData<Dispatcher>(back_pointer_, dispatcher);
@@ -855,7 +869,14 @@ class CompletionCallbackFactory {
   CompletionCallbackFactory(const CompletionCallbackFactory&);
   CompletionCallbackFactory& operator=(const CompletionCallbackFactory&);
 
+  // Never changed once initialized so does not need protection by the lock.
   T* object_;
+
+  // Protects the back pointer.
+  typename ThreadTraits::Lock lock_;
+
+  // Protected by the lock. This will get reset when you do CancelAll, for
+  // example.
   BackPointer* back_pointer_;
 };
 
diff --git a/ppapi/utility/completion_callback_factory_thread_traits.h b/ppapi/utility/completion_callback_factory_thread_traits.h
new file mode 100644
index 0000000..2242b5c
--- /dev/null
+++ b/ppapi/utility/completion_callback_factory_thread_traits.h
@@ -0,0 +1,178 @@
+// 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 PPAPI_UTILITY_THREAD_SAFE_THREAD_TRAITS_H_

+#define PPAPI_UTILITY_THREAD_SAFE_THREAD_TRAITS_H_

+

+#include "ppapi/cpp/logging.h"

+#include "ppapi/cpp/module.h"

+#include "ppapi/utility/threading/lock.h"

+

+/// @file

+/// Defines the traits structures for thread-safety of a completion callback

+/// factory. We provide thread-safe and non-thread-safe version. The thread-safe

+/// version is always correct (if you follow the thread usage rules of the

+/// callback factory), but if you know your object will only be used on one

+/// thread, you can uses the non-thread-safe version.

+///

+/// The traits defines three nested classes to perform reference counting,

+/// locks, and scoped locking.

+

+namespace pp {

+

+/// The thread-safe version of thread traits. Using this class as the "traits"

+/// template argument to a completion callback factory will make it "somewhat

+/// thread-friendly." It will allow you to create completion callbacks from

+/// background threads and post them to another thread to run.

+///

+/// Care still must be taken to ensure that the completion callbacks are

+/// executed on the same thread that the factory is destroyed on to avoid a

+/// race on destruction.

+///

+/// Implementation note: this uses a lock instead of atomic add instructions.

+/// The number of platforms we need to support right now makes atomic

+/// operations unwieldy for this case that we don't actually use that often.

+/// As a further optimization, we can add support for this later.

+class ThreadSafeThreadTraits {

+ public:

+  class RefCount {

+   public:

+    /// Default constructor. In debug mode, this checks that the object is being

+    /// created on the main thread.

+    RefCount() : ref_(0) {

+    }

+

+    /// AddRef() increments the reference counter.

+    ///

+    /// @return An int32_t with the incremented reference counter.

+    int32_t AddRef() {

+      AutoLock lock(lock_);

+      return ++ref_;

+    }

+

+    /// Release() decrements the reference counter.

+    ///

+    /// @return An int32_t with the decremeneted reference counter.

+    int32_t Release() {

+      AutoLock lock(lock_);

+      PP_DCHECK(ref_ > 0);

+      return --ref_;

+    }

+

+   private:

+    Lock lock_;

+    int32_t ref_;

+  };

+

+  typedef pp::Lock Lock;

+  typedef pp::AutoLock AutoLock;

+};

+

+/// The non-thread-safe version of thread traits. Using this class as the

+/// "traits" template argument to a completion callback factory will make it

+/// not thread-safe but with potential extra performance.

+class NonThreadSafeThreadTraits {

+ public:

+  /// A simple reference counter that is not thread-safe.

+  ///

+  /// <strong>Note:</strong> in Debug mode, it checks that it is either called

+  /// on the main thread, or always called on another thread.

+  class RefCount {

+   public:

+    /// Default constructor. In debug mode, this checks that the object is being

+    /// created on the main thread.

+    RefCount() : ref_(0) {

+#ifndef NDEBUG

+      is_main_thread_ = Module::Get()->core()->IsMainThread();

+#endif

+    }

+

+    /// Destructor.

+    ~RefCount() {

+      PP_DCHECK(is_main_thread_ == Module::Get()->core()->IsMainThread());

+    }

+

+    /// AddRef() increments the reference counter.

+    ///

+    /// @return An int32_t with the incremented reference counter.

+    int32_t AddRef() {

+      PP_DCHECK(is_main_thread_ == Module::Get()->core()->IsMainThread());

+      return ++ref_;

+    }

+

+    /// Release() decrements the reference counter.

+    ///

+    /// @return An int32_t with the decremeneted reference counter.

+    int32_t Release() {

+      PP_DCHECK(is_main_thread_ == Module::Get()->core()->IsMainThread());

+      return --ref_;

+    }

+

+   private:

+    int32_t ref_;

+#ifndef NDEBUG

+    bool is_main_thread_;

+#endif

+  };

+

+  /// A simple object that acts like a lock but does nothing.

+  ///

+  /// MStrong>Note:</strong> in Debug mode, it checks that it is either

+  /// called on the main thread, or always called on another thread. It also

+  /// asserts that the caller does not recursively lock.

+  class Lock {

+   public:

+    Lock() {

+#ifndef NDEBUG

+      is_main_thread_ = Module::Get()->core()->IsMainThread();

+      lock_held_ = false;

+#endif

+    }

+

+    ~Lock() {

+      PP_DCHECK(is_main_thread_ == Module::Get()->core()->IsMainThread());

+    }

+

+    /// Acquires the fake "lock". This does nothing except perform checks in

+    /// debug mode.

+    void Acquire() {

+#ifndef NDEBUG

+      PP_DCHECK(!lock_held_);

+      lock_held_ = true;

+#endif

+    }

+

+    /// Releases the fake "lock". This does nothing except perform checks in

+    /// debug mode.

+    void Release() {

+#ifndef NDEBUG

+      PP_DCHECK(lock_held_);

+      lock_held_ = false;

+#endif

+    }

+

+   private:

+#ifndef NDEBUG

+    bool is_main_thread_;

+    bool lock_held_;

+#endif

+  };

+

+  class AutoLock {

+   public:

+    explicit AutoLock(Lock& lock) : lock_(lock) {

+      lock_.Acquire();

+    }

+    ~AutoLock() {

+      lock_.Release();

+    }

+

+   private:

+    Lock& lock_;

+  };

+};

+

+}  // namespace pp

+

+#endif  // PPAPI_UTILITY_THREAD_SAFE_THREAD_TRAITS_H_

diff --git a/ppapi/utility/non_thread_safe_ref_count.h b/ppapi/utility/non_thread_safe_ref_count.h
index db00b7d..32943fe 100644
--- a/ppapi/utility/non_thread_safe_ref_count.h
+++ b/ppapi/utility/non_thread_safe_ref_count.h
@@ -1,60 +1 @@
-// Copyright (c) 2011 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 PPAPI_UTILITY_NON_THREAD_SAFE_REF_COUNT_H_
-#define PPAPI_UTILITY_NON_THREAD_SAFE_REF_COUNT_H_
-
-#include "ppapi/cpp/core.h"
-#include "ppapi/cpp/logging.h"
-#include "ppapi/cpp/module.h"
-
-/// @file
-/// This file defines the APIs for maintaining a reference counter.
-namespace pp {
-
-/// A simple reference counter that is not thread-safe. <strong>Note:</strong>
-/// in Debug mode, it checks that it is either called on the main thread, or
-/// always called on another thread.
-class NonThreadSafeRefCount {
- public:
-  /// Default constructor. In debug mode, this checks that the object is being
-  /// created on the main thread.
-  NonThreadSafeRefCount()
-      : ref_(0) {
-#ifndef NDEBUG
-    is_main_thread_ = Module::Get()->core()->IsMainThread();
-#endif
-  }
-
-  /// Destructor.
-  ~NonThreadSafeRefCount() {
-    PP_DCHECK(is_main_thread_ == Module::Get()->core()->IsMainThread());
-  }
-
-  /// AddRef() increments the reference counter.
-  ///
-  /// @return An int32_t with the incremented reference counter.
-  int32_t AddRef() {
-    PP_DCHECK(is_main_thread_ == Module::Get()->core()->IsMainThread());
-    return ++ref_;
-  }
-
-  /// Release() decrements the reference counter.
-  ///
-  /// @return An int32_t with the decremeneted reference counter.
-  int32_t Release() {
-    PP_DCHECK(is_main_thread_ == Module::Get()->core()->IsMainThread());
-    return --ref_;
-  }
-
- private:
-  int32_t ref_;
-#ifndef NDEBUG
-  bool is_main_thread_;
-#endif
-};
-
-}  // namespace pp
-
-#endif  // PPAPI_UTILITY_NON_THREAD_SAFE_REF_COUNT_H_
+// This file should be erased
diff --git a/ppapi/utility/threading/lock.cc b/ppapi/utility/threading/lock.cc
new file mode 100644
index 0000000..12cbd08
--- /dev/null
+++ b/ppapi/utility/threading/lock.cc
@@ -0,0 +1,49 @@
+// 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.

+

+#include "ppapi/utility/threading/lock.h"

+

+namespace pp {

+

+#ifdef WIN32  // Windows implementation for native plugins.

+

+Lock::Lock() {

+  // The second parameter is the spin count; for short-held locks it avoids the

+  // contending thread from going to sleep which helps performance greatly.

+  ::InitializeCriticalSectionAndSpinCount(&os_lock_, 2000);

+}

+

+Lock::~Lock() {

+  ::DeleteCriticalSection(&os_lock_);

+}

+

+void Lock::Acquire() {

+  ::EnterCriticalSection(&os_lock_);

+}

+

+void Lock::Release() {

+  ::LeaveCriticalSection(&os_lock_);

+}

+

+#else  // Posix implementation.

+

+Lock::Lock() {

+  pthread_mutex_init(&os_lock_, NULL);

+}

+

+Lock::~Lock() {

+  pthread_mutex_destroy(&os_lock_);

+}

+

+void Lock::Acquire() {

+  pthread_mutex_lock(&os_lock_);

+}

+

+void Lock::Release() {

+  pthread_mutex_unlock(&os_lock_);

+}

+

+#endif

+

+}  // namespace pp

diff --git a/ppapi/utility/threading/lock.h b/ppapi/utility/threading/lock.h
new file mode 100644
index 0000000..71a9876
--- /dev/null
+++ b/ppapi/utility/threading/lock.h
@@ -0,0 +1,84 @@
+// 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 PPAPI_UTILITY_THREADING_LOCK_H_

+#define PPAPI_UTILITY_THREADING_LOCK_H_

+

+#ifdef WIN32

+#include <windows.h>

+#else

+#include <pthread.h>

+#endif

+

+namespace pp {

+

+/// A simple wrapper around a platform-specific lock. See also AutoLock.

+class Lock {

+ public:

+  /// Creates a lock in the "not held" state.

+  Lock();

+

+  /// Destroys the lock.

+  ~Lock();

+

+  /// Acquires the lock, blocking if it's already held by a different thread.

+  /// The lock must not already be held on the current thread (i.e. recursive

+  /// locks are not supported).

+  ///

+  /// Most callers should consider using an AutoLock instead to automatically

+  /// acquire and release the lock.

+  void Acquire();

+

+  /// Releases the lock. This must be paired with a call to Acquire().

+  void Release();

+

+ private:

+#if defined(WIN32)

+  typedef CRITICAL_SECTION OSLockType;

+#else

+  typedef pthread_mutex_t OSLockType;

+#endif

+

+  OSLockType os_lock_;

+

+  // Copy and assign not supported.

+  Lock(const Lock&);

+  Lock& operator=(const Lock&);

+};

+

+/// A helper class that scopes holding a lock.

+///

+/// @code

+///   class MyClass {

+///    public:

+///     void DoSomething() {

+///       pp::AutoLock lock(lock_);

+///       ...do something with the lock held...

+///     }

+///

+///    private:

+///     pp::Lock lock_;

+///   };

+/// @endcode

+class AutoLock {

+ public:

+  explicit AutoLock(Lock& lock) : lock_(lock) {

+    lock_.Acquire();

+  }

+

+  ~AutoLock() {

+    lock_.Release();

+  }

+

+ private:

+  Lock& lock_;

+

+  // Copy and assign not supported.

+  AutoLock(const AutoLock&);

+  AutoLock& operator=(const AutoLock&);

+};

+

+}  // namespace pp

+

+#endif  // PPAPI_UTILITY_THREADING_LOCK_H_

diff --git a/ppapi/utility/websocket/websocket_api.cc b/ppapi/utility/websocket/websocket_api.cc
index db4e01a..c251539 100644
--- a/ppapi/utility/websocket/websocket_api.cc
+++ b/ppapi/utility/websocket/websocket_api.cc
@@ -13,6 +13,10 @@
 #include "ppapi/cpp/websocket.h"
 #include "ppapi/utility/completion_callback_factory.h"
 
+#ifdef SendMessage
+#undef SendMessage
+#endif
+
 namespace pp {
 
 class WebSocketAPI::Implement : public WebSocket {