Add a load_status callback hook that is invoked at the end of chrome LoadApp.

Add a load_status callback hook that is invoked at the end of chrome LoadApp.

This callback will be used to communicate the load status (success
or error) in the place of the start_module SRPC call, so that we can
deprecate the "start_module" call later. Blocking on "start_module"
is kept for now.

Next step is to use it in chrome like so:
https://codereview.chromium.org/1090233003/

When the hook is set, assume that we want to reap logs on error,
so that we can remove the SRPC "log" command as well.

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

Committed: https://chromium.googlesource.com/native_client/src/native_client/+/2d7fb8345670874fb63d715a18a5ce6fd69d3cb5

[jvoung (off chromium), 2015-04-17 22:50:03]

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

[Mark Seaborn, 2015-04-17 23:15:54]

https://codereview.chromium.org/1089323006/diff/40001/src/public/chrome_main.h
File src/public/chrome_main.h (right):

https://codereview.chromium.org/1089323006/diff/40001/src/public/chrome_main....
src/public/chrome_main.h:242: void NaClSetLoadStatusCallback(void (*func)(int
load_status));
Can you make this callback a field in NaClChromeMainArgs rather than a global,
please?

NaClSetFatalErrorCallback() only sets a global because it affects how
NaClLog()+LOG_FATAL is handled.

[jvoung (off chromium), 2015-04-18 00:58:17]

https://codereview.chromium.org/1089323006/diff/40001/src/public/chrome_main.h
File src/public/chrome_main.h (right):

https://codereview.chromium.org/1089323006/diff/40001/src/public/chrome_main....
src/public/chrome_main.h:242: void NaClSetLoadStatusCallback(void (*func)(int
load_status));
On 2015/04/17 23:15:54, Mark Seaborn wrote:
> Can you make this callback a field in NaClChromeMainArgs rather than a global,
> please?
> 
> NaClSetFatalErrorCallback() only sets a global because it affects how
> NaClLog()+LOG_FATAL is handled.

Done -- ah, didn't realize why that was global.

[Mark Seaborn, 2015-04-18 01:14:15]

LGTM, thanks!

https://codereview.chromium.org/1089323006/diff/60001/src/public/chrome_main.h
File src/public/chrome_main.h (right):

https://codereview.chromium.org/1089323006/diff/60001/src/public/chrome_main....
src/public/chrome_main.h:167: * NaClChromeMainStart is known (load_status is a
value from the
Can you also say that it's called from the thread that called
NaClChromeMainStart()?

https://codereview.chromium.org/1089323006/diff/60001/src/public/chrome_main....
src/public/chrome_main.h:168: * NaClErrorCode enumeration and may indicate
success or error).
Technically NaClErrorCode is not defined in src/public/ yet, so can you say:

load_status is zero on success, or a non-zero error code (from the NaClErrorCode
enumeration) on failure.

This way, at least success vs. failure can be determined via the src/public/
interface. :-)

https://codereview.chromium.org/1089323006/diff/60001/src/public/chrome_main....
src/public/chrome_main.h:169: */
Add: "Optional; may be NULL" to follow the style of the other comments.

https://codereview.chromium.org/1089323006/diff/60001/src/trusted/service_run...
File src/trusted/service_runtime/sel_main_chrome.c (right):

https://codereview.chromium.org/1089323006/diff/60001/src/trusted/service_run...
src/trusted/service_runtime/sel_main_chrome.c:386: * the load_status.
Nit: "... after Chromium no longer calls start_module"?

https://codereview.chromium.org/1089323006/diff/60001/src/trusted/service_run...
src/trusted/service_runtime/sel_main_chrome.c:394: NaClLog(LOG_ERROR, "reap
logs\n");
Can you put a less cryptic message here, please?

[jvoung (off chromium), 2015-04-20 18:49:47]

https://codereview.chromium.org/1089323006/diff/60001/src/public/chrome_main.h
File src/public/chrome_main.h (right):

https://codereview.chromium.org/1089323006/diff/60001/src/public/chrome_main....
src/public/chrome_main.h:167: * NaClChromeMainStart is known (load_status is a
value from the
On 2015/04/18 01:14:14, Mark Seaborn wrote:
> Can you also say that it's called from the thread that called
> NaClChromeMainStart()?

Good idea -- done.

https://codereview.chromium.org/1089323006/diff/60001/src/public/chrome_main....
src/public/chrome_main.h:168: * NaClErrorCode enumeration and may indicate
success or error).
On 2015/04/18 01:14:14, Mark Seaborn wrote:
> Technically NaClErrorCode is not defined in src/public/ yet, so can you say:
> 
> load_status is zero on success, or a non-zero error code (from the
NaClErrorCode
> enumeration) on failure.
> 
> This way, at least success vs. failure can be determined via the src/public/
> interface. :-)

Done.

https://codereview.chromium.org/1089323006/diff/60001/src/public/chrome_main....
src/public/chrome_main.h:169: */
On 2015/04/18 01:14:14, Mark Seaborn wrote:
> Add: "Optional; may be NULL" to follow the style of the other comments.

Done.

https://codereview.chromium.org/1089323006/diff/60001/src/trusted/service_run...
File src/trusted/service_runtime/sel_main_chrome.c (right):

https://codereview.chromium.org/1089323006/diff/60001/src/trusted/service_run...
src/trusted/service_runtime/sel_main_chrome.c:386: * the load_status.
On 2015/04/18 01:14:14, Mark Seaborn wrote:
> Nit: "... after Chromium no longer calls start_module"?

Done.

Hmm there is still a race condition if I leave out the
NaClBlockIfCommandChannelExists actually.

What happens is if there is a load error, then the initialization state is set
to ERROR, which makes NaClWaitForStartModuleCommand return immediately. Then
this process reports the error to the renderer and the loader exits.

The renderer/plugin currently will get the load error and report it -- sending a
progress event. However it will continue to try and do "StartNexe" anyway,
because that part isn't yet blocked on knowing the initial load status before
attempting StartNexe. So it will start setting up the command channel, but that
will fail without NaClBlockIfCommandChannelExists because the loader has already
exited. Then it will try to report a second error / send a second progress
event, but that will fail/cause confusion because an error has already been
reported.

So would it make sense to keep this NaClBlockIfCommandChannelExists for now
(whether or not there is a callback). Do the chrome side change for the
callback's initial setup. Remove the NaClWaitForStartModuleCommand on the NaCl
side. Change the chrome side again to wait for a load status before doing
SetupCommand and also remove that side's start_module call?

https://codereview.chromium.org/1089323006/diff/60001/src/trusted/service_run...
src/trusted/service_runtime/sel_main_chrome.c:394: NaClLog(LOG_ERROR, "reap
logs\n");
On 2015/04/18 01:14:14, Mark Seaborn wrote:
> Can you put a less cryptic message here, please?

Done.

[Mark Seaborn, 2015-04-21 16:41:23]

LGTM

https://codereview.chromium.org/1089323006/diff/120001/src/trusted/service_ru...
File src/trusted/service_runtime/sel_main_chrome.c (right):

https://codereview.chromium.org/1089323006/diff/120001/src/trusted/service_ru...
src/trusted/service_runtime/sel_main_chrome.c:395: /* Fall through and run
NaClBlockIfCommandChannelExists.
Nit: Use the NaCl style for multiline comments, with "/*" and "*/" on separate
lines

https://codereview.chromium.org/1089323006/diff/120001/src/trusted/service_ru...
src/trusted/service_runtime/sel_main_chrome.c:400: * Otherwise there is a race
between this processes's exit / load error
"process's"

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

https://codereview.chromium.org/1089323006/diff/120001/src/trusted/service_ru...
File src/trusted/service_runtime/sel_main_chrome.c (right):

https://codereview.chromium.org/1089323006/diff/120001/src/trusted/service_ru...
src/trusted/service_runtime/sel_main_chrome.c:395: /* Fall through and run
NaClBlockIfCommandChannelExists.
On 2015/04/21 16:41:23, Mark Seaborn wrote:
> Nit: Use the NaCl style for multiline comments, with "/*" and "*/" on separate
> lines

Done.

https://codereview.chromium.org/1089323006/diff/120001/src/trusted/service_ru...
src/trusted/service_runtime/sel_main_chrome.c:400: * Otherwise there is a race
between this processes's exit / load error
On 2015/04/21 16:41:23, Mark Seaborn wrote:
> "process's"

Done.

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

The CQ bit was checked by jvoung@chromium.org

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

The patchset sent to the CQ was uploaded after l-g-t-m from
mseaborn@chromium.org
Link to the patchset: https://codereview.chromium.org/1089323006/#ps140001
(title: "udpate comment")

[commit-bot: I haz the power, 2015-04-21 17:10:41]

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

[commit-bot: I haz the power, 2015-04-21 19:19:58]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/native_client/src/native_client/+/2d7fb8345...