Skip to content

Commit

Permalink
inspector, trace_events: make sure messages are sent on a main thread
Browse files Browse the repository at this point in the history
Fixes: #23185
PR-URL: #24814
Reviewed-By: James M Snell <[email protected]>
  • Loading branch information
eugeneo authored and addaleax committed Jan 30, 2019
1 parent 4fe0819 commit 6967407
Show file tree
Hide file tree
Showing 6 changed files with 117 additions and 26 deletions.
12 changes: 9 additions & 3 deletions src/inspector/main_thread_interface.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
Expand Down
1 change: 1 addition & 0 deletions src/inspector/main_thread_interface.h
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
101 changes: 88 additions & 13 deletions src/inspector/tracing_agent.cc
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#include "tracing_agent.h"
#include "main_thread_interface.h"
#include "node_internals.h"

#include "env-inl.h"
Expand All @@ -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 {
Expand All @@ -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);
}

Expand All @@ -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();
}
Expand Down
8 changes: 6 additions & 2 deletions src/inspector/tracing_agent.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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_;
};


Expand Down
20 changes: 12 additions & 8 deletions src/inspector_agent.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}

Expand Down Expand Up @@ -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;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,4 @@ session.post('NodeTracing.start', {
'Tracing properties can only be changed through main thread sessions'
});
}));
session.disconnect();

0 comments on commit 6967407

Please sign in to comment.