PAPI: Fix bug in RunWhileLocked, add support for params

ppapi/shared_impl/proxy_lock.h

 // 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_SHARED_IMPL_PROXY_LOCK_H_
 #define PPAPI_SHARED_IMPL_PROXY_LOCK_H_
 
 #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"
 
 namespace base {
 class Lock;
 }
 
 namespace ppapi {
 
 // This is the one lock to rule them all for the ppapi proxy. All PPB interface
 // functions that need to be synchronized should lock this lock on entry. This
 // is normally accomplished by using an appropriate Enter RAII object at the
 // beginning of each thunk function.
 //
 // TODO(dmichael): If this turns out to be too slow and contentious, we'll want
 // to use multiple locks. E.g., one for the var tracker, one for the resource
 // 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;
   // see PluginResourceTracker for where the lock gets set for the out-of-
   // process plugin case.
   static void Acquire();
   // Relinquish the proxy lock. If the lock has not been set, this does nothing.
   static void Release();
 
   // Assert that the lock is owned by the current thread (in the plugin
   // 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);
 };
 
 // A simple RAII class for locking the PPAPI proxy lock on entry and releasing
 // on exit. This is for simple interfaces that don't use the 'thunk' system,
 // such as PPB_Var and PPB_Core.
 class ProxyAutoLock {
  public:
   ProxyAutoLock() {
     ProxyLock::Acquire();
@@ skipping 71 matching lines @@
                              const P1& p1,
                              const P2& p2,
                              const P3& p3,
                              const P4& p4,
                              const P5& p5) {
   ProxyAutoUnlock unlock;
   return 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
 
 #endif  // PPAPI_SHARED_IMPL_PROXY_LOCK_H_