From 6967407b19706d55895464c568c15b0734216bc0 Mon Sep 17 00:00:00 2001 From: Eugene Ostroukhov Date: Mon, 3 Dec 2018 13:25:30 -0800 Subject: [PATCH] inspector, trace_events: make sure messages are sent on a main thread Fixes: https://github.com/nodejs/node/issues/23185 PR-URL: https://github.com/nodejs/node/pull/24814 Reviewed-By: James M Snell --- src/inspector/main_thread_interface.cc | 12 ++- src/inspector/main_thread_interface.h | 1 + src/inspector/tracing_agent.cc | 101 +++++++++++++++--- src/inspector/tracing_agent.h | 8 +- src/inspector_agent.cc | 20 ++-- ...-events-dynamic-enable-workers-disabled.js | 1 + 6 files changed, 117 insertions(+), 26 deletions(-) diff --git a/src/inspector/main_thread_interface.cc b/src/inspector/main_thread_interface.cc index 2ed6362df53fda..15ffb49d74d1d4 100644 --- a/src/inspector/main_thread_interface.cc +++ b/src/inspector/main_thread_interface.cc @@ -316,15 +316,21 @@ void MainThreadInterface::RemoveObject(int id) { } Deletable* MainThreadInterface::GetObject(int id) { - auto iterator = managed_objects_.find(id); + Deletable* pointer = GetObjectIfExists(id); // This would mean the object is requested after it was disposed, which is // a coding error. - CHECK_NE(managed_objects_.end(), iterator); - Deletable* pointer = iterator->second.get(); CHECK_NE(nullptr, pointer); return pointer; } +Deletable* MainThreadInterface::GetObjectIfExists(int id) { + auto iterator = managed_objects_.find(id); + if (iterator == managed_objects_.end()) { + return nullptr; + } + return iterator->second.get(); +} + std::unique_ptr Utf8ToStringView(const std::string& message) { icu::UnicodeString utf16 = icu::UnicodeString::fromUTF8( icu::StringPiece(message.data(), message.length())); diff --git a/src/inspector/main_thread_interface.h b/src/inspector/main_thread_interface.h index 7092310e553c19..3e8eb13645b009 100644 --- a/src/inspector/main_thread_interface.h +++ b/src/inspector/main_thread_interface.h @@ -84,6 +84,7 @@ class MainThreadInterface { } void AddObject(int handle, std::unique_ptr object); Deletable* GetObject(int id); + Deletable* GetObjectIfExists(int id); void RemoveObject(int handle); private: diff --git a/src/inspector/tracing_agent.cc b/src/inspector/tracing_agent.cc index e8f9d569f8ee69..79fccbf8aa47ac 100644 --- a/src/inspector/tracing_agent.cc +++ b/src/inspector/tracing_agent.cc @@ -1,4 +1,5 @@ #include "tracing_agent.h" +#include "main_thread_interface.h" #include "node_internals.h" #include "env-inl.h" @@ -14,10 +15,76 @@ namespace protocol { namespace { using v8::platform::tracing::TraceWriter; +class DeletableFrontendWrapper : public Deletable { + public: + explicit DeletableFrontendWrapper( + std::weak_ptr frontend) + : frontend_(frontend) {} + + // This should only be called from the main thread, meaning frontend should + // not be destroyed concurrently. + NodeTracing::Frontend* get() { return frontend_.lock().get(); } + + private: + std::weak_ptr frontend_; +}; + +class CreateFrontendWrapperRequest : public Request { + public: + CreateFrontendWrapperRequest(int object_id, + std::weak_ptr frontend) + : object_id_(object_id) { + frontend_wrapper_ = std::make_unique(frontend); + } + + void Call(MainThreadInterface* thread) override { + thread->AddObject(object_id_, std::move(frontend_wrapper_)); + } + + private: + int object_id_; + std::unique_ptr frontend_wrapper_; +}; + +class DestroyFrontendWrapperRequest : public Request { + public: + explicit DestroyFrontendWrapperRequest(int object_id) + : object_id_(object_id) {} + + void Call(MainThreadInterface* thread) override { + thread->RemoveObject(object_id_); + } + + private: + int object_id_; +}; + +class SendMessageRequest : public Request { + public: + explicit SendMessageRequest(int object_id, const std::string& message) + : object_id_(object_id), message_(message) {} + + void Call(MainThreadInterface* thread) override { + DeletableFrontendWrapper* frontend_wrapper = + static_cast( + thread->GetObjectIfExists(object_id_)); + if (frontend_wrapper == nullptr) return; + auto frontend = frontend_wrapper->get(); + if (frontend != nullptr) { + frontend->sendRawNotification(message_); + } + } + + private: + int object_id_; + std::string message_; +}; + class InspectorTraceWriter : public node::tracing::AsyncTraceWriter { public: - explicit InspectorTraceWriter(NodeTracing::Frontend* frontend) - : frontend_(frontend) {} + explicit InspectorTraceWriter(int frontend_object_id, + std::shared_ptr main_thread) + : frontend_object_id_(frontend_object_id), main_thread_(main_thread) {} void AppendTraceEvent( v8::platform::tracing::TraceObject* trace_event) override { @@ -35,27 +102,35 @@ class InspectorTraceWriter : public node::tracing::AsyncTraceWriter { std::ostringstream::ate); result << stream_.str(); result << "}"; - frontend_->sendRawNotification(result.str()); + main_thread_->Post(std::make_unique(frontend_object_id_, + result.str())); stream_.str(""); } private: std::unique_ptr json_writer_; std::ostringstream stream_; - NodeTracing::Frontend* frontend_; + int frontend_object_id_; + std::shared_ptr main_thread_; }; } // namespace -TracingAgent::TracingAgent(Environment* env) - : env_(env) { -} +TracingAgent::TracingAgent(Environment* env, + std::shared_ptr main_thread) + : env_(env), main_thread_(main_thread) {} TracingAgent::~TracingAgent() { trace_writer_.reset(); + main_thread_->Post( + std::make_unique(frontend_object_id_)); } void TracingAgent::Wire(UberDispatcher* dispatcher) { - frontend_.reset(new NodeTracing::Frontend(dispatcher->channel())); + // Note that frontend is still owned by TracingAgent + frontend_ = std::make_shared(dispatcher->channel()); + frontend_object_id_ = main_thread_->newObjectId(); + main_thread_->Post(std::make_unique( + frontend_object_id_, frontend_)); NodeTracing::Dispatcher::wire(dispatcher, this); } @@ -81,11 +156,11 @@ DispatchResponse TracingAgent::start( tracing::AgentWriterHandle* writer = GetTracingAgentWriter(); if (writer != nullptr) { - trace_writer_ = writer->agent()->AddClient( - categories_set, - std::unique_ptr( - new InspectorTraceWriter(frontend_.get())), - tracing::Agent::kIgnoreDefaultCategories); + trace_writer_ = + writer->agent()->AddClient(categories_set, + std::make_unique( + frontend_object_id_, main_thread_), + tracing::Agent::kIgnoreDefaultCategories); } return DispatchResponse::OK(); } diff --git a/src/inspector/tracing_agent.h b/src/inspector/tracing_agent.h index 029fce7c191b42..29587b03c88211 100644 --- a/src/inspector/tracing_agent.h +++ b/src/inspector/tracing_agent.h @@ -10,11 +10,13 @@ namespace node { class Environment; namespace inspector { +class MainThreadHandle; + namespace protocol { class TracingAgent : public NodeTracing::Backend { public: - explicit TracingAgent(Environment*); + explicit TracingAgent(Environment*, std::shared_ptr); ~TracingAgent() override; void Wire(UberDispatcher* dispatcher); @@ -29,8 +31,10 @@ class TracingAgent : public NodeTracing::Backend { void DisconnectTraceClient(); Environment* env_; + std::shared_ptr main_thread_; tracing::AgentWriterHandle trace_writer_; - std::unique_ptr frontend_; + int frontend_object_id_; + std::shared_ptr frontend_; }; diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index dcc256b7f70434..2d919b9210bf65 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -211,14 +211,15 @@ class ChannelImpl final : public v8_inspector::V8Inspector::Channel, const std::unique_ptr& inspector, std::shared_ptr worker_manager, std::unique_ptr delegate, + std::shared_ptr main_thread_, bool prevent_shutdown) - : delegate_(std::move(delegate)), - prevent_shutdown_(prevent_shutdown) { + : delegate_(std::move(delegate)), prevent_shutdown_(prevent_shutdown) { session_ = inspector->connect(1, this, StringView()); - node_dispatcher_.reset(new protocol::UberDispatcher(this)); - tracing_agent_.reset(new protocol::TracingAgent(env)); + node_dispatcher_ = std::make_unique(this); + tracing_agent_ = + std::make_unique(env, main_thread_); tracing_agent_->Wire(node_dispatcher_.get()); - worker_agent_.reset(new protocol::WorkerAgent(worker_manager)); + worker_agent_ = std::make_unique(worker_manager); worker_agent_->Wire(node_dispatcher_.get()); } @@ -467,9 +468,12 @@ class NodeInspectorClient : public V8InspectorClient { bool prevent_shutdown) { events_dispatched_ = true; int session_id = next_session_id_++; - channels_[session_id] = - std::make_unique(env_, client_, getWorkerManager(), - std::move(delegate), prevent_shutdown); + channels_[session_id] = std::make_unique(env_, + client_, + getWorkerManager(), + std::move(delegate), + getThreadHandle(), + prevent_shutdown); return session_id; } diff --git a/test/parallel/test-trace-events-dynamic-enable-workers-disabled.js b/test/parallel/test-trace-events-dynamic-enable-workers-disabled.js index 87ce617e8e221c..634017f95ad051 100644 --- a/test/parallel/test-trace-events-dynamic-enable-workers-disabled.js +++ b/test/parallel/test-trace-events-dynamic-enable-workers-disabled.js @@ -25,3 +25,4 @@ session.post('NodeTracing.start', { 'Tracing properties can only be changed through main thread sessions' }); })); +session.disconnect();