Ensure that errors that occur during load will get crash log info to JS console

Ensure that errors that occur during load will get crash log info to JS console

Tested this by running Duke Nukem w/i NaClBox with
http://codereview.chromium.org/10823018/ patched into native_client,
using the command line:

bash -c 'ulimit -v 1000000;  /mm/bsy/chromium/chromium64.git0/src/out/Release/chrome --user-data-dir=/tmp/udd'

to verify that the crash log shows up.

BUG= http://code.google.com/p/nativeclient/issues/detail?id=2776
TEST= Manual.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148470

[bsy, 2012-07-25 20:21:40]

please take a look

[bbudge, 2012-07-25 20:42:27]

http://codereview.chromium.org/10823019/diff/1/ppapi/native_client/src/truste...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

http://codereview.chromium.org/10823019/diff/1/ppapi/native_client/src/truste...
ppapi/native_client/src/trusted/plugin/plugin.cc:1195: " suppressing\n"));
This will now CopyCrashLogToJsConsole() instead of return. Couldn't that now be
called multiple times?

http://codereview.chromium.org/10823019/diff/1/ppapi/native_client/src/truste...
ppapi/native_client/src/trusted/plugin/plugin.cc:1201:
error_info.SetReport(ERROR_START_PROXY_CRASH,  // Not quite right.
I realize this was here before, but can we improve this comment? I'm not sure
what this means.

http://codereview.chromium.org/10823019/diff/1/ppapi/native_client/src/truste...
File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right):

http://codereview.chromium.org/10823019/diff/1/ppapi/native_client/src/truste...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:667: // start_module
RPC reply has been received.
This comment is hard for me to understand. How about this?

// On a load failure the service runtime does not crash itself to avoid a race
// where the no-more-senders error on the reverse channel service thread
// might cause the crash-detection logic to kick in before the start_module
// RPC reply has been received. So we induce a service runtime crash here.
// We do not release subprocess_ since it's needed to collect crash log
// output after the error is reported.

[bsy, 2012-07-25 22:07:23]

ptal

http://codereview.chromium.org/10823019/diff/1/ppapi/native_client/src/truste...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

http://codereview.chromium.org/10823019/diff/1/ppapi/native_client/src/truste...
ppapi/native_client/src/trusted/plugin/plugin.cc:1195: " suppressing\n"));
On 2012/07/25 20:42:27, bbudge1 wrote:
> This will now CopyCrashLogToJsConsole() instead of return. Couldn't that now
be
> called multiple times?

yes, but that's safe.  i added to the comment just above the
CopyCrashLogToJsConsole below.

http://codereview.chromium.org/10823019/diff/1/ppapi/native_client/src/truste...
ppapi/native_client/src/trusted/plugin/plugin.cc:1201:
error_info.SetReport(ERROR_START_PROXY_CRASH,  // Not quite right.
On 2012/07/25 20:42:27, bbudge1 wrote:
> I realize this was here before, but can we improve this comment? I'm not sure
> what this means.

Done.

http://codereview.chromium.org/10823019/diff/1/ppapi/native_client/src/truste...
File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right):

http://codereview.chromium.org/10823019/diff/1/ppapi/native_client/src/truste...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:667: // start_module
RPC reply has been received.
On 2012/07/25 20:42:27, bbudge1 wrote:
> This comment is hard for me to understand. How about this?
> 
> // On a load failure the service runtime does not crash itself to avoid a race
> // where the no-more-senders error on the reverse channel service thread
> // might cause the crash-detection logic to kick in before the start_module
> // RPC reply has been received. So we induce a service runtime crash here.
> // We do not release subprocess_ since it's needed to collect crash log
> // output after the error is reported.

Done.

[bbudge, 2012-07-25 22:23:14]

lgtm

[commit-bot: I haz the power, 2012-07-25 22:31:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bsy@google.com/10823019/5

[commit-bot: I haz the power, 2012-07-26 01:31:50]

Change committed as 148470