Detect crashes while running native tests in APK.

Detect crashes while running native tests in APK.

Adding signal handlers (for fatal signals) to output a marker indicating
that the test crashed.
BUG=125059
TEST=


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

[John Grabowski, 2012-05-07 19:22:15]

http://codereview.chromium.org/10310046/diff/1/build/android/run_tests.py
File build/android/run_tests.py (right):

http://codereview.chromium.org/10310046/diff/1/build/android/run_tests.py#new...
build/android/run_tests.py:196: return TestResults.FromOkFailedAndCrashed(
Seems a bit verbose now; how about just TestResults.FromRun() ?

http://codereview.chromium.org/10310046/diff/1/build/android/test_package.py
File build/android/test_package.py (right):

http://codereview.chromium.org/10310046/diff/1/build/android/test_package.py#...
build/android/test_package.py:138: re_crash = re.compile('\[ CRASHED     
\](.*)\r\n')
It seems illogical a test will announce it crashed.
Comments in this file seem apropos.

http://codereview.chromium.org/10310046/diff/1/build/android/test_result.py
File build/android/test_result.py (right):

http://codereview.chromium.org/10310046/diff/1/build/android/test_result.py#n...
build/android/test_result.py:64: def FromOkFailedAndCrashed(ok, failed, crashed,
timed_out, overall_fail):
That's a lot of arguments.
Can you make all callers pass by name?  E.g.
ok=[], failed=[], ...
so input is more clear?

http://codereview.chromium.org/10310046/diff/1/testing/android/native_test_la...
File testing/android/native_test_launcher.cc (right):

http://codereview.chromium.org/10310046/diff/1/testing/android/native_test_la...
testing/android/native_test_launcher.cc:3: // found in the LICENSE file.
Add explicit doc on our model: catch signals and print 'CRASHED' to help python
test runner blah blah catch blah.

http://codereview.chromium.org/10310046/diff/1/testing/android/native_test_la...
testing/android/native_test_launcher.cc:40: LOG(ERROR) << "[ CRASHED      ]";
Normally you cannot allocate memory in a signal handler.
Class LogMessage has an ostringstream member, which means it probably allocates
memory.

Consider __android_log_write() instead.

[Yaron, 2012-05-07 19:22:19]

lg but wait for jrg

http://codereview.chromium.org/10310046/diff/1/testing/android/native_test_la...
File testing/android/native_test_launcher.cc (right):

http://codereview.chromium.org/10310046/diff/1/testing/android/native_test_la...
testing/android/native_test_launcher.cc:29: // The list of signals which we
consider to be crashes. We rethrow the
Nit: Don't use "we" in comments.

[nilesh, 2012-05-08 01:38:10]

http://codereview.chromium.org/10310046/diff/1/build/android/run_tests.py
File build/android/run_tests.py (right):

http://codereview.chromium.org/10310046/diff/1/build/android/run_tests.py#new...
build/android/run_tests.py:196: return TestResults.FromOkFailedAndCrashed(
On 2012/05/07 19:22:15, John Grabowski wrote:
> Seems a bit verbose now; how about just TestResults.FromRun() ?

Done.

http://codereview.chromium.org/10310046/diff/1/build/android/test_package.py
File build/android/test_package.py (right):

http://codereview.chromium.org/10310046/diff/1/build/android/test_package.py#...
build/android/test_package.py:138: re_crash = re.compile('\[ CRASHED     
\](.*)\r\n')
On 2012/05/07 19:22:15, John Grabowski wrote:
> It seems illogical a test will announce it crashed.
> Comments in this file seem apropos.

I am not sure I understand what you want here. Can you please explain a bit
more.

http://codereview.chromium.org/10310046/diff/1/build/android/test_result.py
File build/android/test_result.py (right):

http://codereview.chromium.org/10310046/diff/1/build/android/test_result.py#n...
build/android/test_result.py:64: def FromOkFailedAndCrashed(ok, failed, crashed,
timed_out, overall_fail):
On 2012/05/07 19:22:15, John Grabowski wrote:
> That's a lot of arguments.
> Can you make all callers pass by name?  E.g.
> ok=[], failed=[], ...
> so input is more clear?

Done.

http://codereview.chromium.org/10310046/diff/1/testing/android/native_test_la...
File testing/android/native_test_launcher.cc (right):

http://codereview.chromium.org/10310046/diff/1/testing/android/native_test_la...
testing/android/native_test_launcher.cc:3: // found in the LICENSE file.
On 2012/05/07 19:22:15, John Grabowski wrote:
> Add explicit doc on our model: catch signals and print 'CRASHED' to help
python
> test runner blah blah catch blah.

Done.

http://codereview.chromium.org/10310046/diff/1/testing/android/native_test_la...
testing/android/native_test_launcher.cc:29: // The list of signals which we
consider to be crashes. We rethrow the
On 2012/05/07 19:22:19, Yaron wrote:
> Nit: Don't use "we" in comments.

Done.

http://codereview.chromium.org/10310046/diff/1/testing/android/native_test_la...
testing/android/native_test_launcher.cc:40: LOG(ERROR) << "[ CRASHED      ]";
On 2012/05/07 19:22:15, John Grabowski wrote:
> Normally you cannot allocate memory in a signal handler.
> Class LogMessage has an ostringstream member, which means it probably
allocates
> memory.
> 
> Consider __android_log_write() instead.

Done. Thanks for catching this.

[John Grabowski, 2012-05-08 12:54:10]

LGTM with style nits

http://codereview.chromium.org/10310046/diff/1/build/android/test_package.py
File build/android/test_package.py (right):

http://codereview.chromium.org/10310046/diff/1/build/android/test_package.py#...
build/android/test_package.py:138: re_crash = re.compile('\[ CRASHED     
\](.*)\r\n')
On 2012/05/08 01:38:10, nileshagrawal1 wrote:
> On 2012/05/07 19:22:15, John Grabowski wrote:
> > It seems illogical a test will announce it crashed.
> > Comments in this file seem apropos.
> 
> I am not sure I understand what you want here. Can you please explain a bit
> more.

Sure.

At first glance it does not make sense that a test can announce it crashed.

At this point in the py code explain how that does in fact work (e.g. catch
signal, print, and exit.)

http://codereview.chromium.org/10310046/diff/3002/build/android/single_test_r...
File build/android/single_test_runner.py (right):

http://codereview.chromium.org/10310046/diff/3002/build/android/single_test_r...
build/android/single_test_runner.py:275: ok=list(executed_results -
failed_results),
use consistent indent (e.g. unindent line 276 by 2)

http://codereview.chromium.org/10310046/diff/3002/build/android/test_result.py
File build/android/test_result.py (right):

http://codereview.chromium.org/10310046/diff/3002/build/android/test_result.p...
build/android/test_result.py:64: def FromRun(ok=[], failed=[], crashed=[],
timed_out=False,
default args which are lists violate the style guide.
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Default_Argume...

Recommend using default of None, then doing e.g.
  ret.crashed = crashed or []

http://codereview.chromium.org/10310046/diff/3002/testing/android/native_test...
File testing/android/native_test_launcher.cc (right):

http://codereview.chromium.org/10310046/diff/3002/testing/android/native_test...
testing/android/native_test_launcher.cc:38: static const int kExceptionSignals[]
= {
'static' redundant (see below)

http://codereview.chromium.org/10310046/diff/3002/testing/android/native_test...
testing/android/native_test_launcher.cc:42: static struct sigaction
old_sa[NSIG];
is inside an anonymous namespace so no need to also declare this as static.
In addition, per style guide, name this g_old_sa.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Static_and_Glo...

[nilesh, 2012-05-08 22:18:43]

Thanks for the review.

http://codereview.chromium.org/10310046/diff/1/build/android/test_package.py
File build/android/test_package.py (right):

http://codereview.chromium.org/10310046/diff/1/build/android/test_package.py#...
build/android/test_package.py:138: re_crash = re.compile('\[ CRASHED     
\](.*)\r\n')
On 2012/05/08 12:54:10, John Grabowski wrote:
> On 2012/05/08 01:38:10, nileshagrawal1 wrote:
> > On 2012/05/07 19:22:15, John Grabowski wrote:
> > > It seems illogical a test will announce it crashed.
> > > Comments in this file seem apropos.
> > 
> > I am not sure I understand what you want here. Can you please explain a bit
> > more.
> 
> Sure.
> 
> At first glance it does not make sense that a test can announce it crashed.
> 
> At this point in the py code explain how that does in fact work (e.g. catch
> signal, print, and exit.)

Done.

http://codereview.chromium.org/10310046/diff/3002/build/android/single_test_r...
File build/android/single_test_runner.py (right):

http://codereview.chromium.org/10310046/diff/3002/build/android/single_test_r...
build/android/single_test_runner.py:275: ok=list(executed_results -
failed_results),
On 2012/05/08 12:54:10, John Grabowski wrote:
> use consistent indent (e.g. unindent line 276 by 2)

Done.

http://codereview.chromium.org/10310046/diff/3002/build/android/test_result.py
File build/android/test_result.py (right):

http://codereview.chromium.org/10310046/diff/3002/build/android/test_result.p...
build/android/test_result.py:64: def FromRun(ok=[], failed=[], crashed=[],
timed_out=False,
On 2012/05/08 12:54:10, John Grabowski wrote:
> default args which are lists violate the style guide.
>
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Default_Argume...
> 
> Recommend using default of None, then doing e.g.
>   ret.crashed = crashed or []
> 

Done.

http://codereview.chromium.org/10310046/diff/3002/testing/android/native_test...
File testing/android/native_test_launcher.cc (right):

http://codereview.chromium.org/10310046/diff/3002/testing/android/native_test...
testing/android/native_test_launcher.cc:38: static const int kExceptionSignals[]
= {
On 2012/05/08 12:54:10, John Grabowski wrote:
> 'static' redundant (see below)

Done.

http://codereview.chromium.org/10310046/diff/3002/testing/android/native_test...
testing/android/native_test_launcher.cc:42: static struct sigaction
old_sa[NSIG];
On 2012/05/08 12:54:10, John Grabowski wrote:
> is inside an anonymous namespace so no need to also declare this as static.
> In addition, per style guide, name this g_old_sa.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Static_and_Glo...

Done.

[commit-bot: I haz the power, 2012-05-08 22:32:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nileshagrawal@chromium.org/10310046/13001

[commit-bot: I haz the power, 2012-05-09 02:29:59]

Try job failure for 10310046-13001 (retry) (retry) (retry) on win_rel for step
"compile" (clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-05-09 15:42:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nileshagrawal@chromium.org/10310046/13001

[commit-bot: I haz the power, 2012-05-09 16:59:16]

Change committed as 136050