From 171409a2aad51ceb5d12e7b6799770fa12340e60 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 22 Mar 2020 21:07:21 +0100 Subject: [PATCH 1/7] src: make `Environment::interrupt_data_` atomic Otherwise this potentially leads to race conditions when used in a multi-threaded environment (i.e. when used for its very purpose). --- src/env.cc | 16 ++++++++++------ src/env.h | 2 +- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/src/env.cc b/src/env.cc index 7d76ee8186ad28..5c561f5d6b306f 100644 --- a/src/env.cc +++ b/src/env.cc @@ -394,7 +394,8 @@ Environment::Environment(IsolateData* isolate_data, } Environment::~Environment() { - if (interrupt_data_ != nullptr) *interrupt_data_ = nullptr; + if (Environment** interrupt_data = interrupt_data_.load()) + *interrupt_data = nullptr; // FreeEnvironment() should have set this. CHECK(is_stopping()); @@ -735,12 +736,15 @@ void Environment::RunAndClearNativeImmediates(bool only_refed) { } void Environment::RequestInterruptFromV8() { - if (interrupt_data_ != nullptr) return; // Already scheduled. - // The Isolate may outlive the Environment, so some logic to handle the // situation in which the Environment is destroyed before the handler runs // is required. - interrupt_data_ = new Environment*(this); + Environment** interrupt_data = new Environment*(this); + Environment** dummy = nullptr; + if (!interrupt_data_.compare_exchange_strong(dummy, interrupt_data)) { + delete interrupt_data; + return; // Already scheduled. + } isolate()->RequestInterrupt([](Isolate* isolate, void* data) { std::unique_ptr env_ptr { static_cast(data) }; @@ -751,9 +755,9 @@ void Environment::RequestInterruptFromV8() { // handled during cleanup. return; } - env->interrupt_data_ = nullptr; + env->interrupt_data_.store(nullptr); env->RunAndClearInterrupts(); - }, interrupt_data_); + }, interrupt_data); } void Environment::ScheduleTimer(int64_t duration_ms) { diff --git a/src/env.h b/src/env.h index 719aca19a13803..30aec9ae805cfa 100644 --- a/src/env.h +++ b/src/env.h @@ -1442,7 +1442,7 @@ class Environment : public MemoryRetainer { void RunAndClearNativeImmediates(bool only_refed = false); void RunAndClearInterrupts(); - Environment** interrupt_data_ = nullptr; + std::atomic interrupt_data_ {nullptr}; void RequestInterruptFromV8(); static void CheckImmediate(uv_check_t* handle); From 4de323c39a9ca2d96ce9f7c65d8ad126f7282f62 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 27 Mar 2020 21:38:39 +0100 Subject: [PATCH 2/7] src: fix cleanup hook removal for InspectorTimer 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. --- src/inspector_agent.cc | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index b7ccf641c299f5..a3d67f317d7156 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -364,13 +364,12 @@ class InspectorTimer { 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(&timer_), TimerClosedCb); - } + env_->RemoveCleanupHook(CleanupHook, this); + timer_.data = nullptr; + uv_timer_stop(&timer_); + env_->CloseHandle(reinterpret_cast(&timer_), TimerClosedCb); } private: From 23b2da658b4c915dc7668268d71ba363fec4a2a7 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 22 Mar 2020 19:03:24 +0100 Subject: [PATCH 3/7] src: use env->RequestInterrupt() for inspector io thread start This cleans up `Agent::RequestIoThreadStart()` significantly. --- src/inspector_agent.cc | 27 ++++----------------------- 1 file changed, 4 insertions(+), 23 deletions(-) diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index a3d67f317d7156..ab1d23c64a5545 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -47,7 +47,6 @@ using v8::Message; using v8::Object; using v8::String; using v8::Task; -using v8::TaskRunner; using v8::Value; using v8_inspector::StringBuffer; @@ -63,18 +62,6 @@ static std::atomic_bool start_io_thread_async_initialized { false }; // Protects the Agent* stored in start_io_thread_async.data. static Mutex start_io_thread_async_mutex; -class StartIoTask : public Task { - public: - explicit StartIoTask(Agent* agent) : agent(agent) {} - - void Run() override { - agent->StartIoThread(); - } - - private: - Agent* agent; -}; - std::unique_ptr ToProtocolString(Isolate* isolate, Local value) { TwoByteValue buffer(isolate, value); @@ -86,10 +73,6 @@ void StartIoThreadAsyncCallback(uv_async_t* handle) { static_cast(handle->data)->StartIoThread(); } -void StartIoInterrupt(Isolate* isolate, void* agent) { - static_cast(agent)->StartIoThread(); -} - #ifdef __POSIX__ static void StartIoThreadWakeup(int signo, siginfo_t* info, void* ucontext) { @@ -969,12 +952,10 @@ void Agent::RequestIoThreadStart() { // for IO events) CHECK(start_io_thread_async_initialized); uv_async_send(&start_io_thread_async); - Isolate* isolate = parent_env_->isolate(); - v8::Platform* platform = parent_env_->isolate_data()->platform(); - std::shared_ptr taskrunner = - platform->GetForegroundTaskRunner(isolate); - taskrunner->PostTask(std::make_unique(this)); - isolate->RequestInterrupt(StartIoInterrupt, this); + parent_env_->RequestInterrupt([this](Environment*) { + StartIoThread(); + }); + CHECK(start_io_thread_async_initialized); uv_async_send(&start_io_thread_async); } From 62889dfd2d897b81aa0660b697a84d1a2d5d91b2 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 22 Mar 2020 19:04:57 +0100 Subject: [PATCH 4/7] src: use env->RequestInterrupt() for inspector MainThreadInterface This simplifies the code significantly, and removes the dependency of the inspector code on the availability of a `MultiIsolatePlatform` (by removing the dependency on a platform altogether). It also fixes a memory leak that occurs when `RequestInterrupt()` is used, but the interrupt handler is never called before the Isolate is destroyed. One downside is that this leads to a slight change in timing, because inspector messages are not dispatched in a re-entrant way. This means having to harden one test to account for that possibility by making sure that the stack is always clear through a `setImmediate()`. This does not affect the assertion made by the test, which is that messages will not be delivered synchronously while other code is executing. https://github.com/nodejs/node/issues/32415 --- src/env.h | 5 ++- src/inspector/main_thread_interface.cc | 41 ++++--------------- src/inspector/main_thread_interface.h | 5 +-- src/inspector_agent.cc | 6 +-- src/inspector_agent.h | 2 + src/inspector_js_api.cc | 2 +- .../test-inspector-connect-main-thread.js | 6 +++ 7 files changed, 23 insertions(+), 44 deletions(-) diff --git a/src/env.h b/src/env.h index 30aec9ae805cfa..a3df37250d4674 100644 --- a/src/env.h +++ b/src/env.h @@ -1256,6 +1256,9 @@ class Environment : public MemoryRetainer { inline void set_main_utf16(std::unique_ptr); + void RunAndClearNativeImmediates(bool only_refed = false); + void RunAndClearInterrupts(); + private: template inline void CreateImmediate(Fn&& cb, bool ref); @@ -1440,8 +1443,6 @@ class Environment : public MemoryRetainer { // yet or already have been destroyed. bool task_queues_async_initialized_ = false; - void RunAndClearNativeImmediates(bool only_refed = false); - void RunAndClearInterrupts(); std::atomic interrupt_data_ {nullptr}; void RequestInterruptFromV8(); static void CheckImmediate(uv_check_t* handle); diff --git a/src/inspector/main_thread_interface.cc b/src/inspector/main_thread_interface.cc index 48a964d564fec2..a15cd52d239e40 100644 --- a/src/inspector/main_thread_interface.cc +++ b/src/inspector/main_thread_interface.cc @@ -1,5 +1,6 @@ #include "main_thread_interface.h" +#include "env-inl.h" #include "node_mutex.h" #include "v8-inspector.h" #include "util-inl.h" @@ -85,19 +86,6 @@ class CallRequest : public Request { Fn fn_; }; -class DispatchMessagesTask : public v8::Task { - public: - explicit DispatchMessagesTask(std::weak_ptr thread) - : thread_(thread) {} - - void Run() override { - if (auto thread = thread_.lock()) thread->DispatchMessages(); - } - - private: - std::weak_ptr thread_; -}; - template class AnotherThreadObjectReference { public: @@ -212,12 +200,7 @@ class ThreadSafeDelegate : public InspectorSessionDelegate { } // namespace -MainThreadInterface::MainThreadInterface(Agent* agent, uv_loop_t* loop, - v8::Isolate* isolate, - v8::Platform* platform) - : agent_(agent), isolate_(isolate), - platform_(platform) { -} +MainThreadInterface::MainThreadInterface(Agent* agent) : agent_(agent) {} MainThreadInterface::~MainThreadInterface() { if (handle_) @@ -225,23 +208,15 @@ MainThreadInterface::~MainThreadInterface() { } void MainThreadInterface::Post(std::unique_ptr request) { + CHECK_NOT_NULL(agent_); Mutex::ScopedLock scoped_lock(requests_lock_); bool needs_notify = requests_.empty(); requests_.push_back(std::move(request)); if (needs_notify) { - if (isolate_ != nullptr && platform_ != nullptr) { - std::shared_ptr taskrunner = - platform_->GetForegroundTaskRunner(isolate_); - std::weak_ptr* interface_ptr = - new std::weak_ptr(shared_from_this()); - taskrunner->PostTask( - std::make_unique(*interface_ptr)); - isolate_->RequestInterrupt([](v8::Isolate* isolate, void* opaque) { - std::unique_ptr> interface_ptr { - static_cast*>(opaque) }; - if (auto iface = interface_ptr->lock()) iface->DispatchMessages(); - }, static_cast(interface_ptr)); - } + std::weak_ptr weak_self {shared_from_this()}; + agent_->env()->RequestInterrupt([weak_self](Environment*) { + if (auto iface = weak_self.lock()) iface->DispatchMessages(); + }); } incoming_message_cond_.Broadcast(scoped_lock); } @@ -274,7 +249,7 @@ void MainThreadInterface::DispatchMessages() { std::swap(dispatching_message_queue_.front(), task); dispatching_message_queue_.pop_front(); - v8::SealHandleScope seal_handle_scope(isolate_); + v8::SealHandleScope seal_handle_scope(agent_->env()->isolate()); task->Call(this); } } while (had_messages); diff --git a/src/inspector/main_thread_interface.h b/src/inspector/main_thread_interface.h index 78337a25e43808..3ec5b3db3e1600 100644 --- a/src/inspector/main_thread_interface.h +++ b/src/inspector/main_thread_interface.h @@ -72,8 +72,7 @@ class MainThreadHandle : public std::enable_shared_from_this { class MainThreadInterface : public std::enable_shared_from_this { public: - MainThreadInterface(Agent* agent, uv_loop_t*, v8::Isolate* isolate, - v8::Platform* platform); + explicit MainThreadInterface(Agent* agent); ~MainThreadInterface(); void DispatchMessages(); @@ -98,8 +97,6 @@ class MainThreadInterface : ConditionVariable incoming_message_cond_; // Used from any thread Agent* const agent_; - v8::Isolate* const isolate_; - v8::Platform* const platform_; std::shared_ptr handle_; std::unordered_map> managed_objects_; }; diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index ab1d23c64a5545..6980bea51d17ed 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -654,8 +654,7 @@ class NodeInspectorClient : public V8InspectorClient { std::shared_ptr getThreadHandle() { if (!interface_) { interface_ = std::make_shared( - env_->inspector_agent(), env_->event_loop(), env_->isolate(), - env_->isolate_data()->platform()); + env_->inspector_agent()); } return interface_->GetHandle(); } @@ -691,10 +690,9 @@ class NodeInspectorClient : public V8InspectorClient { running_nested_loop_ = true; - MultiIsolatePlatform* platform = env_->isolate_data()->platform(); while (shouldRunMessageLoop()) { if (interface_) interface_->WaitForFrontendEvent(); - while (platform->FlushForegroundTasks(env_->isolate())) {} + env_->RunAndClearInterrupts(); } running_nested_loop_ = false; } diff --git a/src/inspector_agent.h b/src/inspector_agent.h index 089077370db049..efd090c49b4311 100644 --- a/src/inspector_agent.h +++ b/src/inspector_agent.h @@ -116,6 +116,8 @@ class Agent { // Interface for interacting with inspectors in worker threads std::shared_ptr GetWorkerManager(); + inline Environment* env() const { return parent_env_; } + private: void ToggleAsyncHook(v8::Isolate* isolate, const v8::Global& fn); diff --git a/src/inspector_js_api.cc b/src/inspector_js_api.cc index ed3b36ad5ca80e..5c9ad5e946424b 100644 --- a/src/inspector_js_api.cc +++ b/src/inspector_js_api.cc @@ -84,7 +84,7 @@ class JSBindingsConnection : public AsyncWrap { private: Environment* env_; - JSBindingsConnection* connection_; + BaseObjectPtr connection_; }; JSBindingsConnection(Environment* env, diff --git a/test/parallel/test-inspector-connect-main-thread.js b/test/parallel/test-inspector-connect-main-thread.js index bb21a54e90f1d1..be34981cd0c5be 100644 --- a/test/parallel/test-inspector-connect-main-thread.js +++ b/test/parallel/test-inspector-connect-main-thread.js @@ -76,6 +76,12 @@ function doConsoleLog(arrayBuffer) { // and do not interrupt the main code. Interrupting the code flow // can lead to unexpected behaviors. async function ensureListenerDoesNotInterrupt(session) { + // Make sure that the following code is not affected by the fact that it may + // run inside an inspector message callback, during which other inspector + // message callbacks (such as the one triggered by doConsoleLog()) would + // not be processed. + await new Promise(setImmediate); + const currentTime = Date.now(); let consoleLogHappened = false; session.once('Runtime.consoleAPICalled', From d9689dbda366da38f96e7f345364e06b998abbad Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 27 Mar 2020 23:02:24 +0100 Subject: [PATCH 5/7] fixup! src: fix cleanup hook removal for InspectorTimer --- src/inspector_agent.cc | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index 6980bea51d17ed..74b7fc13cde4c0 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -340,8 +340,6 @@ 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; @@ -349,22 +347,19 @@ class InspectorTimer { void Stop() { if (timer_.data == nullptr) return; - env_->RemoveCleanupHook(CleanupHook, this); timer_.data = nullptr; uv_timer_stop(&timer_); env_->CloseHandle(reinterpret_cast(&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(data)->Stop(); - } - static void TimerClosedCb(uv_handle_t* uvtimer) { std::unique_ptr timer( node::ContainerOf(&InspectorTimer::timer_, @@ -387,16 +382,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(data)->Stop(); + } + InspectorTimer* timer_; }; @@ -717,8 +725,9 @@ class NodeInspectorClient : public V8InspectorClient { bool is_main_; bool running_nested_loop_ = false; std::unique_ptr client_; - std::unordered_map> channels_; + // Note: ~ChannelImpl may access timers_ so timers_ has to come first. std::unordered_map timers_; + std::unordered_map> channels_; int next_session_id_ = 1; bool waiting_for_resume_ = false; bool waiting_for_frontend_ = false; From aeb354d7e4643f5f8805066dd8d8d2111e2e92aa Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 28 Mar 2020 01:14:01 +0100 Subject: [PATCH 6/7] fixup! src: make `Environment::interrupt_data_` atomic --- src/env.cc | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/env.cc b/src/env.cc index 5c561f5d6b306f..5e88629d136c03 100644 --- a/src/env.cc +++ b/src/env.cc @@ -739,6 +739,14 @@ void Environment::RequestInterruptFromV8() { // The Isolate may outlive the Environment, so some logic to handle the // situation in which the Environment is destroyed before the handler runs // is required. + + // We allocate a new pointer to a pointer to this Environment instance, and + // try to set it as interrupt_data_. If interrupt_data_ was already set, then + // callbacks are already scheduled to run and we can delete our own pointer + // and just return. If it was nullptr previously, the Environment** is stored; + // ~Environment sets the Environment* contained in it to nullptr, so that + // the callback can check whether ~Environment has already run and it is thus + // not safe to access the Environment instance itself. Environment** interrupt_data = new Environment*(this); Environment** dummy = nullptr; if (!interrupt_data_.compare_exchange_strong(dummy, interrupt_data)) { From 7e75cc14e11ee252becaed58bc6009bc7a2a4eb3 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 28 Mar 2020 01:14:06 +0100 Subject: [PATCH 7/7] src: flush V8 interrupts from Environment dtor This avoids an edge-case memory leak. Refs: https://github.com/nodejs/node/pull/32523#discussion_r399554491 --- src/env.cc | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/src/env.cc b/src/env.cc index 5e88629d136c03..eeab8711e3a512 100644 --- a/src/env.cc +++ b/src/env.cc @@ -42,11 +42,13 @@ using v8::NewStringType; using v8::Number; using v8::Object; using v8::Private; +using v8::Script; using v8::SnapshotCreator; using v8::StackTrace; using v8::String; using v8::Symbol; using v8::TracingController; +using v8::TryCatch; using v8::Undefined; using v8::Value; using worker::Worker; @@ -394,9 +396,31 @@ Environment::Environment(IsolateData* isolate_data, } Environment::~Environment() { - if (Environment** interrupt_data = interrupt_data_.load()) + if (Environment** interrupt_data = interrupt_data_.load()) { + // There are pending RequestInterrupt() callbacks. Tell them not to run, + // then force V8 to run interrupts by compiling and running an empty script + // so as not to leak memory. *interrupt_data = nullptr; + Isolate::AllowJavascriptExecutionScope allow_js_here(isolate()); + HandleScope handle_scope(isolate()); + TryCatch try_catch(isolate()); + Context::Scope context_scope(context()); + +#ifdef DEBUG + bool consistency_check = false; + isolate()->RequestInterrupt([](Isolate*, void* data) { + *static_cast(data) = true; + }, &consistency_check); +#endif + + Local