vm: test for abort condition of current invocation

When a vm script aborted after a timeout/signal interruption, test whether the local timeout/signal watchdog was responsible for terminating the execution. Without this, when a shorter timer from an outer `vm.run*` invocation fires before an inner timeout, the inner timeout would throw an error instead of the outer one, but because it did not witness the timeout itself, it would assume the termination was the result of a signal interruption. PR-URL: #7373 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Author: addaleax

src/node_contextify.cc

@@ -836,14 +836,17 @@ class ContextifyScript : public BaseObject{
 
  Local<Value> result;
 bool timed_out = false;
+bool received_signal = false;
 if (break_on_sigint && timeout != -1){
  Watchdog wd(env->isolate(), timeout);
  SigintWatchdog swd(env->isolate());
  result = script->Run();
  timed_out = wd.HasTimedOut();
+ received_signal = swd.HasReceivedSignal();
  } elseif (break_on_sigint){
  SigintWatchdog swd(env->isolate());
  result = script->Run();
+ received_signal = swd.HasReceivedSignal();
  } elseif (timeout != -1){
  Watchdog wd(env->isolate(), timeout);
  result = script->Run();
@@ -852,14 +855,26 @@ class ContextifyScript : public BaseObject{
  result = script->Run();
  }
 
-if (try_catch.HasCaught() && try_catch.HasTerminated()){
- env->isolate()->CancelTerminateExecution();
+if (try_catch.HasCaught()){
+if (try_catch.HasTerminated())
+ env->isolate()->CancelTerminateExecution();
+
+// It is possible that execution was terminated by another timeout in
+// which this timeout is nested, so check whether one of the watchdogs
+// from this invocation is responsible for termination.
 if (timed_out){
  env->ThrowError("Script execution timed out.");
- } else{
+ } elseif (received_signal) {
  env->ThrowError("Script execution interrupted.");
  }
+
+// If there was an exception thrown during script execution, re-throw it.
+// If one of the above checks threw, re-throw the exception instead of
+// letting try_catch catch it.
+// If execution has been terminated, but not by one of the watchdogs from
+// this invocation, this will re-throw a `null` value.
  try_catch.ReThrow();
+
 returnfalse;
  }
 

src/node_watchdog.cc

@@ -99,7 +99,7 @@ void SigintWatchdog::Dispose(){
 
 
 SigintWatchdog::SigintWatchdog(v8::Isolate* isolate)
- : isolate_(isolate), destroyed_(false){
+ : isolate_(isolate), received_signal_(false), destroyed_(false){
 // Register this watchdog with the global SIGINT/Ctrl+C listener.
 SigintWatchdogHelper::GetInstance()->Register(this);
 // Start the helper thread, if that has not already happened.
@@ -120,6 +120,7 @@ void SigintWatchdog::Destroy(){
 
 
 voidSigintWatchdog::HandleSigint(){
+ received_signal_ = true;
  isolate_->TerminateExecution();
 }
 

src/node_watchdog.h

@@ -46,11 +46,13 @@ class SigintWatchdog{
 voidDispose();
 
  v8::Isolate* isolate(){return isolate_}
+boolHasReceivedSignal(){return received_signal_}
 voidHandleSigint();
 private:
 voidDestroy();
 
  v8::Isolate* isolate_;
+bool received_signal_;
 bool destroyed_;
 };
 

test/parallel/test-vm-timeout.js

@@ -32,3 +32,26 @@ assert.throws(function(){
 vm.runInNewContext('runInVM(10)',context,{timeout: 10000});
 thrownewError('Test 5 failed');
 },/Scriptexecutiontimedout./);
+
+// Test 6: Nested vm timeouts, outer timeout is shorter and fires first.
+assert.throws(function(){
+constcontext={
+runInVM: function(timeout){
+vm.runInNewContext('while(true){}',context,{timeout: timeout});
+}
+};
+vm.runInNewContext('runInVM(10000)',context,{timeout: 100});
+thrownewError('Test 6 failed');
+},/Scriptexecutiontimedout./);
+
+// Test 7: Nested vm timeouts, inner script throws an error.
+assert.throws(function(){
+constcontext={
+runInVM: function(timeout){
+vm.runInNewContext('throw new Error(\'foobar\')',context,{
+timeout: timeout
+});
+}
+};
+vm.runInNewContext('runInVM(10000)',context,{timeout: 100000});
+},/foobar/);