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.

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
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
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
11 changes: 11 additions & 0 deletions src/env.cc
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,13 @@ 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

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
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.

#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
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
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
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