From b3d01501edc0b2617ecdfa2382a4e43b76ff7d76 Mon Sep 17 00:00:00 2001 From: Charles Kerr Date: Fri, 4 Sep 2020 19:33:22 -0500 Subject: [PATCH 1/4] 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. --- shell/common/api/electron_bindings.cc | 10 +++--- shell/common/api/electron_bindings.h | 3 +- shell/common/node_bindings.cc | 43 +++++++++++----------- shell/common/node_bindings.h | 52 ++++++++++++++++++++++++++- 4 files changed, 79 insertions(+), 29 deletions(-) diff --git a/shell/common/api/electron_bindings.cc b/shell/common/api/electron_bindings.cc index d078f37700512..afecb9f5d3699 100644 --- a/shell/common/api/electron_bindings.cc +++ b/shell/common/api/electron_bindings.cc @@ -56,14 +56,12 @@ void V8FatalErrorCallback(const char* location, const char* message) { } // namespace ElectronBindings::ElectronBindings(uv_loop_t* loop) { - uv_async_init(loop, &call_next_tick_async_, OnCallNextTick); - call_next_tick_async_.data = this; + uv_async_init(loop, call_next_tick_async_.get(), OnCallNextTick); + call_next_tick_async_.get()->data = this; metrics_ = base::ProcessMetrics::CreateCurrentProcessMetrics(); } -ElectronBindings::~ElectronBindings() { - uv_close(reinterpret_cast(&call_next_tick_async_), nullptr); -} +ElectronBindings::~ElectronBindings() {} // static void ElectronBindings::BindProcess(v8::Isolate* isolate, @@ -131,7 +129,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 diff --git a/shell/common/api/electron_bindings.h b/shell/common/api/electron_bindings.h index d489d3feb6390..79a68fcd5a2b2 100644 --- a/shell/common/api/electron_bindings.h +++ b/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 dump); - uv_async_t call_next_tick_async_; + UvHandle call_next_tick_async_; std::list pending_next_ticks_; std::unique_ptr metrics_; diff --git a/shell/common/node_bindings.cc b/shell/common/node_bindings.cc index 84b4a914632f2..25a3ae5452a00 100644 --- a/shell/common/node_bindings.cc +++ b/shell/common/node_bindings.cc @@ -104,24 +104,26 @@ namespace { void stop_and_close_uv_loop(uv_loop_t* loop) { uv_stop(loop); - int error = uv_loop_close(loop); - - while (error) { - uv_run(loop, UV_RUN_DEFAULT); - uv_stop(loop); - uv_walk( - loop, - [](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); + auto const ensure_closing = [](uv_handle_t* handle, void*) { + // We should be using the UvHandle wrapper everywhere, in which case + // all handles should already be in a closing state... + DCHECK(uv_is_closing(handle)); + // ...but if a raw handle got through, through, do the right thing anyway + if (!uv_is_closing(handle)) { + uv_close(handle, nullptr); + } + }; + + 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(uv_loop_alive(loop) == 0); } bool g_is_initialized = false; @@ -236,7 +238,7 @@ NodeBindings::~NodeBindings() { // Clear uv. uv_sem_destroy(&embed_sem_); - uv_close(reinterpret_cast(&dummy_uv_handle_), nullptr); + dummy_uv_handle_.reset(); // Clean up worker loop if (in_worker_loop()) @@ -398,7 +400,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); @@ -455,8 +457,7 @@ void NodeBindings::WakeupMainThread() { } void NodeBindings::WakeupEmbedThread() { - if (!in_worker_loop()) - uv_async_send(&dummy_uv_handle_); + uv_async_send(dummy_uv_handle_.get()); } // static diff --git a/shell/common/node_bindings.h b/shell/common/node_bindings.h index 944025cc09492..d401db348459b 100644 --- a/shell/common/node_bindings.h +++ b/shell/common/node_bindings.h @@ -5,6 +5,8 @@ #ifndef SHELL_COMMON_NODE_BINDINGS_H_ #define SHELL_COMMON_NODE_BINDINGS_H_ +#include + #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 ::value || + std::is_same::value || + std::is_same::value || + std::is_same::value || + std::is_same::value || + std::is_same::value || + std::is_same::value || + std::is_same::value || + std::is_same::value || + std::is_same::value || + std::is_same::value || + std::is_same::value || + std::is_same::value || + std::is_same::value || + std::is_same::value>::type* = nullptr> +class UvHandle { + public: + UvHandle() : t_(new T) {} + ~UvHandle() { reset(); } + T* get() { return t_; } + uv_handle_t* handle() { return reinterpret_cast(t_); } + + void reset() { + auto* h = handle(); + if (h != nullptr) { + DCHECK(!uv_is_closing(handle())); + uv_close(handle(), OnClosed); + t_ = nullptr; + } + } + + private: + static void OnClosed(uv_handle_t* handle) { + delete reinterpret_cast(handle); + } + + T* t_ = {}; +}; + class NodeBindings { public: enum class BrowserEnvironment { @@ -99,7 +149,7 @@ class NodeBindings { uv_loop_t worker_loop_; // Dummy handle to make uv's loop not quit. - uv_async_t dummy_uv_handle_; + UvHandle dummy_uv_handle_; // Thread for polling events. uv_thread_t embed_thread_; From 5bff2332af857589eee61fa0a577d79dc3e7fd8d Mon Sep 17 00:00:00 2001 From: Charles Kerr Date: Fri, 4 Sep 2020 20:09:40 -0500 Subject: [PATCH 2/4] chore: make lint happy --- shell/common/node_bindings.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shell/common/node_bindings.cc b/shell/common/node_bindings.cc index 25a3ae5452a00..832cc2b7f6060 100644 --- a/shell/common/node_bindings.cc +++ b/shell/common/node_bindings.cc @@ -123,7 +123,7 @@ void stop_and_close_uv_loop(uv_loop_t* loop) { if (uv_run(loop, UV_RUN_DEFAULT) == 0) break; - DCHECK(uv_loop_alive(loop) == 0); + DCHECK_EQ(0, uv_loop_alive(loop)); } bool g_is_initialized = false; From 0ecf83d7f9a6606eeb62f5d0271d9fe286867b0a Mon Sep 17 00:00:00 2001 From: Charles Kerr Date: Fri, 4 Sep 2020 20:19:10 -0500 Subject: [PATCH 3/4] refactor: use DCHECK_EQ() instead of DCHECK() --- shell/common/node_bindings.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shell/common/node_bindings.h b/shell/common/node_bindings.h index d401db348459b..7eb64e763d227 100644 --- a/shell/common/node_bindings.h +++ b/shell/common/node_bindings.h @@ -60,7 +60,7 @@ class UvHandle { void reset() { auto* h = handle(); if (h != nullptr) { - DCHECK(!uv_is_closing(handle())); + DCHECK_EQ(0, uv_is_closing(handle())); uv_close(handle(), OnClosed); t_ = nullptr; } From fa07f153f28ba0516ab907acf37e75cdc236ef31 Mon Sep 17 00:00:00 2001 From: Charles Kerr Date: Fri, 4 Sep 2020 20:20:21 -0500 Subject: [PATCH 4/4] refactor: fix oops --- shell/common/node_bindings.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/shell/common/node_bindings.h b/shell/common/node_bindings.h index 7eb64e763d227..c59c48c3f10fb 100644 --- a/shell/common/node_bindings.h +++ b/shell/common/node_bindings.h @@ -60,8 +60,8 @@ class UvHandle { void reset() { auto* h = handle(); if (h != nullptr) { - DCHECK_EQ(0, uv_is_closing(handle())); - uv_close(handle(), OnClosed); + DCHECK_EQ(0, uv_is_closing(h)); + uv_close(h, OnClosed); t_ = nullptr; } }