refactor: remove use of Node's DebugOptions (#22083)

patches/node/.patches

@@ -7,7 +7,6 @@ expose_get_builtin_module_function.patch
 fix_build_and_expose_inspector_agent.patch
 fix_expose_internalcallbackscope.patch
 build_add_gn_build_files.patch
-fix_export_debugoptions.patch
 fix_add_default_values_for_enable_lto_and_build_v8_with_gn_in.patch
 feat_add_new_built_with_electron_variable_to_config_gypi.patch
 feat_add_flags_for_low-level_hooks_and_exceptions.patch
@@ -18,7 +17,6 @@ fixme_use_redefined_version_of_internalmodulestat.patch
 fixme_remove_async_id_assertion_check.patch
 fixme_comment_trace_event_macro.patch
 fix_key_gen_apis_are_not_available_in_boringssl.patch
-fix_do_not_define_debugoptions_s_constructors_in_header.patch
 build_modify_js2c_py_to_allow_injection_of_original-fs_and_custom_embedder_js.patch
 refactor_allow_embedder_overriding_of_internal_fs_calls.patch
 chore_prevent_warn_non_context-aware_native_modules_being_loaded.patch

patches/node/fix_do_not_define_debugoptions_s_constructors_in_header.patch

@@ -1,54 +0,0 @@
-From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
-From: Cheng Zhao <[email protected]>
-Date: Fri, 29 Mar 2019 17:17:30 +0900
-Subject: fix: do not define DebugOptions's constructors in header
-
-The e3ca89ef49 changed NODE_EXTERN to always expose symbols for
-DebugOptions, and since the constructors of DebugOptions are defined in
-header, the compiler would end up generating no constructor
-implementations for DebugOptions.
-
-Which means we would encounter crash when constructors of DebugOptions
-is called.
-
-By putting the definitions of constructors into the implementation file,
-we can avoid this problem.
-
-diff --git a/src/node_options.cc b/src/node_options.cc
-index 2dd90e9a8cf9381c8608b73ead247bcf86b72ab9..051e7b084b6770215bd009b0e37feabbb7df7389 100644
---- a/src/node_options.cc
-+++ b/src/node_options.cc
-@@ -26,6 +26,12 @@ Mutex cli_options_mutex;
- std::shared_ptr<PerProcessOptions> cli_options{new PerProcessOptions()};
- }  // namespace per_process
- 
-+DebugOptions::DebugOptions() = default;
-+DebugOptions::DebugOptions(const DebugOptions&) = default;
-+DebugOptions::DebugOptions(DebugOptions&&) = default;
-+DebugOptions& DebugOptions::operator=(const DebugOptions&) = default;
-+DebugOptions& DebugOptions::operator=(DebugOptions&&) = default;
-+
- void DebugOptions::CheckOptions(std::vector<std::string>* errors) {
- #if !NODE_USE_V8_PLATFORM && !HAVE_INSPECTOR
-   if (inspector_enabled) {
-diff --git a/src/node_options.h b/src/node_options.h
-index 40c19ea6ff4d98a1a1da59bca76087209445af81..4ce5551284bb5b1b4194905a9fe619f852933405 100644
---- a/src/node_options.h
-+++ b/src/node_options.h
-@@ -61,11 +61,11 @@ struct InspectPublishUid {
- // per-Isolate, rather than per-Environment.
- class NODE_EXTERN DebugOptions : public Options {
-  public:
--  DebugOptions() = default;
--  DebugOptions(const DebugOptions&) = default;
--  DebugOptions& operator=(const DebugOptions&) = default;
--  DebugOptions(DebugOptions&&) = default;
--  DebugOptions& operator=(DebugOptions&&) = default;
-+  DebugOptions();
-+  DebugOptions(const DebugOptions&);
-+  DebugOptions& operator=(const DebugOptions&);
-+  DebugOptions(DebugOptions&&);
-+  DebugOptions& operator=(DebugOptions&&);
- 
-   // --inspect
-   bool inspector_enabled = false;

patches/node/fix_export_debugoptions.patch

@@ -1,57 +0,0 @@
-From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
-From: Nitish Sakhawalkar <[email protected]>
-Date: Thu, 11 Apr 2019 11:50:49 -0700
-Subject: fix: export DebugOptions
-
-This exports DebugOptions so we can parse args like `--inspect-brk`.
-
-diff --git a/src/node_options.cc b/src/node_options.cc
-index 35cac18a916c2ce104a3e7b6760d0280b33c2892..2dd90e9a8cf9381c8608b73ead247bcf86b72ab9 100644
---- a/src/node_options.cc
-+++ b/src/node_options.cc
-@@ -210,11 +210,6 @@ void EnvironmentOptions::CheckOptions(std::vector<std::string>* errors) {
- 
- namespace options_parser {
- 
--class DebugOptionsParser : public OptionsParser<DebugOptions> {
-- public:
--  DebugOptionsParser();
--};
--
- class EnvironmentOptionsParser : public OptionsParser<EnvironmentOptions> {
-  public:
-   EnvironmentOptionsParser();
-diff --git a/src/node_options.h b/src/node_options.h
-index 8937bfd9011e4795d22e232886e18183d698b8d4..40c19ea6ff4d98a1a1da59bca76087209445af81 100644
---- a/src/node_options.h
-+++ b/src/node_options.h
-@@ -59,7 +59,7 @@ struct InspectPublishUid {
- // to keep them separate since they are a group of options applying to a very
- // specific part of Node. It might also make more sense for them to be
- // per-Isolate, rather than per-Environment.
--class DebugOptions : public Options {
-+class NODE_EXTERN DebugOptions : public Options {
-  public:
-   DebugOptions() = default;
-   DebugOptions(const DebugOptions&) = default;
-@@ -244,7 +244,7 @@ class PerProcessOptions : public Options {
- 
- namespace options_parser {
- 
--HostPort SplitHostPort(const std::string& arg,
-+HostPort NODE_EXTERN SplitHostPort(const std::string& arg,
-     std::vector<std::string>* errors);
- void GetOptions(const v8::FunctionCallbackInfo<v8::Value>& args);
- 
-@@ -437,6 +437,11 @@ class OptionsParser {
-   friend void GetOptions(const v8::FunctionCallbackInfo<v8::Value>& args);
- };
- 
-+class NODE_EXTERN DebugOptionsParser : public OptionsParser<DebugOptions> {
-+ public:
-+  DebugOptionsParser();
-+};
-+
- using StringVector = std::vector<std::string>;
- template <class OptionsType, class = Options>
- void Parse(

shell/browser/node_debugger.cc

@@ -27,34 +27,13 @@ void NodeDebugger::Start() {
   if (inspector == nullptr)
     return;
 
-  std::vector<std::string> args;
-  for (auto& arg : base::CommandLine::ForCurrentProcess()->argv()) {
-#if defined(OS_WIN)
-    args.push_back(base::UTF16ToUTF8(arg));
-#else
-    args.push_back(arg);
-#endif
-  }
-
-  node::DebugOptions options;
-  std::vector<std::string> exec_args;
-  std::vector<std::string> v8_args;
-  std::vector<std::string> errors;
-
-  // TODO(codebytere): remove this parsing and use ProcessGlobalArgs
-  node::options_parser::Parse(&args, &exec_args, &v8_args, &options,
-                              node::kDisallowedInEnvironment, &errors);
-
-  if (!errors.empty()) {
-    // TODO(jeremy): what's the appropriate behaviour here?
-    LOG(ERROR) << "Error parsing node options: "
-               << base::JoinString(errors, " ");
-  }
-
-  const char* path = "";
-  if (inspector->Start(path, options,
-                       std::make_shared<node::HostPort>(options.host_port),
-                       true /* is_main */))
+  // DebugOptions will already have been set by ProcessGlobalArgs,
+  // so just pull the ones from there to pass to the InspectorAgent
+  const auto debug_options = env_->options()->debug_options();
+  if (inspector->Start(
+          "" /* path */, debug_options,
+          std::make_shared<node::HostPort>(debug_options.host_port),
+          true /* is_main */))
     DCHECK(env_->inspector_agent()->IsListening());
 }
 

shell/common/node_bindings.cc

@@ -164,7 +164,9 @@ void SetNodeCliFlags() {
     const auto& option = arg;
 #endif
     const auto stripped = base::StringPiece(option).substr(0, option.find('='));
-    if (allowed.count(stripped) != 0)
+
+    // Only allow in no-op (--) option or DebugOptions
+    if (allowed.count(stripped) != 0 || stripped == "--")
       args.push_back(option);
   }
 
@@ -172,13 +174,13 @@ void SetNodeCliFlags() {
   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) {
-    if (!errors.empty())
-      LOG(INFO) << base::JoinString(errors, " ");
-    else
-      LOG(INFO) << "Error parsing Node.js cli flags";
+    LOG(ERROR) << err_str;
+  } else if (!errors.empty()) {
+    LOG(ERROR) << err_str << base::JoinString(errors, " ");
   }
-}
+}  // namespace
 
 // Initialize NODE_OPTIONS to pass to Node.js
 // See https://nodejs.org/api/cli.html#cli_node_options_options

shell/common/node_includes.h

@@ -64,6 +64,7 @@
 #include "node.h"
 #include "node_buffer.h"
 #include "node_internals.h"
+#include "node_options-inl.h"
 #include "node_options.h"
 #include "node_platform.h"