PAPI: Fix bug in RunWhileLocked, add support for params

PAPI: Fix bug in RunWhileLocked, add support for params

This CL:
- Fixes a bug whereby a Callback bound by RunWhileLocked would release the lock before releasing bound values. This could lead to de-refing and possibly destroying objects (like Resources) that should only be manipulated with the ProxyLock held.
- Adds support for up to 3 parameters to RunWhileLocked. Should be easy to extend to more if/when we want.
- Adds a unit test that checks the bug and tests other utilities from ProxyLock.
There is still possibly a rare issue with the Callback being destroyed without being run, e.g. at process shutdown. I'm going to clean that up in a separate CL: https://codereview.chromium.org/19492014/

BUG=
R=bbudge@chromium.org, teravest@chromium.org, yzshen@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=213693

[dmichael (off chromium), 2013-07-24 16:31:52]

This is the CL for fixing RunWhileLocked and adding support for params. PTAL.

When this is done, I'll finish up the locking-in-the-destructor change here:
https://codereview.chromium.org/19492014/

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

https://codereview.chromium.org/19678028/diff/6001/ppapi/shared_impl/proxy_lo...
File ppapi/shared_impl/proxy_lock.h (right):

https://codereview.chromium.org/19678028/diff/6001/ppapi/shared_impl/proxy_lo...
ppapi/shared_impl/proxy_lock.h:33: // you need access to the ProxyLock for e.g.
a condition variable.
/for e.g./for example/

https://codereview.chromium.org/19678028/diff/6001/ppapi/shared_impl/proxy_lo...
ppapi/shared_impl/proxy_lock.h:276: // Will fail when the callback is destroyed,
because the constructor expects the
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.

[teravest, 2013-07-24 17:02:18]

On Wed, Jul 24, 2013 at 10:38 AM,  <teravest@chromium.org> wrote:
>
>
https://codereview.chromium.org/19678028/diff/6001/ppapi/shared_impl/proxy_lo...
> File ppapi/shared_impl/proxy_lock.h (right):
>
>
https://codereview.chromium.org/19678028/diff/6001/ppapi/shared_impl/proxy_lo...
> ppapi/shared_impl/proxy_lock.h:33: // you need access to the ProxyLock
> for e.g. a condition variable.
> /for e.g./for example/
>
>
https://codereview.chromium.org/19678028/diff/6001/ppapi/shared_impl/proxy_lo...
> ppapi/shared_impl/proxy_lock.h:276: // Will fail when the callback is
> destroyed, because the constructor expects the
> 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.

Actually, I guess this falls under "assign to member variable"? I just
got screwy here (and messed up whether RunWhileLock returns a
pointer).

Still, it seems like it would be clearer to simply say that the
ProxyLock must not be held when the returned callback is destroyed.

[bbudge, 2013-07-24 17:57:39]

LGTM

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

https://codereview.chromium.org/19678028/diff/6001/ppapi/shared_impl/proxy_lo...
File ppapi/shared_impl/proxy_lock.h (left):

https://codereview.chromium.org/19678028/diff/6001/ppapi/shared_impl/proxy_lo...
ppapi/shared_impl/proxy_lock.h:43: private:
Out of curiosity: Why removing "private:"?

https://codereview.chromium.org/19678028/diff/6001/ppapi/shared_impl/proxy_lo...
File ppapi/shared_impl/proxy_lock.h (right):

https://codereview.chromium.org/19678028/diff/6001/ppapi/shared_impl/proxy_lo...
ppapi/shared_impl/proxy_lock.h:266: // Note that the returned Callback must be:
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().

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

Comments addressed, PTAL.

I don't want to do ProxyLock::IsAcquiredByCurrentThread()... it would be roughly
morally equivalent to a recursive lock, and I don't want to expose that. We
actually have a similar issue in how the proxy lock works for Graphics3D calls,
since they can originate in the proxy or the plugin. But in that case and here,
we look for very specific state...
- In this CL: has the callback been cleared or run?
- In Graphics3D: Is the openGL stuff being invoked from with the Graphics3D
proxy?

If we instead exposed IsAcquiredByCurrentThread and started using it, it would
be much easier for things to appear to work correctly because we accidentally
are holding the lock, for example. I'd like to stick to knowing exactly why we
do or don't already have the lock in any piece of code that has to choose
whether to lock.

https://codereview.chromium.org/19678028/diff/6001/ppapi/shared_impl/proxy_lo...
File ppapi/shared_impl/proxy_lock.h (left):

https://codereview.chromium.org/19678028/diff/6001/ppapi/shared_impl/proxy_lo...
ppapi/shared_impl/proxy_lock.h:43: private:
On 2013/07/24 18:45:19, yzshen1 wrote:
> Out of curiosity: Why removing "private:"?
Oops, that was an accident when I was deleting stuff to make a smaller CL. I put
it back, thanks.

https://codereview.chromium.org/19678028/diff/6001/ppapi/shared_impl/proxy_lo...
File ppapi/shared_impl/proxy_lock.h (right):

https://codereview.chromium.org/19678028/diff/6001/ppapi/shared_impl/proxy_lo...
ppapi/shared_impl/proxy_lock.h:33: // you need access to the ProxyLock for e.g.
a condition variable.
On 2013/07/24 16:38:44, teravest wrote:
> /for e.g./for example/

Done.

[yzshen1, 2013-07-24 22:54:10]

Thanks for the detailed reply! I agree that the current way is probably better
than IsAcquiredByCurrentThread().

LGTM

[teravest, 2013-07-25 13:46:28]

lgtm

[dmichael (off chromium), 2013-07-25 17:40:02]

I had to do one more small change to fix a deadlock in the Audio tests. Feel
free to look, but I suspect it's non-controversial.

I'm going to flip the CQ bit, but please stop me if something looks wrong to
you. Thanks for the reviews!

[commit-bot: I haz the power, 2013-07-25 18:06:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/19678028/42001

[dmichael (off chromium), 2013-07-25 21:09:19]

Committed patchset #6 manually as r213693 (presubmit successful).