refactor: add a wrapper for wrangling uv handles. (#25332)

* refactor: add a wrapper for wrangling uv handles.

Part 1 of a fix for #25248, #22069.

Place the uv_asyncs owned by NodeBindings, ElectronBindings inside a new
UvHandle wrapper class which manages uv_handles' need for their closed()
callback to be invoked before the handles' memory can be freed.

* chore: make lint happy

* refactor: use DCHECK_EQ() instead of DCHECK()

* refactor: fix oops

shell/common/api/electron_bindings.cc

@@ -34,14 +34,12 @@
 namespace electron {
 
 ElectronBindings::ElectronBindings(uv_loop_t* loop) {
-  uv_async_init(loop, &call_next_tick_async_, OnCallNextTick);
+  uv_async_init(loop, call_next_tick_async_.get(), OnCallNextTick);
-  call_next_tick_async_.data = this;
+  call_next_tick_async_.get()->data = this;
   metrics_ = base::ProcessMetrics::CreateCurrentProcessMetrics();
 }
 
-ElectronBindings::~ElectronBindings() {
+ElectronBindings::~ElectronBindings() {}
-  uv_close(reinterpret_cast<uv_handle_t*>(&call_next_tick_async_), nullptr);
-}
 
 // static
 void ElectronBindings::BindProcess(v8::Isolate* isolate,
@@ -107,7 +105,7 @@ void ElectronBindings::ActivateUVLoop(v8::Isolate* isolate) {
     return;
 
   pending_next_ticks_.push_back(env);
-  uv_async_send(&call_next_tick_async_);
+  uv_async_send(call_next_tick_async_.get());
 }
 
 // static

shell/common/api/electron_bindings.h

@@ -14,6 +14,7 @@
 #include "base/process/process_metrics.h"
 #include "base/strings/string16.h"
 #include "shell/common/gin_helper/promise.h"
+#include "shell/common/node_bindings.h"
 #include "uv.h"  // NOLINT(build/include_directory)
 
 namespace gin_helper {
@@ -74,7 +75,7 @@ class ElectronBindings {
       bool success,
       std::unique_ptr<memory_instrumentation::GlobalMemoryDump> dump);
 
-  uv_async_t call_next_tick_async_;
+  UvHandle<uv_async_t> call_next_tick_async_;
   std::list<node::Environment*> pending_next_ticks_;
   std::unique_ptr<base::ProcessMetrics> metrics_;
 

shell/common/node_bindings.cc

@@ -102,24 +102,26 @@ namespace {
 
 void stop_and_close_uv_loop(uv_loop_t* loop) {
   uv_stop(loop);
-  int error = uv_loop_close(loop);
 
-  while (error) {
+  auto const ensure_closing = [](uv_handle_t* handle, void*) {
-    uv_run(loop, UV_RUN_DEFAULT);
+    // We should be using the UvHandle wrapper everywhere, in which case
-    uv_stop(loop);
+    // all handles should already be in a closing state...
-    uv_walk(
+    DCHECK(uv_is_closing(handle));
-        loop,
+    // ...but if a raw handle got through, through, do the right thing anyway
-        [](uv_handle_t* handle, void*) {
     if (!uv_is_closing(handle)) {
       uv_close(handle, nullptr);
     }
-        },
+  };
-        nullptr);
-    uv_run(loop, UV_RUN_DEFAULT);
-    error = uv_loop_close(loop);
-  }
 
-  DCHECK_EQ(error, 0);
+  uv_walk(loop, ensure_closing, nullptr);
+
+  // All remaining handles are in a closing state now.
+  // Pump the event loop so that they can finish closing.
+  for (;;)
+    if (uv_run(loop, UV_RUN_DEFAULT) == 0)
+      break;
+
+  DCHECK_EQ(0, uv_loop_alive(loop));
 }
 
 bool g_is_initialized = false;
@@ -292,7 +294,7 @@ NodeBindings::~NodeBindings() {
 
   // Clear uv.
   uv_sem_destroy(&embed_sem_);
-  uv_close(reinterpret_cast<uv_handle_t*>(&dummy_uv_handle_), nullptr);
+  dummy_uv_handle_.reset();
 
   // Clean up worker loop
   if (in_worker_loop())
@@ -476,7 +478,7 @@ void NodeBindings::LoadEnvironment(node::Environment* env) {
 void NodeBindings::PrepareMessageLoop() {
   // Add dummy handle for libuv, otherwise libuv would quit when there is
   // nothing to do.
-  uv_async_init(uv_loop_, &dummy_uv_handle_, nullptr);
+  uv_async_init(uv_loop_, dummy_uv_handle_.get(), nullptr);
 
   // Start worker that will interrupt main loop when having uv events.
   uv_sem_init(&embed_sem_, 0);
@@ -533,8 +535,7 @@ void NodeBindings::WakeupMainThread() {
 }
 
 void NodeBindings::WakeupEmbedThread() {
-  if (!in_worker_loop())
+  uv_async_send(dummy_uv_handle_.get());
-    uv_async_send(&dummy_uv_handle_);
 }
 
 // static

shell/common/node_bindings.h

@@ -5,6 +5,8 @@
 #ifndef SHELL_COMMON_NODE_BINDINGS_H_
 #define SHELL_COMMON_NODE_BINDINGS_H_
 
+#include <type_traits>
+
 #include "base/files/file_path.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
@@ -24,6 +26,54 @@ class IsolateData;
 
 namespace electron {
 
+// A helper class to manage uv_handle_t types, e.g. uv_async_t.
+//
+// As per the uv docs: "uv_close() MUST be called on each handle before
+// memory is released. Moreover, the memory can only be released in
+// close_cb or after it has returned." This class encapsulates the work
+// needed to follow those requirements.
+template <typename T,
+          typename std::enable_if<
+              // these are the C-style 'subclasses' of uv_handle_t
+              std::is_same<T, uv_async_t>::value ||
+              std::is_same<T, uv_check_t>::value ||
+              std::is_same<T, uv_fs_event_t>::value ||
+              std::is_same<T, uv_fs_poll_t>::value ||
+              std::is_same<T, uv_idle_t>::value ||
+              std::is_same<T, uv_pipe_t>::value ||
+              std::is_same<T, uv_poll_t>::value ||
+              std::is_same<T, uv_prepare_t>::value ||
+              std::is_same<T, uv_process_t>::value ||
+              std::is_same<T, uv_signal_t>::value ||
+              std::is_same<T, uv_stream_t>::value ||
+              std::is_same<T, uv_tcp_t>::value ||
+              std::is_same<T, uv_timer_t>::value ||
+              std::is_same<T, uv_tty_t>::value ||
+              std::is_same<T, uv_udp_t>::value>::type* = nullptr>
+class UvHandle {
+ public:
+  UvHandle() : t_(new T) {}
+  ~UvHandle() { reset(); }
+  T* get() { return t_; }
+  uv_handle_t* handle() { return reinterpret_cast<uv_handle_t*>(t_); }
+
+  void reset() {
+    auto* h = handle();
+    if (h != nullptr) {
+      DCHECK_EQ(0, uv_is_closing(h));
+      uv_close(h, OnClosed);
+      t_ = nullptr;
+    }
+  }
+
+ private:
+  static void OnClosed(uv_handle_t* handle) {
+    delete reinterpret_cast<T*>(handle);
+  }
+
+  T* t_ = {};
+};
+
 class NodeBindings {
  public:
   enum class BrowserEnvironment { BROWSER, RENDERER, WORKER };
@@ -95,7 +145,7 @@ class NodeBindings {
   uv_loop_t worker_loop_;
 
   // Dummy handle to make uv's loop not quit.
-  uv_async_t dummy_uv_handle_;
+  UvHandle<uv_async_t> dummy_uv_handle_;
 
   // Thread for polling events.
   uv_thread_t embed_thread_;