From 2e43ee7c02fa0f1c438ec630b3e2b1e632545dd4 Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Tue, 9 Jul 2024 12:14:43 +0100 Subject: [PATCH] src: reject per-process isolate flags in workers 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. --- src/api/environment.cc | 3 +- src/node_options.cc | 104 +++++++++++++------------ src/node_options.h | 2 +- test/parallel/test-cli-node-options.js | 45 +++++------ 4 files changed, 80 insertions(+), 74 deletions(-) diff --git a/src/api/environment.cc b/src/api/environment.cc index 6524eb440bd69f..f89cd88240a770 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -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); } diff --git a/src/node_options.cc b/src/node_options.cc index 2e43bf8242c499..4b63fb098e593c 100644 --- a/src/node_options.cc +++ b/src/node_options.cc @@ -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", @@ -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); @@ -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, @@ -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, @@ -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) { diff --git a/src/node_options.h b/src/node_options.h index 489638739c32d6..26721b21945ccd 100644 --- a/src/node_options.h +++ b/src/node_options.h @@ -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; @@ -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 diff --git a/test/parallel/test-cli-node-options.js b/test/parallel/test-cli-node-options.js index 8d614e607177cd..02f16d06a6fb9c 100644 --- a/test/parallel/test-cli-node-options.js +++ b/test/parallel/test-cli-node-options.js @@ -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'); @@ -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',