Add termination of the script to the bailout check.

Include check for execution termination into bailout check.

This prevents re-entry into JS during stack unwinding caused by TerminateExecution().

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

[Mads Ager (chromium), 2010-05-19 11:01:23]

http://codereview.chromium.org/2123005/diff/1/2
File src/api.cc (right):

http://codereview.chromium.org/2123005/diff/1/2#newcode63
src/api.cc:63: if (IsDeadCheck(location) || i::V8::ExecutionWasTerminated()) { \
I'm not sure I like making this check more expensive. This will not show up on
pure JS benchmarks, but it will probably show up on DOM manipulation benchmarks
where the API is used heavily. We should measure the impact on some of those as
well.

http://codereview.chromium.org/2123005/diff/1/5
File src/v8.h (right):

http://codereview.chromium.org/2123005/diff/1/5#newcode120
src/v8.h:120: static bool execution_was_terminated_;
Instead of using this separate boolean, you should perform the same check as
IsExecutionTerminating. In this change, it seems that you do not allow any calls
into V8 after a call to TerminateExecution at all (you never reset this boolean
to false?).

http://codereview.chromium.org/2123005/diff/1/6
File test/cctest/test-thread-termination.cc (right):

http://codereview.chromium.org/2123005/diff/1/6#newcode349
test/cctest/test-thread-termination.cc:349:
CHECK(!v8::V8::IsExecutionTerminating());
You should check that after attempting re-entry, you can still use V8 and get
meaningful results when v8::V8::IsExecutionTerminating returns false.

[Dmitry Titov, 2010-05-21 04:01:44]

The next iteration. Updated for feedback, run some tests.

On 2010/05/19 11:01:23, Mads Ager wrote:
> http://codereview.chromium.org/2123005/diff/1/2
> File src/api.cc (right):
> 
> http://codereview.chromium.org/2123005/diff/1/2#newcode63
> src/api.cc:63: if (IsDeadCheck(location) || i::V8::ExecutionWasTerminated()) {
\
> I'm not sure I like making this check more expensive. This will not show up on
> pure JS benchmarks, but it will probably show up on DOM manipulation
benchmarks
> where the API is used heavily. We should measure the impact on some of those
as
> well.

I've bulit Chrome with the patch and without, and run 'dom' and 'jslib' test
suites from Dromaeo. Here are results:
http://spreadsheets.google.com/pub?key=tiq3754EfcswNPIn8CWzTOg&output=html

As far as I can see, there is no regression, but maybe I don't read the result
right, or perhaps there are more tests to run, please advise.

> 
> http://codereview.chromium.org/2123005/diff/1/5
> File src/v8.h (right):
> 
> http://codereview.chromium.org/2123005/diff/1/5#newcode120
> src/v8.h:120: static bool execution_was_terminated_;
> Instead of using this separate boolean, you should perform the same check as
> IsExecutionTerminating. In this change, it seems that you do not allow any
calls
> into V8 after a call to TerminateExecution at all (you never reset this
boolean
> to false?).

Right, I've removed the boolean and simply call v8::V8::IsExecutionTerminating.
That takes care about resetting as well.

> 
> http://codereview.chromium.org/2123005/diff/1/6
> File test/cctest/test-thread-termination.cc (right):
> 
> http://codereview.chromium.org/2123005/diff/1/6#newcode349
> test/cctest/test-thread-termination.cc:349:
> CHECK(!v8::V8::IsExecutionTerminating());
> You should check that after attempting re-entry, you can still use V8 and get
> meaningful results when v8::V8::IsExecutionTerminating returns false.

Added this to the test.

[Mads Ager (chromium), 2010-05-21 09:09:27]

LGTM