Skip to content

Commit

Permalink
src: reject per-process isolate flags in workers
Browse files Browse the repository at this point in the history 
V8 flags are saved in a per-process storages and were ignored in the
Worker constructor options. Reject V8 flags in the Worker constructor
proactively to indicate these options should be set at process level.
  • Loading branch information
@legendecas
legendecas committed Jul 9, 2024
1 parent c0962dc commit 2e43ee7
Show file tree
Hide file tree
Showing 4 changed files with 80 additions and 74 deletions.
3 changes: 1 addition & 2 deletions src/api/environment.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -262,8 +262,7 @@ void SetIsolateMiscHandlers(v8::Isolate* isolate, const IsolateSettings& s) {
isolate->SetWasmStreamingCallback(wasm_web_api::StartStreamingCompilation);
}

if (per_process::cli_options->get_per_isolate_options()
->experimental_shadow_realm) {
if (per_process::cli_options->experimental_shadow_realm) {
isolate->SetHostCreateShadowRealmContextCallback(
shadow_realm::HostCreateShadowRealmContextCallback);
}
Expand Down
104 changes: 55 additions & 49 deletions src/node_options.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -485,7 +485,6 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
AddOption("--experimental-report", "", NoOp{}, kAllowedInEnvvar);
AddOption(
"--experimental-wasi-unstable-preview1", "", NoOp{}, kAllowedInEnvvar);
AddOption("--expose-gc", "expose gc extension", V8Option{}, kAllowedInEnvvar);
AddOption("--expose-internals", "", &EnvironmentOptions::expose_internals);
AddOption("--frozen-intrinsics",
"experimental frozen intrinsics support",
Expand Down Expand Up @@ -565,9 +564,6 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
"preserve symbolic links when resolving the main module",
&EnvironmentOptions::preserve_symlinks_main,
kAllowedInEnvvar);
AddOption("--prof",
"Generate V8 profiler output.",
V8Option{});
AddOption("--prof-process",
"process V8 profiler output generated using --prof",
&EnvironmentOptions::prof_process);
Expand Down Expand Up @@ -820,42 +816,14 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {

PerIsolateOptionsParser::PerIsolateOptionsParser(
const EnvironmentOptionsParser& eop) {
// Generally, V8 flags are saved in per-process storage and should be added
// to PerProcessOptionsParser.

AddOption("--track-heap-objects",
"track heap object allocations for heap snapshots",
&PerIsolateOptions::track_heap_objects,
kAllowedInEnvvar);

// Explicitly add some V8 flags to mark them as allowed in NODE_OPTIONS.
AddOption("--abort-on-uncaught-exception",
"aborting instead of exiting causes a core file to be generated "
"for analysis",
V8Option{},
kAllowedInEnvvar);
AddOption("--interpreted-frames-native-stack",
"help system profilers to translate JavaScript interpreted frames",
V8Option{},
kAllowedInEnvvar);
AddOption("--max-old-space-size", "", V8Option{}, kAllowedInEnvvar);
AddOption("--max-semi-space-size", "", V8Option{}, kAllowedInEnvvar);
AddOption("--perf-basic-prof", "", V8Option{}, kAllowedInEnvvar);
AddOption(
"--perf-basic-prof-only-functions", "", V8Option{}, kAllowedInEnvvar);
AddOption("--perf-prof", "", V8Option{}, kAllowedInEnvvar);
AddOption("--perf-prof-unwinding-info", "", V8Option{}, kAllowedInEnvvar);
AddOption("--stack-trace-limit", "", V8Option{}, kAllowedInEnvvar);
AddOption("--disallow-code-generation-from-strings",
"disallow eval and friends",
V8Option{},
kAllowedInEnvvar);
AddOption("--huge-max-old-generation-size",
"increase default maximum heap size on machines with 16GB memory "
"or more",
V8Option{},
kAllowedInEnvvar);
AddOption("--jitless",
"disable runtime allocation of executable memory",
V8Option{},
kAllowedInEnvvar);
AddOption("--report-uncaught-exception",
"generate diagnostic report on uncaught exceptions",
&PerIsolateOptions::report_uncaught_exception,
Expand All @@ -870,21 +838,7 @@ PerIsolateOptionsParser::PerIsolateOptionsParser(
&PerIsolateOptions::report_signal,
kAllowedInEnvvar);
Implies("--report-signal", "--report-on-signal");
AddOption("--enable-etw-stack-walking",
"provides heap data to ETW Windows native tracing",
V8Option{},
kAllowedInEnvvar);

AddOption("--experimental-top-level-await", "", NoOp{}, kAllowedInEnvvar);

AddOption("--experimental-shadow-realm",
"",
&PerIsolateOptions::experimental_shadow_realm,
kAllowedInEnvvar);
AddOption("--harmony-shadow-realm", "", V8Option{});
Implies("--experimental-shadow-realm", "--harmony-shadow-realm");
Implies("--harmony-shadow-realm", "--experimental-shadow-realm");
ImpliesNot("--no-harmony-shadow-realm", "--experimental-shadow-realm");
AddOption("--build-snapshot",
"Generate a snapshot blob when the process exits.",
&PerIsolateOptions::build_snapshot,
Expand Down Expand Up @@ -1092,6 +1046,58 @@ PerProcessOptionsParser::PerProcessOptionsParser(
"performance.",
&PerProcessOptions::disable_wasm_trap_handler,
kAllowedInEnvvar);

// Explicitly add some V8 flags to mark them as allowed in NODE_OPTIONS.
// V8 flags are be per-process options and should not be modified with
// NODE_OPTIONS or execArgv with a Worker constructor.
AddOption("--abort-on-uncaught-exception",
"aborting instead of exiting causes a core file to be generated "
"for analysis",
V8Option{},
kAllowedInEnvvar);
AddOption("--expose-gc", "expose gc extension", V8Option{}, kAllowedInEnvvar);
AddOption("--interpreted-frames-native-stack",
"help system profilers to translate JavaScript interpreted frames",
V8Option{},
kAllowedInEnvvar);
AddOption("--max-old-space-size", "", V8Option{}, kAllowedInEnvvar);
AddOption("--max-semi-space-size", "", V8Option{}, kAllowedInEnvvar);
AddOption("--perf-basic-prof", "", V8Option{}, kAllowedInEnvvar);
AddOption(
"--perf-basic-prof-only-functions", "", V8Option{}, kAllowedInEnvvar);
AddOption("--perf-prof", "", V8Option{}, kAllowedInEnvvar);
AddOption("--perf-prof-unwinding-info", "", V8Option{}, kAllowedInEnvvar);
AddOption("--prof", "Generate V8 profiler output.", V8Option{});
AddOption("--stack-trace-limit", "", V8Option{}, kAllowedInEnvvar);
AddOption("--disallow-code-generation-from-strings",
"disallow eval and friends",
V8Option{},
kAllowedInEnvvar);
AddOption("--huge-max-old-generation-size",
"increase default maximum heap size on machines with 16GB memory "
"or more",
V8Option{},
kAllowedInEnvvar);
AddOption("--jitless",
"disable runtime allocation of executable memory",
V8Option{},
kAllowedInEnvvar);
AddOption("--enable-etw-stack-walking",
"provides heap data to ETW Windows native tracing",
V8Option{},
kAllowedInEnvvar);

AddOption("--experimental-top-level-await", "", NoOp{}, kAllowedInEnvvar);

AddOption("--experimental-shadow-realm",
"",
&PerProcessOptions::experimental_shadow_realm,
kAllowedInEnvvar);
AddOption("--harmony-shadow-realm", "", V8Option{});

Implies("--experimental-shadow-realm", "--harmony-shadow-realm");
Implies("--harmony-shadow-realm", "--experimental-shadow-realm");
ImpliesNot("--no-harmony-shadow-realm", "--experimental-shadow-realm");
}

inline std::string RemoveBrackets(const std::string& host) {
Expand Down
2 changes: 1 addition & 1 deletion src/node_options.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,6 @@ class PerIsolateOptions : public Options {
bool track_heap_objects = false;
bool report_uncaught_exception = false;
bool report_on_signal = false;
bool experimental_shadow_realm = false;
std::string report_signal = "SIGUSR2";
bool build_snapshot = false;
std::string build_snapshot_config;
Expand Down Expand Up @@ -289,6 +288,7 @@ class PerProcessOptions : public Options {
bool print_v8_help = false;
bool print_version = false;
std::string experimental_sea_config;
bool experimental_shadow_realm = false;
std::string run;

#ifdef NODE_HAVE_I18N_SUPPORT
Expand Down
45 changes: 23 additions & 22 deletions test/parallel/test-cli-node-options.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,16 +49,6 @@ expectNoWorker('--trace-event-file-pattern {pid}-${rotation}.trace_events ' +
'--trace-event-categories node.async_hooks', 'B\n');
expect('--unhandled-rejections=none', 'B\n');

if (common.isLinux) {
expect('--perf-basic-prof', 'B\n');
expect('--perf-basic-prof-only-functions', 'B\n');

if (['arm', 'x64'].includes(process.arch)) {
expect('--perf-prof', 'B\n');
expect('--perf-prof-unwinding-info', 'B\n');
}
}

if (common.hasCrypto) {
expectNoWorker('--use-openssl-ca', 'B\n');
expectNoWorker('--use-bundled-ca', 'B\n');
Expand All @@ -67,20 +57,31 @@ if (common.hasCrypto) {
}

// V8 options
expect('--abort_on-uncaught_exception', 'B\n');
expect('--disallow-code-generation-from-strings', 'B\n');
expect('--expose-gc', 'B\n');
expect('--huge-max-old-generation-size', 'B\n');
expect('--jitless', 'B\n');
expect('--max-old-space-size=0', 'B\n');
expect('--max-semi-space-size=0', 'B\n');
expect('--stack-trace-limit=100',
/(\s*at f \(\[(eval|worker eval)\]:1:\d*\)\r?\n)/,
'(function f() { f(); })();',
true);
expectNoWorker('--abort_on-uncaught_exception', 'B\n');
expectNoWorker('--disallow-code-generation-from-strings', 'B\n');
expectNoWorker('--expose-gc', 'B\n');
expectNoWorker('--huge-max-old-generation-size', 'B\n');
expectNoWorker('--jitless', 'B\n');
expectNoWorker('--max-old-space-size=0', 'B\n');
expectNoWorker('--max-semi-space-size=0', 'B\n');
expectNoWorker('--stack-trace-limit=100',
/(\s*at f \(\[eval\]:1:\d*\)\r?\n)/,
'(function f() { f(); })();',
true);
// Unsupported on arm. See https://crbug.com/v8/8713.
if (!['arm', 'arm64'].includes(process.arch))
expect('--interpreted-frames-native-stack', 'B\n');
expectNoWorker('--interpreted-frames-native-stack', 'B\n');

// Linux only V8 options
if (common.isLinux) {
expectNoWorker('--perf-basic-prof', 'B\n');
expectNoWorker('--perf-basic-prof-only-functions', 'B\n');

if (['arm', 'x64'].includes(process.arch)) {
expectNoWorker('--perf-prof', 'B\n');
expectNoWorker('--perf-prof-unwinding-info', 'B\n');
}
}

// Workers can't eval as ES Modules. https://github.com/nodejs/node/issues/30682
expectNoWorker('--input-type=module',
Expand Down

0 comments on commit 2e43ee7

Please sign in to comment.