PPAPI: Purposely leak ProxyLock, fix shutdown race

ppapi/shared_impl/proxy_lock.h

Index: ppapi/shared_impl/proxy_lock.h
diff --git a/ppapi/shared_impl/proxy_lock.h b/ppapi/shared_impl/proxy_lock.h
index e0e97e4e38f4071ed592a4b673a4578265fadf17..c3c7f720c336bbf02c801709c43161a1fcc84983 100644
--- a/ppapi/shared_impl/proxy_lock.h
+++ b/ppapi/shared_impl/proxy_lock.h
@@ -8,6 +8,7 @@
 #include "base/basictypes.h"
 #include "base/bind.h"
 #include "base/callback.h"
+#include "base/threading/thread_checker.h"
 
 #include "ppapi/shared_impl/ppapi_shared_export.h"
 
@@ -132,21 +133,186 @@ ReturnType CallWhileUnlocked(ReturnType (*function)(P1, P2, P3, P4, P5),
 }
 void PPAPI_SHARED_EXPORT CallWhileUnlocked(const base::Closure& closure);
 
-// CallWhileLocked locks the ProxyLock and runs the given closure immediately.
-// The lock is released when CallWhileLocked returns. This function assumes the
-// lock is not held. This is mostly for use in RunWhileLocked; see below.
-void PPAPI_SHARED_EXPORT CallWhileLocked(const base::Closure& closure);
+namespace internal {
 
-// RunWhileLocked binds the given closure with CallWhileLocked and returns the
-// new Closure. This is for cases where you want to run a task, but you want to
-// ensure that the ProxyLock is acquired for the duration of the task.
+template <typename RunType>
+class RunWhileLockedHelper;
+
+template <>
+class RunWhileLockedHelper<void ()> {
+ public:
+  typedef base::Callback<void ()> CallbackType;
+  explicit RunWhileLockedHelper(const CallbackType& callback)
+      : callback_(new CallbackType(callback)) {
+    // Copying |callback| may adjust reference counts for bound Vars or
+    // Resources; we should have the lock already.
+    ProxyLock::AssertAcquired();
+    // CallWhileLocked and destruction might happen on a different thread from
+    // creation.
+    thread_checker_.DetachFromThread();
+  }
+  void CallWhileLocked() {
+    // Bind thread_checker_ to this thread so we can check in the destructor.
+    DCHECK(thread_checker_.CalledOnValidThread());
+    ProxyAutoLock lock;
+    {
+      // Use a scope and local Callback to ensure that the callback is cleared
+      // before the lock is released, even in the unlikely event that Run()
+      // throws an exception.
+      scoped_ptr<CallbackType> temp_callback(callback_.Pass());
+      temp_callback->Run();
+    }
+  }
+
+  ~RunWhileLockedHelper() {
+    // Check that the Callback is destroyed on the same thread as where
+    // CallWhileLocked happened (if CallWhileLocked happened).
+    DCHECK(thread_checker_.CalledOnValidThread());
+    // Here we read callback_ without the lock. This is why the callback must be
+    // destroyed on the same thread where it runs. There are 2 cases where
+    // callback_ will be NULL:
+    //   1) This is the original RunWhileLockedHelper that RunWhileLocked
+    //      created. When it was copied somewhere else (e.g., to a MessageLoop
+    //      queue), callback_ was passed to the new copy, and the original
+    //      RunWhileLockedHelper's callback_ was set to NULL (since scoped_ptrs
+    //      only ever have 1 owner).

[teravest, 2013/07/22 20:36:00]

You should mention here that the proxy lock is still held, in this case, which
is why it must be checked without trying to take it again.

+    //   2) callback_ has already been run via CallWhileLocked.
+    if (callback_) {
+      // If the callback was not run, we still need to have the lock when we
+      // destroy the callback in case it had a Resource bound to it. This
+      // ensures that the Resource's destructor is invoked only with the lock
+      // held.
+      //
+      // Also: Resource and Var inherit RefCounted (not ThreadSafeRefCounted),
+      // and these callbacks need to be usable on any thread. So we need to lock
+      // when releasing the callback to avoid ref counting races.
+      ProxyAutoLock lock;
+      callback_.reset();
+    }
+  }
+ private:
+  scoped_ptr<CallbackType> callback_;
+
+  // Used to ensure that the Callback is run and deleted on the same thread.
+  base::ThreadCheckerImpl thread_checker_;
+};
+
+template <typename P1>
+class RunWhileLockedHelper<void (P1)> {
+ public:
+  typedef base::Callback<void (P1)> CallbackType;
+  explicit RunWhileLockedHelper(const CallbackType& callback)
+      : callback_(new CallbackType(callback)) {
+    ProxyLock::AssertAcquired();
+    thread_checker_.DetachFromThread();
+  }
+  void CallWhileLocked(P1 p1) {
+    DCHECK(thread_checker_.CalledOnValidThread());
+    ProxyAutoLock lock;
+    {
+      scoped_ptr<CallbackType> temp_callback(callback_.Pass());
+      temp_callback->Run(p1);
+    }
+  }
+
+  ~RunWhileLockedHelper() {
+    DCHECK(thread_checker_.CalledOnValidThread());
+    if (callback_) {
+      ProxyAutoLock lock;
+      callback_.reset();
+    }
+  }
+ private:
+  scoped_ptr<CallbackType> callback_;
+  base::ThreadCheckerImpl thread_checker_;
+};
+
+template <typename P1, typename P2>
+class RunWhileLockedHelper<void (P1, P2)> {
+ public:
+  typedef base::Callback<void (P1, P2)> CallbackType;
+  explicit RunWhileLockedHelper(const CallbackType& callback)
+      : callback_(new CallbackType(callback)) {
+    ProxyLock::AssertAcquired();
+    thread_checker_.DetachFromThread();
+  }
+  void CallWhileLocked(P1 p1, P2 p2) {
+    DCHECK(thread_checker_.CalledOnValidThread());
+    ProxyAutoLock lock;
+    {
+      scoped_ptr<CallbackType> temp_callback(callback_.Pass());
+      temp_callback->Run(p1, p2);
+    }
+  }
+
+  ~RunWhileLockedHelper() {
+    DCHECK(thread_checker_.CalledOnValidThread());
+    if (callback_) {
+      ProxyAutoLock lock;
+      callback_.reset();
+    }
+  }
+ private:
+  scoped_ptr<CallbackType> callback_;
+  base::ThreadCheckerImpl thread_checker_;
+};
+
+template <typename P1, typename P2, typename P3>
+class RunWhileLockedHelper<void (P1, P2, P3)> {
+ public:
+  typedef base::Callback<void (P1, P2, P3)> CallbackType;
+  explicit RunWhileLockedHelper(const CallbackType& callback)
+      : callback_(new CallbackType(callback)) {
+    ProxyLock::AssertAcquired();
+    thread_checker_.DetachFromThread();
+  }
+  void CallWhileLocked(P1 p1, P2 p2, P3 p3) {
+    DCHECK(thread_checker_.CalledOnValidThread());
+    ProxyAutoLock lock;
+    {
+      scoped_ptr<CallbackType> temp_callback(callback_.Pass());
+      temp_callback->Run(p1, p2, p3);
+    }
+  }
+
+  ~RunWhileLockedHelper() {
+    DCHECK(thread_checker_.CalledOnValidThread());
+    if (callback_) {
+      ProxyAutoLock lock;
+      callback_.reset();
+    }
+  }
+ private:
+  scoped_ptr<CallbackType> callback_;
+  base::ThreadCheckerImpl thread_checker_;
+};
+
+}  // namespace internal
+
+// RunWhileLocked wraps the given Callback in a new Callback that, when invoked:
+//  1) Locks the ProxyLock.
+//  2) Runs the original Callback (forwarding arguments, if any).
+//  3) Clears the original Callback (while the lock is held).
+//  4) Unlocks the ProxyLock.
+// Note that it's important that the callback is cleared in step (3), in case
+// clearing the Callback causes a destructor (e.g., for a Resource) to run,
+// which should hold the ProxyLock to avoid data races.
+//
+// This is for cases where you want to run a task or store a Callback, but you
+// want to ensure that the ProxyLock is acquired for the duration of the task.
 // Example usage:
 //   GetMainThreadMessageLoop()->PostDelayedTask(
 //     FROM_HERE,
 //     RunWhileLocked(base::Bind(&CallbackWrapper, callback, result)),
 //     delay_in_ms);
-inline base::Closure RunWhileLocked(const base::Closure& closure) {
-  return base::Bind(CallWhileLocked, closure);
+template <class FunctionType>
+inline base::Callback<FunctionType>
+RunWhileLocked(const base::Callback<FunctionType>& callback) {
+  internal::RunWhileLockedHelper<FunctionType>* helper =
+      new internal::RunWhileLockedHelper<FunctionType>(callback);
+  return base::Bind(
+      &internal::RunWhileLockedHelper<FunctionType>::CallWhileLocked,
+      base::Owned(helper));
 }
 
 }  // namespace ppapi