Add support for forceful termination of JavaScript execution.

Add support for forceful termination of JavaScript execution.

The termination is achieved by throwing an exception that is uncatchable by JavaScript exception handlers.

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

[Christian Plesner Hansen, 2009-08-19 13:19:53]

Lgtm

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

http://codereview.chromium.org/174056/diff/1/5#newcode2229
Line 2229: * The thread id for a thread should be retrieved immediately after
You should be more specific -- what does "immediately after" mean and what
happens if you do it at other times?

http://codereview.chromium.org/174056/diff/1/5#newcode2262
Line 2262: static void TerminateExecution();
Should the current thread bail out after calling this function as if
TerminateExecution had been called by a different thread?

http://codereview.chromium.org/174056/diff/1/5#newcode2327
Line 2327: bool IsTerminationException() const;
I'm wondering: maybe the it would be more obvious how to use this if there was a
more abstract "CanContinue" method (or some other name) that covered any
situation where applications have to bail out.  That way we can adapt it if we
ever decide to add other cases -- like out of memory -- without requiring people
to change their code.

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

http://codereview.chromium.org/174056/diff/1/6#newcode3365
Line 3365: return i::Top::thread_id();
Consider adding an API_ENTRY_CHECK to ensure that a Locker has been acquired
before this method is called.

http://codereview.chromium.org/174056/diff/1/6#newcode3371
Line 3371: // If the thread_id identifies the current thread just terminate
Ditto API_ENTRY_CHECK.

http://codereview.chromium.org/174056/diff/1/6#newcode3384
Line 3384: i::StackGuard::TerminateExecution();
Ditto API_ENTRY_CHECK.

http://codereview.chromium.org/174056/diff/1/17
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/174056/diff/1/17#newcode5845
Line 5845: // Special handling of out of memory excpetions.
Typo, excpetions.

http://codereview.chromium.org/174056/diff/1/17#newcode5857
Line 5857: // Special handling of termination exceptions which are uncatchable
It would be nice if handling of termination and OOM could share more code.  But
I understand if you don't want to touch OOM at this point.

http://codereview.chromium.org/174056/diff/1/20
File src/execution.cc (right):

http://codereview.chromium.org/174056/diff/1/20#newcode657
Line 657: return Top::TerminateExecution();
Regexp native code might not be prepared to handle this kind of interruption. 
I'm not sure though.

http://codereview.chromium.org/174056/diff/1/8
File src/top.cc (right):

http://codereview.chromium.org/174056/diff/1/8#newcode724
Line 724: !catchable_by_javascript);
Why not do (a ||b || c) instead of ((a || b) || c) ?

http://codereview.chromium.org/174056/diff/1/8#newcode749
Line 749: ShouldReportException(&is_caught_externally,
!is_termination_exception);
Shouldn't is_out_of_memory imply !catchable_by_javascript?  If that is the case
then maybe define catchable_by_javascript locally as (!is_out_of_memory ||
!is_termination_exception) and use it the three places below where that
expression is used.

http://codereview.chromium.org/174056/diff/1/8#newcode751
Line 751: !is_out_of_memory && !is_termination_exception &&
should_return_exception;
This code has become messy -- ShouldReportException no longer calculates if the
message should be reported which is confusing.  Maybe rename it
ShouldReturnException to match how it is used.

http://codereview.chromium.org/174056/diff/1/13
File src/v8threads.cc (right):

http://codereview.chromium.org/174056/diff/1/13#newcode324
Line 324: if (!Thread::HasThreadLocal(thread_id_key)) {
I would suggest adding an ASSERT(Locker::IsLocked()) here, otherwise the
next_id++ looks really suspect.

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

http://codereview.chromium.org/174056/diff/1/3#newcode124
Line 124: global->Set(v8::String::New("terminate"),
v8::FunctionTemplate::New(Signal));
This code occurs (almost) three times.  Maybe factor it out into a separate
function.

[Lasse Reichstein, 2009-08-19 14:53:35]

http://codereview.chromium.org/174056/diff/1/20
File src/execution.cc (right):

http://codereview.chromium.org/174056/diff/1/20#newcode657
Line 657: return Top::TerminateExecution();
It should work. Top::TerminateExecution() returns Failure::Exception, which is
what the native RegExp checks against (result->IsException). It will be handled
just as any other exception by the RegExp calling code, which means terminating
the regexp and returning Failure::Exception() from the runtime call.

[Mads Ager (chromium), 2009-08-19 15:12:18]

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

http://codereview.chromium.org/174056/diff/1/5#newcode2229
Line 2229: * The thread id for a thread should be retrieved immediately after
On 2009/08/19 13:19:54, Christian Plesner Hansen wrote:
> You should be more specific -- what does "immediately after" mean and what
> happens if you do it at other times?

Right.  You just have to hold the V8 lock with that thread when getting its
thread id.  Documentation updated.

http://codereview.chromium.org/174056/diff/1/5#newcode2262
Line 2262: static void TerminateExecution();
On 2009/08/19 13:19:54, Christian Plesner Hansen wrote:
> Should the current thread bail out after calling this function as if
> TerminateExecution had been called by a different thread?

You can do anything you want after calling TerminateExecution.  The next time
JavaScript is entered a termination exception will be thrown (at the first stack
guard check).

http://codereview.chromium.org/174056/diff/1/5#newcode2327
Line 2327: bool IsTerminationException() const;
On 2009/08/19 13:19:54, Christian Plesner Hansen wrote:
> I'm wondering: maybe the it would be more obvious how to use this if there was
a
> more abstract "CanContinue" method (or some other name) that covered any
> situation where applications have to bail out.  That way we can adapt it if we
> ever decide to add other cases -- like out of memory -- without requiring
people
> to change their code.

That makes sense, thanks.  I have replaced it by a CanContinue method.

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

http://codereview.chromium.org/174056/diff/1/6#newcode3365
Line 3365: return i::Top::thread_id();
On 2009/08/19 13:19:54, Christian Plesner Hansen wrote:
> Consider adding an API_ENTRY_CHECK to ensure that a Locker has been acquired
> before this method is called.

Yes, done.

http://codereview.chromium.org/174056/diff/1/6#newcode3371
Line 3371: // If the thread_id identifies the current thread just terminate
On 2009/08/19 13:19:54, Christian Plesner Hansen wrote:
> Ditto API_ENTRY_CHECK.

Yes, done.

http://codereview.chromium.org/174056/diff/1/6#newcode3384
Line 3384: i::StackGuard::TerminateExecution();
On 2009/08/19 13:19:54, Christian Plesner Hansen wrote:
> Ditto API_ENTRY_CHECK.

This one does not need it since messing with the stack guards are protected
internally in V8.  This version of TerminateExecution can be called from any
thread without holding the V8 lock.  I have updated the documentation for this
method with that information.

http://codereview.chromium.org/174056/diff/1/17
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/174056/diff/1/17#newcode5845
Line 5845: // Special handling of out of memory excpetions.
On 2009/08/19 13:19:54, Christian Plesner Hansen wrote:
> Typo, excpetions.

Done.

http://codereview.chromium.org/174056/diff/1/17#newcode5857
Line 5857: // Special handling of termination exceptions which are uncatchable
On 2009/08/19 13:19:54, Christian Plesner Hansen wrote:
> It would be nice if handling of termination and OOM could share more code. 
But
> I understand if you don't want to touch OOM at this point.

Right, this change is complicated as is and I didn't want to mess with the OOM
handling at the same time.

http://codereview.chromium.org/174056/diff/1/20
File src/execution.cc (right):

http://codereview.chromium.org/174056/diff/1/20#newcode657
Line 657: return Top::TerminateExecution();
On 2009/08/19 14:53:35, Lasse Reichstein wrote:
> It should work. Top::TerminateExecution() returns Failure::Exception, which is
> what the native RegExp checks against (result->IsException). It will be
handled
> just as any other exception by the RegExp calling code, which means
terminating
> the regexp and returning Failure::Exception() from the runtime call.

Thanks for the clearification Lasse - I was not sure. :)

http://codereview.chromium.org/174056/diff/1/8
File src/top.cc (right):

http://codereview.chromium.org/174056/diff/1/8#newcode724
Line 724: !catchable_by_javascript);
On 2009/08/19 13:19:54, Christian Plesner Hansen wrote:
> Why not do (a ||b || c) instead of ((a || b) || c) ?

Done.

http://codereview.chromium.org/174056/diff/1/8#newcode749
Line 749: ShouldReportException(&is_caught_externally,
!is_termination_exception);
On 2009/08/19 13:19:54, Christian Plesner Hansen wrote:
> Shouldn't is_out_of_memory imply !catchable_by_javascript?  If that is the
case
> then maybe define catchable_by_javascript locally as (!is_out_of_memory ||
> !is_termination_exception) and use it the three places below where that
> expression is used.

Done.

http://codereview.chromium.org/174056/diff/1/8#newcode751
Line 751: !is_out_of_memory && !is_termination_exception &&
should_return_exception;
On 2009/08/19 13:19:54, Christian Plesner Hansen wrote:
> This code has become messy -- ShouldReportException no longer calculates if
the
> message should be reported which is confusing.  Maybe rename it
> ShouldReturnException to match how it is used.
> 

Done.

http://codereview.chromium.org/174056/diff/1/13
File src/v8threads.cc (right):

http://codereview.chromium.org/174056/diff/1/13#newcode324
Line 324: if (!Thread::HasThreadLocal(thread_id_key)) {
On 2009/08/19 13:19:54, Christian Plesner Hansen wrote:
> I would suggest adding an ASSERT(Locker::IsLocked()) here, otherwise the
> next_id++ looks really suspect.

I have added the assertion.  Right now, AssignId is only called from the Locker
constructor itself after the V8 lock has been successfully acquired.  However,
adding the assert makes sense.

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

http://codereview.chromium.org/174056/diff/1/3#newcode124
Line 124: global->Set(v8::String::New("terminate"),
v8::FunctionTemplate::New(Signal));
On 2009/08/19 13:19:54, Christian Plesner Hansen wrote:
> This code occurs (almost) three times.  Maybe factor it out into a separate
> function.

Done.