Make much of the proxy thread-safe with 1 great big lock.

Add global lock for Enter* classes. Add ScopedProxyLock for non-thunk proxies. Add multi-threading test for PPB_Var.

BUG=92909
TEST=None yet

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=104931

[dmichael (off chromium), 2011-09-26 21:36:53]

Next up:
- Lots and lots of testing.
- Support for blocking completion callbacks.
Note I'm NaCl sheriff this week, so this may be my last substantive CL for the
week.

[dmichael (off chromium), 2011-09-26 22:58:56]

No need to hurry on this... it looks like the Graphics2D ui_test is hanging on
Linux. Maybe a deadlock :-(

[brettw, 2011-09-28 19:58:42]

Whenever we do a blocking send we need to unlock so the browser can call us
back.

I'm guessing the deadlock is in the testing interface running the nested message
loop. We'll also have to manage reentrancy and unlocking for the old-style
scripting interface.

I think we should have an ifdef for this so the lock is disabled by default, but
then we can enable it as-needed for work. That way we can incrementally do
changes.

[dmichael(do not use this one), 2011-09-28 20:20:40]

On Wed, Sep 28, 2011 at 1:58 PM, <brettw@chromium.org> wrote:

> Whenever we do a blocking send we need to unlock so the browser can call us
> back.
>
> I'm guessing the deadlock is in the testing interface running the nested
> message
> loop. We'll also have to manage reentrancy and unlocking for the old-style
> scripting interface.
>
It does appear to be reentrancy in the browser side of the proxy. I was
considering just making the locking be no-ops on the host side (make the
plugin dispatcher inject the lock at initialization, instead of the lock
always being there for both sides). That would fix my immediate problem, but
scripting at least could still get deadlock.

Do you have any recommendations? I don't think there's an easy way to do a
recursive lock in base/synchronization. I could add that, maybe. Just
unlocking on reentry to the plugin doesn't seem ideal, because there's no
guarantee the reentrant thread will be the next one to grab the lock.

I think we should have an ifdef for this so the lock is disabled by default,
> but
> then we can enable it as-needed for work. That way we can incrementally do
> changes.

Good idea. I'll add that to the CL.

>
>
>
http://codereview.chromium.**org/8016008/<http://codereview.chromium.org/8016...
>

[brettw, 2011-09-28 20:24:00]

On Wed, Sep 28, 2011 at 1:20 PM, David Michael <dmichael@google.com> wrote:
>
> On Wed, Sep 28, 2011 at 1:58 PM, <brettw@chromium.org> wrote:
>>
>> Whenever we do a blocking send we need to unlock so the browser can call
>> us
>> back.
>>
>> I'm guessing the deadlock is in the testing interface running the nested
>> message
>> loop. We'll also have to manage reentrancy and unlocking for the old-style
>> scripting interface.
>
> It does appear to be reentrancy in the browser side of the proxy. I was
> considering just making the locking be no-ops on the host side (make the
> plugin dispatcher inject the lock at initialization, instead of the lock
> always being there for both sides). That would fix my immediate problem, but
> scripting at least could still get deadlock.

I think the browser process should not be locking. Since it will be
only one thread anyway, it will just slow things down, and it will
cause problems like this. Maybe we have some lock object that in the
browser process it's set up to do nothing but in the plugin it's set
up to do everything we need.

> Do you have any recommendations? I don't think there's an easy way to do a
> recursive lock in base/synchronization. I could add that, maybe. Just
> unlocking on reentry to the plugin doesn't seem ideal, because there's no
> guarantee the reentrant thread will be the next one to grab the lock.

I don't think we want to add recursive locks.

Brett

[dmichael (off chromium), 2011-09-28 20:39:25]

On Wed, Sep 28, 2011 at 2:23 PM, Brett Wilson <brettw@chromium.org> wrote:

> On Wed, Sep 28, 2011 at 1:20 PM, David Michael <dmichael@google.com>
> wrote:
> >
> > On Wed, Sep 28, 2011 at 1:58 PM, <brettw@chromium.org> wrote:
> >>
> >> Whenever we do a blocking send we need to unlock so the browser can call
> >> us
> >> back.
> >>
> >> I'm guessing the deadlock is in the testing interface running the nested
> >> message
> >> loop. We'll also have to manage reentrancy and unlocking for the
> old-style
> >> scripting interface.
> >
> > It does appear to be reentrancy in the browser side of the proxy. I was
> > considering just making the locking be no-ops on the host side (make the
> > plugin dispatcher inject the lock at initialization, instead of the lock
> > always being there for both sides). That would fix my immediate problem,
> but
> > scripting at least could still get deadlock.
>
> I think the browser process should not be locking. Since it will be
> only one thread anyway, it will just slow things down, and it will
> cause problems like this. Maybe we have some lock object that in the
> browser process it's set up to do nothing but in the plugin it's set
> up to do everything we need.
>
Yes, that's what I was trying to describe. The locking will be a no-op by
default. The plugin dispatcher will set the global lock up initially
(something similar to how the tracker is set up now).

>
> > Do you have any recommendations? I don't think there's an easy way to do
> a
> > recursive lock in base/synchronization. I could add that, maybe. Just
> > unlocking on reentry to the plugin doesn't seem ideal, because there's no
> > guarantee the reentrant thread will be the next one to grab the lock.
>
> I don't think we want to add recursive locks.
>
Then I probably need to do something to check whether the lock is held by
the current thread, so that in some cases I can skip acquiring it. Unlocking
doesn't seem like an option.

>
> Brett
>

[brettw, 2011-09-28 21:10:55]

On Wed, Sep 28, 2011 at 1:39 PM, David Michael <dmichael@chromium.org> wrote:
>
> On Wed, Sep 28, 2011 at 2:23 PM, Brett Wilson <brettw@chromium.org> wrote:
>>
>> On Wed, Sep 28, 2011 at 1:20 PM, David Michael <dmichael@google.com>
>> wrote:
>> >
>> > On Wed, Sep 28, 2011 at 1:58 PM, <brettw@chromium.org> wrote:
>> >>
>> >> Whenever we do a blocking send we need to unlock so the browser can
>> >> call
>> >> us
>> >> back.
>> >>
>> >> I'm guessing the deadlock is in the testing interface running the
>> >> nested
>> >> message
>> >> loop. We'll also have to manage reentrancy and unlocking for the
>> >> old-style
>> >> scripting interface.
>> >
>> > It does appear to be reentrancy in the browser side of the proxy. I was
>> > considering just making the locking be no-ops on the host side (make the
>> > plugin dispatcher inject the lock at initialization, instead of the lock
>> > always being there for both sides). That would fix my immediate problem,
>> > but
>> > scripting at least could still get deadlock.
>>
>> I think the browser process should not be locking. Since it will be
>> only one thread anyway, it will just slow things down, and it will
>> cause problems like this. Maybe we have some lock object that in the
>> browser process it's set up to do nothing but in the plugin it's set
>> up to do everything we need.
>
> Yes, that's what I was trying to describe. The locking will be a no-op by
> default. The plugin dispatcher will set the global lock up initially
> (something similar to how the tracker is set up now).
>>
>> > Do you have any recommendations? I don't think there's an easy way to do
>> > a
>> > recursive lock in base/synchronization. I could add that, maybe. Just
>> > unlocking on reentry to the plugin doesn't seem ideal, because there's
>> > no
>> > guarantee the reentrant thread will be the next one to grab the lock.
>>
>> I don't think we want to add recursive locks.
>
> Then I probably need to do something to check whether the lock is held by
> the current thread, so that in some cases I can skip acquiring it. Unlocking
> doesn't seem like an option.

We should be able to unlock. Pepper shouldn't be recursively calling
into the plugin except in very rare cases, and in those cases we
should be unlocking in every case.

I was thinking of having two send functions for sending IPC messages.
The one we have now which would be for async messages only (we can
inspect the message and assert that this is the case). And a sync one
which does the unlocking followed by a blocking send, followed by a
lock-re-acquire.

We could have only one function know to unlock when we send a sync
message, but I want to make it more explicit. When such a message
returns the code needs to be careful to make sure the resources are
still valid, etc. that it may have on the stack. So I think a more
scary function name is in order.

Brett

[dmichael (off chromium), 2011-10-06 16:01:48]

Okay, I've made the following changes:
- Locking only happens in the plugin side of the proxy.
- The locking and related Var test are off by default.
- Turn on with the 'enable_pepper_threading' gyp flag. (I can add instructions
to the 'pepper plugin implementation' page when this lands).

I have not yet worried about unlocking for sync calls, to keep the CL sane.

PTAL

[brettw, 2011-10-07 00:01:58]

Keep in mind there will be multiple dispatchers in the plugin process, one for
each renderer that plugin is talking to (in NaCl this is currently 1:1, but this
will be different for native plugins).

I'd suggest that rather than have the global lock in the dispatcher, add a
virtual getter to the TrackerBase in ppapi/shared_impl, and implement that for
the plugin.

The TrackerBase is really the global singleton that exists in every process we
have pepper. I've been hoping to clean this up a bit but haven't gotten that
far.

Actually, I wonder if the tracker base shouldn't just have a lock and an unlock
function. In the webkit/plugins/ppapi implementation of it they can be a NOP
since we don't support threadsafety. Then in the plugin side we can actually
implement this in terms of a real lock.

[dmichael (off chromium), 2011-10-07 22:17:04]

On 2011/10/07 00:01:58, brettw wrote:
> Keep in mind there will be multiple dispatchers in the plugin process, one for
> each renderer that plugin is talking to (in NaCl this is currently 1:1, but
this
> will be different for native plugins).
Ouch, thanks, I don't think I realized the trusted plugins would run in the same
process. We don't test that currently so far as I know (unless Flash and PDF
both work this way in ChromeOS right now), so I wouldn't be surprised if there
are other problems.

So...  this makes me even more concerned about having one big global lock, since
now this means the contention is spread across multiple plugins, instead of just
multiple threads in one plugin.

And I was realizing that we need to lock some Browser->Plugin calls, e.g.
passing a Resource or Var that the plugin hasn't seen will access the tracker.
But meanwhile, a background thread could be trying to add one. So it kind of
seems like we might need finer-grained locking anyway, to guard the trackers.
Trying to reason about when to lock a global lock might actually be more
confusing.

How would you feel about it if I backed off the global lock thing and tried the
finer-grained approach? I think we'll have a lock per plugin-side tracker and
likely one in the plugin dispatcher. More may come up, but I think we won't need
a lock-per-Var, or anything like that. The global lock has my spidey-sense
tingling, and I think I'm likely to regret it if I go too far down that path.

[brettw, 2011-10-07 22:39:17]

You misunderstood a bit.

There is one plugin process for each native plugin DLL. Pepper Flash does this,
or else we'd have one Flash process for each renderer (since so many pages use
Flash) and resource consumption would be insane.

Different plugin types (e.g. Flash vs. PDF) run in separate processes (PDF is
currently in process so this isn't actually true, but this will be changed at
some point).

The global lock would be acquired every time we enter from the plugin or from a
message from the browser. If there's a reentrancy possibility (we're sending a
blocking message from the plugin to the browser), we'd need to unlock. If we
call into the plugin, we temporarily unlock for that call.

I guess I still don't see the problem with this. The code we would have the lock
around is 98% boilerplate bookkeeping code and is fast. It doesn't seem to me
that there would be a perf problem outside of the OpenGL interface (which we'll
have to do something special with anyway). And it seems that life is pretty
simple since you can easily describe the locking state. Using per-resource locks
seems like it still has the same complexity (reentrant possibilities and
unlocking when we call into the plugin) and also has the possibility of deadlock
if you ever have resources talking to each other (think about the URLRequest and
the URLResponseInfo it's associated with, if each had a separate lock, you can
easily deadlock).

I think we also need to protect ourselves to prevent a resource or instance from
being deleted out from under us. I don't think the Pepper interface should crash
if you mess up your refcounting. Especially at shutdown when the plugin instance
and the associated resources are unceremoniously deleted whether the plugin
wants it or not, it doesn't necessarily know on the background thread that those
resource/instance IDs are invalid and will likely continue using them. We need
to behave reasonably in this case.

You're certainly welcome to experiment with other approaches if you feel it
would be superior.

[dmichael (off chromium), 2011-10-08 02:46:16]

On Fri, Oct 7, 2011 at 4:39 PM, <brettw@chromium.org> wrote:

> You misunderstood a bit.
>
> There is one plugin process for each native plugin DLL. Pepper Flash does
> this,
> or else we'd have one Flash process for each renderer (since so many pages
> use
> Flash) and resource consumption would be insane.
>
Okay, got it now. That makes a lot more sense; I didn't read carefully
enough. So much still to learn!

>
> Different plugin types (e.g. Flash vs. PDF) run in separate processes (PDF
> is
> currently in process so this isn't actually true, but this will be changed
> at
> some point).
>
> The global lock would be acquired every time we enter from the plugin or
> from a
> message from the browser. If there's a reentrancy possibility (we're
> sending a

blocking message from the plugin to the browser), we'd need to unlock. If we
> call into the plugin, we temporarily unlock for that call.
>
I think I finally have the right mental model for what you're talking about.
I don't think I had the understanding that we would always be locked when
the proxy code is active, and always unlocked otherwise (with the exception
of async calls which are quick). My natural inclination is to lock at a
finer granularity, and it was easier for me to think about that way. E.g.,
the VarTracker has a lock. When you access the VarTracker to do something
(e.g. adding a new var to the map), it's locked during that operation and
unlocked at the end. It doesn't matter whether the browser called it or the
plugin called it; it just works. The finer granularity is simpler in *some*ways.

>
> I guess I still don't see the problem with this. The code we would have the
> lock
> around is 98% boilerplate bookkeeping code and is fast.

True, most of it should be pretty fast. But not all. E.g., creating a string
Var means allocating a (potentially large) buffer and copying to it. We also
should be doing a check for UTF validity (though I don't think the proxy is
doing it yet). Once we support dictionaries and arrays, we'll be doing
structured clone while holding the lock. We might still get acceptable
performance, but it seems strange to me to block another thread that might
be, e.g., querying an input event, if there's not a very good technical
reason for it.

> It doesn't seem to me
> that there would be a perf problem outside of the OpenGL interface (which
> we'll
> have to do something special with anyway). And it seems that life is pretty
> simple since you can easily describe the locking state.

Now that I finally think I 'get it', it is pretty simple. It was just kind
of counter-intuitive to me.

> Using per-resource locks
>
I think I'm still undecided as to whether that's necessary. We could put the
onus on the end-developer, so they only lock if they know they're sharing
the resource across threads. Also, some resources that are immutable (e.g.
input events) wouldn't really need to be locked at all (though the trackers
would be locked for reference counting individual resources).

> seems like it still has the same complexity (reentrant possibilities and
> unlocking when we call into the plugin)

True. We likely still need a lock in the plugin dispatcher, and if so, it
would definitely need to unlock when calling in to the plugin. Something
like AutoUnlock.

> and also has the possibility of deadlock
> if you ever have resources talking to each other (think about the
> URLRequest and
> the URLResponseInfo it's associated with, if each had a separate lock, you
> can
> easily deadlock).
>
I'm not sure how? I'm imagining locking only during the context of a given
call, e.g. PPB_URLRequest.GetResponseInfo would lock internally, but unlock
when it exits scope. Similarly, PPB_URLResponseInfo.GetProperty would lock
only in the context of the impl code. Any time user code is active, the lock
is not held. I don't think this is any more prone to deadlock than locking
all* *calls in to proxy code with 1 lock. I think that in any case where my
design would lock A then B, the single lock design would lock A then A, and
it's already hosed.

>
> I think we also need to protect ourselves to prevent a resource or instance
> from
> being deleted out from under us. I don't think the Pepper interface should
> crash
> if you mess up your refcounting. Especially at shutdown when the plugin
> instance
> and the associated resources are unceremoniously deleted whether the plugin
> wants it or not, it doesn't necessarily know on the background thread that
> those
> resource/instance IDs are invalid and will likely continue using them. We
> need
> to behave reasonably in this case.
>
Hmm, good point. It's still something of a problem even in the single-lock
case, I think. Really, it's going to have to happen in EnterResource, which
will have to lock to enter the ResourceTracker in either case. I'm not sure
if it will end up easier in the single-lock case or not.

>
> You're certainly welcome to experiment with other approaches if you feel it
> would be superior.
>
My instinct says it's the right thing to do. If I'm wrong, I'm sure I'll
learn a lot :-)

>
>
http://codereview.chromium.**org/8016008/<http://codereview.chromium.org/8016...
>

[brettw, 2011-10-08 05:13:19]

Many plugins create many hundreds of resources. This will equate to many
hundreds of locks, which has performance problems on its own.

Many small locks will be slower for many cases than one big one right now.
Implementing something that uses vars like PPB_Var_Deprecated::GetProperty would
lock and unlock five times by my count. Each of these times has a performance
penalty.

If we have any expensive operations that are causing thread contention we can
optimize later and have a separate lock for those specific classes where we
unlock the "big lock" while doing those operations.

The deadlock point I was making above is when two objects reference each other.
There are a lot of ways this can happen. For example, if the instance deals with
a resource, presumably it will be doing so while holding the instance object's
lock. If it calls into that resource, presumably that resource would lock with a
separate lock. But there are a lot of things that the resource could do that
would directly or indirectly call back into the instance. This would attempt to
lock again, resulting in deadlock.

I do not think jerry-rigging a system to support recursive locking is a good
idea. Chrome team has explicitly chosen to disallow this when it's come up
before. We've even added code to assert if you try. Please talk to jar@ if you
want to discuss more.

I do not want to debug such a system with hundreds of locks floating around.

I don't think you can implement the requirement that you don't crash if a
resource is deleted on another thread without either one big lock or recursive
locks. Resources commonly ask the resource tracker to do stuff.

I'm not going to be around when you implement this, but I think the small lock
approach is really a bad idea.

[dmichael (off chromium), 2011-10-09 02:13:23]

On Fri, Oct 7, 2011 at 11:13 PM, <brettw@chromium.org> wrote:

> Many plugins create many hundreds of resources. This will equate to many
> hundreds of locks, which has performance problems on its own.
>
> Many small locks will be slower for many cases than one big one right now.
> Implementing something that uses vars like PPB_Var_Deprecated::**GetProperty
> would
> lock and unlock five times by my count. Each of these times has a
> performance
> penalty.
>
> If we have any expensive operations that are causing thread contention we
> can
> optimize later and have a separate lock for those specific classes where we
> unlock the "big lock" while doing those operations.
>
> The deadlock point I was making above is when two objects reference each
> other.
> There are a lot of ways this can happen. For example, if the instance deals
> with
> a resource, presumably it will be doing so while holding the instance
> object's
> lock. If it calls into that resource, presumably that resource would lock
> with a
> separate lock. But there are a lot of things that the resource could do
> that
> would directly or indirectly call back into the instance. This would
> attempt to
> lock again, resulting in deadlock.
>
> I do not think jerry-rigging a system to support recursive locking is a
> good
> idea. Chrome team has explicitly chosen to disallow this when it's come up
> before. We've even added code to assert if you try. Please talk to jar@ if
> you
> want to discuss more.
>
> I do not want to debug such a system with hundreds of locks floating
> around.


> I don't think you can implement the requirement that you don't crash if a
> resource is deleted on another thread without either one big lock or
> recursive
> locks. Resources commonly ask the resource tracker to do stuff.


> I'm not going to be around when you implement this, but I think the small
> lock
> approach is really a bad idea.
>
I've thought about it more since yesterday. I don't think I fully understood
your approach before. Now I'm more confident it will do the job and that I
understand it well enough to implement it. It's definitely more
straightforward and maintainable, which is really important for
multi-threaded stuff, so I'm sold. Premature optimization, and all that.
I'll clean up this CL early this coming week to use a truly global lock.
Unlocking sync calls and calls to the plugin should probably wait for later
CLs.

>
>
http://codereview.chromium.**org/8016008/<http://codereview.chromium.org/8016...
>

[brettw, 2011-10-09 03:33:28]

Sweet :) You plan sounds good to me.

[dmichael (off chromium), 2011-10-10 18:10:14]

I used the ProxyLock class I already defined and just moved setting the lock in
to PluginResourceTracker. I think the ProxyLock class itself and the use of it
at call-sites is pretty clear this way. If you'd rather I put virtual functions
in TrackerBase, I can do that. I just didn't see a clear advantage (except maybe
having all global stuff in one place?)

[brettw, 2011-10-11 18:25:09]

LGTM

http://codereview.chromium.org/8016008/diff/35002/ppapi/thunk/enter.h
File ppapi/thunk/enter.h (right):

http://codereview.chromium.org/8016008/diff/35002/ppapi/thunk/enter.h#newcode56
ppapi/thunk/enter.h:56: // TODO(brettw) assert the lock is held.
Nice try :)
brettw -> dmichael

http://codereview.chromium.org/8016008/diff/35002/ppapi/thunk/enter.h#newcode173
ppapi/thunk/enter.h:173: class PPAPI_THUNK_EXPORT EnterInstance
For future reference, I think some of the places this is being used is wrong (in
case you run into problems). I think we'll need to make a new
EnterInstanceNoLock and have some callers use.