Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

inspector: add contexts when using vm module #9272

Closed
wants to merge 4 commits into from
Closed

inspector: add contexts when using vm module #9272

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2e4b318
inspector: add contexts when using vm module
bmeck Oct 24, 2016
759333f
Fixes for reviews:
bmeck Oct 26, 2016
8f351b2
fix accidental line added during fixes
bmeck Oct 26, 2016
de390f7
changes for @bnoordhuis
bmeck Nov 17, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions src/env-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be enclosed in #if HAVE_INSPECTOR check?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good find

inspector_agent()->ContextCreated(info);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to always call ContextCreated/ContextDestroyed - and then let inspector_agent check the IsStarted. This way in this header it will be a single line call (also, contexts vector will live in inspector then).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eugeneo the way the inspector_agent is setup, it is unsafe to call prior to it being started since platform_ does not exist.

}
}
inline void Environment::context_destroyed(v8::Local<v8::Context> 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;
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could simplify this with std::find or std::find_if.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the erase needs an index not value.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

find() and find_if() return iterators, not values.


inline void Environment::ThrowError(const char* errmsg) {
ThrowError(v8::Exception::Error, errmsg);
}
Expand Down
5 changes: 5 additions & 0 deletions src/env.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this related to this change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unclear how this got in here, should be removed.


uv_check_init(event_loop(), immediate_check_handle());
uv_unref(reinterpret_cast<uv_handle_t*>(immediate_check_handle()));

Expand Down
8 changes: 8 additions & 0 deletions src/env.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include "debug-agent.h"
#if HAVE_INSPECTOR
#include "inspector_agent.h"
#include <vector>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Including STL header on the high level header used to cause some problems with the memory allocation on AIX (it was using a "wrong" alloc). I do not know if they were fixed...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these includes are also present in existing inspector files

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I had to make sure they do not leak outside of inspector_agent translation unit. This issue might've been fixed since.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see it all over https://github.com/nodejs/node/search?utf8=%E2%9C%93&q=path%3Asrc+%22include+%3Cvector%3E%22&type=Code , presuming this is fixed.

#endif
#include "handle_wrap.h"
#include "req-wrap.h"
Expand Down Expand Up @@ -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<v8::Context> context);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you call these ContextCreated() and ContextDestroyed()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good

inline std::vector<v8_inspector::V8ContextInfo>* contexts() {
return &contexts_;
}
#endif

typedef ListHead<HandleWrap, &HandleWrap::handle_wrap_queue_> HandleWrapQueue;
Expand Down Expand Up @@ -564,6 +571,7 @@ class Environment {
debugger::Agent debugger_agent_;
#if HAVE_INSPECTOR
inspector::Agent inspector_agent_;
std::vector<v8_inspector::V8ContextInfo> contexts_;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think V8ContextInfo is safe to use here because it doesn't look like it's supposed to outlive a HandleScope. The context is stored as a v8::Local<v8::Context> and Locals are clobbered when the HandleScope is destroyed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can do it and match the other request to limit usage of v8_inspector namespace usage

#endif

HandleWrapQueue handle_wrap_queue_;
Expand Down
29 changes: 27 additions & 2 deletions src/inspector_agent.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<v8::Context> context);

bool IsStarted();
bool IsConnected() { return state_ == State::kConnected; }
Expand Down Expand Up @@ -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())) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need parens around env->contexts().

contextCreated(it);
}
}

void contextCreated(const v8_inspector::V8ContextInfo& info) {
inspector()->contextCreated(info);
}
void contextDestroyed(v8::Local<v8::Context> context) {
inspector()->contextDestroyed(context);
}

void runMessageLoopOnPause(int context_group_id) override {
Expand Down Expand Up @@ -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<v8::Context> context) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you insert a blank line here and on line 894?

inspector_->contextDestroyed(context);
}

bool AgentImpl::IsStarted() {
return !!platform_;
}
Expand Down Expand Up @@ -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<v8::Context> context) {
impl->ContextDestroyed(context);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, is the two or three levels of forwarding strictly necessary?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in order to pass things to the right place and get through all the levels of abstraction, yes.


bool Agent::IsStarted() {
return impl->IsStarted();
}
Expand Down
5 changes: 5 additions & 0 deletions src/inspector_agent.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -33,6 +35,9 @@ class Agent {
// Stop the inspector agent
void Stop();

void ContextCreated(const v8_inspector::V8ContextInfo& info);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it would be better if this call accepted v8::Context (and, optionally, a string) rather then V8ContextInfo. This would allow us no to leak inspector API to the bigger Node.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

possible, but there is some complexity here since the v8 inspector is already tightly bound in inspector_agent.cc, if we had a generic API I would agree, but I don't see that currently.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's still some churn in inspector API (e.g. #9028 is quite major) so I feel like it would be safer to only rely on v8 namespace.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i saw the Threads bit in the new devtools, is that the purpose of the contextGroupId? Should we be including that as well?

void ContextDestroyed(v8::Local<v8::Context> context);

bool IsStarted();
bool IsConnected();
void WaitForDisconnect();
Expand Down
11 changes: 11 additions & 0 deletions src/node_contextify.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -207,6 +211,10 @@ class ContextifyContext {
object_template->SetHandler(config);

Local<Context> 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");
Expand Down Expand Up @@ -323,6 +331,9 @@ class ContextifyContext {

static void WeakCallback(const WeakCallbackInfo<ContextifyContext>& data) {
ContextifyContext* context = data.GetParameter();
#if HAVE_INSPECTOR
context->env()->context_destroyed(context->context());
#endif
delete context;
}

Expand Down