src: fix cleanup hook removal for InspectorTimer

addaleax · addaleax · commit 4704e586dcc2 · 2020-09-23T20:07:56.000+02:00
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 <mat@mmarchini.me> Reviewed-By: James M Snell <jasnell@gmail.com>
diff --git a/src/inspector_agent.cc b/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;
