Pepper: Report NaCl exit status over Chromium IPC.

Pepper: Report NaCl exit status over Chromium IPC.

This change uses a NaCl embedder interface for getting the exit status inside
the loader process instead of receiving that information over SRPC.

The IPC message introduced here must be synchronous so that we're guaranteed to
report the exit status before the loader process exits.

BUG=397161
TEST=NaClBrowserTest.ExitStatus*
R=dmichael@chromium.org, jschuh@chromium.org, mseaborn@chromium.org

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

[Mark Seaborn, 2014-08-21 21:12:32]

Could you say, in TEST=, which test covers the exit status reporting?

(As an aside: I think that's a test which isn't enabled for Non-SFI NaCl, and
which doesn't pass there, which we might want to fix later.)

https://codereview.chromium.org/484783002/diff/120001/components/nacl/common/...
File components/nacl/common/nacl_renderer_messages.h (right):

https://codereview.chromium.org/484783002/diff/120001/components/nacl/common/...
components/nacl/common/nacl_renderer_messages.h:12:
IPC_SYNC_MESSAGE_CONTROL1_0(NaClRendererMsg_ReportExitStatus,
Can you comment here why the message needs to be synchronous?

https://codereview.chromium.org/484783002/diff/120001/components/nacl/loader/...
File components/nacl/loader/nacl_listener.cc (right):

https://codereview.chromium.org/484783002/diff/120001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:427: // TODO(teravest): Use a separate
is_helper_process field instead of assuming
How about sending it unconditionally but having TrustedPluginChannel know
whether the message should be ignored or not?

https://codereview.chromium.org/484783002/diff/120001/components/nacl/rendere...
File components/nacl/renderer/trusted_plugin_channel.cc (right):

https://codereview.chromium.org/484783002/diff/120001/components/nacl/rendere...
components/nacl/renderer/trusted_plugin_channel.cc:49: if (load_manager)
When would this be NULL?  Should there be a CHECK or DCHECK here?

Do PNaCl translator processes have the PP_Instance associated with them?

[teravest, 2014-08-22 16:07:12]

I've added the relevant test to the TEST= line.

I'd like to get this working for Non-SFI as well and make sure that's tested,
though I don't see the need to do that in this change.

https://codereview.chromium.org/484783002/diff/120001/components/nacl/common/...
File components/nacl/common/nacl_renderer_messages.h (right):

https://codereview.chromium.org/484783002/diff/120001/components/nacl/common/...
components/nacl/common/nacl_renderer_messages.h:12:
IPC_SYNC_MESSAGE_CONTROL1_0(NaClRendererMsg_ReportExitStatus,
On 2014/08/21 21:12:32, Mark Seaborn wrote:
> Can you comment here why the message needs to be synchronous?

Done.

https://codereview.chromium.org/484783002/diff/120001/components/nacl/loader/...
File components/nacl/loader/nacl_listener.cc (right):

https://codereview.chromium.org/484783002/diff/120001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:427: // TODO(teravest): Use a separate
is_helper_process field instead of assuming
On 2014/08/21 21:12:32, Mark Seaborn wrote:
> How about sending it unconditionally but having TrustedPluginChannel know
> whether the message should be ignored or not?

Done.

https://codereview.chromium.org/484783002/diff/120001/components/nacl/rendere...
File components/nacl/renderer/trusted_plugin_channel.cc (right):

https://codereview.chromium.org/484783002/diff/120001/components/nacl/rendere...
components/nacl/renderer/trusted_plugin_channel.cc:49: if (load_manager)
On 2014/08/21 21:12:32, Mark Seaborn wrote:
> When would this be NULL?  Should there be a CHECK or DCHECK here?
> 
> Do PNaCl translator processes have the PP_Instance associated with them?

I've changed this to a DCHECK, storing a NexeLoadManager* in this class instead
of a PP_Instance.

Using PP_Instance, I think I can imagine a case where this would be NULL.
On instance destruction, the NexeLoadManager is removed from the global map of
NexeLoadManagers before it's destroyed. If this channel received this message
after it's removed from the map, but before it's destroyed, load_manager could
be NULL here.

PNaCl translator processes do have the PP_Instance associated with them.

[dmichael (off chromium), 2014-08-22 16:41:07]

lgtm

https://codereview.chromium.org/484783002/diff/140001/components/nacl/common/...
File components/nacl/common/nacl_renderer_messages.cc (right):

https://codereview.chromium.org/484783002/diff/140001/components/nacl/common/...
components/nacl/common/nacl_renderer_messages.cc:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
nit: I guess it's slightly preferred not to do the (c) anymore

https://codereview.chromium.org/484783002/diff/140001/ppapi/native_client/src...
File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right):

https://codereview.chromium.org/484783002/diff/140001/ppapi/native_client/src...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:240: void
PluginReverseInterface::ReportExitStatus(int exit_status) {
So can we get rid of this function?

[teravest, 2014-08-22 16:51:58]

https://codereview.chromium.org/484783002/diff/140001/ppapi/native_client/src...
File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right):

https://codereview.chromium.org/484783002/diff/140001/ppapi/native_client/src...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:240: void
PluginReverseInterface::ReportExitStatus(int exit_status) {
On 2014/08/22 16:41:07, dmichael wrote:
> So can we get rid of this function?

Not yet.
nacl::ReverseInterface::ReportExitStatus is pure virtual.

After this change, there could be another NaCl change to remove the function
there, and then this one could be removed.

[Mark Seaborn, 2014-08-22 17:36:55]

LGTM

https://codereview.chromium.org/484783002/diff/140001/components/nacl/rendere...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/484783002/diff/140001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:409: // TODO(teravest): Use a
separate is_helper_process field instead of
Do you think you'll do that soon after?  I was wondering if
https://code.google.com/p/nativeclient/issues/detail?id=3914 would get blocked
on this.

https://codereview.chromium.org/484783002/diff/140001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:411: bool report_exit_status =
PP_ToBool(uses_irt);
Does the current test catch the problem if this is always set to true?  I'm just
curious whether the test checks the exitStatus property before the process has
exited.

https://codereview.chromium.org/484783002/diff/140001/components/nacl/rendere...
File components/nacl/renderer/trusted_plugin_channel.h (right):

https://codereview.chromium.org/484783002/diff/140001/components/nacl/rendere...
components/nacl/renderer/trusted_plugin_channel.h:42: // Non-owning pointer.
Can you comment here why the non-owning pointer is memory-safe?  What ensures
that the lifetime of TrustedPluginChannel is <= that of NexeLoadManager?

[Mark Seaborn, 2014-08-22 17:38:44]

On 22 August 2014 09:07, <teravest@chromium.org> wrote:

> I'd like to get this working for Non-SFI as well and make sure that's
> tested,
> though I don't see the need to do that in this change.


Right, I wasn't asking you to do that in this change, which is why I said
"later". :-)  I was just thinking out loud when considering whether/how
this change affects Non-SFI mode.

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-08-22 17:39:00]

https://codereview.chromium.org/484783002/diff/140001/components/nacl/rendere...
File components/nacl/renderer/trusted_plugin_channel.h (right):

https://codereview.chromium.org/484783002/diff/140001/components/nacl/rendere...
components/nacl/renderer/trusted_plugin_channel.h:42: // Non-owning pointer.
On 2014/08/22 17:36:55, Mark Seaborn wrote:
> Can you comment here why the non-owning pointer is memory-safe?  What ensures
> that the lifetime of TrustedPluginChannel is <= that of NexeLoadManager?
NexeLoadManager owns the TrustedPluginChannel. Though a comment wouldn't hurt, I
suppose.

[teravest, 2014-08-22 18:54:56]

https://codereview.chromium.org/484783002/diff/140001/components/nacl/rendere...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/484783002/diff/140001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:409: // TODO(teravest): Use a
separate is_helper_process field instead of
On 2014/08/22 17:36:55, Mark Seaborn wrote:
> Do you think you'll do that soon after?  I was wondering if
> https://code.google.com/p/nativeclient/issues/detail?id=3914 would get blocked
> on this.

I just realized I can use the main_service_runtime argument which is passed
here. Changed the code to use that (and removed the todo)

https://codereview.chromium.org/484783002/diff/140001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:411: bool report_exit_status =
PP_ToBool(uses_irt);
On 2014/08/22 17:36:55, Mark Seaborn wrote:
> Does the current test catch the problem if this is always set to true?  I'm
just
> curious whether the test checks the exitStatus property before the process has
> exited.

I don't believe our current tests cover this. The exit status tests don't
translate a pexe, and I'm pretty sure the test that translates doesn't monitor
the exit status.

https://codereview.chromium.org/484783002/diff/140001/components/nacl/rendere...
File components/nacl/renderer/trusted_plugin_channel.h (right):

https://codereview.chromium.org/484783002/diff/140001/components/nacl/rendere...
components/nacl/renderer/trusted_plugin_channel.h:42: // Non-owning pointer.
On 2014/08/22 17:39:00, dmichael wrote:
> On 2014/08/22 17:36:55, Mark Seaborn wrote:
> > Can you comment here why the non-owning pointer is memory-safe?  What
ensures
> > that the lifetime of TrustedPluginChannel is <= that of NexeLoadManager?
> NexeLoadManager owns the TrustedPluginChannel. Though a comment wouldn't hurt,
I
> suppose.

I've added a comment.

[teravest, 2014-08-22 18:55:58]

+jschuh for components/nacl/common/nacl_renderer_messages.*

This change adds a sync IPC message for reporting the exit status of a NaCl
application. This replaces an existing SRPC message.

[jschuh, 2014-08-22 19:17:45]

ipc security lgtm

[teravest, 2014-08-22 19:45:35]

Committed patchset #5 manually as 291480 (presubmit successful).