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 3 commits
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
22 changes: 22 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,28 @@ inline IsolateData* Environment::isolate_data() const {
return isolate_data_;
}

#if HAVE_INSPECTOR
inline void Environment::ContextCreated(node::inspector::ContextInfo* 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::ContextDestroyed(v8::Local<v8::Context> context) {
for (auto i = std::begin(*contexts()); i != std::end(*contexts()); ++i) {
auto it = *i;
if (it->context(isolate()) == context) {
delete it;
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.

#endif
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 write this as #endif // HAVE_INSPECTOR? Note the two spaces after #endif.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixing, but copy paste only shows

#endif [<= one space?]// HAVE_INSPECTOR


inline void Environment::ThrowError(const char* errmsg) {
ThrowError(v8::Exception::Error, errmsg);
}
Expand Down
9 changes: 9 additions & 0 deletions src/env.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@

#include <stdio.h>

#if HAVE_INSPECTOR
#include "inspector_agent.h"
#endif

namespace node {

using v8::Context;
Expand All @@ -30,6 +34,11 @@ void Environment::Start(int argc,
HandleScope handle_scope(isolate());
Context::Scope context_scope(context());

#if HAVE_INSPECTOR
ContextCreated(
new node::inspector::ContextInfo(context(), 1, "NodeJS Main Context"));
Copy link
Member

Choose a reason for hiding this comment

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

Line continuations should have four spaces of indent. (Ditto in other files, I won't point them out separately.)

#endif

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 ContextCreated(node::inspector::ContextInfo* 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 still have an issue with the parameter asymmetry - especially, considering that ContextInfo is instantiated in call side in both cases (and will likely be for all future uses).

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 on what you suggest as an alternative?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am suggesting not to expose ContextInfo outside of inspector-agent.cc

inline void ContextDestroyed(v8::Local<v8::Context> context);
inline std::vector<node::inspector::ContextInfo*>* 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<node::inspector::ContextInfo*> contexts_;
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO, this vector should be a member of inspector::AgentImpl.

Copy link
Member Author

Choose a reason for hiding this comment

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

disagree since inspector isn't really usable until it starts

Copy link
Contributor

Choose a reason for hiding this comment

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

But you're calling IsStarted - so it is usable, just not started.

I'm wondering if there's some way to introduce some sort of context registry that is not inspector specific. There might already be one in v8...

Copy link
Member Author

Choose a reason for hiding this comment

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

IsStarted just checks if things are usable, platform_ being uninitialized resulted in segfaults when I tried to call contextCreated early on

#endif

HandleWrapQueue handle_wrap_queue_;
Expand Down
34 changes: 32 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 node::inspector::ContextInfo* info);
void ContextDestroyed(v8::Local<v8::Context> context);

bool IsStarted();
bool IsConnected() { return state_ == State::kConnected; }
Expand Down Expand Up @@ -323,8 +325,22 @@ 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 node::inspector::ContextInfo* info) {
inspector()->contextCreated(
v8_inspector::V8ContextInfo(
info->context(env_->isolate()),
info->groupId(),
info->name()));
}

void contextDestroyed(v8::Local<v8::Context> context) {
inspector()->contextDestroyed(context);
}

void runMessageLoopOnPause(int context_group_id) override {
Expand Down Expand Up @@ -510,6 +526,13 @@ void AgentImpl::Stop() {
delete inspector_;
}

void AgentImpl::ContextCreated(const node::inspector::ContextInfo* 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 +888,13 @@ void Agent::Stop() {
impl->Stop();
}

void Agent::ContextCreated(const node::inspector::ContextInfo* 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
27 changes: 27 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 All @@ -23,6 +25,28 @@ namespace inspector {

class AgentImpl;

class ContextInfo {
public:
explicit ContextInfo(v8::Local<v8::Context> context, int groupId,
const char* name)
: groupId_(groupId),
name_(name) {
context_.Reset(context->GetIsolate(), context);
}
~ContextInfo() {
context_.Reset();
Copy link
Member

Choose a reason for hiding this comment

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

Not necessary, ~Persistent() resets by default.

}
inline v8::Local<v8::Context> context(v8::Isolate* isolate) const {
return context_.Get(isolate);
}
int groupId() const { return groupId_; }
const char* name() const { return name_; }
private:
v8::Persistent<v8::Context> context_;
int groupId_;
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 snake_case it to group_id_? If it never changes, consider making it const (same for name_.)

const char* name_;
};

class Agent {
public:
explicit Agent(node::Environment* env);
Expand All @@ -33,6 +57,9 @@ class Agent {
// Stop the inspector agent
void Stop();

void ContextCreated(const ContextInfo* info);
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 "inspector_agent.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->ContextCreated(
new node::inspector::ContextInfo(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()->ContextDestroyed(context->context());
#endif
delete context;
}

Expand Down