refactor: clean up Node.js cli arg parsing (#39510)

* refactor: clean up Node.js arg parsing

Co-authored-by: Shelley Vohr <[email protected]>

* chore: feedback from review

Co-authored-by: Shelley Vohr <[email protected]>

---------

Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <[email protected]>

docs/api/command-line-switches.md

@@ -116,14 +116,20 @@ Ignore the connections limit for `domains` list separated by `,`.
 
 ### --js-flags=`flags`
 
-Specifies the flags passed to the Node.js engine. It has to be passed when starting
-Electron if you want to enable the `flags` in the main process.
+Specifies the flags passed to the [V8 engine](https://v8.dev). In order to enable the `flags` in the main process,
+this switch must be passed on startup.
 
 ```sh
 $ electron --js-flags="--harmony_proxies --harmony_collections" your-app
 ```
 
-See the [Node.js documentation][node-cli] or run `node --help` in your terminal for a list of available flags. Additionally, run `node --v8-options` to see a list of flags that specifically refer to Node.js's V8 JavaScript engine.
+Run `node --v8-options` or `electron --js-flags="--help"` in your terminal for the list of available flags.  These can be used to enable early-stage JavaScript features, or log and manipulate garbage collection, among other things.
+
+For example, to trace V8 optimization and deoptimization:
+
+```sh
+$ electron --js-flags="--trace-opt --trace-deopt" your-app
+```
 
 ### --lang
 

shell/app/node_main.cc

@@ -50,10 +50,10 @@
 
 namespace {
 
-// Initialize Node.js cli options to pass to Node.js
+// Preparse Node.js cli options to pass to Node.js
 // See https://nodejs.org/api/cli.html#cli_options
-int SetNodeCliFlags() {
-  // Options that are unilaterally disallowed
+void ExitIfContainsDisallowedFlags(const std::vector<std::string>& argv) {
+  // Options that are unilaterally disallowed.
   static constexpr auto disallowed = base::MakeFixedFlatSet<base::StringPiece>({
       "--enable-fips",
       "--force-fips",
@@ -62,40 +62,18 @@ int SetNodeCliFlags() {
       "--use-openssl-ca",
   });
 
-  const auto argv = base::CommandLine::ForCurrentProcess()->argv();
-  std::vector<std::string> args;
-
-  // TODO(codebytere): We need to set the first entry in args to the
-  // process name owing to src/node_options-inl.h#L286-L290 but this is
-  // redundant and so should be refactored upstream.
-  args.reserve(argv.size() + 1);
-  args.emplace_back("electron");
-
   for (const auto& arg : argv) {
-#if BUILDFLAG(IS_WIN)
-    const auto& option = base::WideToUTF8(arg);
-#else
-    const auto& option = arg;
-#endif
-    const auto stripped = base::StringPiece(option).substr(0, option.find('='));
-    if (disallowed.contains(stripped)) {
-      LOG(ERROR) << "The Node.js cli flag " << stripped
+    const auto key = base::StringPiece(arg).substr(0, arg.find('='));
+    if (disallowed.contains(key)) {
+      LOG(ERROR) << "The Node.js cli flag " << key
                  << " is not supported in Electron";
       // Node.js returns 9 from ProcessGlobalArgs for any errors encountered
       // when setting up cli flags and env vars. Since we're outlawing these
-      // flags (making them errors) return 9 here for consistency.
-      return 9;
-    } else {
-      args.push_back(option);
+      // flags (making them errors) exit with the same error code for
+      // consistency.
+      exit(9);
     }
   }
-
-  std::vector<std::string> errors;
-
-  // Node.js itself will output parsing errors to
-  // console so we don't need to handle that ourselves
-  return ProcessGlobalArgs(&args, nullptr, &errors,
-                           node::kDisallowedInEnvironment);
 }
 
 #if IS_MAS_BUILD()
@@ -116,7 +94,11 @@ v8::Local<v8::Value> GetParameters(v8::Isolate* isolate) {
 }
 
 int NodeMain(int argc, char* argv[]) {
-  base::CommandLine::Init(argc, argv);
+  bool initialized = base::CommandLine::Init(argc, argv);
+  if (!initialized) {
+    LOG(ERROR) << "Failed to initialize CommandLine";
+    exit(1);
+  }
 
 #if BUILDFLAG(IS_WIN)
   v8_crashpad_support::SetUp();
@@ -153,15 +135,13 @@ int NodeMain(int argc, char* argv[]) {
     // Explicitly register electron's builtin bindings.
     NodeBindings::RegisterBuiltinBindings();
 
-    // Parse and set Node.js cli flags.
-    int flags_exit_code = SetNodeCliFlags();
-    if (flags_exit_code != 0)
-      exit(flags_exit_code);
-
     // Hack around with the argv pointer. Used for process.title = "blah".
     argv = uv_setup_args(argc, argv);
 
+    // Parse Node.js cli flags and strip out disallowed options.
     std::vector<std::string> args(argv, argv + argc);
+    ExitIfContainsDisallowedFlags(args);
+
     std::unique_ptr<node::InitializationResult> result =
         node::InitializeOncePerProcess(
             args,

shell/common/node_bindings.cc

@@ -387,7 +387,7 @@ bool NodeBindings::IsInitialized() {
 
 // Initialize Node.js cli options to pass to Node.js
 // See https://nodejs.org/api/cli.html#cli_options
-void NodeBindings::SetNodeCliFlags() {
+std::vector<std::string> NodeBindings::ParseNodeCliFlags() {
   const auto argv = base::CommandLine::ForCurrentProcess()->argv();
   std::vector<std::string> args;
 
@@ -404,9 +404,7 @@ void NodeBindings::SetNodeCliFlags() {
     const auto& option = arg;
 #endif
     const auto stripped = base::StringPiece(option).substr(0, option.find('='));
-
-    // Only allow in no-op (--) option or a small set of debug
-    // and trace related options.
+    // Only allow no-op or a small set of debug/trace related options.
     if (IsAllowedOption(stripped) || stripped == "--")
       args.push_back(option);
   }
@@ -418,16 +416,7 @@ void NodeBindings::SetNodeCliFlags() {
     args.push_back("--no-experimental-fetch");
   }
 
-  std::vector<std::string> errors;
-  const int exit_code = ProcessGlobalArgs(&args, nullptr, &errors,
-                                          node::kDisallowedInEnvironment);
-
-  const std::string err_str = "Error parsing Node.js cli flags ";
-  if (exit_code != 0) {
-    LOG(ERROR) << err_str;
-  } else if (!errors.empty()) {
-    LOG(ERROR) << err_str << base::JoinString(errors, " ");
-  }
+  return args;
 }
 
 void NodeBindings::Initialize(v8::Local<v8::Context> context) {
@@ -443,13 +432,11 @@ void NodeBindings::Initialize(v8::Local<v8::Context> context) {
   // Explicitly register electron's builtin bindings.
   RegisterBuiltinBindings();
 
-  // Parse and set Node.js cli flags.
-  SetNodeCliFlags();
-
   auto env = base::Environment::Create();
   SetNodeOptions(env.get());
 
-  std::vector<std::string> argv = {"electron"};
+  // Parse and set Node.js cli flags.
+  std::vector<std::string> argv = ParseNodeCliFlags();
   std::vector<std::string> exec_argv;
   std::vector<std::string> errors;
   uint64_t process_flags = node::ProcessFlags::kNoFlags;

shell/common/node_bindings.h

@@ -89,7 +89,7 @@ class NodeBindings {
   // Setup V8, libuv.
   void Initialize(v8::Local<v8::Context> context);
 
-  void SetNodeCliFlags();
+  std::vector<std::string> ParseNodeCliFlags();
 
   // Create the environment and load node.js.
   node::Environment* CreateEnvironment(v8::Handle<v8::Context> context,