Add methods to allow resuming execution after calling TerminateExecution().

Add methods to allow resuming execution after calling TerminateExecution().

Two new methods are added to allow embedders to determine that execution
should be resumed at a particular point in the stack without being forced
to unwind all JS frames.

* V8::CancelTerminateExecution() -- When execution is terminated via a call
  to V8::TerminateExecution(), this method can be called to clear the
  termination exception so that the engine can continue to be used.

* TryCatch::HasTerminated() -- When a TryCatch has caught a termination
  exception, HasTerminated() will return true to indicate it is valid to
  call V8::ResumeExecution() if desired.

A test case is added to cctest/test-thread-termination.cc.

BUG=v8:2361
Patch from Andrew Paprocki <andrew@ishiboo.com>;.

Committed: https://code.google.com/p/v8/source/detail?r=14022

[Vyacheslav Egorov (Google), 2012-12-06 14:35:29]

Forwarding to Daniel Clifford (V8 TLM) for review.

https://codereview.chromium.org/11142013/diff/1/AUTHORS
File AUTHORS (right):

https://codereview.chromium.org/11142013/diff/1/AUTHORS#newcode12
AUTHORS:12: Bloomberg Finance L.P.
If you are contributing on organization's behalf please ensure that corporate
CLA is signed and you are listed there: 

https://code.google.com/legal/corporate-cla-v1.0.html

and remove yourself from the individual list below.

Otherwise if you are contributing as an individual please ensure you have signed
individual CLA (https://code.google.com/legal/individual-cla-v1.0.html) and  
remove organization from this list.

[apaprocki, 2012-12-06 14:56:45]

On 2012/12/06 14:35:29, Vyacheslav Egorov (Google) wrote:
> Forwarding to Daniel Clifford (V8 TLM) for review.
> 
> https://codereview.chromium.org/11142013/diff/1/AUTHORS
> File AUTHORS (right):
> 
> https://codereview.chromium.org/11142013/diff/1/AUTHORS#newcode12
> AUTHORS:12: Bloomberg Finance L.P.
> If you are contributing on organization's behalf please ensure that corporate
> CLA is signed and you are listed there: 
> 
> https://code.google.com/legal/corporate-cla-v1.0.html
> 
> and remove yourself from the individual list below.
> 
> Otherwise if you are contributing as an individual please ensure you have
signed
> individual CLA (https://code.google.com/legal/individual-cla-v1.0.html) and  
> remove organization from this list.

I removed myself from AUTHORS. I'm working on getting the corporate CLA
signed/submitted today.

[Sven Panne, 2012-12-06 15:07:34]

Just a single quick drive-by comment. Apart from this, I would be interested in
the use case for the new API entries, it is not immediately obvious, at least
not for me. :-)

https://codereview.chromium.org/11142013/diff/10001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/11142013/diff/10001/include/v8.h#newcode3424
include/v8.h:3424: static void ResumeExecution(Isolate* isolate = NULL);
In general, we are moving into the direction of requiring Isolate parameters
when the implementation of the API function needs an Isolate. So I would very
much prefer making the parameter non-optional and avoid introducing yet another
old-style API entry.

[apaprocki, 2012-12-06 15:15:59]

On 2012/12/06 15:07:34, Sven Panne wrote:
> Just a single quick drive-by comment. Apart from this, I would be interested
in
> the use case for the new API entries, it is not immediately obvious, at least
> not for me. :-)

Basically, I am submitting a patch to add a 'timeout' parameter to the Node.js
'vm' code. In order to support timeouts, the code has to call
TerminateExecution, but since Node.js itself is written in JS and is executing
JS code, the termination must only terminate the application code and not fully
unwind the entire stack, including all of the Node.js JS frames. This change is
needed for the Node.js code to work properly so that if the timeout does fire,
execution will continue from the correct point in code.

[Sven Panne, 2012-12-07 09:04:27]

OK, the use case sounds, well, useful. :-) From my point of view this CL is OK
if the Isolate parameter is mandatory, but I would like to have a 2nd opinion
from our exception specialist...

[Yang, 2012-12-07 15:03:48]

LGTM with some comments. I'll land this if you change the API wrt implicit
Isolate parameter and my comments :)

https://codereview.chromium.org/11142013/diff/10001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/11142013/diff/10001/include/v8.h#newcode3424
include/v8.h:3424: static void ResumeExecution(Isolate* isolate = NULL);
On 2012/12/06 15:07:34, Sven Panne wrote:
> In general, we are moving into the direction of requiring Isolate parameters
> when the implementation of the API function needs an Isolate. So I would very
> much prefer making the parameter non-optional and avoid introducing yet
another
> old-style API entry.

Agreed.

https://codereview.chromium.org/11142013/diff/10001/src/execution.cc
File src/execution.cc (right):

https://codereview.chromium.org/11142013/diff/10001/src/execution.cc#newcode426
src/execution.cc:426: thread_local_.interrupt_flags_ &=
~static_cast<int>(TERMINATE);
If I'm not mistaken, this is already being done in Execution::HandleStackGuard.
An ASSERT would be enough.

https://codereview.chromium.org/11142013/diff/10001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/11142013/diff/10001/src/isolate.cc#newcode947
src/isolate.cc:947: && pending_exception() == heap_.termination_exception()) {
Please put the && at the end of the line and align pending_exception() with
has_pending_exception()

https://codereview.chromium.org/11142013/diff/10001/src/isolate.cc#newcode952
src/isolate.cc:952: && scheduled_exception() == heap_.termination_exception()) {
Ditto.

[apaprocki, 2012-12-13 02:20:43]

Updated the patch according to comments.

Rather than duplicate the guts of StackGuard::Continue() inside
StackGuard::ResumeExecution(), I just call Continue(TERMINATE) there instead
since the ExecutionAccess guard is recursive. 

The TERMINATE flag must be cleared at this point so that when
Execution::HandleStackGuardInterrupt() runs, stack_guard->IsTerminateException()
returns false.

[Yang, 2012-12-13 15:51:00]

On 2012/12/13 02:20:43, apaprocki wrote:
> Updated the patch according to comments.
> 
> Rather than duplicate the guts of StackGuard::Continue() inside
> StackGuard::ResumeExecution(), I just call Continue(TERMINATE) there instead
> since the ExecutionAccess guard is recursive. 
> 
> The TERMINATE flag must be cleared at this point so that when
> Execution::HandleStackGuardInterrupt() runs,
stack_guard->IsTerminateException()
> returns false.

LGTM. But before I land this, I'd like to clear up one thing: what is the
affiliation with Bloomberg Finance L.P.? I have checked and don't see Bloomberg
Finance L.P. registered as have signed the corporate CLA. You however are,
registered as individual. I'm sort of confused.

[apaprocki, 2012-12-13 15:56:46]

I'm employed by Bloomberg but my open-source work is done under my personal
e-mail address. I e-mailed the signed corp CLA .pdf to
cla-submissions@google.com yesterday at 2PM. I suppose there is some delay until
it shows wherever you are looking?

-Andrew

On 2012/12/13 15:51:00, Yang wrote:
> On 2012/12/13 02:20:43, apaprocki wrote:
> > Updated the patch according to comments.
> > 
> > Rather than duplicate the guts of StackGuard::Continue() inside
> > StackGuard::ResumeExecution(), I just call Continue(TERMINATE) there instead
> > since the ExecutionAccess guard is recursive. 
> > 
> > The TERMINATE flag must be cleared at this point so that when
> > Execution::HandleStackGuardInterrupt() runs,
> stack_guard->IsTerminateException()
> > returns false.
> 
> LGTM. But before I land this, I'd like to clear up one thing: what is the
> affiliation with Bloomberg Finance L.P.? I have checked and don't see
Bloomberg
> Finance L.P. registered as have signed the corporate CLA. You however are,
> registered as individual. I'm sort of confused.

[apaprocki, 2013-03-16 15:36:14]

Yang,

I made the appropriate changes to address the failures that caused this to be
backed out before. Can you re-review and give this another try?

Thanks,
-Andrew

[Yang, 2013-03-21 10:47:42]

https://codereview.chromium.org/11142013/diff/30002/test/cctest/test-thread-t...
File test/cctest/test-thread-termination.cc (right):

https://codereview.chromium.org/11142013/diff/30002/test/cctest/test-thread-t...
test/cctest/test-thread-termination.cc:391:
CHECK(!v8::V8::IsExecutionTerminating());
If you remove the two lines above, the test would still pass, since it would
essentially be the same as the test TerminateAndReenterFromThreadItself. What
you want to test is that the script continues from where it left off when it was
terminated.

https://codereview.chromium.org/11142013/diff/30002/test/cctest/test-thread-t...
test/cctest/test-thread-termination.cc:410: v8::Script::Compile(source)->Run();
See above. What you want to test is that the script continues from where it left
off when it was terminated, not that it can restart with another script.

I took the liberty to change these last few lines to

  v8::Handle<v8::String> source =
      v8::String::New("try { doloop(); } catch(e) { fail(); } 'completed';");
  // Check that execution completed with correct return value.
  CHECK(v8::Script::Compile(source)->Run()->Equals(v8_str("completed")));

[Yang, 2013-03-21 10:47:47]

Committed patchset #6 manually as r14022 (presubmit successful).

[Yang, 2013-03-21 10:47:50]

On 2013/03/21 10:47:42, Yang wrote:
>
https://codereview.chromium.org/11142013/diff/30002/test/cctest/test-thread-t...
> File test/cctest/test-thread-termination.cc (right):
> 
>
https://codereview.chromium.org/11142013/diff/30002/test/cctest/test-thread-t...
> test/cctest/test-thread-termination.cc:391:
> CHECK(!v8::V8::IsExecutionTerminating());
> If you remove the two lines above, the test would still pass, since it would
> essentially be the same as the test TerminateAndReenterFromThreadItself. What
> you want to test is that the script continues from where it left off when it
was
> terminated.
> 
>
https://codereview.chromium.org/11142013/diff/30002/test/cctest/test-thread-t...
> test/cctest/test-thread-termination.cc:410:
v8::Script::Compile(source)->Run();
> See above. What you want to test is that the script continues from where it
left
> off when it was terminated, not that it can restart with another script.
> 
> I took the liberty to change these last few lines to
> 
>   v8::Handle<v8::String> source =
>       v8::String::New("try { doloop(); } catch(e) { fail(); } 'completed';");
>   // Check that execution completed with correct return value.
>   CHECK(v8::Script::Compile(source)->Run()->Equals(v8_str("completed")));

LGTM and landed.

[Yang, 2013-03-21 14:02:51]

On 2013/03/21 10:47:50, Yang wrote:
> On 2013/03/21 10:47:42, Yang wrote:
> >
>
https://codereview.chromium.org/11142013/diff/30002/test/cctest/test-thread-t...
> > File test/cctest/test-thread-termination.cc (right):
> > 
> >
>
https://codereview.chromium.org/11142013/diff/30002/test/cctest/test-thread-t...
> > test/cctest/test-thread-termination.cc:391:
> > CHECK(!v8::V8::IsExecutionTerminating());
> > If you remove the two lines above, the test would still pass, since it would
> > essentially be the same as the test TerminateAndReenterFromThreadItself.
What
> > you want to test is that the script continues from where it left off when it
> was
> > terminated.
> > 
> >
>
https://codereview.chromium.org/11142013/diff/30002/test/cctest/test-thread-t...
> > test/cctest/test-thread-termination.cc:410:
> v8::Script::Compile(source)->Run();
> > See above. What you want to test is that the script continues from where it
> left
> > off when it was terminated, not that it can restart with another script.
> > 
> > I took the liberty to change these last few lines to
> > 
> >   v8::Handle<v8::String> source =
> >       v8::String::New("try { doloop(); } catch(e) { fail(); }
'completed';");
> >   // Check that execution completed with correct return value.
> >   CHECK(v8::Script::Compile(source)->Run()->Equals(v8_str("completed")));
> 
> LGTM and landed.

Unfortunately, we will have to revert this change for now. We are branching this
week and require the API to remain stable during this period. The CL by itself
is fine, and we will reland this in probably a week from now. Sorry for any
inconvenience!

[apaprocki, 2013-04-22 12:33:36]

On 2013/03/21 14:02:51, Yang wrote:
> Unfortunately, we will have to revert this change for now. We are branching
this
> week and require the API to remain stable during this period. The CL by itself
> is fine, and we will reland this in probably a week from now. Sorry for any
> inconvenience!

Just pinging -- can this land now?

Thanks,
-Andrew