Set up a NaCl load status callback to start replacing "start_module".

Set up a NaCl load status callback to start replacing "start_module".

Uses the hook from: https://codereview.chromium.org/1089323006/

The NaClListener will set up a callback, and when the load status
is known at the end of sel_main_chrome's LoadApp, it will use that
to send the load_status to the renderer via Chrome IPC, instead of
getting the load status from a "start_module" SRPC call.

The renderer will use that load_status to report UMA stats
and send progress events. This replaces the load_status coming
from the SRPC "start_module::i" invocation. However, the invocation
is kept for the moment because we currently block the sel_main_chrome
until it is received to avoid a race condition. After this lands,
we can remove the blocking and then remove the invocation.

This also replaces the call to ReapLogs since that will be done
once the load_status is known to be an error on the sel_main_chrome
side instead.

BUG= https://code.google.com/p/chromium/issues/detail?id=459250

Committed: https://crrev.com/6b7051836e5604d42723871b26c17146cc4f14c0
Cr-Commit-Position: refs/heads/master@{#326393}

[Mark Seaborn, 2015-04-17 23:30:58]

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

[Mark Seaborn, 2015-04-17 23:30:58]

https://codereview.chromium.org/1090233003/diff/1/components/nacl/loader/nacl...
File components/nacl/loader/nacl_listener.h (right):

https://codereview.chromium.org/1090233003/diff/1/components/nacl/loader/nacl...
components/nacl/loader/nacl_listener.h:61: NaClTrustedListener*
trusted_listener() { return trusted_listener_.get(); }
Nit: you could put this next to crash_info_shmem_memory() since they're used in
the same way.  Also add "const" to match.

https://codereview.chromium.org/1090233003/diff/1/components/nacl/renderer/tr...
File components/nacl/renderer/trusted_plugin_channel.cc (right):

https://codereview.chromium.org/1090233003/diff/1/components/nacl/renderer/tr...
components/nacl/renderer/trusted_plugin_channel.cc:67: // and not for helper
nexe (like the PNaCl translators).
Nit: "nexes"

https://codereview.chromium.org/1090233003/diff/1/components/nacl/renderer/tr...
File components/nacl/renderer/trusted_plugin_channel.h (right):

https://codereview.chromium.org/1090233003/diff/1/components/nacl/renderer/tr...
components/nacl/renderer/trusted_plugin_channel.h:31: bool
main_service_runtime);
This name is rather cryptic and its meaning isn't commented in the existing
code.  Maybe you could use a more descriptive name such as "is_helper_nexe" or
"is_pnacl_translator", and invert the variable?

[jvoung (off chromium), 2015-04-18 01:00:47]

https://codereview.chromium.org/1090233003/diff/1/components/nacl/loader/nacl...
File components/nacl/loader/nacl_listener.h (right):

https://codereview.chromium.org/1090233003/diff/1/components/nacl/loader/nacl...
components/nacl/loader/nacl_listener.h:61: NaClTrustedListener*
trusted_listener() { return trusted_listener_.get(); }
On 2015/04/17 23:30:58, Mark Seaborn wrote:
> Nit: you could put this next to crash_info_shmem_memory() since they're used
in
> the same way.  Also add "const" to match.

Done.

https://codereview.chromium.org/1090233003/diff/1/components/nacl/renderer/tr...
File components/nacl/renderer/trusted_plugin_channel.cc (right):

https://codereview.chromium.org/1090233003/diff/1/components/nacl/renderer/tr...
components/nacl/renderer/trusted_plugin_channel.cc:67: // and not for helper
nexe (like the PNaCl translators).
On 2015/04/17 23:30:58, Mark Seaborn wrote:
> Nit: "nexes"

Done.

https://codereview.chromium.org/1090233003/diff/1/components/nacl/renderer/tr...
File components/nacl/renderer/trusted_plugin_channel.h (right):

https://codereview.chromium.org/1090233003/diff/1/components/nacl/renderer/tr...
components/nacl/renderer/trusted_plugin_channel.h:31: bool
main_service_runtime);
On 2015/04/17 23:30:58, Mark Seaborn wrote:
> This name is rather cryptic and its meaning isn't commented in the existing
> code.  Maybe you could use a more descriptive name such as "is_helper_nexe" or
> "is_pnacl_translator", and invert the variable?

Done.

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

https://codereview.chromium.org/1090233003/diff/20001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:509:
!PP_ToBool(main_service_runtime)));
I think we can replace this with checking the "PP_NaClAppProcessType
pp_process_type" instead, but I haven't done that.

[Mark Seaborn, 2015-04-18 14:07:53]

LGTM.  Thanks for doing this!

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

https://codereview.chromium.org/1090233003/diff/20001/components/nacl/common/...
components/nacl/common/nacl_renderer_messages.h:20: int /* exit_status */)
"load_status", to distinguish from NaClRendererMsg_ReportExitStatus.

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

https://codereview.chromium.org/1090233003/diff/20001/components/nacl/rendere...
components/nacl/renderer/plugin/service_runtime.cc:92: // sel_main_chrome, but
the load_status is obtained by a different hook.
Nit: How about "sel_main_chrome.c on the NaCl side" to make it clearer where to
look?

https://codereview.chromium.org/1090233003/diff/20001/components/nacl/rendere...
File components/nacl/renderer/ppb_nacl_private_impl.cc (left):

https://codereview.chromium.org/1090233003/diff/20001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:504: bool report_exit_status =
PP_ToBool(main_service_runtime);
Nit: How about:

bool is_helper_nexe = !PP_ToBool(main_service_runtime);
// Then pass is_helper_nexe as the argument.

This just names the argument to make it clearer what its meaning is without
having to look at TrustedPluginChannel's constructor.

[denielamokachi, 2015-04-18 14:24:34]

denielamokachi@gmail.com changed reviewers:
+ denielamokachi@gmail.com

[denielamokachi, 2015-04-18 14:24:34]

lgtm

[denielamokachi, 2015-04-19 14:10:56]

denielamokachi@gmail.com changed reviewers:
+ g@e.ajt.im
- mseaborn@chromium.org

[denielamokachi, 2015-04-19 14:10:57]

lgtm

[jvoung (off chromium), 2015-04-20 17:24:32]

jvoung@chromium.org changed reviewers:
+ mseaborn@chromium.org
- denielamokachi@gmail.com, g@e.ajt.im

[jvoung (off chromium), 2015-04-21 17:11:21]

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

https://codereview.chromium.org/1090233003/diff/20001/components/nacl/common/...
components/nacl/common/nacl_renderer_messages.h:20: int /* exit_status */)
On 2015/04/18 14:07:53, Mark Seaborn wrote:
> "load_status", to distinguish from NaClRendererMsg_ReportExitStatus.

Oops copy-paste error -- Done.

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

https://codereview.chromium.org/1090233003/diff/20001/components/nacl/rendere...
components/nacl/renderer/plugin/service_runtime.cc:92: // sel_main_chrome, but
the load_status is obtained by a different hook.
On 2015/04/18 14:07:53, Mark Seaborn wrote:
> Nit: How about "sel_main_chrome.c on the NaCl side" to make it clearer where
to
> look?

Done.

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

https://codereview.chromium.org/1090233003/diff/40001/components/nacl/rendere...
components/nacl/renderer/plugin/service_runtime.cc:109:
GetNaClInterface()->ReportSelLdrStatus(pp_instance_,
Also removed this, now that it isn't used.

[jvoung (off chromium), 2015-04-21 23:25:00]

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

[Mark Seaborn, 2015-04-21 23:29:56]

LGTM

https://codereview.chromium.org/1090233003/diff/60001/components/nacl/rendere...
File components/nacl/renderer/ppb_nacl_private_impl.cc (left):

https://codereview.chromium.org/1090233003/diff/60001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:1496: // Gather data to see if
being installed changes load outcomes.
Keep this comment in your replacement in trusted_plugin_channel.cc?

[jvoung (off chromium), 2015-04-21 23:32:34]

+ Will for components/nacl/common/nacl_renderer_messages.h OWNERS

[jvoung (off chromium), 2015-04-21 23:38:49]

https://codereview.chromium.org/1090233003/diff/60001/components/nacl/rendere...
File components/nacl/renderer/ppb_nacl_private_impl.cc (left):

https://codereview.chromium.org/1090233003/diff/60001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:1496: // Gather data to see if
being installed changes load outcomes.
On 2015/04/21 23:29:56, Mark Seaborn wrote:
> Keep this comment in your replacement in trusted_plugin_channel.cc?

Done.

[Will Harris, 2015-04-22 00:41:01]

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

https://codereview.chromium.org/1090233003/diff/80001/components/nacl/common/...
components/nacl/common/nacl_renderer_messages.h:20: int /* load_status */)
Any reason why you use int here?

This should be enum NaClErrorCode with IPC_ENUM_TRAITS_MAX_VALUE set to
NACL_ERROR_CODE_MAX.

http://www.chromium.org/Home/chromium-security/education/security-tips-for-ip....

[Mark Seaborn, 2015-04-22 21:26:59]

On 21 April 2015 at 17:41, <wfh@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1090233003/diff/80001/components/nacl/common/...
> File components/nacl/common/nacl_renderer_messages.h (right):
>
>
>
https://codereview.chromium.org/1090233003/diff/80001/components/nacl/common/...
> components/nacl/common/nacl_renderer_messages.h:20: int /* load_status
> */)
> Any reason why you use int here?
>
> This should be enum NaClErrorCode with IPC_ENUM_TRAITS_MAX_VALUE set to
> NACL_ERROR_CODE_MAX.
>
>
>
http://www.chromium.org/Home/chromium-security/education/security-tips-for-ip...
> .
>

Sounds OK to me.  Jan, can I suggest you add nacl_error_code.h to DEPS as
needed for now, and we can clean up those DEPS entries afterwards?

Removing "start_module" is more beneficial as a cleanup than minimising
#includes of native_client headers, so I wouldn't want the latter to block
the former. :-)

Cheers,
Mark

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

[jvoung (off chromium), 2015-04-22 21:45:21]

Okay added the DEPS for that.

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

https://codereview.chromium.org/1090233003/diff/80001/components/nacl/common/...
components/nacl/common/nacl_renderer_messages.h:20: int /* load_status */)
On 2015/04/22 00:41:01, Will Harris wrote:
> Any reason why you use int here?
> 
> This should be enum NaClErrorCode with IPC_ENUM_TRAITS_MAX_VALUE set to
> NACL_ERROR_CODE_MAX.
> 
>
http://www.chromium.org/Home/chromium-security/education/security-tips-for-ip....

Done.

[Will Harris, 2015-04-22 22:19:19]

lgtm. thanks!

[jvoung (off chromium), 2015-04-22 22:27:35]

The CQ bit was checked by jvoung@chromium.org

[jvoung (off chromium), 2015-04-22 22:27:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
denielamokachi@gmail.com, mseaborn@chromium.org
Link to the patchset: https://codereview.chromium.org/1090233003/#ps100001
(title: "use the enum instead of int")

[commit-bot: I haz the power, 2015-04-22 22:29:41]

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

[commit-bot: I haz the power, 2015-04-22 23:24:29]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-04-22 23:25:26]

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

[horo, 2015-04-23 03:53:09]

I think this change make nacl_helper bigger than the expectation (8529465 byte).
https://code.google.com/p/chromium/codesearch#chromium/src/tools/perf_expecta...

Could you please update perf_expectations.json to fix the build failure?
https://crbug.com/480093