Skip to content

Commit

Permalink
vm: add SafeForTerminationScopes for SIGINT interruptions
Browse files Browse the repository at this point in the history
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.

PR-URL: #36344
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Gus Caplan <me@gus.host>
  • Loading branch information
addaleax authored and danielleadams committed Dec 7, 2020
1 parent 6033d30 commit 8731a80
Show file tree
Hide file tree
Showing 3 changed files with 61 additions and 0 deletions.
1 change: 1 addition & 0 deletions src/module_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
1 change: 1 addition & 0 deletions src/node_contextify.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand Down
59 changes: 59 additions & 0 deletions test/cctest/test_environment.cc
Original file line number Diff line number Diff line change
Expand Up @@ -559,3 +559,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

0 comments on commit 8731a80

Please sign in to comment.