fix: delete UvTaskRunner's timers only after they're closed (#43597)

* fix: free UvTaskRunner timers only after they are closed

Co-authored-by: Charles Kerr <[email protected]>

* refactor: UvTaskRunner now holds UvHandles

Co-authored-by: Charles Kerr <[email protected]>

---------

Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com>
Co-authored-by: Charles Kerr <[email protected]>

shell/app/uv_task_runner.cc

@@ -2,31 +2,33 @@
 // Use of this source code is governed by the MIT license that can be
 // found in the LICENSE file.
 
+#include "shell/app/uv_task_runner.h"
+
 #include <utility>
 
 #include "base/location.h"
 #include "base/time/time.h"
-#include "shell/app/uv_task_runner.h"
 
 namespace electron {
 
-UvTaskRunner::UvTaskRunner(uv_loop_t* loop) : loop_(loop) {}
+UvTaskRunner::UvTaskRunner(uv_loop_t* loop) : loop_{loop} {}
 
-UvTaskRunner::~UvTaskRunner() {
-  for (auto& iter : tasks_) {
-    uv_unref(reinterpret_cast<uv_handle_t*>(iter.first));
-    delete iter.first;
-  }
-}
+UvTaskRunner::~UvTaskRunner() = default;
 
 bool UvTaskRunner::PostDelayedTask(const base::Location& from_here,
                                    base::OnceClosure task,
                                    base::TimeDelta delay) {
-  auto* timer = new uv_timer_t;
+  auto on_timeout = [](uv_timer_t* timer) {
+    auto& tasks = static_cast<UvTaskRunner*>(timer->data)->tasks_;
+    if (auto iter = tasks.find(timer); iter != tasks.end())
+      std::move(tasks.extract(iter).mapped()).Run();
+  };
+
+  auto timer = UvHandle<uv_timer_t>{};
   timer->data = this;
-  uv_timer_init(loop_, timer);
-  uv_timer_start(timer, UvTaskRunner::OnTimeout, delay.InMilliseconds(), 0);
-  tasks_[timer] = std::move(task);
+  uv_timer_init(loop_, timer.get());
+  uv_timer_start(timer.get(), on_timeout, delay.InMilliseconds(), 0);
+  tasks_.insert_or_assign(std::move(timer), std::move(task));
   return true;
 }
 
@@ -40,22 +42,4 @@ bool UvTaskRunner::PostNonNestableDelayedTask(const base::Location& from_here,
   return PostDelayedTask(from_here, std::move(task), delay);
 }
 
-// static
-void UvTaskRunner::OnTimeout(uv_timer_t* timer) {
-  auto& tasks = static_cast<UvTaskRunner*>(timer->data)->tasks_;
-  const auto iter = tasks.find(timer);
-  if (iter == std::end(tasks))
-    return;
-
-  std::move(iter->second).Run();
-  tasks.erase(iter);
-  uv_timer_stop(timer);
-  uv_close(reinterpret_cast<uv_handle_t*>(timer), UvTaskRunner::OnClose);
-}
-
-// static
-void UvTaskRunner::OnClose(uv_handle_t* handle) {
-  delete reinterpret_cast<uv_timer_t*>(handle);
-}
-
 }  // namespace electron

shell/app/uv_task_runner.h

@@ -9,7 +9,7 @@
 
 #include "base/memory/raw_ptr.h"
 #include "base/task/single_thread_task_runner.h"
-#include "uv.h"  // NOLINT(build/include_directory)
+#include "shell/common/node_bindings.h"
 
 namespace base {
 class Location;
@@ -38,12 +38,10 @@ class UvTaskRunner : public base::SingleThreadTaskRunner {
 
  private:
   ~UvTaskRunner() override;
-  static void OnTimeout(uv_timer_t* timer);
-  static void OnClose(uv_handle_t* handle);
 
   raw_ptr<uv_loop_t> loop_;
 
-  std::map<uv_timer_t*, base::OnceClosure> tasks_;
+  std::map<UvHandle<uv_timer_t>, base::OnceClosure, UvHandleCompare> tasks_;
 };
 
 }  // namespace electron

shell/common/node_bindings.h

@@ -15,6 +15,7 @@
 #include "base/memory/raw_ptr.h"
 #include "base/memory/raw_ptr_exclusion.h"
 #include "base/memory/weak_ptr.h"
+#include "base/types/to_address.h"
 #include "gin/public/context_holder.h"
 #include "gin/public/gin_embedders.h"
 #include "uv.h"  // NOLINT(build/include_directory)
@@ -58,11 +59,25 @@ template <typename T,
               std::is_same<T, uv_udp_t>::value>::type* = nullptr>
 class UvHandle {
  public:
-  UvHandle() : t_(new T) {}
+  UvHandle() : t_{new T} {}
   ~UvHandle() { reset(); }
+
+  UvHandle(UvHandle&&) = default;
+  UvHandle& operator=(UvHandle&&) = default;
+
+  UvHandle(const UvHandle&) = delete;
+  UvHandle& operator=(const UvHandle&) = delete;
+
   T* get() { return t_; }
+  T* operator->() { return t_; }
+  const T* get() const { return t_; }
+  const T* operator->() const { return t_; }
+
   uv_handle_t* handle() { return reinterpret_cast<uv_handle_t*>(t_); }
 
+  // compare by handle pointer address
+  auto operator<=>(const UvHandle& that) const = default;
+
   void reset() {
     auto* h = handle();
     if (h != nullptr) {
@@ -80,6 +95,16 @@ class UvHandle {
   RAW_PTR_EXCLUSION T* t_ = {};
 };
 
+// Helper for comparing UvHandles and raw uv pointers, e.g. as map keys
+struct UvHandleCompare {
+  using is_transparent = void;
+
+  template <typename U, typename V>
+  bool operator()(U const& u, V const& v) const {
+    return base::to_address(u) < base::to_address(v);
+  }
+};
+
 class NodeBindings {
  public:
   enum class BrowserEnvironment { kBrowser, kRenderer, kUtility, kWorker };