PAPI: Fix bug in RunWhileLocked, add support for params

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..b73498e1a34c1b07972058ad50f8f2b0ed7b5ad5 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"
 
@@ -27,6 +28,11 @@ namespace ppapi {
 // tracker, etc.
 class PPAPI_SHARED_EXPORT ProxyLock {
  public:
+  // Return the globally unique ProxyLock. Normally, you should not access this
+  // directly but instead use ProxyAutoLock or ProxyAutoUnlock. But sometimes
+  // you need access to the ProxyLock for e.g. a condition variable.

[teravest, 2013/07/24 16:38:44]

/for e.g./for example/

[dmichael (off chromium), 2013/07/24 22:30:11]

Done.

+  static base::Lock* Get();
+
   // Acquire the proxy lock. If it is currently held by another thread, block
   // until it is available. If the lock has not been set using the 'Set' method,
   // this operation does nothing. That is the normal case for the host side;
@@ -40,7 +46,6 @@ class PPAPI_SHARED_EXPORT ProxyLock {
   // process). Does nothing when running in-process (or in the host process).
   static void AssertAcquired();
 
- private:

[yzshen1, 2013/07/24 18:45:19]

Out of curiosity: Why removing "private:"?

[dmichael (off chromium), 2013/07/24 22:30:11]

Oops, that was an accident when I was deleting stuff to make a smaller CL. I put
it back, thanks.

   DISALLOW_IMPLICIT_CONSTRUCTORS(ProxyLock);
 };
 
@@ -132,21 +137,153 @@ 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.
-// Example usage:
+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();
+    }
+  }
+
+ private:
+  scoped_ptr<CallbackType> callback_;
+
+  // Used to ensure that the Callback is run and deleted on the same thread.
+  base::ThreadChecker 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);
+    }
+  }
+
+ private:
+  scoped_ptr<CallbackType> callback_;
+  base::ThreadChecker 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);
+    }
+  }
+
+ private:
+  scoped_ptr<CallbackType> callback_;
+  base::ThreadChecker 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);
+    }
+  }
+
+ private:
+  scoped_ptr<CallbackType> callback_;
+  base::ThreadChecker 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);
+//
+// Note that the returned Callback must be:

[yzshen1, 2013/07/24 18:45:19]

I think the comment is still pretty hard to understand.
Does the following description accurate?
===================================
The returned Callback must be:
(1) copied to another Callback. And then the original callback can be destroyed
with/without proxy lock, while the assigned callback has to conform to these
restrictions. Or
(2) run without proxy lock acquired (e.g., being posted to a MessageLoop and run
there). The callback must be destroyed on the same thread (whether the proxy
lock is acquired doesn't matter). Or
(3) destroyed without proxy lock acquired.
===================================

BTW, I am not sure about the performance impact, but if we provide the following
method:
  bool ProxyLock::IsAcquiredByCurrentThread(),
then we can simply check whether the lock is held in CallWhileLocked() and
destructor, and lock if necessary. In that case, the logic is simplified and
there is no need to pay special attention to handle the returned Callback of
RunWhileLocked().

+//  - Posted to a MessageLoop.
+//  - Copied to another Callback (e.g. a member variable).
+//  or
+//  - Run().
+// E.g., code like the following:
+// {
+//   base::Callback<...> callback_a = RunWhileLocked(callback_b);
+//   // Do nothing about callback_a and let it go out of scope.
+// }
+// Will fail when the callback is destroyed, because the constructor expects the

[teravest, 2013/07/24 16:38:44]

What do you mean by fail here? It'd be good if you're more descriptive about the
consequences.

It's not clear what would happen in this case:
scoped_ptr<base::Callback<void()>> cb;
{
  AutoProxyLock l;
  cb.reset(RunWhileLocked(...));
}
cb.reset();

This violates your requirements for what must be done with the returned
callback, but shouldn't run into the locking problems you allude to.

+// proxy lock to already be acquired, while the destructor expects to need to
+// acquire the lock (if the callback has not been run, like in the above case).
+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