From 845bcfa1ce5f086ba1bcb0a98a78e1c392ef23f0 Mon Sep 17 00:00:00 2001
From: Joyee Cheung <joyeec9h3@gmail.com>
Date: Mon, 21 Jan 2019 22:57:51 +0800
Subject: [PATCH] src: simplify inspector initialization in node::Start()

Remove the `StartInspector` and `InspectorStarted` abstraction
out of `v8_platform`, and error out early and directly in the
option parser if Node is configured with NODE_USE_V8_PLATFORM and
inspector enabled but the user still tries to use inspector options.

PR-URL: https://github.com/nodejs/node/pull/25612
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
---
 src/node.cc         | 53 +++++++++++++--------------------------------
 src/node_options.cc |  9 ++++++++
 src/node_options.h  |  2 ++
 3 files changed, 26 insertions(+), 38 deletions(-)

diff --git a/src/node.cc b/src/node.cc
index 1accbd615d5334..85e1f8ccb4c441 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -262,23 +262,6 @@ static struct {
     platform_->CancelPendingDelayedTasks(isolate);
   }
 
-#if HAVE_INSPECTOR
-  bool StartInspector(Environment* env, const char* script_path) {
-    // Inspector agent can't fail to start, but if it was configured to listen
-    // right away on the websocket port and fails to bind/etc, this will return
-    // false.
-    return env->inspector_agent()->Start(
-        script_path == nullptr ? "" : script_path,
-        env->options()->debug_options(),
-        env->inspector_host_port(),
-        true);
-  }
-
-  bool InspectorStarted(Environment* env) {
-    return env->inspector_agent()->IsListening();
-  }
-#endif  // HAVE_INSPECTOR
-
   void StartTracingAgent() {
     if (per_process::cli_options->trace_event_categories.empty()) {
       tracing_file_writer_ = tracing_agent_->DefaultHandle();
@@ -317,10 +300,6 @@ static struct {
   void Dispose() {}
   void DrainVMTasks(Isolate* isolate) {}
   void CancelVMTasks(Isolate* isolate) {}
-  bool StartInspector(Environment* env, const char* script_path) {
-    env->ThrowError("Node compiled with NODE_USE_V8_PLATFORM=0");
-    return true;
-  }
 
   void StartTracingAgent() {
     if (!trace_enabled_categories.empty()) {
@@ -338,12 +317,6 @@ static struct {
     return nullptr;
   }
 #endif  // !NODE_USE_V8_PLATFORM
-
-#if !NODE_USE_V8_PLATFORM || !HAVE_INSPECTOR
-  bool InspectorStarted(Environment* env) {
-    return false;
-  }
-#endif  //  !NODE_USE_V8_PLATFORM || !HAVE_INSPECTOR
 } v8_platform;
 
 tracing::AgentWriterHandle* GetTracingAgentWriter() {
@@ -774,13 +747,6 @@ void StartExecution(Environment* env, const char* main_script_id) {
       env->context(), Undefined(env->isolate()), arraysize(argv), argv));
 }
 
-static void StartInspector(Environment* env, const char* path) {
-#if HAVE_INSPECTOR
-  CHECK(!env->inspector_agent()->IsListening());
-  v8_platform.StartInspector(env, path);
-#endif  // HAVE_INSPECTOR
-}
-
 
 #ifdef __POSIX__
 void RegisterSignalHandler(int signal,
@@ -1280,13 +1246,24 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data,
   Environment env(isolate_data, context);
   env.Start(args, exec_args, per_process::v8_is_profiling);
 
-  const char* path = args.size() > 1 ? args[1].c_str() : nullptr;
-  StartInspector(&env, path);
-
+#if HAVE_INSPECTOR && NODE_USE_V8_PLATFORM
+  CHECK(!env.inspector_agent()->IsListening());
+  // Inspector agent can't fail to start, but if it was configured to listen
+  // right away on the websocket port and fails to bind/etc, this will return
+  // false.
+  env.inspector_agent()->Start(args.size() > 1 ? args[1].c_str() : "",
+                               env.options()->debug_options(),
+                               env.inspector_host_port(),
+                               true);
   if (env.options()->debug_options().inspector_enabled &&
-      !v8_platform.InspectorStarted(&env)) {
+      !env.inspector_agent()->IsListening()) {
     return 12;  // Signal internal error.
   }
+#else
+  // inspector_enabled can't be true if !HAVE_INSPECTOR or !NODE_USE_V8_PLATFORM
+  // - the option parser should not allow that.
+  CHECK(!env.options()->debug_options().inspector_enabled);
+#endif  // HAVE_INSPECTOR && NODE_USE_V8_PLATFORM
 
   {
     Environment::AsyncCallbackScope callback_scope(&env);
diff --git a/src/node_options.cc b/src/node_options.cc
index 7c796a433a9645..cc8bf61928be77 100644
--- a/src/node_options.cc
+++ b/src/node_options.cc
@@ -23,6 +23,15 @@ Mutex cli_options_mutex;
 std::shared_ptr<PerProcessOptions> cli_options{new PerProcessOptions()};
 }  // namespace per_process
 
+void DebugOptions::CheckOptions(std::vector<std::string>* errors) {
+#if !NODE_USE_V8_PLATFORM
+  if (inspector_enabled) {
+    errors->push_back("Inspector is not available when Node is compiled "
+                      "--without-v8-platform");
+  }
+#endif
+}
+
 void PerProcessOptions::CheckOptions(std::vector<std::string>* errors) {
 #if HAVE_OPENSSL
   if (use_openssl_ca && use_bundled_ca) {
diff --git a/src/node_options.h b/src/node_options.h
index a2dfab15ee6b78..4a53c5694eee21 100644
--- a/src/node_options.h
+++ b/src/node_options.h
@@ -88,6 +88,8 @@ class DebugOptions : public Options {
   bool wait_for_connect() const {
     return break_first_line || break_node_first_line;
   }
+
+  void CheckOptions(std::vector<std::string>* errors);
 };
 
 class EnvironmentOptions : public Options {