Browse Source

refactor: use compile-time cli arg sets. (#38770)

We're currently building these on the heap with `std::set<std::string>`
but this can be a very small compile-time container instead.

Marking as 'refactor' rather than 'perf' since this isn't called often,
but moving from heap to compile-time is good and using this container
makes the code more readable.
trop[bot] 1 year ago
parent
commit
8df8cad3a0
2 changed files with 40 additions and 34 deletions
  1. 9 5
      shell/app/node_main.cc
  2. 31 29
      shell/common/node_bindings.cc

+ 9 - 5
shell/app/node_main.cc

@@ -7,12 +7,12 @@
 #include <map>
 #include <memory>
 #include <string>
-#include <unordered_set>
 #include <utility>
 #include <vector>
 
 #include "base/base_switches.h"
 #include "base/command_line.h"
+#include "base/containers/fixed_flat_set.h"
 #include "base/feature_list.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
@@ -54,9 +54,13 @@ namespace {
 // See https://nodejs.org/api/cli.html#cli_options
 int SetNodeCliFlags() {
   // Options that are unilaterally disallowed
-  const std::unordered_set<base::StringPiece, base::StringPieceHash>
-      disallowed = {"--openssl-config", "--use-bundled-ca", "--use-openssl-ca",
-                    "--force-fips", "--enable-fips"};
+  static constexpr auto disallowed = base::MakeFixedFlatSet<base::StringPiece>({
+      "--enable-fips",
+      "--force-fips",
+      "--openssl-config",
+      "--use-bundled-ca",
+      "--use-openssl-ca",
+  });
 
   const auto argv = base::CommandLine::ForCurrentProcess()->argv();
   std::vector<std::string> args;
@@ -74,7 +78,7 @@ int SetNodeCliFlags() {
     const auto& option = arg;
 #endif
     const auto stripped = base::StringPiece(option).substr(0, option.find('='));
-    if (disallowed.count(stripped) != 0) {
+    if (disallowed.contains(stripped)) {
       LOG(ERROR) << "The Node.js cli flag " << stripped
                  << " is not supported in Electron";
       // Node.js returns 9 from ProcessGlobalArgs for any errors encountered

+ 31 - 29
shell/common/node_bindings.cc

@@ -6,14 +6,13 @@
 
 #include <algorithm>
 #include <memory>
-#include <set>
 #include <string>
-#include <unordered_set>
 #include <utility>
 #include <vector>
 
 #include "base/base_paths.h"
 #include "base/command_line.h"
+#include "base/containers/fixed_flat_set.h"
 #include "base/environment.h"
 #include "base/path_service.h"
 #include "base/run_loop.h"
@@ -236,32 +235,39 @@ void ErrorMessageListener(v8::Local<v8::Message> message,
   }
 }
 
-const std::unordered_set<base::StringPiece, base::StringPieceHash>
-GetAllowedDebugOptions() {
-  if (electron::fuses::IsNodeCliInspectEnabled()) {
-    // Only allow DebugOptions in non-ELECTRON_RUN_AS_NODE mode
-    return {
-        "--inspect",          "--inspect-brk",
-        "--inspect-port",     "--debug",
-        "--debug-brk",        "--debug-port",
-        "--inspect-brk-node", "--inspect-publish-uid",
-    };
-  }
-  // If node CLI inspect support is disabled, allow no debug options.
-  return {};
+// Only allow DebugOptions in non-ELECTRON_RUN_AS_NODE mode.
+// If node CLI inspect support is disabled, allow no debug options.
+bool IsAllowedDebugOption(base::StringPiece option) {
+  static constexpr auto options = base::MakeFixedFlatSet<base::StringPiece>({
+      "--debug",
+      "--debug-brk",
+      "--debug-port",
+      "--inspect",
+      "--inspect-brk",
+      "--inspect-brk-node",
+      "--inspect-port",
+      "--inspect-publish-uid",
+  });
+
+  return electron::fuses::IsNodeCliInspectEnabled() && options.contains(option);
 }
 
 // Initialize NODE_OPTIONS to pass to Node.js
 // See https://nodejs.org/api/cli.html#cli_node_options_options
 void SetNodeOptions(base::Environment* env) {
   // Options that are unilaterally disallowed
-  const std::set<std::string> disallowed = {
-      "--openssl-config", "--use-bundled-ca", "--use-openssl-ca",
-      "--force-fips", "--enable-fips"};
-
-  // Subset of options allowed in packaged apps
-  const std::set<std::string> allowed_in_packaged = {"--max-http-header-size",
-                                                     "--http-parser"};
+  static constexpr auto disallowed = base::MakeFixedFlatSet<base::StringPiece>({
+      "--enable-fips",
+      "--force-fips",
+      "--openssl-config",
+      "--use-bundled-ca",
+      "--use-openssl-ca",
+  });
+
+  static constexpr auto pkg_opts = base::MakeFixedFlatSet<base::StringPiece>({
+      "--http-parser",
+      "--max-http-header-size",
+  });
 
   if (env->HasVar("NODE_OPTIONS")) {
     if (electron::fuses::IsNodeOptionsEnabled()) {
@@ -276,13 +282,12 @@ void SetNodeOptions(base::Environment* env) {
         // Strip off values passed to individual NODE_OPTIONs
         std::string option = part.substr(0, part.find('='));
 
-        if (is_packaged_app &&
-            allowed_in_packaged.find(option) == allowed_in_packaged.end()) {
+        if (is_packaged_app && !pkg_opts.contains(option)) {
           // Explicitly disallow majority of NODE_OPTIONS in packaged apps
           LOG(ERROR) << "Most NODE_OPTIONs are not supported in packaged apps."
                      << " See documentation for more details.";
           options.erase(options.find(option), part.length());
-        } else if (disallowed.find(option) != disallowed.end()) {
+        } else if (disallowed.contains(option)) {
           // Remove NODE_OPTIONS specifically disallowed for use in Node.js
           // through Electron owing to constraints like BoringSSL.
           LOG(ERROR) << "The NODE_OPTION " << option
@@ -383,9 +388,6 @@ 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() {
-  const std::unordered_set<base::StringPiece, base::StringPieceHash> allowed =
-      GetAllowedDebugOptions();
-
   const auto argv = base::CommandLine::ForCurrentProcess()->argv();
   std::vector<std::string> args;
 
@@ -404,7 +406,7 @@ void NodeBindings::SetNodeCliFlags() {
     const auto stripped = base::StringPiece(option).substr(0, option.find('='));
 
     // Only allow in no-op (--) option or DebugOptions
-    if (allowed.count(stripped) != 0 || stripped == "--")
+    if (IsAllowedDebugOption(stripped) || stripped == "--")
       args.push_back(option);
   }