inspector: fix abort when two Environments own the inspector

src/inspector_agent.cc

@@ -72,12 +72,12 @@ using v8_inspector::V8InspectorClient;
 #ifdef __POSIX__
 static uv_sem_t start_io_thread_semaphore;
 #endif  // __POSIX__
-static uv_async_t start_io_thread_async;
-// This is just an additional check to make sure start_io_thread_async
-// is not accidentally re-used or used when uninitialized.
-static std::atomic_bool start_io_thread_async_initialized { false };
-// Protects the Agent* stored in start_io_thread_async.data.
-static Mutex start_io_thread_async_mutex;
+// Agents that asked for the debug signal handler; SIGUSR1 (or the Windows
+// remote thread) starts the io thread of each. The mutex also guards the
+// once-per-process watchdog setup.
+static Mutex start_io_thread_agents_mutex;
+static std::vector<Agent*> start_io_thread_agents;
+static bool debug_signal_handler_started = false;
 
 template <typename Defer, typename OnFailure>
 void SyncJavaScriptHookState(Environment* env,
@@ -125,12 +125,11 @@ void SyncJavaScriptHookState(Environment* env,
   }
 }
 
-// Called on the main thread.
-void StartIoThreadAsyncCallback(uv_async_t* handle) {
-  static_cast<Agent*>(handle->data)->StartIoThread();
+static void RequestIoThreadStartOnAgents() {
+  Mutex::ScopedLock lock(start_io_thread_agents_mutex);
+  for (Agent* agent : start_io_thread_agents) agent->RequestIoThreadStart();
 }
 
-
 #ifdef __POSIX__
 static void StartIoThreadWakeup(int signo, siginfo_t* info, void* ucontext) {
   uv_sem_post(&start_io_thread_semaphore);
@@ -140,16 +139,11 @@ inline void* StartIoThreadMain(void* unused) {
   uv_thread_setname("SignalInspector");
   for (;;) {
     uv_sem_wait(&start_io_thread_semaphore);
-    Mutex::ScopedLock lock(start_io_thread_async_mutex);
-
-    CHECK(start_io_thread_async_initialized);
-    Agent* agent = static_cast<Agent*>(start_io_thread_async.data);
-    if (agent != nullptr)
-      agent->RequestIoThreadStart();
+    RequestIoThreadStartOnAgents();
   }
 }
 
-static int StartDebugSignalHandler() {
+static int StartWatchdogThread() {
   // Start a watchdog thread for calling v8::Debug::DebugBreak() because
   // it's not safe to call directly from the signal handler, it can
   // deadlock with the thread it interrupts.
@@ -184,14 +178,28 @@ static int StartDebugSignalHandler() {
     fprintf(stderr, "node[%u]: pthread_create: %s\n",
             uv_os_getpid(), strerror(err));
     fflush(stderr);
-    // Leave SIGUSR1 blocked.  We don't install a signal handler,
-    // receiving the signal would terminate the process.
+    uv_sem_destroy(&start_io_thread_semaphore);
     return -err;
   }
   RegisterSignalHandler(SIGUSR1, StartIoThreadWakeup);
   // Restore original mask
   CHECK_EQ(0, pthread_sigmask(SIG_SETMASK, &sigmask, nullptr));
-  // Unblock SIGUSR1.  A pending SIGUSR1 signal will now be delivered.
+  return 0;
+}
+
+static int StartDebugSignalHandler() {
+  {
+    Mutex::ScopedLock lock(start_io_thread_agents_mutex);
+    if (!debug_signal_handler_started) {
+      // Leave SIGUSR1 blocked on failure. We don't install a signal handler,
+      // receiving the signal would terminate the process.
+      if (int err = StartWatchdogThread()) return err;
+      debug_signal_handler_started = true;
+    }
+  }
+  // Unblock SIGUSR1 on this thread; PlatformInit() left it blocked. A pending
+  // SIGUSR1 signal will now be delivered.
+  sigset_t sigmask;
   sigemptyset(&sigmask);
   sigaddset(&sigmask, SIGUSR1);
   CHECK_EQ(0, pthread_sigmask(SIG_UNBLOCK, &sigmask, nullptr));
@@ -202,11 +210,7 @@ static int StartDebugSignalHandler() {
 
 #ifdef _WIN32
 DWORD WINAPI StartIoThreadProc(void* arg) {
-  Mutex::ScopedLock lock(start_io_thread_async_mutex);
-  CHECK(start_io_thread_async_initialized);
-  Agent* agent = static_cast<Agent*>(start_io_thread_async.data);
-  if (agent != nullptr)
-    agent->RequestIoThreadStart();
+  RequestIoThreadStartOnAgents();
   return 0;
 }
 
@@ -216,6 +220,9 @@ static int GetDebugSignalHandlerMappingName(DWORD pid, wchar_t* buf,
 }
 
 static int StartDebugSignalHandler() {
+  Mutex::ScopedLock lock(start_io_thread_agents_mutex);
+  if (debug_signal_handler_started) return 0;
+  debug_signal_handler_started = true;
   wchar_t mapping_name[32];
   HANDLE mapping_handle;
   DWORD pid;
@@ -899,7 +906,21 @@ Agent::Agent(Environment* env)
       debug_options_(env->options()->debug_options()),
       host_port_(env->inspector_host_port()) {}
 
-Agent::~Agent() = default;
+Agent::~Agent() {
+  StopAcceptingIoThreadStarts();
+}
+
+void Agent::StopAcceptingIoThreadStarts() {
+  if (start_io_thread_async_ == nullptr) return;
+  {
+    Mutex::ScopedLock lock(start_io_thread_agents_mutex);
+    std::erase(start_io_thread_agents, this);
+  }
+  parent_env_->RemoveCleanupHook(StopAcceptingIoThreadStartsHook, this);
+  parent_env_->CloseHandle(start_io_thread_async_,
+                           [](uv_async_t* handle) { delete handle; });
+  start_io_thread_async_ = nullptr;
+}
 
 bool Agent::Start(const std::string& path,
                   const DebugOptions& options,
@@ -911,33 +932,25 @@ bool Agent::Start(const std::string& path,
   host_port_ = host_port;
 
   client_ = std::make_shared<NodeInspectorClient>(parent_env_, is_main);
-  if (parent_env_->owns_inspector()) {
-    Mutex::ScopedLock lock(start_io_thread_async_mutex);
-    CHECK_EQ(start_io_thread_async_initialized.exchange(true), false);
-    CHECK_EQ(0, uv_async_init(parent_env_->event_loop(),
-                              &start_io_thread_async,
-                              StartIoThreadAsyncCallback));
-    uv_unref(reinterpret_cast<uv_handle_t*>(&start_io_thread_async));
-    start_io_thread_async.data = this;
-    if (parent_env_->should_start_debug_signal_handler()) {
-      // Ignore failure, SIGUSR1 won't work, but that should not block node
-      // start.
-      StartDebugSignalHandler();
+  if (parent_env_->owns_inspector() &&
+      parent_env_->should_start_debug_signal_handler()) {
+    start_io_thread_async_ = new uv_async_t;
+    start_io_thread_async_->data = this;
+    CHECK_EQ(0,
+             uv_async_init(parent_env_->event_loop(),
+                           start_io_thread_async_,
+                           [](uv_async_t* handle) {
+                             static_cast<Agent*>(handle->data)->StartIoThread();
+                           }));
+    uv_unref(reinterpret_cast<uv_handle_t*>(start_io_thread_async_));
+    {
+      Mutex::ScopedLock lock(start_io_thread_agents_mutex);
+      start_io_thread_agents.push_back(this);
     }
-
-    parent_env_->AddCleanupHook([](void* data) {
-      Environment* env = static_cast<Environment*>(data);
-
-      {
-        Mutex::ScopedLock lock(start_io_thread_async_mutex);
-        start_io_thread_async.data = nullptr;
-      }
-
-      // This is global, will never get freed
-      env->CloseHandle(&start_io_thread_async, [](uv_async_t*) {
-        CHECK(start_io_thread_async_initialized.exchange(false));
-      });
-    }, parent_env_);
+    parent_env_->AddCleanupHook(StopAcceptingIoThreadStartsHook, this);
+    // Ignore failure, SIGUSR1 won't work, but that should not block node
+    // start.
+    StartDebugSignalHandler();
   }
 
   AtExit(parent_env_, [](void* env) {
@@ -1175,21 +1188,21 @@ void Agent::AllAsyncTasksCanceled() {
   client_->AllAsyncTasksCanceled();
 }
 
+void Agent::StopAcceptingIoThreadStartsHook(void* agent) {
+  static_cast<Agent*>(agent)->StopAcceptingIoThreadStarts();
+}
+
 void Agent::RequestIoThreadStart() {
   // We need to attempt to interrupt V8 flow (in case Node is running
   // continuous JS code) and to wake up libuv thread (in case Node is waiting
   // for IO events)
   if (!options().allow_attaching_debugger) {
     return;
   }
-  CHECK(start_io_thread_async_initialized);
-  uv_async_send(&start_io_thread_async);
   parent_env_->RequestInterrupt([this](Environment*) {
     StartIoThread();
   });
-
-  CHECK(start_io_thread_async_initialized);
-  uv_async_send(&start_io_thread_async);
+  uv_async_send(start_io_thread_async_);
 }
 
 void Agent::ContextCreated(Local<Context> context, const ContextInfo& info) {

src/inspector_agent.h

@@ -8,6 +8,7 @@
 #endif
 
 #include "node_options.h"
+#include "uv.h"
 #include "v8.h"
 
 #include <cstddef>
@@ -123,7 +124,8 @@ class Agent {
   // Can only be called from the main thread.
   bool StartIoThread();
 
-  // Calls StartIoThread() from off the main thread.
+  // Calls StartIoThread() from off the main thread. Only valid while the
+  // Environment owns the inspector and has not started cleanup.
   void RequestIoThreadStart();
 
   const DebugOptions& options() { return debug_options_; }
@@ -160,6 +162,12 @@ class Agent {
   // reconciles the two when it is possible and safe to call into JS.
   JavaScriptHookState async_hook_state_;
 
+  // Woken by the SIGUSR1 watchdog; closed by the cleanup hook or ~Agent(),
+  // whichever runs first, and freed by its close callback.
+  uv_async_t* start_io_thread_async_ = nullptr;
+  void StopAcceptingIoThreadStarts();
+  static void StopAcceptingIoThreadStartsHook(void* agent);
+
   // Network tracking uses JS hooks. Reconcile the protocol requested and
   // applied states after leaving a V8 interrupt.
   JavaScriptHookState network_tracking_state_;

src/node.h

@@ -210,8 +210,10 @@ enum Flags : uint32_t {
   kNoICU = 1 << 3,
   // Do not modify stdio file descriptor or TTY state.
   kNoStdioInitialization = 1 << 4,
-  // Do not register Node.js-specific signal handlers
-  // and reset other signal handlers to default state.
+  // Do not register Node.js-specific signal handlers, reset other signal
+  // handlers to default state, or replace the calling thread's signal mask
+  // (without this flag, POSIX builds with the inspector set it to block
+  // SIGUSR1 and nothing else).
   kNoDefaultSignalHandling = 1 << 5,
   // Do not perform V8 initialization.
   kNoInitializeV8 = 1 << 6,

test/cctest/test_environment.cc

@@ -341,9 +341,8 @@ TEST_F(EnvironmentTest, RemoveEnvironmentCleanupHookDuringCleanup) {
 TEST_F(EnvironmentTest, MultipleEnvironmentsPerIsolate) {
   const v8::HandleScope handle_scope(isolate_);
   const Argv argv;
-  // Only one of the Environments can have default flags and own the inspector.
   Env env1 {handle_scope, argv};
-  Env env2 {handle_scope, argv, node::EnvironmentFlags::kNoFlags};
+  Env env2{handle_scope, argv};
 
   AtExit(*env1, at_exit_callback1, nullptr);
   AtExit(*env2, at_exit_callback2, nullptr);