inspector: make sure timer handles are cleaned up

It is not obvious that timer handles are cleaned up properly
when the current `Environment` exits, nor that the `Environment`
knows to keep track of the closing handles.

This change may not be necessary, because timer handles
close without non-trivial delay (i.e. at the end of the current
event loop term), and JS-based inspector sessions (which are
the only ones we can easily test) are destroyed when cleaning up,
closing the timers as a result. I don’t know what happens
for other kinds of inspector sessions, though.

PR-URL: https://github.com/nodejs/node/pull/26088
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

1 files changed, 21 insertions, 8 deletions

diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc
index 8d7aad70e6..ef5646eb56 100644
--- a/src/inspector_agent.cc
+++ b/src/inspector_agent.cc
@@ -301,22 +301,30 @@ class ChannelImpl final : public v8_inspector::V8Inspector::Channel,
 
 class InspectorTimer {
  public:
-  InspectorTimer(uv_loop_t* loop,
+  InspectorTimer(Environment* env,
                  double interval_s,
                  V8InspectorClient::TimerCallback callback,
-                 void* data) : timer_(),
+                 void* data) : env_(env),
                                callback_(callback),
                                data_(data) {
-    uv_timer_init(loop, &timer_);
+    uv_timer_init(env->event_loop(), &timer_);
     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() {
-    uv_timer_stop(&timer_);
-    uv_close(reinterpret_cast<uv_handle_t*>(&timer_), TimerClosedCb);
+    env_->RemoveCleanupHook(CleanupHook, this);
+
+    if (timer_.data == this) {
+      timer_.data = nullptr;
+      uv_timer_stop(&timer_);
+      env_->CloseHandle(reinterpret_cast<uv_handle_t*>(&timer_), TimerClosedCb);
+    }
   }
 
  private:
@@ -325,6 +333,10 @@ class InspectorTimer {
     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_,
@@ -334,6 +346,7 @@ class InspectorTimer {
 
   ~InspectorTimer() {}
 
+  Environment* env_;
   uv_timer_t timer_;
   V8InspectorClient::TimerCallback callback_;
   void* data_;
@@ -343,9 +356,9 @@ class InspectorTimer {
 
 class InspectorTimerHandle {
  public:
-  InspectorTimerHandle(uv_loop_t* loop, double interval_s,
+  InspectorTimerHandle(Environment* env, double interval_s,
                        V8InspectorClient::TimerCallback callback, void* data) {
-    timer_ = new InspectorTimer(loop, interval_s, callback, data);
+    timer_ = new InspectorTimer(env, interval_s, callback, data);
   }
 
   InspectorTimerHandle(const InspectorTimerHandle&) = delete;
@@ -540,7 +553,7 @@ class NodeInspectorClient : public V8InspectorClient {
                            TimerCallback callback,
                            void* data) override {
     timers_.emplace(std::piecewise_construct, std::make_tuple(data),
-                    std::make_tuple(env_->event_loop(), interval_s, callback,
+                    std::make_tuple(env_, interval_s, callback,
                                     data));
   }