vm: add SafeForTerminationScopes for SIGINT interruptions

Some embedders, like Electron, choose to start Node.js with `only_terminate_in_safe_scope` set to `true`. In those cases, parts of the API that expect execution termination to happen need to be marked as able to receive those events. In our case, this is the Ctrl+C support of the `vm` module (and Workers, but since we’re in control of creating the `Isolate` for them, that’s a non-concern there). Add those scopes and add a regression test.
nodejs · addaleax · Dec 1, 2020 · Dec 2, 2020 · Dec 1, 2020 · 7449ba3e52d540d9231b9155d81b056f60301ba2
commit 7449ba3e52d540d9231b9155d81b056f60301ba2
diff --git a/src/module_wrap.cc b/src/module_wrap.cc
@@ -350,6 +350,7 @@ void ModuleWrap::Evaluate(const FunctionCallbackInfo<Value>& args) {
 
   ShouldNotAbortOnUncaughtScope no_abort_scope(env);
   TryCatchScope try_catch(env);
+  Isolate::SafeForTerminationScope safe_for_termination(env->isolate());
 
   bool timed_out = false;
   bool received_signal = false;

diff --git a/src/node_contextify.cc b/src/node_contextify.cc
@@ -933,6 +933,7 @@ bool ContextifyScript::EvalMachine(Environment* env,
     return false;
   }
   TryCatchScope try_catch(env);
+  Isolate::SafeForTerminationScope safe_for_termination(env->isolate());
   ContextifyScript* wrapped_script;
   ASSIGN_OR_RETURN_UNWRAP(&wrapped_script, args.Holder(), false);
   Local<UnboundScript> unbound_script =

diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc
@@ -549,3 +549,62 @@ TEST_F(EnvironmentTest, SetImmediateMicrotasks) {
 
   EXPECT_EQ(called, 1);
 }
+
+#ifndef _WIN32  // No SIGINT on Windows.
+TEST_F(NodeZeroIsolateTestFixture, CtrlCWithOnlySafeTerminationTest) {
+  // We need to go through the whole setup dance here because we want to
+  // set only_terminate_in_safe_scope.
+  // Allocate and initialize isolate
+  v8::Isolate::CreateParams create_params;
+  create_params.array_buffer_allocator = allocator.get();
+  create_params.only_terminate_in_safe_scope = true;
+  v8::Isolate* isolate = v8::Isolate::Allocate();
+  CHECK_NOT_NULL(isolate);
+  platform->RegisterIsolate(isolate, &current_loop);
+  v8::Isolate::Initialize(isolate, create_params);
+
+  // Try creating Context + IsolateData + Environment
+  {
+    v8::Isolate::Scope isolate_scope(isolate);
+    v8::HandleScope handle_scope(isolate);
+
+    auto context = node::NewContext(isolate);
+    CHECK(!context.IsEmpty());
+    v8::Context::Scope context_scope(context);
+
+    std::unique_ptr<node::IsolateData, decltype(&node::FreeIsolateData)>
+      isolate_data{node::CreateIsolateData(isolate,
+                                           &current_loop,
+                                           platform.get()),
+                   node::FreeIsolateData};
+    CHECK(isolate_data);
+
+    std::unique_ptr<node::Environment, decltype(&node::FreeEnvironment)>
+      environment{node::CreateEnvironment(isolate_data.get(),
+                                          context,
+                                          {},
+                                          {}),
+                  node::FreeEnvironment};
+    CHECK(environment);
+
+    v8::Local<v8::Value> main_ret =
+        node::LoadEnvironment(environment.get(),
+            "'use strict';\n"
+            "const { runInThisContext } = require('vm');\n"
+            "try {\n"
+            "  runInThisContext("
+            "    `process.kill(process.pid, 'SIGINT'); while(true){}`, "
+            "    { breakOnSigint: true });\n"
+            "  return 'unreachable';\n"
+            "} catch (err) {\n"
+            "  return err.code;\n"
+            "}").ToLocalChecked();
+    node::Utf8Value main_ret_str(isolate, main_ret);
+    EXPECT_EQ(std::string(*main_ret_str), "ERR_SCRIPT_EXECUTION_INTERRUPTED");
+  }
+
+  // Cleanup
+  platform->UnregisterIsolate(isolate);
+  isolate->Dispose();
+}
+#endif  // _WIN32