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: #25612 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
nodejs · Jan 28, 2019 · 845bcfa · 845bcfa
1 parent 7d68223
commit 845bcfa
Show file tree

Hide file tree

Showing 3 changed files with 26 additions and 38 deletions.
diff --git 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
@@ -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
@@ -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 {