By default disable the general protection fault message box when...

By default disable the general protection fault message box when
running tests on Windows. This requires 

Added the option --win-error-box to enable general protection fault
message box which can be convenient when debugging failing tests on
Windows.

Added crash detection when running tests on Windows. The output
is not fully polished but crashed indications are printed for the
different progess indicators.

Changed the OS::Abort on Windows from generating a "crash" (int3)
to calling abort(). This is to avoid tests which are known to fail
with out of memory errors to be detected as crashed tests.

Committed: http://code.google.com/p/v8/source/detail?r=633

[Nicolas Sylvain, 2008-10-28 21:45:38]

http://codereview.chromium.org/8676/diff/1/3
File src/platform-win32.cc (right):

http://codereview.chromium.org/8676/diff/1/3#newcode786
Line 786: abort();
AFAIK abort is not going to work in the sandbox because it tries to display a
message box, and that does not work in the sandbox.   When is OS::Abort()
called? Is it considered a fatal failure? If so, we need to be sure that it
generates a crash and breakpad catches it.

[sgjesse_gmail.com, 2008-10-28 22:15:57]

http://codereview.chromium.org/8676/diff/1/3
File src/platform-win32.cc (right):

http://codereview.chromium.org/8676/diff/1/3#newcode786
Line 786: abort();
On 2008/10/28 21:45:38, Nicolas Sylvain wrote:
> AFAIK abort is not going to work in the sandbox because it tries to display a
> message box, and that does not work in the sandbox.   When is OS::Abort()
> called? Is it considered a fatal failure? If so, we need to be sure that it
> generates a crash and breakpad catches it.

You are right that in a Windows application abort() by default puts up a message
box whereas it in a console application will print a message before it exits the
program with exit code 3 skipping most at_exit processing.

OS::Abort is called by default by V8 when there is a fatal error e.g. out of
memory condition. Through the V8 API a fatal error handler can be registered,
and in http://codereview.chromium.org/8838 I updated the fatal error handler
added by Chrome to ensure a crash making Chrome independant of whatever default
fatal error handling V8 supplies.

I will add calls to _set_abort_behavior to make abort() totally silent.

[sgjesse_gmail.com, 2008-10-28 22:18:17]

http://codereview.chromium.org/8676/diff/1/3
File src/platform-win32.cc (right):

http://codereview.chromium.org/8676/diff/1/3#newcode786
Line 786: abort();
On 2008/10/28 22:15:57, sgjesse wrote:
> On 2008/10/28 21:45:38, Nicolas Sylvain wrote:
> > AFAIK abort is not going to work in the sandbox because it tries to display
a
> > message box, and that does not work in the sandbox.   When is OS::Abort()
> > called? Is it considered a fatal failure? If so, we need to be sure that it
> > generates a crash and breakpad catches it.
> 
> You are right that in a Windows application abort() by default puts up a
message
> box whereas it in a console application will print a message before it exits
the
> program with exit code 3 skipping most at_exit processing.
> 
> OS::Abort is called by default by V8 when there is a fatal error e.g. out of
> memory condition. Through the V8 API a fatal error handler can be registered,
> and in http://codereview.chromium.org/8838 I updated the fatal error handler
> added by Chrome to ensure a crash making Chrome independant of whatever
default
> fatal error handling V8 supplies.
> 
> I will add calls to _set_abort_behavior to make abort() totally silent.

I will also clean up the comment in OS::Abort which is now void and confusing in
a non Chrome context.

[Christian Plesner Hansen, 2008-10-29 07:53:43]

There's a few bugs that have to be fixed before you can submit.  Otherwise lgtm.

http://codereview.chromium.org/8676/diff/205/8
File src/platform-win32.cc (right):

http://codereview.chromium.org/8676/diff/205/8#newcode785
Line 785: _set_abort_behavior(0, _WRITE_ABORT_MSG);
How about making this conditional on a command-line flag -- always overriding
these seems questionable.

http://codereview.chromium.org/8676/diff/205/7
File tools/test.py (right):

http://codereview.chromium.org/8676/diff/205/7#newcode175
Line 175: if len(self.crashed) > 0:
You never use the contents of the crashed list, only its length.  You could use
a counter instead.

http://codereview.chromium.org/8676/diff/205/7#newcode176
Line 176: print "=== %i tests CRASHED" % len(self.failed)
I think you mean len(self.crashed).

http://codereview.chromium.org/8676/diff/205/7#newcode404
Line 404: except:
Catchall exception handlers in python are extremely evil.  You should only catch
the kind of exceptions you expect, in this case ImportError.

http://codereview.chromium.org/8676/diff/205/7#newcode405
Line 405: Pass
It's 'pass', not 'Pass'.  Ahem, did you test this?

http://codereview.chromium.org/8676/diff/205/7#newcode1075
Line 1075: result.add_option("--win-error-box", help="Display the Windows
general protection fault message box",
How about only adding this if we're actually on windows, and then removing the
win- part of the name.  Also, I think error-box is a somewhat confusing name;
maybe something like --suppress-dialogs would be better?

[Søren Thygesen Gjesse, 2008-10-29 09:03:50]

New snapshot please take a look.

http://codereview.chromium.org/8676/diff/205/8
File src/platform-win32.cc (right):

http://codereview.chromium.org/8676/diff/205/8#newcode785
Line 785: _set_abort_behavior(0, _WRITE_ABORT_MSG);
On 2008/10/29 07:53:43, Christian Plesner Hansen wrote:
> How about making this conditional on a command-line flag -- always overriding
> these seems questionable.

I think it is OK to do supress the abort message. All the places in the code
that calls OS::Abort() already has printed the cause of the abort, and having
the MSVCRT add an additional "This application has requested the Runtime to
terminate it in an unusual way. Please contact the application's support team
for more information." seems weird. AFAIK Linux does not print anything when
abort() is called.

http://codereview.chromium.org/8676/diff/205/7
File tools/test.py (right):

http://codereview.chromium.org/8676/diff/205/7#newcode175
Line 175: if len(self.crashed) > 0:
On 2008/10/29 07:53:43, Christian Plesner Hansen wrote:
> You never use the contents of the crashed list, only its length.  You could
use
> a counter instead.

Changed to a counter. Details on the crashed tests can be found in the failed
list if we where to print details no these.

http://codereview.chromium.org/8676/diff/205/7#newcode176
Line 176: print "=== %i tests CRASHED" % len(self.failed)
On 2008/10/29 07:53:43, Christian Plesner Hansen wrote:
> I think you mean len(self.crashed).

Fixed.

http://codereview.chromium.org/8676/diff/205/7#newcode404
Line 404: except:
On 2008/10/29 07:53:43, Christian Plesner Hansen wrote:
> Catchall exception handlers in python are extremely evil.  You should only
catch
> the kind of exceptions you expect, in this case ImportError.

Changed to catch only ImportError.

http://codereview.chromium.org/8676/diff/205/7#newcode405
Line 405: Pass
On 2008/10/29 07:53:43, Christian Plesner Hansen wrote:
> It's 'pass', not 'Pass'.  Ahem, did you test this?

Fixed. I did test it when I used "None" instead of "Pass".

http://codereview.chromium.org/8676/diff/205/7#newcode1075
Line 1075: result.add_option("--win-error-box", help="Display the Windows
general protection fault message box",
On 2008/10/29 07:53:43, Christian Plesner Hansen wrote:
> How about only adding this if we're actually on windows, and then removing the
> win- part of the name.  Also, I think error-box is a somewhat confusing name;
> maybe something like --suppress-dialogs would be better?

Agree that --suppress-dialogs is a much better name. Defaults to True with the
additional option --no-suppress-dialogs to turn it off. Only available on
Windows.