src: fix cleanup hook removal for InspectorTimer

Fix this to account for the fact that `Stop()` may already have been
called from a cleanup hook when the `inspector::Agent` is deleted
along with the `Environment`, at which point cleanup hooks are no
longer available.

Backport-PR-URL: #35241
PR-URL: #32523
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: James M Snell <[email protected]>

Author: addaleax

src/inspector_agent.cc

@@ -355,32 +355,26 @@ class InspectorTimer {
     int64_t interval_ms = 1000 * interval_s;
     uv_timer_start(&timer_, OnTimer, interval_ms, interval_ms);
     timer_.data = this;
-
-    env->AddCleanupHook(CleanupHook, this);
   }
 
   InspectorTimer(const InspectorTimer&) = delete;
 
   void Stop() {
-    env_->RemoveCleanupHook(CleanupHook, this);
+    if (timer_.data == nullptr) return;
 
-    if (timer_.data == this) {
-      timer_.data = nullptr;
-      uv_timer_stop(&timer_);
-      env_->CloseHandle(reinterpret_cast<uv_handle_t*>(&timer_), TimerClosedCb);
-    }
+    timer_.data = nullptr;
+    uv_timer_stop(&timer_);
+    env_->CloseHandle(reinterpret_cast<uv_handle_t*>(&timer_), TimerClosedCb);
   }
 
+  inline Environment* env() const { return env_; }
+
  private:
   static void OnTimer(uv_timer_t* uvtimer) {
     InspectorTimer* timer = node::ContainerOf(&InspectorTimer::timer_, uvtimer);
     timer->callback_(timer->data_);
   }
 
-  static void CleanupHook(void* data) {
-    static_cast<InspectorTimer*>(data)->Stop();
-  }
-
   static void TimerClosedCb(uv_handle_t* uvtimer) {
     std::unique_ptr<InspectorTimer> timer(
         node::ContainerOf(&InspectorTimer::timer_,
@@ -403,16 +397,29 @@ class InspectorTimerHandle {
   InspectorTimerHandle(Environment* env, double interval_s,
                        V8InspectorClient::TimerCallback callback, void* data) {
     timer_ = new InspectorTimer(env, interval_s, callback, data);
+
+    env->AddCleanupHook(CleanupHook, this);
   }
 
   InspectorTimerHandle(const InspectorTimerHandle&) = delete;
 
   ~InspectorTimerHandle() {
-    CHECK_NOT_NULL(timer_);
-    timer_->Stop();
-    timer_ = nullptr;
+    Stop();
   }
+
  private:
+  void Stop() {
+    if (timer_ != nullptr) {
+      timer_->env()->RemoveCleanupHook(CleanupHook, this);
+      timer_->Stop();
+    }
+    timer_ = nullptr;
+  }
+
+  static void CleanupHook(void* data) {
+    static_cast<InspectorTimerHandle*>(data)->Stop();
+  }
+
   InspectorTimer* timer_;
 };
 
@@ -735,8 +742,9 @@ class NodeInspectorClient : public V8InspectorClient {
   bool is_main_;
   bool running_nested_loop_ = false;
   std::unique_ptr<V8Inspector> client_;
-  std::unordered_map<int, std::unique_ptr<ChannelImpl>> channels_;
+  // Note: ~ChannelImpl may access timers_ so timers_ has to come first.
   std::unordered_map<void*, InspectorTimerHandle> timers_;
+  std::unordered_map<int, std::unique_ptr<ChannelImpl>> channels_;
   int next_session_id_ = 1;
   bool waiting_for_resume_ = false;
   bool waiting_for_frontend_ = false;