Never lock the Pepper proxy lock on the IO thread

Don't lock ProxyLock on the IO thread

Introduce ResourceMessageFilter and UDPSocketFilter to receive messages on the IO thread. This allows us to queue messages up when there's no callback, and dispatch them directly to the appropriate thread when there is a callback pending. (This depends on TrackedCallback being Run()-able without the ProxyLock; see https://codereview.chromium.org/923263003/)

BUG=439588
Committed: https://crrev.com/b11ca7b7dac766989002bede109c81a3f8a8bc89
Cr-Commit-Position: refs/heads/master@{#323517}

[dmichael (off chromium), 2015-02-24 22:08:17]

Patchset #11 (id:200001) has been deleted

[dmichael (off chromium), 2015-02-27 22:40:42]

Patchset #1 (id:1) has been deleted

[dmichael (off chromium), 2015-02-27 22:40:50]

Patchset #1 (id:20001) has been deleted

[dmichael (off chromium), 2015-02-27 22:41:02]

Patchset #1 (id:40001) has been deleted

[dmichael (off chromium), 2015-02-27 22:41:09]

Patchset #1 (id:60001) has been deleted

[dmichael (off chromium), 2015-02-27 22:41:17]

Patchset #1 (id:80001) has been deleted

[dmichael (off chromium), 2015-02-27 22:41:27]

Patchset #1 (id:100001) has been deleted

[dmichael (off chromium), 2015-02-27 22:41:36]

Patchset #1 (id:120001) has been deleted

[dmichael (off chromium), 2015-02-27 22:41:46]

Patchset #1 (id:140001) has been deleted

[dmichael (off chromium), 2015-02-27 22:42:01]

Patchset #1 (id:160001) has been deleted

[dmichael (off chromium), 2015-02-27 22:42:09]

Patchset #1 (id:180001) has been deleted

[dmichael (off chromium), 2015-02-27 22:42:16]

Patchset #1 (id:220001) has been deleted

[dmichael (off chromium), 2015-02-27 22:42:25]

Patchset #1 (id:240001) has been deleted

[dmichael (off chromium), 2015-02-27 23:53:24]

dmichael@chromium.org changed reviewers:
+ yzshen@chromium.org

[dmichael (off chromium), 2015-02-27 23:53:25]

Yuzhu, do you have time or desire to look at this? If not, I can ask raymes or
bbudge

[yzshen1, 2015-02-27 23:59:12]

Hi, David.

I am going to take a three-week leave starting from next Monday. (My
remaining paternity leave.)

If you don't mind, you could assign it to either raymes or bill, and I
could take a look and maybe add some drive-by comments when I have got a
chance. But there is no need to block on my feedback.

Thanks for working on this issue!

On Fri, Feb 27, 2015 at 3:53 PM, <dmichael@chromium.org> wrote:

> Reviewers: yzshen1,
>
> Message:
> Yuzhu, do you have time or desire to look at this? If not, I can ask
> raymes or
> bbudge
>
> Description:
> Don't lock ProxyLock on the IO thread
>
> Introduce ResourceMessageFilter and UDPSocketFilter to receive messages on
> the
> IO thread. This allows us to queue messages up when there's no callback,
> and
> dispatch them directly to the appropriate thread when there is a callback
> pending. (This depends on TrackedCallback being Run()-able without the
> ProxyLock; see https://codereview.chromium.org/923263003/)
>
> BUG=439588
>
> Please review this at https://codereview.chromium.org/869883003/
>
> Base URL: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+530, -225 lines):
>   M content/browser/renderer_host/pepper/pepper_udp_socket_
> message_filter.cc
>   M content/ppapi_plugin/ppapi_thread.cc
>   M ppapi/nacl_irt/ppapi_dispatcher.cc
>   M ppapi/ppapi_proxy.gypi
>   M ppapi/proxy/plugin_globals.h
>   M ppapi/proxy/plugin_globals.cc
>   M ppapi/proxy/plugin_message_filter.h
>   M ppapi/proxy/plugin_message_filter.cc
>   A ppapi/proxy/resource_message_filter.h
>   M ppapi/proxy/resource_reply_thread_registrar.h
>   M ppapi/proxy/resource_reply_thread_registrar.cc
>   A ppapi/proxy/udp_socket_filter.h
>   A ppapi/proxy/udp_socket_filter.cc
>   M ppapi/proxy/udp_socket_resource_base.h
>   M ppapi/proxy/udp_socket_resource_base.cc
>   M ppapi/shared_impl/callback_tracker.cc
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmichael (off chromium), 2015-03-02 21:10:58]

dmichael@chromium.org changed reviewers:
+ raymes@chromium.org
- yzshen@chromium.org

[dmichael (off chromium), 2015-03-02 21:10:59]

raymes@, do you have time for this review? Kind of a thorny one. bbudge is
reviewing the precursor to this (make TrackedCallback threadsafe enough to be
able to call "Run" from the IO thread without the proxy lock.)

[raymes, 2015-03-03 03:07:44]

Hey Dave! I started to look at this. Not sure if I'm the best person to review
it (I don't know much of this code) but will continue to look if there's no one
else it might just take me a little while :)

https://codereview.chromium.org/869883003/diff/300001/content/ppapi_plugin/pp...
File content/ppapi_plugin/ppapi_thread.cc (right):

https://codereview.chromium.org/869883003/diff/300001/content/ppapi_plugin/pp...
content/ppapi_plugin/ppapi_thread.cc:120:
globals->RegisterResourceMessageFilters(plugin_filter.get());
I guess both these add references?

https://codereview.chromium.org/869883003/diff/300001/ppapi/proxy/plugin_mess...
File ppapi/proxy/plugin_message_filter.cc (left):

https://codereview.chromium.org/869883003/diff/300001/ppapi/proxy/plugin_mess...
ppapi/proxy/plugin_message_filter.cc:90: DispatchResourceReply(reply_params,
nested_msg);
Why can we remove this check?

[dmichael (off chromium), 2015-03-03 18:27:57]

Thanks, Raymes!

I know Bill spent some time learning about the issues I'm trying to tackle, so I
could punt it over to him...  I was just trying to spread the load a little bit
since I already stuck him with the similarly thorny TrackedCallback change.

CC bbudge

https://codereview.chromium.org/869883003/diff/300001/content/ppapi_plugin/pp...
File content/ppapi_plugin/ppapi_thread.cc (right):

https://codereview.chromium.org/869883003/diff/300001/content/ppapi_plugin/pp...
content/ppapi_plugin/ppapi_thread.cc:120:
globals->RegisterResourceMessageFilters(plugin_filter.get());
On 2015/03/03 03:07:44, raymes wrote:
> I guess both these add references?
This is a little confusing, and I struggled with how to organize this and name
stuff.

As it's organized in this patch, PluginGlobals knows about any special
plugin-side filters that need to be able to listen on the IO thread. In this
case, that's only UDPSocketFilter. There needs to be exactly one instance of
UDPSocketFilter, and it must be accessible from two places:
- UDPSocketResourceBase (for adding/removing itself and draining its message
queue)
- PluginMessageFilter has to be able to pass messages to it.

I chose to make PluginMessageFilter have no special knowledge about what
particular plugin filters there are. Instead, it asks PluginGlobals to add all
the filters that PluginGlobals knows about (see the implementation of
RegisterResourceMessageFilters).

So effectively, this is adding UDPSocketFilter to PluginMessageFilter's list of
ResourceMessageFilters.

PluginGlobals will not have a reference on PluginMessageFilter. On the other
hand, PluginMessageFilter and PluginGlobals *will* both have a reference on
UDPSocketFilter (maybe that's what you meant)

https://codereview.chromium.org/869883003/diff/300001/ppapi/proxy/plugin_mess...
File ppapi/proxy/plugin_message_filter.cc (left):

https://codereview.chromium.org/869883003/diff/300001/ppapi/proxy/plugin_mess...
ppapi/proxy/plugin_message_filter.cc:90: DispatchResourceReply(reply_params,
nested_msg);
On 2015/03/03 03:07:44, raymes wrote:
> Why can we remove this check?
This was a special case in Yuzhu's version to allow handling an event on the IO
thread. I'm removing that capability in this CL, because I don't want us to ever
acquire the ProxyLock on the IO thread (see DispatchResourceReply below).

Also in this change: 
https://codereview.chromium.org/869883003/diff/300001/ppapi/proxy/resource_re...

[raymes, 2015-03-09 07:43:01]

Hey David, thanks for the reply! Sorry I'm really slow on this. I'm starting to
understand more though. I really just need to review the filter code in more
detail.

It's unfortunate to add the extra complexity but I looked at the bug and it
looks like there isn't much alternative :(

https://codereview.chromium.org/869883003/diff/300001/ppapi/proxy/udp_socket_...
File ppapi/proxy/udp_socket_resource_base.cc (right):

https://codereview.chromium.org/869883003/diff/300001/ppapi/proxy/udp_socket_...
ppapi/proxy/udp_socket_resource_base.cc:53:
base::Bind(&UDPSocketResourceBase::SlotBecameAvailable, this));
Is this meant to be SlotBecameAvailableOnIOThread?

[raymes, 2015-03-10 02:53:15]

Hey David - a more general question about the approach. What if we immediately
posted the data received on the IO thread to the thread where the callback is
due to run and did all of the processing there. Would that simplify any of this
code? We could assume the ProxyLock was being held then, is that correct?

https://codereview.chromium.org/869883003/diff/300001/ppapi/proxy/udp_socket_...
File ppapi/proxy/udp_socket_filter.cc (right):

https://codereview.chromium.org/869883003/diff/300001/ppapi/proxy/udp_socket_...
ppapi/proxy/udp_socket_filter.cc:67: const base::Closure&
slot_available_callback) {
Should we assert the proxy lock here too?

https://codereview.chromium.org/869883003/diff/300001/ppapi/proxy/udp_socket_...
ppapi/proxy/udp_socket_filter.cc:71: instance, private_api,
slot_available_callback)));
nit: indentation

https://codereview.chromium.org/869883003/diff/300001/ppapi/proxy/udp_socket_...
ppapi/proxy/udp_socket_filter.cc:74: void
UDPSocketFilter::RemoveUDPResource(PP_Resource resource) {
Should we assert the proxy lock here too?

https://codereview.chromium.org/869883003/diff/300001/ppapi/proxy/udp_socket_...
ppapi/proxy/udp_socket_filter.cc:85: const scoped_refptr<TrackedCallback>&
callback) {
Should we assert the proxy lock here too?

https://codereview.chromium.org/869883003/diff/300001/ppapi/proxy/udp_socket_...
ppapi/proxy/udp_socket_filter.cc:117: const PP_NetAddress_Private& addr) {
Is it possible to add an assertion that we're on the IOThread here?

https://codereview.chromium.org/869883003/diff/300001/ppapi/proxy/udp_socket_...
ppapi/proxy/udp_socket_filter.cc:150: const PP_NetAddress_Private& addr) {
Is it possible to add an assertion that we're on the IOThread here?

https://codereview.chromium.org/869883003/diff/300001/ppapi/proxy/udp_socket_...
File ppapi/proxy/udp_socket_filter.h (right):

https://codereview.chromium.org/869883003/diff/300001/ppapi/proxy/udp_socket_...
ppapi/proxy/udp_socket_filter.h:124: mutable base::Lock lock_;
Maybe make a note that this protects queues_?

https://codereview.chromium.org/869883003/diff/300001/ppapi/proxy/udp_socket_...
File ppapi/proxy/udp_socket_resource_base.cc (right):

https://codereview.chromium.org/869883003/diff/300001/ppapi/proxy/udp_socket_...
ppapi/proxy/udp_socket_resource_base.cc:250: void
UDPSocketResourceBase::SlotBecameAvailableOnIOThread() {
Is it possible to add an assertion that we're on the IOThread here?

[dmichael (off chromium), 2015-03-20 23:06:07]

On 2015/03/10 02:53:15, raymes wrote:
> Hey David - a more general question about the approach. What if we immediately
> posted the data received on the IO thread to the thread where the callback is
> due to run and did all of the processing there. Would that simplify any of
this
> code? We could assume the ProxyLock was being held then, is that correct?
Hi Raymes, thanks for looking, sorry for being slow. I'm still working on your
review comments (I have to plumb a way through to get the IO thread task runner
when I don't have the proxy lock, but it seems worth doing). But I wanted to
address this question before the weekend...

I thought about the approach you mentioned. The big problem is we don't
necessarily know what thread the callback will run on. We don't even always have
a callback :). The data is streaming in as it arrives, and may arrive before
there's a callback registered to receive it. We could consider doing a heuristic
like "use the thread of the last callback we saw", but we still have a
bootstrapping problem, and we could also guess wrong (and then we have to make
sure that the order of the messages is still preserved). So the complexity of
the approach in this CL seemed appropriate to have a robust solution.

[raymes, 2015-03-22 23:55:58]

On 2015/03/20 23:06:07, dmichael wrote:
> On 2015/03/10 02:53:15, raymes wrote:
> > Hey David - a more general question about the approach. What if we
immediately
> > posted the data received on the IO thread to the thread where the callback
is
> > due to run and did all of the processing there. Would that simplify any of
> this
> > code? We could assume the ProxyLock was being held then, is that correct?
> Hi Raymes, thanks for looking, sorry for being slow. I'm still working on your
> review comments (I have to plumb a way through to get the IO thread task
runner
> when I don't have the proxy lock, but it seems worth doing). But I wanted to
> address this question before the weekend...
> 
> I thought about the approach you mentioned. The big problem is we don't
> necessarily know what thread the callback will run on. We don't even always
have
> a callback :). The data is streaming in as it arrives, and may arrive before
> there's a callback registered to receive it. We could consider doing a
heuristic
> like "use the thread of the last callback we saw", but we still have a
> bootstrapping problem, and we could also guess wrong (and then we have to make
> sure that the order of the messages is still preserved). So the complexity of
> the approach in this CL seemed appropriate to have a robust solution.

Thanks! I guess I'm still curious if we can find a more generic way to do this
that would remove most of the UDP socket-specific logic that occurs on the IO
thread and apply more broadly if there was something else that had to do
something similar. What I mean is, could we queue up IPC::Messages on the IO
thread until it is known what thread to dispatch them on (e.g. when a callback
is registered) and then dispatch them to the correct thread? The UDP socket
filter would turn into something more generic. The API might look like the
consumer setting a thread to post the data to, when it is known and then the
next time data comes in the IPC is posted to that thread, or alternatively if
the message is already available it could be returned through the API directly.
This would allow us to do all the resource-specific logic under the proxy lock.

If this sounds like it would be too much work or not worth it or you've already
considered it, please let me know! :)

[dmichael (off chromium), 2015-03-23 17:58:14]

On 2015/03/22 23:55:58, raymes wrote:
> On 2015/03/20 23:06:07, dmichael wrote:
> > On 2015/03/10 02:53:15, raymes wrote:
> > > Hey David - a more general question about the approach. What if we
> immediately
> > > posted the data received on the IO thread to the thread where the callback
> is
> > > due to run and did all of the processing there. Would that simplify any of
> > this
> > > code? We could assume the ProxyLock was being held then, is that correct?
> > Hi Raymes, thanks for looking, sorry for being slow. I'm still working on
your
> > review comments (I have to plumb a way through to get the IO thread task
> runner
> > when I don't have the proxy lock, but it seems worth doing). But I wanted to
> > address this question before the weekend...
> > 
> > I thought about the approach you mentioned. The big problem is we don't
> > necessarily know what thread the callback will run on. We don't even always
> have
> > a callback :). The data is streaming in as it arrives, and may arrive before
> > there's a callback registered to receive it. We could consider doing a
> heuristic
> > like "use the thread of the last callback we saw", but we still have a
> > bootstrapping problem, and we could also guess wrong (and then we have to
make
> > sure that the order of the messages is still preserved). So the complexity
of
> > the approach in this CL seemed appropriate to have a robust solution.
> 
> Thanks! I guess I'm still curious if we can find a more generic way to do this
> that would remove most of the UDP socket-specific logic that occurs on the IO
> thread and apply more broadly if there was something else that had to do
> something similar. What I mean is, could we queue up IPC::Messages on the IO
> thread until it is known what thread to dispatch them on (e.g. when a callback
> is registered) and then dispatch them to the correct thread? The UDP socket
> filter would turn into something more generic. The API might look like the
> consumer setting a thread to post the data to, when it is known and then the
> next time data comes in the IPC is posted to that thread, or alternatively if
> the message is already available it could be returned through the API
directly.
> This would allow us to do all the resource-specific logic under the proxy
lock.
> 
> If this sounds like it would be too much work or not worth it or you've
already
> considered it, please let me know! :)
I like the idea in a way; it would be nice to be general. I tried to make this
so it wouldn't be too hard to add a different kind of filter if we wanted, but
it would be nice if we could add something new without mucking about with a new
Filter type.

Unfortunately, there is some special stuff that happens here, like only sending
a certain number of slots at a time (including sending a message to the browser
when a slot opens up). Another challenge is that we have to support blocking
completion callbacks. In that case, we have to unpack the IPC::Message on the IO
thread and invoke the TrackedCallback to wake up the calling thread. Note that
in that case, the calling thread might not even have a message loop (we allow
blocking calls without a PPB_MessageLoop). So any general design would have to
allow for unpacking the message and correctly invoking the TrackedCallback from
the IO thread...  so just defining a Filter might be the most straightforward
way to do that.

I think trying to generalize too much, I'd be very likely to make something
harder to understand that might not even work for the next use case. My
inclination is to not try to generalize too much until we have 2, 3, or more.

(PS: I forgot, I still have to properly merge in some changes to make the new
UDP multicast support work in this CL)

[dmichael (off chromium), 2015-03-23 20:50:06]

I think I addressed your comments in the latest patch set. Thanks!

https://codereview.chromium.org/869883003/diff/300001/ppapi/proxy/udp_socket_...
File ppapi/proxy/udp_socket_filter.cc (right):

https://codereview.chromium.org/869883003/diff/300001/ppapi/proxy/udp_socket_...
ppapi/proxy/udp_socket_filter.cc:67: const base::Closure&
slot_available_callback) {
On 2015/03/10 02:53:14, raymes wrote:
> Should we assert the proxy lock here too?

Done.

https://codereview.chromium.org/869883003/diff/300001/ppapi/proxy/udp_socket_...
ppapi/proxy/udp_socket_filter.cc:71: instance, private_api,
slot_available_callback)));
On 2015/03/10 02:53:14, raymes wrote:
> nit: indentation
Ah, thanks & sorry. I ran git cl format in the latest PS, so hopefully that will
fix this and some other spots.

https://codereview.chromium.org/869883003/diff/300001/ppapi/proxy/udp_socket_...
ppapi/proxy/udp_socket_filter.cc:74: void
UDPSocketFilter::RemoveUDPResource(PP_Resource resource) {
On 2015/03/10 02:53:14, raymes wrote:
> Should we assert the proxy lock here too?

Done.

https://codereview.chromium.org/869883003/diff/300001/ppapi/proxy/udp_socket_...
ppapi/proxy/udp_socket_filter.cc:85: const scoped_refptr<TrackedCallback>&
callback) {
On 2015/03/10 02:53:14, raymes wrote:
> Should we assert the proxy lock here too?

Done.

https://codereview.chromium.org/869883003/diff/300001/ppapi/proxy/udp_socket_...
ppapi/proxy/udp_socket_filter.cc:102:
PPAPI_DISPATCH_PLUGIN_RESOURCE_CALL_UNHANDLED(handled = false)
...Re: git cl format; it does these "wrong" so I "fixed" it to usual Chromium
style. I personally am fine with the git cl format way, but some people object
strongly.

https://codereview.chromium.org/869883003/diff/300001/ppapi/proxy/udp_socket_...
ppapi/proxy/udp_socket_filter.cc:117: const PP_NetAddress_Private& addr) {
On 2015/03/10 02:53:14, raymes wrote:
> Is it possible to add an assertion that we're on the IOThread here?
Good idea, done. I had to add a way to get the IPC TaskRunner to PluginGlobals
so that I can get it in a way that's safe without the proxy lock (looking up the
dispatcher by instance doesn't seem safe).

https://codereview.chromium.org/869883003/diff/300001/ppapi/proxy/udp_socket_...
ppapi/proxy/udp_socket_filter.cc:150: const PP_NetAddress_Private& addr) {
On 2015/03/10 02:53:14, raymes wrote:
> Is it possible to add an assertion that we're on the IOThread here?

Done.

https://codereview.chromium.org/869883003/diff/300001/ppapi/proxy/udp_socket_...
File ppapi/proxy/udp_socket_filter.h (right):

https://codereview.chromium.org/869883003/diff/300001/ppapi/proxy/udp_socket_...
ppapi/proxy/udp_socket_filter.h:124: mutable base::Lock lock_;
On 2015/03/10 02:53:15, raymes wrote:
> Maybe make a note that this protects queues_?

Done.

https://codereview.chromium.org/869883003/diff/300001/ppapi/proxy/udp_socket_...
File ppapi/proxy/udp_socket_resource_base.cc (right):

https://codereview.chromium.org/869883003/diff/300001/ppapi/proxy/udp_socket_...
ppapi/proxy/udp_socket_resource_base.cc:53:
base::Bind(&UDPSocketResourceBase::SlotBecameAvailable, this));
On 2015/03/09 07:43:01, raymes wrote:
> Is this meant to be SlotBecameAvailableOnIOThread?
Good catch, thanks.

https://codereview.chromium.org/869883003/diff/300001/ppapi/proxy/udp_socket_...
ppapi/proxy/udp_socket_resource_base.cc:250: void
UDPSocketResourceBase::SlotBecameAvailableOnIOThread() {
On 2015/03/10 02:53:15, raymes wrote:
> Is it possible to add an assertion that we're on the IOThread here?

Done.

[raymes, 2015-03-24 04:29:10]

Hey David,

Thanks - it looks like this is just a hard thing to do. This is something that
is necessary to resolve the deadlock bug referenced, is that correct? I had
another look over the CL and I'm a little concerned about the complexity that
gets added but I'm not sure if we can do anything about it without a lot more
work. 

Rather than adding a separate filter class, is it possible to look up the
resource directly and call into its message handler? That feels like it could
simplify things a bit.

Alternatively, if we are heading down the route of doing something specific for
UDP, would it be better to be more specific across the CL? Would it be better to
remove the ResourceMessageFilter class and just have a specific class there too?
How likely do you think it is that we would have to do this for something else? 

Thanks!
Raymes

[dmichael (off chromium), 2015-03-24 15:17:34]

On 2015/03/24 04:29:10, raymes wrote:
> Hey David,
> 
> Thanks - it looks like this is just a hard thing to do. This is something that
> is necessary to resolve the deadlock bug referenced, is that correct?
Correct. Also, fixing this deadlock is a pre-requisite for fixing a different
deadlock:
https://code.google.com/p/chromium/issues/detail?id=435414
...so there's some more context in that bug if you want it.

> I had
> another look over the CL and I'm a little concerned about the complexity that
> gets added but I'm not sure if we can do anything about it without a lot more
> work. 
> 
> Rather than adding a separate filter class, is it possible to look up the
> resource directly and call into its message handler? That feels like it could
> simplify things a bit.
That's roughly what the old version does, but we can't safely look up the
Resource from the ResourceTracker and use it unless we're holding the ProxyLock.
The approach I took should keep contention pretty low; the IO thread will have
to lock on the TrackedCallback (which should basically never be contended) and
the CallbackTracker (which will be only rarely contended if another thread is
trying to add or remove a callback).

> 
> Alternatively, if we are heading down the route of doing something specific
for
> UDP, would it be better to be more specific across the CL? Would it be better
to
> remove the ResourceMessageFilter class and just have a specific class there
too?
I thought about doing that, but I wanted to minimize the special knowledge about
UDP; it would seem like a dependency inversion in a way for PluginMessageFilter
to know specifically about UDPResourceFilter. PluginGlobals really needs to know
about it so that UDPSocketResource can access the filter, so I tried to make
PluginGlobals the "one place" where new filters would have to be added. I could
instead make it more specific to UDP if you feel strongly, but in my opinion
ResourceMessageFilter is not overly complicated and is a good abstraction.

> How likely do you think it is that we would have to do this for something
else? 
Maybe 30%? There are other APIs that could probably have latency reduced by
doing this strategy (probably TCP, VideoDecode, MediaStream*Track off the top of
my head). But the truth is we probably won't spend the time to do it.

> 
> Thanks!
> Raymes

[dmichael (off chromium), 2015-03-24 20:10:27]

I had a couple of mistakes, so there's a new patch up. Thanks again for taking
the time to review this.

https://codereview.chromium.org/869883003/diff/380001/ppapi/proxy/udp_socket_...
File ppapi/proxy/udp_socket_resource_base.cc (right):

https://codereview.chromium.org/869883003/diff/380001/ppapi/proxy/udp_socket_...
ppapi/proxy/udp_socket_resource_base.cc:308: // send the message.
This is kind of weird and a bummer... the callback might be invoked:
1) On the IO thread *without* the lock
2) On some other thread *with* the lock
Pretty confusing. I think I'll move this logic in to the filter instead so that
it just always posts to the main thread using RunWhileLocked. It will mean that
in case #2, we do an unnecessary PostTask to the main thread for updating the
slot info, but it will simplify things a bit.
WDYT?

https://codereview.chromium.org/869883003/diff/380001/ppapi/shared_impl/proxy...
File ppapi/shared_impl/proxy_lock.h (left):

https://codereview.chromium.org/869883003/diff/380001/ppapi/shared_impl/proxy...
ppapi/shared_impl/proxy_lock.h:180: ProxyLock::AssertAcquired();
I needed to be able to use RunWhileLocked on the IO thread without the
ProxyLock, and I realized that for a while now, VarTracker and ResourceTracker
do their own checking, so we should be protected from mistakes like binding a
ScopedPPVar without the ProxyLock...
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/shared_impl/...
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/shared_impl/...

[raymes, 2015-03-25 01:33:28]

Hey David!

I still don't feel great about the design here which has made me reluctant to lg
it but this feels like something that has to be done and I don't have any
further good alternative suggestions :( It may be worth discussing the high
level design with bbudge to get a second opinion? Otherwise I'm ok with this
going in but if we implemented something similar for a different API I would
feel quite strongly about making something more general.

Thanks!
Raymes

[raymes, 2015-03-25 01:43:59]

https://codereview.chromium.org/869883003/diff/380001/ppapi/shared_impl/proxy...
File ppapi/shared_impl/proxy_lock.h (left):

https://codereview.chromium.org/869883003/diff/380001/ppapi/shared_impl/proxy...
ppapi/shared_impl/proxy_lock.h:180: ProxyLock::AssertAcquired();
Is the name a bit misleading now then? Should we rename the function?

https://codereview.chromium.org/869883003/diff/400001/ppapi/proxy/udp_socket_...
File ppapi/proxy/udp_socket_resource_base.cc (right):

https://codereview.chromium.org/869883003/diff/400001/ppapi/proxy/udp_socket_...
ppapi/proxy/udp_socket_resource_base.cc:325: // Note, we don't have the
ProxyLock yet, so we use EnterResource to acquire
If we don't have the proxylock why is AssertAcquired true?

https://codereview.chromium.org/869883003/diff/400001/ppapi/proxy/udp_socket_...
ppapi/proxy/udp_socket_resource_base.cc:326: // it. (We couldn't use
RunWhileLocked in SlotBecameAvailableOnIOThread
SlotBecameAvailableOnIOThread isn't around anymore, is that true?

https://codereview.chromium.org/869883003/diff/400001/ppapi/shared_impl/track...
File ppapi/shared_impl/tracked_callback.cc (right):

https://codereview.chromium.org/869883003/diff/400001/ppapi/shared_impl/track...
ppapi/shared_impl/tracked_callback.cc:250:
RunWhileLocked(base::Bind(&TrackedCallback::Run, this, result)));
I don't really understand this change but maybe bbudge would be better to
comment on it since he looked at the TrackedCallback stuff more?

[dmichael (off chromium), 2015-03-25 17:27:04]

On 2015/03/25 01:33:28, raymes wrote:
> Hey David!
> 
> I still don't feel great about the design here which has made me reluctant to
lg
> it but this feels like something that has to be done and I don't have any
> further good alternative suggestions :(
My best alternative would have been to just always go through the main thread;
but I didn't want to regress the latency improvement Yuzhu worked so hard for
(and I think ARC would complain).
 
> It may be worth discussing the high
> level design with bbudge to get a second opinion?
I'll bring it up with him in case he wants to weigh in.

> Otherwise I'm ok with this
> going in but if we implemented something similar for a different API I would
> feel quite strongly about making something more general.
Yeah, I agree. There might be an opportunity to generalize something like... 
pushing data from the browser to the plugin in frames with a fixed number of
slots for some back-pressure. But I'm afraid that if I try to make it too
general I'll just get it wrong anyway. So I hope I struck the right balance.

> 
> Thanks!
> Raymes
Thank you for the thorough review; I know this one's thorny.

[dmichael (off chromium), 2015-03-25 17:33:38]

Patchset #9 (id:420001) has been deleted

[dmichael (off chromium), 2015-03-25 17:33:49]

Patchset #9 (id:440001) has been deleted

[dmichael (off chromium), 2015-03-25 17:34:00]

Patchset #9 (id:460001) has been deleted

[dmichael (off chromium), 2015-03-25 17:34:17]

Patchset #9 (id:480001) has been deleted

[dmichael (off chromium), 2015-03-25 17:35:04]

https://codereview.chromium.org/869883003/diff/380001/ppapi/shared_impl/proxy...
File ppapi/shared_impl/proxy_lock.h (left):

https://codereview.chromium.org/869883003/diff/380001/ppapi/shared_impl/proxy...
ppapi/shared_impl/proxy_lock.h:180: ProxyLock::AssertAcquired();
On 2015/03/25 01:43:59, raymes wrote:
> Is the name a bit misleading now then? Should we rename the function?
The name never really was meant to relate to whether the lock is held at the
time "RunWhileLocked" is invoked. The goal is to return a bound callback that
will run with the lock, so call sites look fairly self-explanatory like:
PostTask(FROM_HERE, RunWhileLocked(base::Bind(Foo::DoStuff)));

Arguably the name has always been misleading (maybe should be something like
BindWithProxyLock), but it looks pretty good at call-sites.

Anywho...  You're right, it doesn't belong in this CL so I moved to the other
one for TrackedCallback. If you still want a name change, you can comment there:
https://codereview.chromium.org/923263003/
...and I'd probably do that as a separate CL:

https://codereview.chromium.org/869883003/diff/400001/ppapi/proxy/udp_socket_...
File ppapi/proxy/udp_socket_resource_base.cc (right):

https://codereview.chromium.org/869883003/diff/400001/ppapi/proxy/udp_socket_...
ppapi/proxy/udp_socket_resource_base.cc:325: // Note, we don't have the
ProxyLock yet, so we use EnterResource to acquire
On 2015/03/25 01:43:59, raymes wrote:
> If we don't have the proxylock why is AssertAcquired true?
Oops, sorry, that comment made sense before I fixed up RunWhileLocked to
simplify this. Removed.

https://codereview.chromium.org/869883003/diff/400001/ppapi/proxy/udp_socket_...
ppapi/proxy/udp_socket_resource_base.cc:326: // it. (We couldn't use
RunWhileLocked in SlotBecameAvailableOnIOThread
On 2015/03/25 01:43:59, raymes wrote:
> SlotBecameAvailableOnIOThread isn't around anymore, is that true?

Done.

https://codereview.chromium.org/869883003/diff/400001/ppapi/shared_impl/track...
File ppapi/shared_impl/tracked_callback.cc (right):

https://codereview.chromium.org/869883003/diff/400001/ppapi/shared_impl/track...
ppapi/shared_impl/tracked_callback.cc:250:
RunWhileLocked(base::Bind(&TrackedCallback::Run, this, result)));
On 2015/03/25 01:43:59, raymes wrote:
> I don't really understand this change but maybe bbudge would be better to
> comment on it since he looked at the TrackedCallback stuff more?
That CL hasn't landed yet, and you're right...  this change really belongs
there, so I moved it out of this CL.

[raymes, 2015-03-26 05:36:31]

On 2015/03/25 17:27:04, dmichael wrote:
> On 2015/03/25 01:33:28, raymes wrote:
> > Hey David!
> > 
> > I still don't feel great about the design here which has made me reluctant
to
> lg
> > it but this feels like something that has to be done and I don't have any
> > further good alternative suggestions :(
> My best alternative would have been to just always go through the main thread;
> but I didn't want to regress the latency improvement Yuzhu worked so hard for
> (and I think ARC would complain).

Do we know for sure whether they saw an improvement after Yuzhu's fix? If they
did then I don't think there is any question about keeping the optimization but
if not then perhaps there is room to regress on it? 

>  
> > It may be worth discussing the high
> > level design with bbudge to get a second opinion?
> I'll bring it up with him in case he wants to weigh in.
> 
> > Otherwise I'm ok with this
> > going in but if we implemented something similar for a different API I would
> > feel quite strongly about making something more general.
> Yeah, I agree. There might be an opportunity to generalize something like... 
> pushing data from the browser to the plugin in frames with a fixed number of
> slots for some back-pressure. But I'm afraid that if I try to make it too
> general I'll just get it wrong anyway. So I hope I struck the right balance.
> 
> > 
> > Thanks!
> > Raymes
> Thank you for the thorough review; I know this one's thorny.

[raymes, 2015-03-26 06:30:52]

lgtm

Thanks!

https://codereview.chromium.org/869883003/diff/380001/ppapi/proxy/udp_socket_...
File ppapi/proxy/udp_socket_resource_base.cc (right):

https://codereview.chromium.org/869883003/diff/380001/ppapi/proxy/udp_socket_...
ppapi/proxy/udp_socket_resource_base.cc:308: // send the message.
Do you still think this is worth doing? If it leads to simpler code here I'm in
favour :)

https://codereview.chromium.org/869883003/diff/380001/ppapi/shared_impl/proxy...
File ppapi/shared_impl/proxy_lock.h (left):

https://codereview.chromium.org/869883003/diff/380001/ppapi/shared_impl/proxy...
ppapi/shared_impl/proxy_lock.h:180: ProxyLock::AssertAcquired();
Ahh that makes complete sense then. No please keep the name as-is especially if
it makes sense from the call sites.

https://codereview.chromium.org/869883003/diff/500001/ppapi/proxy/udp_socket_...
File ppapi/proxy/udp_socket_filter.cc (right):

https://codereview.chromium.org/869883003/diff/500001/ppapi/proxy/udp_socket_...
ppapi/proxy/udp_socket_filter.cc:53: return result;
nit: is it clearer to keep this the same as before, without reusing the result
variable?

https://codereview.chromium.org/869883003/diff/500001/ppapi/proxy/udp_socket_...
ppapi/proxy/udp_socket_filter.cc:209: return PP_ERROR_BADARGUMENT;
we used to bail if the thing wasn't bound here, should we still do that?

[dmichael (off chromium), 2015-03-26 18:15:44]

On 2015/03/26 05:36:31, raymes wrote:
> On 2015/03/25 17:27:04, dmichael wrote:
> > On 2015/03/25 01:33:28, raymes wrote:
> > > Hey David!
> > > 
> > > I still don't feel great about the design here which has made me reluctant
> to
> > lg
> > > it but this feels like something that has to be done and I don't have any
> > > further good alternative suggestions :(
> > My best alternative would have been to just always go through the main
thread;
> > but I didn't want to regress the latency improvement Yuzhu worked so hard
for
> > (and I think ARC would complain).
> 
> Do we know for sure whether they saw an improvement after Yuzhu's fix? If they
> did then I don't think there is any question about keeping the optimization
but
> if not then perhaps there is room to regress on it? 
Yuzhu measured a clear improvement...  see section 4:
https://docs.google.com/document/d/1Udx2GAvgvGYv4Uyh0UiVQGvYF63y4NgWeFOVfII7D...

> 
> >  
> > > It may be worth discussing the high
> > > level design with bbudge to get a second opinion?
> > I'll bring it up with him in case he wants to weigh in.
> > 
> > > Otherwise I'm ok with this
> > > going in but if we implemented something similar for a different API I
would
> > > feel quite strongly about making something more general.
> > Yeah, I agree. There might be an opportunity to generalize something like...

> > pushing data from the browser to the plugin in frames with a fixed number of
> > slots for some back-pressure. But I'm afraid that if I try to make it too
> > general I'll just get it wrong anyway. So I hope I struck the right balance.
> > 
> > > 
> > > Thanks!
> > > Raymes
> > Thank you for the thorough review; I know this one's thorny.

[dmichael (off chromium), 2015-03-26 18:19:43]

Thanks, Raymes! Now I just need to go pick on Bill to finish the review of the
precursor CL ;-)

https://codereview.chromium.org/869883003/diff/380001/ppapi/proxy/udp_socket_...
File ppapi/proxy/udp_socket_resource_base.cc (right):

https://codereview.chromium.org/869883003/diff/380001/ppapi/proxy/udp_socket_...
ppapi/proxy/udp_socket_resource_base.cc:308: // send the message.
On 2015/03/26 06:30:51, raymes wrote:
> Do you still think this is worth doing? If it leads to simpler code here I'm
in
> favour :)
> 
Done. I did it slightly different than I suggested initially...  from the IO
thread, I post to the main thread with the lock to invoke the callback. As
before, the callback can still be invoked from within the call to RequestData.
It's slightly akward that it's called in a "nested" sort of way, but it has the
proxy lock and can call Post immediately without a thread-hop, so it seemed
worthwhile. This bit of UDPSocketResourceBase is nicely simplified.

https://codereview.chromium.org/869883003/diff/500001/ppapi/proxy/udp_socket_...
File ppapi/proxy/udp_socket_filter.cc (right):

https://codereview.chromium.org/869883003/diff/500001/ppapi/proxy/udp_socket_...
ppapi/proxy/udp_socket_filter.cc:53: return result;
On 2015/03/26 06:30:51, raymes wrote:
> nit: is it clearer to keep this the same as before, without reusing the result
> variable?
Wow, not sure what I was smoking when I changed that. Yeah, it was simpler
before.

https://codereview.chromium.org/869883003/diff/500001/ppapi/proxy/udp_socket_...
ppapi/proxy/udp_socket_filter.cc:209: return PP_ERROR_BADARGUMENT;
On 2015/03/26 06:30:51, raymes wrote:
> we used to bail if the thing wasn't bound here, should we still do that?
Good catch. I added that check back to UDPSocketResourceBase.

[dmichael (off chromium), 2015-03-31 19:40:31]

dmichael@chromium.org changed reviewers:
+ piman@chromium.org

[dmichael (off chromium), 2015-03-31 19:40:32]

+piman for ppapi_thread.cc

[piman, 2015-03-31 19:44:27]

lgtm

[dmichael (off chromium), 2015-04-01 21:44:20]

The CQ bit was checked by dmichael@chromium.org

[dmichael (off chromium), 2015-04-01 21:44:21]

The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org,
piman@chromium.org
Link to the patchset: https://codereview.chromium.org/869883003/#ps600001
(title: "rebase to origin/master")

[commit-bot: I haz the power, 2015-04-01 21:44:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/869883003/600001

[commit-bot: I haz the power, 2015-04-01 23:29:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-01 23:29:34]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[dmichael (off chromium), 2015-04-02 15:42:32]

The CQ bit was checked by dmichael@chromium.org

[dmichael (off chromium), 2015-04-02 15:42:33]

The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org,
piman@chromium.org
Link to the patchset: https://codereview.chromium.org/869883003/#ps620001
(title: "fix size_t vs int32_t")

[commit-bot: I haz the power, 2015-04-02 15:43:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/869883003/620001

[commit-bot: I haz the power, 2015-04-02 16:59:49]

Committed patchset #15 (id:620001)

[commit-bot: I haz the power, 2015-04-03 20:26:53]

Patchset 15 (id:??) landed as
https://crrev.com/b11ca7b7dac766989002bede109c81a3f8a8bc89
Cr-Commit-Position: refs/heads/master@{#323517}