PPAPI: Purposely leak ProxyLock, fix shutdown race

PPAPI: Purposely leak ProxyLock, fix shutdown race

This CL:
- Makes callbacks returned by RunWhileLocked acquire the lock when they are destroyed if they are not run. This eliminates some potential race conditions.
- Because MessageLoops can be destroyed after the PpapiGlobals (and thus the ProxyLock), this means we need to make the ProxyLock live longer. So we leak it so that it lives all the way through shutdown.

This issue depends on: https://codereview.chromium.org/19492014/

BUG=
R=yzshen@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222829
Reverted: https://src.chromium.org/viewvc/chrome?view=rev&revision=222840 (static initializer + content_unittests failure)

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

[dmichael (off chromium), 2013-07-19 22:45:17]

bbudge: Primary, and let me know if this works with your patch (though no need
to wait for this to land).
teravest: In case you have objections to adding more slightly templatey junk.

[bbudge, 2013-07-19 23:13:02]

On 2013/07/19 22:45:17, dmichael wrote:
> bbudge: Primary, and let me know if this works with your patch (though no need
> to wait for this to land).
> teravest: In case you have objections to adding more slightly templatey junk.

FYI the unittest file is missing.

[teravest, 2013-07-19 23:20:12]

I feel like I'm misreading this; it looks like public
CallWhileLocked() is removed as part of this change. Any reason why?

I'd name the helper "RunWhileLockedHelper", since that's what it's
providing the implementation for.

On Fri, Jul 19, 2013 at 5:13 PM,  <bbudge@chromium.org> wrote:
> On 2013/07/19 22:45:17, dmichael wrote:
>>
>> bbudge: Primary, and let me know if this works with your patch (though no
>> need
>> to wait for this to land).
>> teravest: In case you have objections to adding more slightly templatey
>> junk.
>
>
> FYI the unittest file is missing.
>
> https://codereview.chromium.org/19492014/

[bbudge, 2013-07-19 23:52:07]

This works for me. I need a 3 argument version so it would be good if you added
that.

[dmichael (off chromium), 2013-07-20 02:13:07]

On 2013/07/19 23:20:12, teravest wrote:
> I feel like I'm misreading this; it looks like public
> CallWhileLocked() is removed as part of this change. Any reason why?
It was only used by RunWhileLocked, and it also made it easy to make the same
mistake that this CL fixes. It will be easy to add again later if it's useful
somewhere.

> 
> I'd name the helper "RunWhileLockedHelper", since that's what it's
> providing the implementation for.
Good idea. I was thinking of it differently when I started out, but I agree that
name makes more sense now.

> 
> On Fri, Jul 19, 2013 at 5:13 PM,  <mailto:bbudge@chromium.org> wrote:
> > On 2013/07/19 22:45:17, dmichael wrote:
> >>
> >> bbudge: Primary, and let me know if this works with your patch (though no
> >> need
> >> to wait for this to land).
> >> teravest: In case you have objections to adding more slightly templatey
> >> junk.
> >
> >
> > FYI the unittest file is missing.
Added, thanks!

> >
> > https://codereview.chromium.org/19492014/

[bbudge, 2013-07-20 04:03:09]

One other thing Yuzhu pointed out to me about my old solution: what happens if
the callback isn't run?

[bbudge, 2013-07-20 14:01:24]

https://codereview.chromium.org/19492014/diff/11001/ppapi/shared_impl/proxy_l...
File ppapi/shared_impl/proxy_lock.h (right):

https://codereview.chromium.org/19492014/diff/11001/ppapi/shared_impl/proxy_l...
ppapi/shared_impl/proxy_lock.h:141: struct RunWhleLockedHelper<void ()> {
sp. RunWhileLockedHelper

[bbudge, 2013-07-20 18:40:35]

On 2013/07/20 04:03:09, bbudge1 wrote:
> One other thing Yuzhu pointed out to me about my old solution: what happens if
> the callback isn't run?

Just to clarify this, what if the callback contains a ref to a Resource and it
is never called? Does the helper dtor grab the lock?

[dmichael (off chromium), 2013-07-22 16:50:40]

On 2013/07/20 18:40:35, bbudge1 wrote:
> On 2013/07/20 04:03:09, bbudge1 wrote:
> > One other thing Yuzhu pointed out to me about my old solution: what happens
if
> > the callback isn't run?
> 
> Just to clarify this, what if the callback contains a ref to a Resource and it
> is never called? Does the helper dtor grab the lock?
That's a good question! In the code I had, obviously it does not. So the
question is... should it?

I think it *almost* doesn't matter. These are almost always going to be owned by
a message loop. So even in a plugin-shutdown situation, they'll be run, but the
TrackedCallback just won't do anything. They should only not run if the plugin
process is shutting down. In that case, I think most of that will happen after
the plugin is torn down. So race conditions are probably going to be unlikely.
But...  the Resource class inherits from non-thread-safe RefCount, and these
callbacks could be deleted on any thread, potentially. So if more than one
Callback exists referring to the same Resource at shut down, the ref-counting
could be racy. So...  I think we do have to lock in the destructor. I also don't
like the idea of trying to be "smart" and only locking if callback_ has not
already been cleared... because these callbacks need to be usable on any thread,
I think that could be dangerous too. I'd rather do it fool-proof, even it means
locking an extra time :-(

What do you think, does my reasoning make sense?

I'm updating it to always lock in the destructor and will upload shortly.

[bbudge, 2013-07-22 16:57:09]

On 2013/07/22 16:50:40, dmichael wrote:
> On 2013/07/20 18:40:35, bbudge1 wrote:
> > On 2013/07/20 04:03:09, bbudge1 wrote:
> > > One other thing Yuzhu pointed out to me about my old solution: what
happens
> if
> > > the callback isn't run?
> > 
> > Just to clarify this, what if the callback contains a ref to a Resource and
it
> > is never called? Does the helper dtor grab the lock?
> That's a good question! In the code I had, obviously it does not. So the
> question is... should it?
> 
> I think it *almost* doesn't matter. These are almost always going to be owned
by
> a message loop. So even in a plugin-shutdown situation, they'll be run, but
the
> TrackedCallback just won't do anything. They should only not run if the plugin
> process is shutting down. In that case, I think most of that will happen after
> the plugin is torn down. So race conditions are probably going to be unlikely.
> But...  the Resource class inherits from non-thread-safe RefCount, and these
> callbacks could be deleted on any thread, potentially. So if more than one
> Callback exists referring to the same Resource at shut down, the ref-counting
> could be racy. So...  I think we do have to lock in the destructor. I also
don't
> like the idea of trying to be "smart" and only locking if callback_ has not
> already been cleared... because these callbacks need to be usable on any
thread,
> I think that could be dangerous too. I'd rather do it fool-proof, even it
means
> locking an extra time :-(
> 
> What do you think, does my reasoning make sense?
> 
> I'm updating it to always lock in the destructor and will upload shortl

So far, this hasn't been a problem in my use case. But it seems possible for the
external code to just not call the callback and then destroy it at some time
that we can't control. However, locking in the destructor could cause a
deadlock, though this seems unlikely.

I agree that it would be better to keep the locking code "dumb" if possible. I
thought a little bit about isolating any "smarts" in our own code but I defer to
your judgement here.

I'll try your new CL to make sure it still works for me. Thanks for doing this!

[yzshen1, 2013-07-22 18:46:37]

On 2013/07/22 16:57:09, bbudge1 wrote:
> On 2013/07/22 16:50:40, dmichael wrote:
> > On 2013/07/20 18:40:35, bbudge1 wrote:
> > > On 2013/07/20 04:03:09, bbudge1 wrote:
> > > > One other thing Yuzhu pointed out to me about my old solution: what
> happens
> > if
> > > > the callback isn't run?
> > > 
> > > Just to clarify this, what if the callback contains a ref to a Resource
and
> it
> > > is never called? Does the helper dtor grab the lock?
> > That's a good question! In the code I had, obviously it does not. So the
> > question is... should it?
> > 
> > I think it *almost* doesn't matter. These are almost always going to be
owned
> by
> > a message loop. So even in a plugin-shutdown situation, they'll be run, but
> the
> > TrackedCallback just won't do anything. They should only not run if the
plugin
> > process is shutting down. In that case, I think most of that will happen
after
> > the plugin is torn down. So race conditions are probably going to be
unlikely.
> > But...  the Resource class inherits from non-thread-safe RefCount, and these
> > callbacks could be deleted on any thread, potentially. So if more than one
> > Callback exists referring to the same Resource at shut down, the
ref-counting
> > could be racy. So...  I think we do have to lock in the destructor. I also
> don't
> > like the idea of trying to be "smart" and only locking if callback_ has not
> > already been cleared... because these callbacks need to be usable on any
> thread,
> > I think that could be dangerous too. I'd rather do it fool-proof, even it
> means
> > locking an extra time :-(
> > 
> > What do you think, does my reasoning make sense?
> > 
> > I'm updating it to always lock in the destructor and will upload shortl
> 
> So far, this hasn't been a problem in my use case. But it seems possible for
the
> external code to just not call the callback and then destroy it at some time
> that we can't control. However, locking in the destructor could cause a
> deadlock, though this seems unlikely.

Does it make sense to support "recursive lock" (allow acquiring the lock when
the current thread owns the lock)?
I know base::Lock doesn't allow this, but it shouldn't be hard to support this
feature on top of that.
With that we don't have the deadlock problem in destrcutor.

(And maybe we can remove all EnterXXXNoLock classes, and use EnterXXX
everywhere?)

> 
> I agree that it would be better to keep the locking code "dumb" if possible. I
> thought a little bit about isolating any "smarts" in our own code but I defer
to
> your judgement here.
> 
> I'll try your new CL to make sure it still works for me. Thanks for doing
this!

[teravest, 2013-07-22 18:54:36]

On Mon, Jul 22, 2013 at 12:46 PM,  <yzshen@chromium.org> wrote:
> On 2013/07/22 16:57:09, bbudge1 wrote:
>>
>> On 2013/07/22 16:50:40, dmichael wrote:
>> > On 2013/07/20 18:40:35, bbudge1 wrote:
>> > > On 2013/07/20 04:03:09, bbudge1 wrote:
>> > > > One other thing Yuzhu pointed out to me about my old solution: what
>> happens
>> > if
>> > > > the callback isn't run?
>> > >
>> > > Just to clarify this, what if the callback contains a ref to a
>> > > Resource
>
> and
>>
>> it
>> > > is never called? Does the helper dtor grab the lock?
>> > That's a good question! In the code I had, obviously it does not. So the
>> > question is... should it?
>> >
>> > I think it *almost* doesn't matter. These are almost always going to be
>
> owned
>>
>> by
>> > a message loop. So even in a plugin-shutdown situation, they'll be run,
>> > but
>> the
>> > TrackedCallback just won't do anything. They should only not run if the
>
> plugin
>>
>> > process is shutting down. In that case, I think most of that will happen
>
> after
>>
>> > the plugin is torn down. So race conditions are probably going to be
>
> unlikely.
>>
>> > But...  the Resource class inherits from non-thread-safe RefCount, and
>> > these
>> > callbacks could be deleted on any thread, potentially. So if more than
>> > one
>> > Callback exists referring to the same Resource at shut down, the
>
> ref-counting
>>
>> > could be racy. So...  I think we do have to lock in the destructor. I
>> > also
>> don't
>> > like the idea of trying to be "smart" and only locking if callback_ has
>> > not
>> > already been cleared... because these callbacks need to be usable on any
>> thread,
>> > I think that could be dangerous too. I'd rather do it fool-proof, even
>> > it
>> means
>> > locking an extra time :-(
>> >
>> > What do you think, does my reasoning make sense?
>> >
>> > I'm updating it to always lock in the destructor and will upload shortl
>
>
>> So far, this hasn't been a problem in my use case. But it seems possible
>> for
>
> the
>>
>> external code to just not call the callback and then destroy it at some
>> time
>> that we can't control. However, locking in the destructor could cause a
>> deadlock, though this seems unlikely.
>
>
> Does it make sense to support "recursive lock" (allow acquiring the lock
> when
> the current thread owns the lock)?
> I know base::Lock doesn't allow this, but it shouldn't be hard to support
> this
> feature on top of that.
> With that we don't have the deadlock problem in destrcutor.
>
> (And maybe we can remove all EnterXXXNoLock classes, and use EnterXXX
> everywhere?)


There's a good discussion on some dangers of reentrant locks at
  http://www.chromium.org/developers/lock-and-condition-variable
under the section "Why can the holder of a Lock not reacquire it?"

>
>
>
>> I agree that it would be better to keep the locking code "dumb" if
>> possible. I
>> thought a little bit about isolating any "smarts" in our own code but I
>> defer
>
> to
>>
>> your judgement here.
>
>
>> I'll try your new CL to make sure it still works for me. Thanks for doing
>
> this!
>
>
>
> https://codereview.chromium.org/19492014/

[dmichael (off chromium), 2013-07-22 20:04:53]

Sorry, I was overly optimistic when I said I'd upload "shortly" :-).

I have something I think is viable up now (though perhaps the trybots will
prove me wrong). It's tricky enough that it needs another look from
reviewers.

Thanks, everyone!

On Mon, Jul 22, 2013 at 12:54 PM, Justin TerAvest <teravest@chromium.org>wrote:

> On Mon, Jul 22, 2013 at 12:46 PM,  <yzshen@chromium.org> wrote:
> > On 2013/07/22 16:57:09, bbudge1 wrote:
> >>
> >> On 2013/07/22 16:50:40, dmichael wrote:
> >> > On 2013/07/20 18:40:35, bbudge1 wrote:
> >> > > On 2013/07/20 04:03:09, bbudge1 wrote:
> >> > > > One other thing Yuzhu pointed out to me about my old solution:
> what
> >> happens
> >> > if
> >> > > > the callback isn't run?
> >> > >
> >> > > Just to clarify this, what if the callback contains a ref to a
> >> > > Resource
> >
> > and
> >>
> >> it
> >> > > is never called? Does the helper dtor grab the lock?
> >> > That's a good question! In the code I had, obviously it does not. So
> the
> >> > question is... should it?
> >> >
> >> > I think it *almost* doesn't matter. These are almost always going to
> be
> >
> > owned
> >>
> >> by
> >> > a message loop. So even in a plugin-shutdown situation, they'll be
> run,
> >> > but
> >> the
> >> > TrackedCallback just won't do anything. They should only not run if
> the
> >
> > plugin
> >>
> >> > process is shutting down. In that case, I think most of that will
> happen
> >
> > after
> >>
> >> > the plugin is torn down. So race conditions are probably going to be
> >
> > unlikely.
> >>
> >> > But...  the Resource class inherits from non-thread-safe RefCount, and
> >> > these
> >> > callbacks could be deleted on any thread, potentially. So if more than
> >> > one
> >> > Callback exists referring to the same Resource at shut down, the
> >
> > ref-counting
> >>
> >> > could be racy. So...  I think we do have to lock in the destructor. I
> >> > also
> >> don't
> >> > like the idea of trying to be "smart" and only locking if callback_
> has
> >> > not
> >> > already been cleared... because these callbacks need to be usable on
> any
> >> thread,
> >> > I think that could be dangerous too. I'd rather do it fool-proof, even
> >> > it
> >> means
> >> > locking an extra time :-(
> >> >
> >> > What do you think, does my reasoning make sense?
> >> >
> >> > I'm updating it to always lock in the destructor and will upload
> shortl
> >
> >
> >> So far, this hasn't been a problem in my use case. But it seems possible
> >> for
> >
> > the
> >>
> >> external code to just not call the callback and then destroy it at some
> >> time
> >> that we can't control. However, locking in the destructor could cause a
> >> deadlock, though this seems unlikely.
>
I checked all cases where we use RunWhileLocked. They all pass the
resulting Callback to a MessageLoop. Your CL is the first that doesn't do
that directly, but the effect is basically the same. If the callback
doesn't get run, it's going to be destroyed in a shutdown context where we
don't already hold the lock. I think we can probably avoid that case
entirely without trying.

That said...  I did have to do some slightly wacky stuff to avoid a
deadlock situation...  see the code for details. It should be safe; I stuck
in a ThreadChecker to try to make sure. Sorry it's gotten more complicated.
Hopefully though we can keep all the nastiness contained this way.


> >
> >
> > Does it make sense to support "recursive lock" (allow acquiring the lock
> > when
> > the current thread owns the lock)?
> > I know base::Lock doesn't allow this, but it shouldn't be hard to support
> > this
> > feature on top of that.
> > With that we don't have the deadlock problem in destrcutor.
> >
> > (And maybe we can remove all EnterXXXNoLock classes, and use EnterXXX
> > everywhere?)
>
>
> There's a good discussion on some dangers of reentrant locks at
>   http://www.chromium.org/developers/lock-and-condition-variable
> under the section "Why can the holder of a Lock not reacquire it?"
>
> I agree with Justin. Through doing this work, having a well-known state
for the lock has been very helpful for tracking down bugs where we forgot
to lock or unlock. I went in to the multi-threaded proxy work wanting to
use recursive locks...  Brett talked me out of it, and with all the bugs
this policy helped me find & fix, I've become a believer in non-recursive
locks.


> >
> >
> >
> >> I agree that it would be better to keep the locking code "dumb" if
> >> possible. I
> >> thought a little bit about isolating any "smarts" in our own code but I
> >> defer
> >
> > to
> >>
> >> your judgement here.
> >
> >
> >> I'll try your new CL to make sure it still works for me. Thanks for
> doing
> >
> > this!
> >
> >
> >
> > https://codereview.chromium.org/19492014/
>

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

https://codereview.chromium.org/19492014/diff/24001/ppapi/shared_impl/proxy_l...
File ppapi/shared_impl/proxy_lock.h (right):

https://codereview.chromium.org/19492014/diff/24001/ppapi/shared_impl/proxy_l...
ppapi/shared_impl/proxy_lock.h:178: //      only ever have 1 owner).
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.

[yzshen1, 2013-07-22 21:37:57]

> > Does it make sense to support "recursive lock" (allow acquiring the lock
> > when
> > the current thread owns the lock)?
> > I know base::Lock doesn't allow this, but it shouldn't be hard to support
> > this
> > feature on top of that.
> > With that we don't have the deadlock problem in destrcutor.
> >
> > (And maybe we can remove all EnterXXXNoLock classes, and use EnterXXX
> > everywhere?)
>
>
> There's a good discussion on some dangers of reentrant locks at
>   http://www.chromium.org/developers/lock-and-condition-variable
> under the section "Why can the holder of a Lock not reacquire it?"
>
> I agree with Justin. Through doing this work, having a well-known state
> for the lock has been very helpful for tracking down bugs where we forgot
> to lock or unlock. I went in to the multi-threaded proxy work wanting to
> use recursive locks...  Brett talked me out of it, and with all the bugs
> this policy helped me find & fix, I've become a believer in non-recursive
> locks.

Thanks for pointing this out! I agree that we should probably avoid recursive
locks.

The solution in patch #3 looks correct to me. My only concern is the returned
base::Callback of RunWhileLocked() needs special attention. For example, the
following code looks innocent, but it actually will cause deadlock.
================================================================
base::Callback<...> callback_a = RunWhileLocked(callback_b);
// Do nothing about callback_a and let it go out of scope.
================================================================

Maybe adding more comments about RunWhileLocked() is sufficient.

[dmichael (off chromium), 2013-07-23 16:23:00]

Okay, so this CL is currently failing 1 unit test, because the PpapiGlobals (and
therefore its proxy lock) get destroyed before the MessageLoop for the test's
primary thread. In the unit test, I can reorder things so that the message loop
is destroyed first. But the same problem appears to exist for a real process.

I'm thinking of making it so that the proxy Lock will be purposely leaked, so
that we can use it at any time during shutdown.

[dmichael (off chromium), 2013-07-23 16:23:20]

On 2013/07/23 16:23:00, dmichael wrote:
> Okay, so this CL is currently failing 1 unit test, because the PpapiGlobals
(and
> therefore its proxy lock) get destroyed before the MessageLoop for the test's
> primary thread. In the unit test, I can reorder things so that the message
loop
> is destroyed first. But the same problem appears to exist for a real process.
> 
> I'm thinking of making it so that the proxy Lock will be purposely leaked, so
> that we can use it at any time during shutdown.

Meant to add:
Does this sound reasonable to you all?

[dmichael (off chromium), 2013-07-23 16:41:30]

On Tue, Jul 23, 2013 at 10:23 AM, <dmichael@chromium.org> wrote:

> On 2013/07/23 16:23:00, dmichael wrote:
>
>> Okay, so this CL is currently failing 1 unit test, because the
>> PpapiGlobals
>>
> (and
>
>> therefore its proxy lock) get destroyed before the MessageLoop for the
>> test's
>> primary thread. In the unit test, I can reorder things so that the message
>>
> loop
>
>> is destroyed first. But the same problem appears to exist for a real
>> process.
>>
>
>  I'm thinking of making it so that the proxy Lock will be purposely
>> leaked, so
>> that we can use it at any time during shutdown.
>>
>
> Meant to add:
> Does this sound reasonable to you all?
>
After talking to Justin, I wanted to clarify what I mean. I want to create
the base::Lock inside ProxyGlobals via "new", and never delete it.

I will annotate it so that it doesn't show up in heap checkers:
https://code.google.com/p/chromium/codesearch#chromium/src/base/debug/leak_an...

There's only 1 proxy Lock per plugin process. The OS should clean up when
the plugin process is destroyed.


>
>
https://codereview.chromium.**org/19492014/<https://codereview.chromium.org/1...
>

[teravest, 2013-07-23 16:43:53]

Sounds good to me. I double checked that there's nothing surprising in
Lock::~Lock().

On Tue, Jul 23, 2013 at 10:41 AM, David Michael <dmichael@chromium.org> wrote:
> On Tue, Jul 23, 2013 at 10:23 AM, <dmichael@chromium.org> wrote:
>>
>> On 2013/07/23 16:23:00, dmichael wrote:
>>>
>>> Okay, so this CL is currently failing 1 unit test, because the
>>> PpapiGlobals
>>
>> (and
>>>
>>> therefore its proxy lock) get destroyed before the MessageLoop for the
>>> test's
>>> primary thread. In the unit test, I can reorder things so that the
>>> message
>>
>> loop
>>>
>>> is destroyed first. But the same problem appears to exist for a real
>>> process.
>>
>>
>>> I'm thinking of making it so that the proxy Lock will be purposely
>>> leaked, so
>>> that we can use it at any time during shutdown.
>>
>>
>> Meant to add:
>> Does this sound reasonable to you all?
>
> After talking to Justin, I wanted to clarify what I mean. I want to create
> the base::Lock inside ProxyGlobals via "new", and never delete it.
>
> I will annotate it so that it doesn't show up in heap checkers:
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/debug/leak_an...
>
> There's only 1 proxy Lock per plugin process. The OS should clean up when
> the plugin process is destroyed.
>
>>
>>
>> https://codereview.chromium.org/19492014/
>
>

[bbudge, 2013-07-23 16:58:51]

On 2013/07/23 16:23:20, dmichael wrote:
> On 2013/07/23 16:23:00, dmichael wrote:
> > Okay, so this CL is currently failing 1 unit test, because the PpapiGlobals
> (and
> > therefore its proxy lock) get destroyed before the MessageLoop for the
test's
> > primary thread. In the unit test, I can reorder things so that the message
> loop
> > is destroyed first. But the same problem appears to exist for a real
process.
> > 
> > I'm thinking of making it so that the proxy Lock will be purposely leaked,
so
> > that we can use it at any time during shutdown.
> 
> Meant to add:
> Does this sound reasonable to you all?

Very reasonable. It's not a real leak, and eliminates worries about order of
destruction.

[yzshen1, 2013-07-23 17:34:02]

On 2013/07/23 16:58:51, bbudge1 wrote:
> On 2013/07/23 16:23:20, dmichael wrote:
> > On 2013/07/23 16:23:00, dmichael wrote:
> > > Okay, so this CL is currently failing 1 unit test, because the
PpapiGlobals
> > (and
> > > therefore its proxy lock) get destroyed before the MessageLoop for the
> test's
> > > primary thread. In the unit test, I can reorder things so that the message
> > loop
> > > is destroyed first. But the same problem appears to exist for a real
> process.
> > > 
> > > I'm thinking of making it so that the proxy Lock will be purposely leaked,
> so
> > > that we can use it at any time during shutdown.
> > 
> > Meant to add:
> > Does this sound reasonable to you all?
> 
> Very reasonable. It's not a real leak, and eliminates worries about order of
> destruction.

That sounds good to me, too. Thanks!

[dmichael (off chromium), 2013-07-24 16:29:43]

I decided to split out the destructor locking in to a separate CL:
https://codereview.chromium.org/19678028/
Because leaking the lock is fairly complicated. I want to land this one, which
adds RunWhileLocked, to unblock bbudge, and because it fixes a bug (the callback
gets destroyed outside the lock). And if I have to revert the more complicated
follow-up, I won't also have to revert bbudge's CL.

PTAL

[dmichael (off chromium), 2013-07-24 16:30:43]

On 2013/07/24 16:29:43, dmichael wrote:
> I decided to split out the destructor locking in to a separate CL:
> https://codereview.chromium.org/19678028/
> Because leaking the lock is fairly complicated. I want to land this one, which
> adds RunWhileLocked, to unblock bbudge, and because it fixes a bug (the
callback
> gets destroyed outside the lock). And if I have to revert the more complicated
> follow-up, I won't also have to revert bbudge's CL.
> 
> PTAL

Oops, I misspoke(typed)...

The OTHER CL is the one I want to land sooner, and I'm going to use this one as
the complicated follow-up. The new CL is here:
https://codereview.chromium.org/19678028/

And this one is not yet ready for review.

[dmichael (off chromium), 2013-07-26 20:17:16]

I think this CL is now ready. It required more messiness than I hoped for in
order to deal with unit tests. Let me know if you have ideas to make it
cleaner/safer :)

[bbudge, 2013-07-29 21:02:18]

https://codereview.chromium.org/19492014/diff/73001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_file_chooser_host_unittest.cc (right):

https://codereview.chromium.org/19492014/diff/73001/content/renderer/pepper/p...
content/renderer/pepper/pepper_file_chooser_host_unittest.cc:18: #include
"ppapi/proxy/proxy_lock.h"
s/proxy/shared_impl

https://codereview.chromium.org/19492014/diff/73001/ppapi/shared_impl/proxy_l...
File ppapi/shared_impl/proxy_lock.h (right):

https://codereview.chromium.org/19492014/diff/73001/ppapi/shared_impl/proxy_l...
ppapi/shared_impl/proxy_lock.h:35: // Return the globally unique ProxyLock.
Normally, you should not access this
s/globally unique/global ?

https://codereview.chromium.org/19492014/diff/73001/ppapi/shared_impl/proxy_l...
ppapi/shared_impl/proxy_lock.h:37: // you need access to the ProxyLock for e.g.
a condition variable.
s/ for e.g./e.g. to create

https://codereview.chromium.org/19492014/diff/73001/ppapi/shared_impl/proxy_l...
ppapi/shared_impl/proxy_lock.h:61: static void EnableLockingOnThreadForTest();
This is hard to understand. How about this?

// Enables locking on the current thread. Although locking is enabled by
default, unit tests
// that rely on the lock being enabled should *still* call this, since a
previous test may have
// disabled locking.

[yzshen1, 2013-07-29 21:18:58]

LGTM

Thanks!

[dmichael (off chromium), 2013-07-29 21:24:03]

https://codereview.chromium.org/19492014/diff/73001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_file_chooser_host_unittest.cc (right):

https://codereview.chromium.org/19492014/diff/73001/content/renderer/pepper/p...
content/renderer/pepper/pepper_file_chooser_host_unittest.cc:18: #include
"ppapi/proxy/proxy_lock.h"
On 2013/07/29 21:02:18, bbudge1 wrote:
> s/proxy/shared_impl
Oops, Done, thanks :)

https://codereview.chromium.org/19492014/diff/73001/ppapi/shared_impl/proxy_l...
File ppapi/shared_impl/proxy_lock.h (right):

https://codereview.chromium.org/19492014/diff/73001/ppapi/shared_impl/proxy_l...
ppapi/shared_impl/proxy_lock.h:35: // Return the globally unique ProxyLock.
Normally, you should not access this
On 2013/07/29 21:02:18, bbudge1 wrote:
> s/globally unique/global ?

Done.

https://codereview.chromium.org/19492014/diff/73001/ppapi/shared_impl/proxy_l...
ppapi/shared_impl/proxy_lock.h:37: // you need access to the ProxyLock for e.g.
a condition variable.
On 2013/07/29 21:02:18, bbudge1 wrote:
> s/ for e.g./e.g. to create

Done.

https://codereview.chromium.org/19492014/diff/73001/ppapi/shared_impl/proxy_l...
ppapi/shared_impl/proxy_lock.h:61: static void EnableLockingOnThreadForTest();
On 2013/07/29 21:02:18, bbudge1 wrote:
> This is hard to understand. How about this?
> 
> // Enables locking on the current thread. Although locking is enabled by
> default, unit tests
> // that rely on the lock being enabled should *still* call this, since a
> previous test may have
> // disabled locking.
Much better, thanks

[commit-bot: I haz the power, 2013-08-09 20:07:09]

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

[commit-bot: I haz the power, 2013-08-09 20:07:24]

Failed to apply patch for content/renderer/pepper/host_globals.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/renderer/pepper/host_globals.cc
  Hunk #1 succeeded at 17 (offset 1 line).
  Hunk #2 FAILED at 82.
  1 out of 3 hunks FAILED -- saving rejects to file
content/renderer/pepper/host_globals.cc.rej

Patch:       content/renderer/pepper/host_globals.cc
Index: content/renderer/pepper/host_globals.cc
diff --git a/content/renderer/pepper/host_globals.cc
b/content/renderer/pepper/host_globals.cc
index
dac943bac489f2f0125f065e41fb68ef196988e8..fcea1512201a877382afb5561fa200b0ddd52432
100644
--- a/content/renderer/pepper/host_globals.cc
+++ b/content/renderer/pepper/host_globals.cc
@@ -16,6 +16,7 @@
 #include "content/renderer/render_thread_impl.h"
 #include "ppapi/shared_impl/api_id.h"
 #include "ppapi/shared_impl/id_assignment.h"
+#include "ppapi/shared_impl/proxy_lock.h"
 #include "third_party/WebKit/public/platform/WebString.h"
 #include "third_party/WebKit/public/web/WebConsoleMessage.h"
 #include "third_party/WebKit/public/web/WebDocument.h"
@@ -81,13 +82,9 @@ HostGlobals::HostGlobals()
       resource_tracker_(ResourceTracker::SINGLE_THREADED) {
   DCHECK(!host_globals_);
   host_globals_ = this;
-}
-
-HostGlobals::HostGlobals(
-    ::ppapi::PpapiGlobals::PerThreadForTest per_thread_for_test)
-    : ::ppapi::PpapiGlobals(per_thread_for_test),
-      resource_tracker_(ResourceTracker::SINGLE_THREADED) {
-  DCHECK(!host_globals_);
+  // We do not support calls off of the main thread on the host side, and thus
+  // do not lock.
+  ::ppapi::ProxyLock::DisableLocking();
 }
 
 HostGlobals::~HostGlobals() {
@@ -141,11 +138,6 @@ void HostGlobals::PreCacheFontForFlash(const void*
logfontw) {
   // Not implemented in-process.
 }
 
-base::Lock* HostGlobals::GetProxyLock() {
-  // We do not lock on the host side.
-  return NULL;
-}
-
 void HostGlobals::LogWithSource(PP_Instance instance,
                                 PP_LogLevel level,
                                 const std::string& source,

[commit-bot: I haz the power, 2013-08-09 20:25:11]

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

[commit-bot: I haz the power, 2013-08-09 20:34:28]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-08-09 21:11:22]

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

[commit-bot: I haz the power, 2013-08-10 04:06:10]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-08-29 16:15:28]

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

[commit-bot: I haz the power, 2013-08-29 16:15:46]

Failed to apply patch for
content/renderer/pepper/pepper_file_chooser_host_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/renderer/pepper/pepper_file_chooser_host_unittest.cc
  Hunk #1 FAILED at 19.
  1 out of 2 hunks FAILED -- saving rejects to file
content/renderer/pepper/pepper_file_chooser_host_unittest.cc.rej

Patch:       content/renderer/pepper/pepper_file_chooser_host_unittest.cc
Index: content/renderer/pepper/pepper_file_chooser_host_unittest.cc
diff --git a/content/renderer/pepper/pepper_file_chooser_host_unittest.cc
b/content/renderer/pepper/pepper_file_chooser_host_unittest.cc
index
98c859820de011b3878cd69f9321064a4a4e8ea7..0afde5be3fdb652c275ef98232c15fa972bff889
100644
--- a/content/renderer/pepper/pepper_file_chooser_host_unittest.cc
+++ b/content/renderer/pepper/pepper_file_chooser_host_unittest.cc
@@ -19,6 +19,7 @@
 #include "ppapi/proxy/resource_message_test_sink.h"
 #include "ppapi/shared_impl/file_ref_create_info.h"
 #include "ppapi/shared_impl/ppapi_permissions.h"
+#include "ppapi/shared_impl/proxy_lock.h"
 #include "ppapi/shared_impl/resource_tracker.h"
 #include "ppapi/shared_impl/test_globals.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -36,6 +37,7 @@ class PepperFileChooserHostTest : public RenderViewTest {
   virtual void SetUp() {
     SetContentClient(&client_);
     RenderViewTest::SetUp();
+    ppapi::ProxyLock::DisableLockingOnThreadForTest();
 
     globals_.GetResourceTracker()->DidCreateInstance(pp_instance_);
   }

[commit-bot: I haz the power, 2013-09-11 16:27:06]

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

[commit-bot: I haz the power, 2013-09-12 02:26:39]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[dmichael (off chromium), 2013-09-12 19:09:08]

Committed patchset #14 manually as r222829 (presubmit successful).

[commit-bot: I haz the power, 2013-09-16 20:31:06]

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

[commit-bot: I haz the power, 2013-09-17 00:29:15]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-09-17 19:37:35]

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

[commit-bot: I haz the power, 2013-09-17 23:30:02]

Change committed as 223738