From 2e4b318e78821cbc9ddc396c58b3326514a875c5 Mon Sep 17 00:00:00 2001 From: Bradley Farias Date: Mon, 24 Oct 2016 11:12:22 -0500 Subject: [PATCH 1/4] inspector: add contexts when using vm module Adds all Contexts created by the vm module to the inspector. This does permanent tracking of Contexts and syncing the Contexts whenever a new inspector is created. Fixes: https://github.com/nodejs/node/issues/7593 --- src/env-inl.h | 19 +++++++++++++++++++ src/env.cc | 5 +++++ src/env.h | 8 ++++++++ src/inspector_agent.cc | 29 +++++++++++++++++++++++++++-- src/inspector_agent.h | 5 +++++ src/node_contextify.cc | 11 +++++++++++ 6 files changed, 75 insertions(+), 2 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index 74e427e40353d9..2abd064d9cd4e7 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -356,6 +356,25 @@ inline IsolateData* Environment::isolate_data() const { return isolate_data_; } +inline void Environment::context_created(v8_inspector::V8ContextInfo info) { + contexts()->push_back(info); + if (inspector_agent()->IsStarted()) { + inspector_agent()->ContextCreated(info); + } +} +inline void Environment::context_destroyed(v8::Local context) { + for (auto i = std::begin(*contexts()); i != std::end(*contexts()); ++i) { + auto it = *i; + if (it.context == context) { + contexts()->erase(i); + if (inspector_agent()->IsStarted()) { + inspector_agent()->ContextDestroyed(context); + } + return; + } + } +} + inline void Environment::ThrowError(const char* errmsg) { ThrowError(v8::Exception::Error, errmsg); } diff --git a/src/env.cc b/src/env.cc index efa2d53f0435b2..1dcd16073719ea 100644 --- a/src/env.cc +++ b/src/env.cc @@ -30,6 +30,11 @@ void Environment::Start(int argc, HandleScope handle_scope(isolate()); Context::Scope context_scope(context()); + context_created( + v8_inspector::V8ContextInfo(context(), 1, "NodeJS Main Context")); + + isolate()->SetAutorunMicrotasks(false); + uv_check_init(event_loop(), immediate_check_handle()); uv_unref(reinterpret_cast(immediate_check_handle())); diff --git a/src/env.h b/src/env.h index fe49ac0c876a47..63c41d5b1537f7 100644 --- a/src/env.h +++ b/src/env.h @@ -7,6 +7,7 @@ #include "debug-agent.h" #if HAVE_INSPECTOR #include "inspector_agent.h" +#include #endif #include "handle_wrap.h" #include "req-wrap.h" @@ -528,6 +529,12 @@ class Environment { inline inspector::Agent* inspector_agent() { return &inspector_agent_; } + + inline void context_created(v8_inspector::V8ContextInfo context); + inline void context_destroyed(v8::Local context); + inline std::vector* contexts() { + return &contexts_; + } #endif typedef ListHead HandleWrapQueue; @@ -564,6 +571,7 @@ class Environment { debugger::Agent debugger_agent_; #if HAVE_INSPECTOR inspector::Agent inspector_agent_; + std::vector contexts_; #endif HandleWrapQueue handle_wrap_queue_; diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index 3f3d548de4cdf6..0a050d0cc6f859 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -191,6 +191,8 @@ class AgentImpl { bool Start(v8::Platform* platform, const char* path, int port, bool wait); // Stop the inspector agent void Stop(); + void ContextCreated(const v8_inspector::V8ContextInfo& info); + void ContextDestroyed(v8::Local context); bool IsStarted(); bool IsConnected() { return state_ == State::kConnected; } @@ -323,8 +325,17 @@ class V8NodeInspector : public v8_inspector::V8InspectorClient { terminated_(false), running_nested_loop_(false), inspector_(V8Inspector::create(env->isolate(), this)) { - inspector_->contextCreated( - v8_inspector::V8ContextInfo(env->context(), 1, "NodeJS Main Context")); + v8::HandleScope handles(env_->isolate()); + for (auto it : *(env->contexts())) { + contextCreated(it); + } + } + + void contextCreated(const v8_inspector::V8ContextInfo& info) { + inspector()->contextCreated(info); + } + void contextDestroyed(v8::Local context) { + inspector()->contextDestroyed(context); } void runMessageLoopOnPause(int context_group_id) override { @@ -510,6 +521,13 @@ void AgentImpl::Stop() { delete inspector_; } +void AgentImpl::ContextCreated(const v8_inspector::V8ContextInfo& info) { + inspector_->contextCreated(info); +} +void AgentImpl::ContextDestroyed(v8::Local context) { + inspector_->contextDestroyed(context); +} + bool AgentImpl::IsStarted() { return !!platform_; } @@ -865,6 +883,13 @@ void Agent::Stop() { impl->Stop(); } +void Agent::ContextCreated(const v8_inspector::V8ContextInfo& info) { + impl->ContextCreated(info); +} +void Agent::ContextDestroyed(v8::Local context) { + impl->ContextDestroyed(context); +} + bool Agent::IsStarted() { return impl->IsStarted(); } diff --git a/src/inspector_agent.h b/src/inspector_agent.h index 3607cffba5d21f..d52bf995e0b526 100644 --- a/src/inspector_agent.h +++ b/src/inspector_agent.h @@ -5,6 +5,8 @@ #error("This header can only be used when inspector is enabled") #endif +#include "platform/v8_inspector/public/V8Inspector.h" + // Forward declaration to break recursive dependency chain with src/env.h. namespace node { class Environment; @@ -33,6 +35,9 @@ class Agent { // Stop the inspector agent void Stop(); + void ContextCreated(const v8_inspector::V8ContextInfo& info); + void ContextDestroyed(v8::Local context); + bool IsStarted(); bool IsConnected(); void WaitForDisconnect(); diff --git a/src/node_contextify.cc b/src/node_contextify.cc index d74b01ea0da371..4a5da3abb2f0fc 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -9,6 +9,10 @@ #include "util-inl.h" #include "v8-debug.h" +#if HAVE_INSPECTOR +#include "platform/v8_inspector/public/V8Inspector.h" +#endif + namespace node { using v8::Array; @@ -207,6 +211,10 @@ class ContextifyContext { object_template->SetHandler(config); Local ctx = Context::New(env->isolate(), nullptr, object_template); +#if HAVE_INSPECTOR + env->context_created( + v8_inspector::V8ContextInfo(ctx, 1, "vm Module Context")); +#endif if (ctx.IsEmpty()) { env->ThrowError("Could not instantiate context"); @@ -323,6 +331,9 @@ class ContextifyContext { static void WeakCallback(const WeakCallbackInfo& data) { ContextifyContext* context = data.GetParameter(); +#if HAVE_INSPECTOR + context->env()->context_destroyed(context->context()); +#endif delete context; } From 759333f5020ca09cabcaf60398d1fc85a3d764d2 Mon Sep 17 00:00:00 2001 From: Bradley Farias Date: Wed, 26 Oct 2016 15:22:26 -0500 Subject: [PATCH 2/4] Fixes for reviews: change context_created to ContextCreated on Environment create node specific class instead of exposing v8_inspector for Context tracking --- src/env-inl.h | 9 ++++++--- src/env.cc | 10 ++++++++-- src/env.h | 8 ++++---- src/inspector_agent.cc | 15 ++++++++++----- src/inspector_agent.h | 24 +++++++++++++++++++++++- src/node_contextify.cc | 8 ++++---- 6 files changed, 55 insertions(+), 19 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index 2abd064d9cd4e7..12ada0ffe27883 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -356,16 +356,18 @@ inline IsolateData* Environment::isolate_data() const { return isolate_data_; } -inline void Environment::context_created(v8_inspector::V8ContextInfo info) { +#if HAVE_INSPECTOR +inline void Environment::ContextCreated(node::inspector::ContextInfo* info) { contexts()->push_back(info); if (inspector_agent()->IsStarted()) { inspector_agent()->ContextCreated(info); } } -inline void Environment::context_destroyed(v8::Local context) { +inline void Environment::ContextDestroyed(v8::Local context) { for (auto i = std::begin(*contexts()); i != std::end(*contexts()); ++i) { auto it = *i; - if (it.context == context) { + if (it->context(isolate()) == context) { + delete it; contexts()->erase(i); if (inspector_agent()->IsStarted()) { inspector_agent()->ContextDestroyed(context); @@ -374,6 +376,7 @@ inline void Environment::context_destroyed(v8::Local context) { } } } +#endif inline void Environment::ThrowError(const char* errmsg) { ThrowError(v8::Exception::Error, errmsg); diff --git a/src/env.cc b/src/env.cc index 1dcd16073719ea..148724a12bb725 100644 --- a/src/env.cc +++ b/src/env.cc @@ -12,6 +12,10 @@ #include +#if HAVE_INSPECTOR +#include "inspector_agent.h" +#endif + namespace node { using v8::Context; @@ -30,8 +34,10 @@ void Environment::Start(int argc, HandleScope handle_scope(isolate()); Context::Scope context_scope(context()); - context_created( - v8_inspector::V8ContextInfo(context(), 1, "NodeJS Main Context")); +#if HAVE_INSPECTOR + ContextCreated( + new node::inspector::ContextInfo(context(), 1, "NodeJS Main Context")); +#endif isolate()->SetAutorunMicrotasks(false); diff --git a/src/env.h b/src/env.h index 63c41d5b1537f7..77d46c1c6912cc 100644 --- a/src/env.h +++ b/src/env.h @@ -530,9 +530,9 @@ class Environment { return &inspector_agent_; } - inline void context_created(v8_inspector::V8ContextInfo context); - inline void context_destroyed(v8::Local context); - inline std::vector* contexts() { + inline void ContextCreated(node::inspector::ContextInfo* info); + inline void ContextDestroyed(v8::Local context); + inline std::vector* contexts() { return &contexts_; } #endif @@ -571,7 +571,7 @@ class Environment { debugger::Agent debugger_agent_; #if HAVE_INSPECTOR inspector::Agent inspector_agent_; - std::vector contexts_; + std::vector contexts_; #endif HandleWrapQueue handle_wrap_queue_; diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index 0a050d0cc6f859..013ac2671f248a 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -191,7 +191,7 @@ class AgentImpl { bool Start(v8::Platform* platform, const char* path, int port, bool wait); // Stop the inspector agent void Stop(); - void ContextCreated(const v8_inspector::V8ContextInfo& info); + void ContextCreated(const node::inspector::ContextInfo* info); void ContextDestroyed(v8::Local context); bool IsStarted(); @@ -331,9 +331,14 @@ class V8NodeInspector : public v8_inspector::V8InspectorClient { } } - void contextCreated(const v8_inspector::V8ContextInfo& info) { - inspector()->contextCreated(info); + void contextCreated(const node::inspector::ContextInfo* info) { + inspector()->contextCreated( + v8_inspector::V8ContextInfo( + info->context(env_->isolate()), + info->groupId(), + info->name())); } + void contextDestroyed(v8::Local context) { inspector()->contextDestroyed(context); } @@ -521,7 +526,7 @@ void AgentImpl::Stop() { delete inspector_; } -void AgentImpl::ContextCreated(const v8_inspector::V8ContextInfo& info) { +void AgentImpl::ContextCreated(const node::inspector::ContextInfo* info) { inspector_->contextCreated(info); } void AgentImpl::ContextDestroyed(v8::Local context) { @@ -883,7 +888,7 @@ void Agent::Stop() { impl->Stop(); } -void Agent::ContextCreated(const v8_inspector::V8ContextInfo& info) { +void Agent::ContextCreated(const node::inspector::ContextInfo* info) { impl->ContextCreated(info); } void Agent::ContextDestroyed(v8::Local context) { diff --git a/src/inspector_agent.h b/src/inspector_agent.h index d52bf995e0b526..6ce583d67ba1e6 100644 --- a/src/inspector_agent.h +++ b/src/inspector_agent.h @@ -25,6 +25,28 @@ namespace inspector { class AgentImpl; +class ContextInfo { + public: + explicit ContextInfo(v8::Local context, int groupId, + const char* name) + : groupId_(groupId), + name_(name) { + context_.Reset(context->GetIsolate(), context); + } + ~ContextInfo() { + context_.Reset(); + } + inline v8::Local context(v8::Isolate* isolate) const { + return context_.Get(isolate); + } + int groupId() const { return groupId_; } + const char* name() const { return name_; } + private: + v8::Persistent context_; + int groupId_; + const char* name_; +}; + class Agent { public: explicit Agent(node::Environment* env); @@ -35,7 +57,7 @@ class Agent { // Stop the inspector agent void Stop(); - void ContextCreated(const v8_inspector::V8ContextInfo& info); + void ContextCreated(const ContextInfo* info); void ContextDestroyed(v8::Local context); bool IsStarted(); diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 4a5da3abb2f0fc..2ad99e9cd7468a 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -10,7 +10,7 @@ #include "v8-debug.h" #if HAVE_INSPECTOR -#include "platform/v8_inspector/public/V8Inspector.h" +#include "inspector_agent.h" #endif namespace node { @@ -212,8 +212,8 @@ class ContextifyContext { Local ctx = Context::New(env->isolate(), nullptr, object_template); #if HAVE_INSPECTOR - env->context_created( - v8_inspector::V8ContextInfo(ctx, 1, "vm Module Context")); + env->ContextCreated( + new node::inspector::ContextInfo(ctx, 1, "vm Module Context")); #endif if (ctx.IsEmpty()) { @@ -332,7 +332,7 @@ class ContextifyContext { static void WeakCallback(const WeakCallbackInfo& data) { ContextifyContext* context = data.GetParameter(); #if HAVE_INSPECTOR - context->env()->context_destroyed(context->context()); + context->env()->ContextDestroyed(context->context()); #endif delete context; } From 8f351b27c8b4448add7195450c0eb3acc81776a5 Mon Sep 17 00:00:00 2001 From: Bradley Farias Date: Wed, 26 Oct 2016 16:26:06 -0500 Subject: [PATCH 3/4] fix accidental line added during fixes --- src/env.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/env.cc b/src/env.cc index 148724a12bb725..575221cb689909 100644 --- a/src/env.cc +++ b/src/env.cc @@ -39,8 +39,6 @@ void Environment::Start(int argc, new node::inspector::ContextInfo(context(), 1, "NodeJS Main Context")); #endif - isolate()->SetAutorunMicrotasks(false); - uv_check_init(event_loop(), immediate_check_handle()); uv_unref(reinterpret_cast(immediate_check_handle())); From de390f7915a0af1af9d35d666a33c13ae0f2f4dc Mon Sep 17 00:00:00 2001 From: Bradley Farias Date: Thu, 17 Nov 2016 10:15:40 -0600 Subject: [PATCH 4/4] changes for @bnoordhuis --- src/env-inl.h | 5 +++-- src/env.cc | 2 +- src/inspector_agent.cc | 10 +++++----- src/inspector_agent.h | 11 ++++------- src/node_contextify.cc | 2 +- 5 files changed, 14 insertions(+), 16 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index 12ada0ffe27883..9e8db822c66580 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -175,7 +175,7 @@ inline Environment::Environment(IsolateData* isolate_data, debugger_agent_(this), #if HAVE_INSPECTOR inspector_agent_(this), -#endif +#endif // HAVE_INSPECTOR handle_cleanup_waiting_(0), http_parser_buffer_(nullptr), context_(context->GetIsolate(), context) { @@ -376,7 +376,8 @@ inline void Environment::ContextDestroyed(v8::Local context) { } } } -#endif + +#endif // HAVE_INSPECTOR inline void Environment::ThrowError(const char* errmsg) { ThrowError(v8::Exception::Error, errmsg); diff --git a/src/env.cc b/src/env.cc index 575221cb689909..ec94e98e645582 100644 --- a/src/env.cc +++ b/src/env.cc @@ -36,7 +36,7 @@ void Environment::Start(int argc, #if HAVE_INSPECTOR ContextCreated( - new node::inspector::ContextInfo(context(), 1, "NodeJS Main Context")); + new node::inspector::ContextInfo(context(), 1, "NodeJS Main Context")); #endif uv_check_init(event_loop(), immediate_check_handle()); diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index 013ac2671f248a..ffca94a839751a 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -326,17 +326,17 @@ class V8NodeInspector : public v8_inspector::V8InspectorClient { running_nested_loop_(false), inspector_(V8Inspector::create(env->isolate(), this)) { v8::HandleScope handles(env_->isolate()); - for (auto it : *(env->contexts())) { + for (auto it : *env->contexts()) { contextCreated(it); } } void contextCreated(const node::inspector::ContextInfo* info) { inspector()->contextCreated( - v8_inspector::V8ContextInfo( - info->context(env_->isolate()), - info->groupId(), - info->name())); + v8_inspector::V8ContextInfo( + info->context(env_->isolate()), + info->groupId(), + info->name())); } void contextDestroyed(v8::Local context) { diff --git a/src/inspector_agent.h b/src/inspector_agent.h index 6ce583d67ba1e6..8667ab664bf038 100644 --- a/src/inspector_agent.h +++ b/src/inspector_agent.h @@ -27,23 +27,20 @@ class AgentImpl; class ContextInfo { public: - explicit ContextInfo(v8::Local context, int groupId, + explicit ContextInfo(v8::Local context, const int groupId, const char* name) - : groupId_(groupId), + : group_id_(groupId), name_(name) { context_.Reset(context->GetIsolate(), context); } - ~ContextInfo() { - context_.Reset(); - } inline v8::Local context(v8::Isolate* isolate) const { return context_.Get(isolate); } - int groupId() const { return groupId_; } + int groupId() const { return group_id_; } const char* name() const { return name_; } private: v8::Persistent context_; - int groupId_; + const int group_id_; const char* name_; }; diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 2ad99e9cd7468a..29ad29947925d0 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -213,7 +213,7 @@ class ContextifyContext { Local ctx = Context::New(env->isolate(), nullptr, object_template); #if HAVE_INSPECTOR env->ContextCreated( - new node::inspector::ContextInfo(ctx, 1, "vm Module Context")); + new node::inspector::ContextInfo(ctx, 1, "vm Module Context")); #endif if (ctx.IsEmpty()) {