NaCl cleanup: Stop connecting to the SRPC trusted command channel

NaCl cleanup: Stop connecting to the SRPC trusted command channel

Before this change, the SetupCommand() call in service_runtime.cc
connected to the SRPC trusted command channel.  We replace this with a
subset of what SetupCommand() did.  (Note that the renderer can still
connect to the *untrusted* SRPC command channel, which is used by the
PNaCl translator.)

Before, we were not sending any messages on the trusted command
channel, but the NaCl loader process was detecting EOF on the channel
and exiting in response (via a call to NaClAppShutdown() in
nacl_secure_service.c).  We replace this with new logic in
nacl_trusted_listener.cc which detects EOF on a Chrome IPC channel.

 * This happens to be tested indirectly via tests for the GDB debug
   stub.  They hang if the NaCl loader process sticks around after an
   <embed> element is deleted.

 * This change changes the debug stub's behaviour slightly: If a NaCl
   process is forcibly terminated (such as by its <embed> being
   deleted), GDB will get an EOF on the debug socket without seeing a
   SIGKILL (-9) exit status being reported first.  I'm not sure it's
   worthwhile doing the extra plumbing to keep the original "-9"
   status reporting (which was added for https://crbug.com/157881).

   Before, GDB prints:
     Program terminated with signal SIGKILL, Killed.
     The program no longer exists.

   After, GDB prints:
     Remote connection closed

 * Remove the existing OnChannelError() because it never gets called.
   A call to it get queued on the main thread, but the main thread
   gets blocked in NaClChromeMainAppStart().

BUG=496287
TEST=debug stub tests: NaClGdbDebugStubTest.*, NaClBrowserTestPnaclDebug.*

Committed: https://crrev.com/2eeecfc4babe46bef3a1a4946a6a77e17b9179fa
Cr-Commit-Position: refs/heads/master@{#334097}

[Mark Seaborn, 2015-06-08 23:17:47]

mseaborn@chromium.org changed reviewers:
+ jvoung@chromium.org

[jvoung (off chromium), 2015-06-09 15:43:44]

lgtm

https://codereview.chromium.org/1153293003/diff/20001/components/nacl/loader/...
File components/nacl/loader/nacl_trusted_listener.cc (right):

https://codereview.chromium.org/1153293003/diff/20001/components/nacl/loader/...
components/nacl/loader/nacl_trusted_listener.cc:18: void OnChannelError()
override {
It wasn't too clear if this should also override OnChannelClosing, but there is
a lot of instances of one or the other and not many instances of both from code
search.

https://codereview.chromium.org/1153293003/diff/20001/components/nacl/rendere...
File components/nacl/renderer/plugin/service_runtime.cc (left):

https://codereview.chromium.org/1153293003/diff/20001/components/nacl/rendere...
components/nacl/renderer/plugin/service_runtime.cc:63:
error_info.SetReport(PP_NACL_ERROR_SEL_LDR_COMMUNICATION_CMD_CHANNEL,
If want to keep this UMA stat (it's about 0.01%), you could place it in the !ok
part of StartNexe.

[Mark Seaborn, 2015-06-10 00:31:04]

https://codereview.chromium.org/1153293003/diff/20001/components/nacl/loader/...
File components/nacl/loader/nacl_trusted_listener.cc (right):

https://codereview.chromium.org/1153293003/diff/20001/components/nacl/loader/...
components/nacl/loader/nacl_trusted_listener.cc:18: void OnChannelError()
override {
On 2015/06/09 15:43:44, jvoung wrote:
> It wasn't too clear if this should also override OnChannelClosing, but there
is
> a lot of instances of one or the other and not many instances of both from
code
> search.

From reading ipc/, it looks like OnChannelClosing() is only called as a result
of someone calling channel->Close().  So:
 * Close() -> OnChannelClosing(): means our channel endpoint is closed.
 * OnChannelError(): means the channel's other endpoint was closed.

channel->Close() should never happen on this channel.  There is a
channel_->Close() call in NaClTrustedListener::OnChannelError() below, but:

 * It's probably unnecessary (just boilerplate?)
 * NaClTrustedListener::OnChannelError() never actually gets called.  A call to
it gets queued on the main thread, but the main thread gets blocked in
NaClChromeMainAppStart().

I could remove NaClTrustedListener::OnChannelError(), if you think it would make
the code clearer?

https://codereview.chromium.org/1153293003/diff/20001/components/nacl/rendere...
File components/nacl/renderer/plugin/service_runtime.cc (left):

https://codereview.chromium.org/1153293003/diff/20001/components/nacl/rendere...
components/nacl/renderer/plugin/service_runtime.cc:63:
error_info.SetReport(PP_NACL_ERROR_SEL_LDR_COMMUNICATION_CMD_CHANNEL,
On 2015/06/09 15:43:44, jvoung wrote:
> If want to keep this UMA stat (it's about 0.01%), you could place it in the
!ok
> part of StartNexe.

Oh, good point!  I was deleting code without really thinking about it.

I've reinstated the error reporting.  I've split it to report two more-specific
error messages in case that helps debugging.

Please take a look.

[jvoung (off chromium), 2015-06-10 16:33:01]

otherwise slgtm, thanks!

https://codereview.chromium.org/1153293003/diff/20001/components/nacl/loader/...
File components/nacl/loader/nacl_trusted_listener.cc (right):

https://codereview.chromium.org/1153293003/diff/20001/components/nacl/loader/...
components/nacl/loader/nacl_trusted_listener.cc:18: void OnChannelError()
override {
On 2015/06/10 00:31:04, Mark Seaborn wrote:
> On 2015/06/09 15:43:44, jvoung wrote:
> > It wasn't too clear if this should also override OnChannelClosing, but there
> is
> > a lot of instances of one or the other and not many instances of both from
> code
> > search.
> 
> From reading ipc/, it looks like OnChannelClosing() is only called as a result
> of someone calling channel->Close().  So:
>  * Close() -> OnChannelClosing(): means our channel endpoint is closed.
>  * OnChannelError(): means the channel's other endpoint was closed.
> 
> channel->Close() should never happen on this channel.  There is a
> channel_->Close() call in NaClTrustedListener::OnChannelError() below, but:
> 
>  * It's probably unnecessary (just boilerplate?)
>  * NaClTrustedListener::OnChannelError() never actually gets called.  A call
to
> it gets queued on the main thread, but the main thread gets blocked in
> NaClChromeMainAppStart().
> 
> I could remove NaClTrustedListener::OnChannelError(), if you think it would
make
> the code clearer?

Okay removing NaClTrustedListener::OnChannelError() seems clearer to me, if it's
not actually being called / can't be called.

[Mark Seaborn, 2015-06-10 17:05:21]

https://codereview.chromium.org/1153293003/diff/20001/components/nacl/loader/...
File components/nacl/loader/nacl_trusted_listener.cc (right):

https://codereview.chromium.org/1153293003/diff/20001/components/nacl/loader/...
components/nacl/loader/nacl_trusted_listener.cc:18: void OnChannelError()
override {
On 2015/06/10 16:33:01, jvoung wrote:
> On 2015/06/10 00:31:04, Mark Seaborn wrote:
> > On 2015/06/09 15:43:44, jvoung wrote:
> > > It wasn't too clear if this should also override OnChannelClosing, but
there
> > is
> > > a lot of instances of one or the other and not many instances of both from
> > code
> > > search.
> > 
> > From reading ipc/, it looks like OnChannelClosing() is only called as a
result
> > of someone calling channel->Close().  So:
> >  * Close() -> OnChannelClosing(): means our channel endpoint is closed.
> >  * OnChannelError(): means the channel's other endpoint was closed.
> > 
> > channel->Close() should never happen on this channel.  There is a
> > channel_->Close() call in NaClTrustedListener::OnChannelError() below, but:
> > 
> >  * It's probably unnecessary (just boilerplate?)
> >  * NaClTrustedListener::OnChannelError() never actually gets called.  A call
> to
> > it gets queued on the main thread, but the main thread gets blocked in
> > NaClChromeMainAppStart().
> > 
> > I could remove NaClTrustedListener::OnChannelError(), if you think it would
> make
> > the code clearer?
> 
> Okay removing NaClTrustedListener::OnChannelError() seems clearer to me, if
it's
> not actually being called / can't be called.

OK, done.

[Mark Seaborn, 2015-06-10 17:12:42]

+bradnelson, mcgrathr and gdeepti, as people who might be interested in the GDB
debug stub:

This CL changes the debug stub's behaviour slightly when a NaCl process is
terminated by closing a tab or navigating away.

Before, the developer would see this in GDB:

> (gdb) cont
> Continuing.
> 
> Program terminated with signal SIGKILL, Killed.
> The program no longer exists.

After, they'll see this:

> (gdb) cont
> Continuing.
> Remote connection closed

(The latter is also what you currently get when killing a NaCl process using the
Task Manager.)

Is this OK with you?  While I could try to preserve the original behaviour of
reporting "SIGKILL" as an exit status (added in https://crbug.com/157881), that
involves more work and complexity, so I'm not sure it's worthwhile.

[Roland McGrath, 2015-06-10 18:24:08]

On 2015/06/10 17:12:42, Mark Seaborn wrote:
> +bradnelson, mcgrathr and gdeepti, as people who might be interested in the
GDB
> debug stub:
> 
> This CL changes the debug stub's behaviour slightly when a NaCl process is
> terminated by closing a tab or navigating away.
> 
> Before, the developer would see this in GDB:
> 
> > (gdb) cont
> > Continuing.
> > 
> > Program terminated with signal SIGKILL, Killed.
> > The program no longer exists.
> 
> After, they'll see this:
> 
> > (gdb) cont
> > Continuing.
> > Remote connection closed
> 
> (The latter is also what you currently get when killing a NaCl process using
the
> Task Manager.)
> 
> Is this OK with you?  While I could try to preserve the original behaviour of
> reporting "SIGKILL" as an exit status (added in https://crbug.com/157881),
that
> involves more work and complexity, so I'm not sure it's worthwhile.

I don't see a problem with that change in behavior.

[bradnelson, 2015-06-11 19:07:30]

bradnelson@chromium.org changed reviewers:
+ bradnelson@chromium.org

[bradnelson, 2015-06-11 19:07:31]

lgtm

[bradn, 2015-06-11 19:51:58]

bradnelson@google.com changed reviewers:
+ bradnelson@google.com

[bradn, 2015-06-11 19:51:59]

https://codereview.chromium.org/1153293003/diff/60001/chrome/test/data/nacl/g...
File chrome/test/data/nacl/gdb_rsp.py (right):

https://codereview.chromium.org/1153293003/diff/60001/chrome/test/data/nacl/g...
chrome/test/data/nacl/gdb_rsp.py:20: 
extra line?

[Mark Seaborn, 2015-06-11 20:05:11]

https://codereview.chromium.org/1153293003/diff/60001/chrome/test/data/nacl/g...
File chrome/test/data/nacl/gdb_rsp.py (right):

https://codereview.chromium.org/1153293003/diff/60001/chrome/test/data/nacl/g...
chrome/test/data/nacl/gdb_rsp.py:20: 
On 2015/06/11 19:51:59, bradn wrote:
> extra line?

Well, it's consistent with "One blank line between method definitions and
between the class line and the first method."
(https://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Namin...)

[Mark Seaborn, 2015-06-11 20:06:20]

The CQ bit was checked by mseaborn@chromium.org

[Mark Seaborn, 2015-06-11 20:06:20]

The patchset sent to the CQ was uploaded after l-g-t-m from jvoung@chromium.org
Link to the patchset: https://codereview.chromium.org/1153293003/#ps60001
(title: "Remove uncalled OnChannelError()")

[commit-bot: I haz the power, 2015-06-11 20:07:37]

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

[commit-bot: I haz the power, 2015-06-11 21:44:41]

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

[commit-bot: I haz the power, 2015-06-11 21:44:42]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mark Seaborn, 2015-06-11 21:46:51]

The CQ bit was checked by mseaborn@chromium.org

[commit-bot: I haz the power, 2015-06-11 21:47:19]

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

[commit-bot: I haz the power, 2015-06-11 23:36:13]

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

[commit-bot: I haz the power, 2015-06-11 23:36:14]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mark Seaborn, 2015-06-11 23:38:14]

The CQ bit was checked by mseaborn@chromium.org

[commit-bot: I haz the power, 2015-06-11 23:38:39]

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

[commit-bot: I haz the power, 2015-06-12 00:49:39]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-06-12 00:50:42]

Patchset 4 (id:??) landed as
https://crrev.com/2eeecfc4babe46bef3a1a4946a6a77e17b9179fa
Cr-Commit-Position: refs/heads/master@{#334097}