Commit 16606d0
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.
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>1 parent a033910 commit 16606d0
3 files changed
Lines changed: 61 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -350,6 +350,7 @@ void ModuleWrap::Evaluate(const FunctionCallbackInfo<Value>& args) { | |||
| 350 | 350 | ||
| 351 | 351 | ShouldNotAbortOnUncaughtScope no_abort_scope(env); | |
| 352 | 352 | TryCatchScope try_catch(env); | |
| 353 | + Isolate::SafeForTerminationScope safe_for_termination(env->isolate()); | ||
| 353 | 354 | ||
| 354 | 355 | bool timed_out = false; | |
| 355 | 356 | bool received_signal = false; | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -933,6 +933,7 @@ bool ContextifyScript::EvalMachine(Environment* env, | |||
| 933 | 933 | return false; | |
| 934 | 934 | } | |
| 935 | 935 | TryCatchScope try_catch(env); | |
| 936 | + Isolate::SafeForTerminationScope safe_for_termination(env->isolate()); | ||
| 936 | 937 | ContextifyScript* wrapped_script; | |
| 937 | 938 | ASSIGN_OR_RETURN_UNWRAP(&wrapped_script, args.Holder(), false); | |
| 938 | 939 | Local<UnboundScript> unbound_script = | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -559,3 +559,62 @@ TEST_F(EnvironmentTest, SetImmediateMicrotasks) { | |||
| 559 | 559 | ||
| 560 | 560 | EXPECT_EQ(called, 1); | |
| 561 | 561 | } | |
| 562 | + | ||
| 563 | + #ifndef _WIN32 // No SIGINT on Windows. | ||
| 564 | + TEST_F(NodeZeroIsolateTestFixture, CtrlCWithOnlySafeTerminationTest) { | ||
| 565 | + // We need to go through the whole setup dance here because we want to | ||
| 566 | + // set only_terminate_in_safe_scope. | ||
| 567 | + // Allocate and initialize Isolate. | ||
| 568 | + v8::Isolate::CreateParams create_params; | ||
| 569 | + create_params.array_buffer_allocator = allocator.get(); | ||
| 570 | + create_params.only_terminate_in_safe_scope = true; | ||
| 571 | + v8::Isolate* isolate = v8::Isolate::Allocate(); | ||
| 572 | + CHECK_NOT_NULL(isolate); | ||
| 573 | + platform->RegisterIsolate(isolate, ¤t_loop); | ||
| 574 | + v8::Isolate::Initialize(isolate, create_params); | ||
| 575 | + | ||
| 576 | + // Try creating Context + IsolateData + Environment. | ||
| 577 | + { | ||
| 578 | + v8::Isolate::Scope isolate_scope(isolate); | ||
| 579 | + v8::HandleScope handle_scope(isolate); | ||
| 580 | + | ||
| 581 | + auto context = node::NewContext(isolate); | ||
| 582 | + CHECK(!context.IsEmpty()); | ||
| 583 | + v8::Context::Scope context_scope(context); | ||
| 584 | + | ||
| 585 | + std::unique_ptr<node::IsolateData, decltype(&node::FreeIsolateData)> | ||
| 586 | + isolate_data{node::CreateIsolateData(isolate, | ||
| 587 | + ¤t_loop, | ||
| 588 | + platform.get()), | ||
| 589 | + node::FreeIsolateData}; | ||
| 590 | + CHECK(isolate_data); | ||
| 591 | + | ||
| 592 | + std::unique_ptr<node::Environment, decltype(&node::FreeEnvironment)> | ||
| 593 | + environment{node::CreateEnvironment(isolate_data.get(), | ||
| 594 | + context, | ||
| 595 | + {}, | ||
| 596 | + {}), | ||
| 597 | + node::FreeEnvironment}; | ||
| 598 | + CHECK(environment); | ||
| 599 | + | ||
| 600 | + v8::Local<v8::Value> main_ret = | ||
| 601 | + node::LoadEnvironment(environment.get(), | ||
| 602 | + "'use strict';\n" | ||
| 603 | + "const { runInThisContext } = require('vm');\n" | ||
| 604 | + "try {\n" | ||
| 605 | + " runInThisContext(" | ||
| 606 | + " `process.kill(process.pid, 'SIGINT'); while(true){}`, " | ||
| 607 | + " { breakOnSigint: true });\n" | ||
| 608 | + " return 'unreachable';\n" | ||
| 609 | + "} catch (err) {\n" | ||
| 610 | + " return err.code;\n" | ||
| 611 | + "}").ToLocalChecked(); | ||
| 612 | + node::Utf8Value main_ret_str(isolate, main_ret); | ||
| 613 | + EXPECT_EQ(std::string(*main_ret_str), "ERR_SCRIPT_EXECUTION_INTERRUPTED"); | ||
| 614 | + } | ||
| 615 | + | ||
| 616 | + // Cleanup. | ||
| 617 | + platform->UnregisterIsolate(isolate); | ||
| 618 | + isolate->Dispose(); | ||
| 619 | + } | ||
| 620 | + #endif // _WIN32 | ||
0 commit comments