Pepper: Make StartSelLdr asynchronous.

Pepper: Make StartSelLdr asynchronous.

Changing the trusted plugin to use Chromium IPC instead of SRPC requires that
we create another channel between the renderer and plugin for communicating the
existing SRPC messages.

This channel will be created in response to the StartSelLdr request. Since we
need that channel to be available immediately, StartSelLdr should not finish
until the created channel is available for use. This requires that we make this
operation asynchronous.

SyncChannel creation is asynchronous because we must wait for the channel to be
connected before attempting to use it for Send(). 
As part of connection, OnChannelConnected must be called on the thread
that created the SyncChannel.

In local testing, calling Send() before the channel had a chance to 
connect led to deadlock.

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

[Mark Seaborn, 2014-02-10 21:59:43]

On 10 February 2014 13:24, <teravest@chromium.org> wrote:

> Reviewers: dmichael,
>
> Description:
> Pepper: Make StartSelLdr asynchronous.
>
> Changing the trusted plugin to use Chromium IPC instead of SRPC requires
> that
> we create another channel between the renderer and plugin for
> communicating the
> existing SRPC messages.
>
> This channel will be created in response to the StartSelLdr request. Since
> we
> need that channel to be available immediately, StartSelLdr should not
> finish
> until the created channel is available for use. This requires that we make
> this
> operation asynchronous.
>
> BUG=333950
>

I don't understand.  Are you going to change nacl_listener.cc to return 3
IPC channels (2 for the renderer, 1 for the browser) instead of the current
2?  If so, then when NaClHostMsg_LaunchNaCl (synchronous) returns, the new
channel will be available for the renderer to use.

StartSelLdr() sends NaClHostMsg_LaunchNaCl, but I don't see why
StartSelLdr() would need to become asynchronous.

Mark

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

[dmichael (off chromium), 2014-02-10 23:31:16]

lgtm

Can you edit the commit description to make it more clear why you need to do
this? IIRC, it's something like this:
1) SyncChannel isn't ready for sending stuff until your listener gets
OnChannelConnected.
2) The IPC::ChannelProxy has its own OnChannelConnected that  (on POSIX) gets
called from here:
https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel_po...
which is called from here:
https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel_re...
etc. From the perspective of the IPC::ChannelProxy, it owns a channel that runs
on the IO thread. Confusingly, the IO thread *is* the listener thread for the
IPC::Channel. The IPC::ChannelProxy has a separate listener thread, which is the
one where it was created. (Recall IPC::SyncChannel is-a ChannelProxy).

That posts a task to the ChannelProxy's listener thread to call *your*
ChannelConnected, posted from here:
https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel_pr...

So, tying up the strings here:
- Your SyncChannel isn't ready until the underlying Channel receives a "Hello"
message.
- The "Hello" message is posted as a task to the "listener" thread.
- If you try to invoke a synchronous call *before* this Hello message has been
received and *before* OnChannelConnected is invoked on the listener thread, it
will hang, because OnChannelConnected can not run. This is because you are
blocking the listener thread by making an outgoing sync call. Posted tasks do
not unblock synchronous calls.




posted to the listener's thread from here:
https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel_pr...

https://codereview.chromium.org/149403005/diff/1/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/plugin.h (right):

https://codereview.chromium.org/149403005/diff/1/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/plugin.h:309: void
SignalStartSelLdrDone(int32_t pp_error,
A "Did" name would be more consistent with others...  e.g.:
SelLdrDidLaunch
SelLdrLaunchDidFinish

https://codereview.chromium.org/149403005/diff/1/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right):

https://codereview.chromium.org/149403005/diff/1/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:601:
callback.pp_completion_callback());
Did you try just passing the pp::CompletionCallback? I was thinking it was safe
to copy it, as long as it is only ever invoked once.

[teravest, 2014-02-11 16:27:00]

On Mon, Feb 10, 2014 at 4:31 PM,  <dmichael@chromium.org> wrote:
> lgtm
>
> Can you edit the commit description to make it more clear why you need to do
> this? IIRC, it's something like this:
> 1) SyncChannel isn't ready for sending stuff until your listener gets
> OnChannelConnected.
> 2) The IPC::ChannelProxy has its own OnChannelConnected that  (on POSIX)
> gets
> called from here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel_po...
> which is called from here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel_re...
> etc. From the perspective of the IPC::ChannelProxy, it owns a channel that
> runs
> on the IO thread. Confusingly, the IO thread *is* the listener thread for
> the
> IPC::Channel. The IPC::ChannelProxy has a separate listener thread, which is
> the
> one where it was created. (Recall IPC::SyncChannel is-a ChannelProxy).
>
> That posts a task to the ChannelProxy's listener thread to call *your*
> ChannelConnected, posted from here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel_pr...
>
> So, tying up the strings here:
> - Your SyncChannel isn't ready until the underlying Channel receives a
> "Hello"
> message.
> - The "Hello" message is posted as a task to the "listener" thread.
> - If you try to invoke a synchronous call *before* this Hello message has
> been
> received and *before* OnChannelConnected is invoked on the listener thread,
> it
> will hang, because OnChannelConnected can not run. This is because you are
> blocking the listener thread by making an outgoing sync call. Posted tasks
> do
> not unblock synchronous calls.

I expanded the commit description a bit.

>
>
>
>
> posted to the listener's thread from here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel_pr...
>
>
>
https://codereview.chromium.org/149403005/diff/1/ppapi/native_client/src/trus...
> File ppapi/native_client/src/trusted/plugin/plugin.h (right):
>
>
https://codereview.chromium.org/149403005/diff/1/ppapi/native_client/src/trus...
> ppapi/native_client/src/trusted/plugin/plugin.h:309: void
> SignalStartSelLdrDone(int32_t pp_error,
> A "Did" name would be more consistent with others...  e.g.:
> SelLdrDidLaunch
> SelLdrLaunchDidFinish

I don't like this because Plugin::SignalStartSelLdrDone is only called
in codepath from Plugin::LoadNaClModuleFromBackgroundThread.
StartSelLdr also happens from the main thread in
Plugin::LoadNaClModule.

>
>
https://codereview.chromium.org/149403005/diff/1/ppapi/native_client/src/trus...
> File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right):
>
>
https://codereview.chromium.org/149403005/diff/1/ppapi/native_client/src/trus...
> ppapi/native_client/src/trusted/plugin/service_runtime.cc:601:
> callback.pp_completion_callback());
> Did you try just passing the pp::CompletionCallback? I was thinking it
> was safe to copy it, as long as it is only ever invoked once.

Done. Thanks.

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

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

[Mark Seaborn, 2014-02-11 16:42:03]

On 10 February 2014 13:59, Mark Seaborn <mseaborn@chromium.org> wrote:

> On 10 February 2014 13:24, <teravest@chromium.org> wrote:
>
>> Reviewers: dmichael,
>>
>> Description:
>> Pepper: Make StartSelLdr asynchronous.
>>
>> Changing the trusted plugin to use Chromium IPC instead of SRPC requires
>> that
>> we create another channel between the renderer and plugin for
>> communicating the
>> existing SRPC messages.
>>
>> This channel will be created in response to the StartSelLdr request.
>> Since we
>> need that channel to be available immediately, StartSelLdr should not
>> finish
>> until the created channel is available for use. This requires that we
>> make this
>> operation asynchronous.
>>
>> BUG=333950
>>
>
> I don't understand.  Are you going to change nacl_listener.cc to return 3
> IPC channels (2 for the renderer, 1 for the browser) instead of the current
> 2?  If so, then when NaClHostMsg_LaunchNaCl (synchronous) returns, the new
> channel will be available for the renderer to use.
>
> StartSelLdr() sends NaClHostMsg_LaunchNaCl, but I don't see why
> StartSelLdr() would need to become asynchronous.
>

Justin explained it to me (restating for my benefit):

Later, load_module will be converted from SRPC to Chrome IPC, but will stay
as a synchronous message for two reasons:
 * The load status will be returned as a result, to preserve the existing
error-handling behaviour.
 * To avoid having to change a lot of the control flow in the trusted
plugin, such as initialising the reverse service afterwards.

But in order to use an IPC::SyncChannel for load_module, the plugin must
wait for an OnChannelConnected() callback.

The reason StartSelLdr() needs to be made async is presumably because of
the awkward division between src/trusted/plugin
and ppb_nacl_private_impl.cc.  If waiting for OnChannelConnected() must be
done in the latter, its interface to src/trusted/plugin must be made async.
 Have I got that right?

Cheers,
Mark

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

[teravest, 2014-02-11 16:47:23]

On Tue, Feb 11, 2014 at 9:42 AM, Mark Seaborn <mseaborn@chromium.org> wrote:
> On 10 February 2014 13:59, Mark Seaborn <mseaborn@chromium.org> wrote:
>>
>> On 10 February 2014 13:24, <teravest@chromium.org> wrote:
>>>
>>> Reviewers: dmichael,
>>>
>>> Description:
>>> Pepper: Make StartSelLdr asynchronous.
>>>
>>> Changing the trusted plugin to use Chromium IPC instead of SRPC requires
>>> that
>>> we create another channel between the renderer and plugin for
>>> communicating the
>>> existing SRPC messages.
>>>
>>> This channel will be created in response to the StartSelLdr request.
>>> Since we
>>> need that channel to be available immediately, StartSelLdr should not
>>> finish
>>> until the created channel is available for use. This requires that we
>>> make this
>>> operation asynchronous.
>>>
>>> BUG=333950
>>
>>
>> I don't understand.  Are you going to change nacl_listener.cc to return 3
>> IPC channels (2 for the renderer, 1 for the browser) instead of the current
>> 2?  If so, then when NaClHostMsg_LaunchNaCl (synchronous) returns, the new
>> channel will be available for the renderer to use.
>>
>> StartSelLdr() sends NaClHostMsg_LaunchNaCl, but I don't see why
>> StartSelLdr() would need to become asynchronous.
>
>
> Justin explained it to me (restating for my benefit):
>
> Later, load_module will be converted from SRPC to Chrome IPC, but will stay
> as a synchronous message for two reasons:
>  * The load status will be returned as a result, to preserve the existing
> error-handling behaviour.
>  * To avoid having to change a lot of the control flow in the trusted
> plugin, such as initialising the reverse service afterwards.
>
> But in order to use an IPC::SyncChannel for load_module, the plugin must
> wait for an OnChannelConnected() callback.
>
> The reason StartSelLdr() needs to be made async is presumably because of the
> awkward division between src/trusted/plugin and ppb_nacl_private_impl.cc.
> If waiting for OnChannelConnected() must be done in the latter, its
> interface to src/trusted/plugin must be made async.  Have I got that right?

Yes! Thanks for explaining it more clearly than I did. :)

>
> Cheers,
> Mark
>

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

[dmichael (off chromium), 2014-02-11 16:54:26]

Mark, thanks for explaining the bigger picture here nicely.

Overall, I think we're going to have a lot of CLs that look like two steps
forward and one step back. I think that's okay; there's too much work for us to
be able to do it "the right way" all at once; it makes sense to break the work
up in to smaller pieces, even if it means we will add some throwaway code. E.g.,
in this case, even if we *didn't* need SyncChannel in the long run, it might
make sense to use SyncChannel in the interim if it lets us move things from SRPC
to IPC sooner. (It happens we do need SyncChannel, so the argument is kind of
moot in this case).

Anywho, lgtm

[teravest, 2014-02-11 17:05:33]

The CQ bit was checked by teravest@chromium.org

[commit-bot: I haz the power, 2014-02-11 17:06:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/149403005/120001

[commit-bot: I haz the power, 2014-02-11 19:00:08]

Change committed as 250461