Exit cleanly for the SRPC "log" requests instead of generating crash reports.

Exit cleanly for the SRPC "log" requests instead of generating crash reports.

This is to avoid spamming the crash report server for
things like validation errors, DEP unsupported, "read
error", or no memory for address space of which
we already have UMA stats for. Of those, validation is
currently the lowest ranking one.

NaClLog(LOG_FATAL, ...) would have caused the code to
run a special "gNaClLogAbortBehavior" hook, which ends up
copying data to the renderer:
https://codereview.chromium.org/469423002/

This change downgrades from LOG_FATAL to LOG_ERROR,
exposes the hook, and manually calls the hook. It leaves other
types of NaClLog(LOG_FATAL, ...) alone and allows generating
a crash report and backtrace.

See Chrome's ServiceRuntime::ReapLogs for the source of
this RPC call:
https://code.google.com/p/chromium/codesearch#chromium/src/components/nacl/renderer/plugin/service_runtime.cc&sq=package:chromium&l=225&q=ReapLogs

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

TEST=

(a) ./scons run_nacl_error_log_test (still runs the
hooks and aborts)

(b) patch into chrome and build/run browser_tests.

"NativeClient: Validation failure..." from NaClBrowserTestNewlib.BadNative

and

"NativeClient: Bad ELF header magic number" from NaClBrowserTestNewlib.Bad

still show up.

(c) Also check manually check the javascript console to see the "reap logs"
and other buffered logs show up.

Committed: https://chromium.googlesource.com/native_client/src/native_client/+/72e0e5f8ec337dcdf90f1a11854a19f9cd8c3399

[jvoung (off chromium), 2015-03-10 22:57:01]

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

[jvoung (off chromium), 2015-03-10 23:37:34]

Is this what you were thinking of Mark?

[Mark Seaborn, 2015-03-11 01:26:14]

LGTM, thanks, but one question...

https://codereview.chromium.org/997683002/diff/1/src/trusted/service_runtime/...
File src/trusted/service_runtime/nacl_secure_service.c (right):

https://codereview.chromium.org/997683002/diff/1/src/trusted/service_runtime/...
src/trusted/service_runtime/nacl_secure_service.c:260: if (LOG_FATAL ==
severity) {
Nit: Use the NaCl style for multiline comments, with "/*" and "*/" on separate
lines, like this:
/*
 * Blah...
 * blah.
 */

https://codereview.chromium.org/997683002/diff/1/src/trusted/service_runtime/...
src/trusted/service_runtime/nacl_secure_service.c:267: NaClExit(1);
You wrote:

> NaClLog(LOG_FATAL, ...) would have caused the code to
> run a special "gNaClLogAbortBehavior" hook, which ends up
> copying data to the renderer

Does this change mean we lose some useful error reporting in the Javascript
console about why NaCl process startup failed?

Did you test an error case manually?  Can you add a TEST= line, please?

If this would lose some error reporting, should we call NaClReportLogMessages()
here (without its NaClAbort() call)?

[jvoung (off chromium), 2015-03-11 18:02:06]

https://codereview.chromium.org/997683002/diff/1/src/trusted/service_runtime/...
File src/trusted/service_runtime/nacl_secure_service.c (right):

https://codereview.chromium.org/997683002/diff/1/src/trusted/service_runtime/...
src/trusted/service_runtime/nacl_secure_service.c:260: if (LOG_FATAL ==
severity) {
On 2015/03/11 01:26:14, Mark Seaborn wrote:
> Nit: Use the NaCl style for multiline comments, with "/*" and "*/" on separate
> lines, like this:
> /*
>  * Blah...
>  * blah.
>  */

Done.

https://codereview.chromium.org/997683002/diff/1/src/trusted/service_runtime/...
src/trusted/service_runtime/nacl_secure_service.c:267: NaClExit(1);
On 2015/03/11 01:26:14, Mark Seaborn wrote:
> You wrote:
> 
> > NaClLog(LOG_FATAL, ...) would have caused the code to
> > run a special "gNaClLogAbortBehavior" hook, which ends up
> > copying data to the renderer
> 
> Does this change mean we lose some useful error reporting in the Javascript
> console about why NaCl process startup failed?
> 
> Did you test an error case manually?  Can you add a TEST= line, please?
> 
> If this would lose some error reporting, should we call
NaClReportLogMessages()
> here (without its NaClAbort() call)?

I'm not sure if they are useful. For bad elf header and validation failures, we
mostly repeat in the NaClLog what shows up in the lastError string.

Just in case, I went ahead and added a getter for the set hook
(NaClReportLogMessages()), and called that hook here. I changed the hook to not
abort by itself, since the usual caller of the hook will abort anyway (and we
don't want to abort here).

Added some detail to the TEST= line

[Mark Seaborn, 2015-03-11 19:08:13]

LGTM

[jvoung (off chromium), 2015-03-11 20:06:36]

The CQ bit was checked by jvoung@chromium.org

[commit-bot: I haz the power, 2015-03-11 20:06:59]

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

[commit-bot: I haz the power, 2015-03-11 20:35:31]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/native_client/src/native_client/+/72e0e5f8e...