inspector, trace_events: make sure messages are sent on a main thread

Fixes: nodejs#23185 PR-URL: nodejs#24814 Reviewed-By: James M Snell <[email protected]>
sxa · Jan 30, 2019 · 80a18ca · 80a18ca
1 parent c64b1ae
commit 80a18ca
Show file tree

Hide file tree

Showing 6 changed files with 117 additions and 26 deletions.
diff --git 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<StringBuffer> 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
@@ -84,6 +84,7 @@ class MainThreadInterface {
   }
   void AddObject(int handle, std::unique_ptr<Deletable> 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
@@ -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<NodeTracing::Frontend> 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<NodeTracing::Frontend> frontend_;
+};
+
+class CreateFrontendWrapperRequest : public Request {
+ public:
+  CreateFrontendWrapperRequest(int object_id,
+                               std::weak_ptr<NodeTracing::Frontend> frontend)
+      : object_id_(object_id) {
+    frontend_wrapper_ = std::make_unique<DeletableFrontendWrapper>(frontend);
+  }
+
+  void Call(MainThreadInterface* thread) override {
+    thread->AddObject(object_id_, std::move(frontend_wrapper_));
+  }
+
+ private:
+  int object_id_;
+  std::unique_ptr<DeletableFrontendWrapper> 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<DeletableFrontendWrapper*>(
+            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<MainThreadHandle> 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<SendMessageRequest>(frontend_object_id_,
+                                                            result.str()));
     stream_.str("");
   }
 
  private:
   std::unique_ptr<TraceWriter> json_writer_;
   std::ostringstream stream_;
-  NodeTracing::Frontend* frontend_;
+  int frontend_object_id_;
+  std::shared_ptr<MainThreadHandle> main_thread_;
 };
 }  // namespace
 
-TracingAgent::TracingAgent(Environment* env)
-                           : env_(env) {
-}
+TracingAgent::TracingAgent(Environment* env,
+                           std::shared_ptr<MainThreadHandle> main_thread)
+    : env_(env), main_thread_(main_thread) {}
 
 TracingAgent::~TracingAgent() {
   trace_writer_.reset();
+  main_thread_->Post(
+      std::make_unique<DestroyFrontendWrapperRequest>(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<NodeTracing::Frontend>(dispatcher->channel());
+  frontend_object_id_ = main_thread_->newObjectId();
+  main_thread_->Post(std::make_unique<CreateFrontendWrapperRequest>(
+      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<InspectorTraceWriter>(
-            new InspectorTraceWriter(frontend_.get())),
-        tracing::Agent::kIgnoreDefaultCategories);
+    trace_writer_ =
+        writer->agent()->AddClient(categories_set,
+                                   std::make_unique<InspectorTraceWriter>(
+                                       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
@@ -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<MainThreadHandle>);
   ~TracingAgent() override;
 
   void Wire(UberDispatcher* dispatcher);
@@ -29,8 +31,10 @@ class TracingAgent : public NodeTracing::Backend {
   void DisconnectTraceClient();
 
   Environment* env_;
+  std::shared_ptr<MainThreadHandle> main_thread_;
   tracing::AgentWriterHandle trace_writer_;
-  std::unique_ptr<NodeTracing::Frontend> frontend_;
+  int frontend_object_id_;
+  std::shared_ptr<NodeTracing::Frontend> frontend_;
 };
 
 

diff --git 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<V8Inspector>& inspector,
                        std::shared_ptr<WorkerManager> worker_manager,
                        std::unique_ptr<InspectorSessionDelegate> delegate,
+                       std::shared_ptr<MainThreadHandle> 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<protocol::UberDispatcher>(this);
+    tracing_agent_ =
+        std::make_unique<protocol::TracingAgent>(env, main_thread_);
     tracing_agent_->Wire(node_dispatcher_.get());
-    worker_agent_.reset(new protocol::WorkerAgent(worker_manager));
+    worker_agent_ = std::make_unique<protocol::WorkerAgent>(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<ChannelImpl>(env_, client_, getWorkerManager(),
-                                      std::move(delegate), prevent_shutdown);
+    channels_[session_id] = std::make_unique<ChannelImpl>(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
@@ -25,3 +25,4 @@ session.post('NodeTracing.start', {
       'Tracing properties can only be changed through main thread sessions'
   });
 }));
+session.disconnect();