NaCl: Detect plugin crashes via EOF on Chromium IPC.

NaCl: Detect plugin crashes via EOF on Chromium IPC.

This change uses the OnChannelError() callback in TrustedPluginChannel to
detect that the channel between the renderer and NaCl processes has closed.
Previously, closure of the SRPC channel between those two processes was used to
indicate a plugin crash.

I wasn't sure at first that OnChannelError() would be called, but it appears to
be called reliably in the testing that I've done.

This change removes a call to ReportCrash() in ServiceRuntime::ReapLogs() since
ReportCrash() has become a no-op.

This change reenables the NaClBrowserTest*.Crash tests on Linux to ensure that
there's good coverage-- they seem to pass.

TEST=NaClBrowserTest*.Crash
BUG=366334, 401105
R=mseaborn@chromium.org

Committed: https://chromium.googlesource.com/chromium/src/+/f3e9e0cb8d6dc6f0a6f5cec9180c0e538e76c184

[teravest, 2014-08-28 18:52:40]

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

[Mark Seaborn, 2014-08-28 20:58:40]

Nit on commit message: Maybe say "Detect plugin crashes via EOF on Chromium IPC"
rather than "Report..." (because, when the NaCl process crashes, no message is
sent to report the crash)?

This is being a bit picky, but it might help others understand the change more
readily when looking through the change history.

https://codereview.chromium.org/512323002/diff/1/chrome/test/nacl/nacl_browse...
File chrome/test/nacl/nacl_browsertest.cc (left):

https://codereview.chromium.org/512323002/diff/1/chrome/test/nacl/nacl_browse...
chrome/test/nacl/nacl_browsertest.cc:111: // crbug.com/366334
Can you add BUG=366334 to the commit message, then, so that Bugdroid updates
that issue?

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

https://codereview.chromium.org/512323002/diff/1/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:504:
rev_interface_->ReportCrash();
It took me a while to understand this change.

Basically: removing this ReportCrash() call is, by itself, a simple cleanup that
doesn't change behaviour, because you also changed ReportCrash() (above) to be a
no-op.

Could you state that explicitly in the commit message?

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

https://codereview.chromium.org/512323002/diff/1/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:477: //
TODO(teravest): Now that crashes are reported over Chrome IPC instead of
Nit: "reported over" -> "detected via"

since the NaCl process doesn't send any messages when it crashes, it just
generates an EOF.

https://codereview.chromium.org/512323002/diff/1/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:478: // through the
reverse service, allow the service runtime to crash itself
Nit: "service runtime" -> "NaCl process" or "service_runtime in the NaCl
process" to be a little clearer?

https://codereview.chromium.org/512323002/diff/1/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:479: // when a module
fails to start.
I find this paragraph a bit confusing.  It says "Now that X, do Y".  But the
code was doing Y (i.e. telling the NaCl process to crash itself) before this
change.

Oh, I noticed that it's a TODO.  Do you mean "TODO: We *should* allow the
service_runtime to crash itself..."?

[teravest, 2014-09-03 15:18:22]

Hopefully this change is a bit easier to understand now, thanks.

https://codereview.chromium.org/512323002/diff/1/chrome/test/nacl/nacl_browse...
File chrome/test/nacl/nacl_browsertest.cc (left):

https://codereview.chromium.org/512323002/diff/1/chrome/test/nacl/nacl_browse...
chrome/test/nacl/nacl_browsertest.cc:111: // crbug.com/366334
On 2014/08/28 20:58:39, Mark Seaborn wrote:
> Can you add BUG=366334 to the commit message, then, so that Bugdroid updates
> that issue?

Done.

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

https://codereview.chromium.org/512323002/diff/1/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:504:
rev_interface_->ReportCrash();
On 2014/08/28 20:58:39, Mark Seaborn wrote:
> It took me a while to understand this change.
> 
> Basically: removing this ReportCrash() call is, by itself, a simple cleanup
that
> doesn't change behaviour, because you also changed ReportCrash() (above) to be
a
> no-op.
> 
> Could you state that explicitly in the commit message?

Done.

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

https://codereview.chromium.org/512323002/diff/1/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:477: //
TODO(teravest): Now that crashes are reported over Chrome IPC instead of
On 2014/08/28 20:58:39, Mark Seaborn wrote:
> Nit: "reported over" -> "detected via"
> 
> since the NaCl process doesn't send any messages when it crashes, it just
> generates an EOF.

I've reworked this whole block since it was confusing.

https://codereview.chromium.org/512323002/diff/1/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:478: // through the
reverse service, allow the service runtime to crash itself
On 2014/08/28 20:58:39, Mark Seaborn wrote:
> Nit: "service runtime" -> "NaCl process" or "service_runtime in the NaCl
> process" to be a little clearer?

Done.

https://codereview.chromium.org/512323002/diff/1/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:479: // when a module
fails to start.
On 2014/08/28 20:58:39, Mark Seaborn wrote:
> I find this paragraph a bit confusing.  It says "Now that X, do Y".  But the
> code was doing Y (i.e. telling the NaCl process to crash itself) before this
> change.
> 
> Oh, I noticed that it's a TODO.  Do you mean "TODO: We *should* allow the
> service_runtime to crash itself..."?

Yes, that's what I meant by the TODO. Is it clearer now?

[teravest, 2014-09-03 17:58:01]

I didn't see an email go out for my last set of comments, so I'm republishing.

[Mark Seaborn, 2014-09-03 23:28:59]

LGTM

[teravest, 2014-09-04 00:28:28]

The CQ bit was checked by teravest@chromium.org

[commit-bot: I haz the power, 2014-09-04 00:30:00]

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

[commit-bot: I haz the power, 2014-09-04 01:18:48]

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

[commit-bot: I haz the power, 2014-09-04 01:18:49]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[teravest, 2014-09-04 14:44:35]

Committed patchset #3 (id:40001) manually as f3e9e0c (presubmit successful).

[commit-bot: I haz the power, 2014-09-10 03:31:23]

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