inspector: add contexts when using vm module #9272

bmeck · 2016-10-25T15:05:26Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

inspector

Description of change

Tracks all vm module contexts for usage in v8_inspector. These are tracked even when the inspector is not running so that they are visible when the inspector is started up.

Fixes: #7593

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: nodejs#7593

jasnell · 2016-10-25T18:16:33Z

/cc @bnoordhuis

eugeneo

Thanks for looking into it. Some comments - mostly about keeping as much gory inspector details within inspector itself.

eugeneo · 2016-10-25T18:30:20Z

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()) {


Should this be enclosed in #if HAVE_INSPECTOR check?

eugeneo · 2016-10-25T18:32:11Z

src/env-inl.h

+inline void Environment::context_created(v8_inspector::V8ContextInfo info) {
+  contexts()->push_back(info);
+  if (inspector_agent()->IsStarted()) {
+    inspector_agent()->ContextCreated(info);


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

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

eugeneo · 2016-10-25T18:34:06Z

src/env.h

@@ -7,6 +7,7 @@
 #include "debug-agent.h"
 #if HAVE_INSPECTOR
 #include "inspector_agent.h"
+#include <vector>


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

these includes are also present in existing inspector files

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

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.

eugeneo · 2016-10-25T18:38:08Z

src/inspector_agent.h

@@ -33,6 +35,9 @@ class Agent {
  // Stop the inspector agent
  void Stop();

+  void ContextCreated(const v8_inspector::V8ContextInfo& info);


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.

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.

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.

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

bnoordhuis · 2016-10-25T20:43:15Z

src/env.h

@@ -564,6 +571,7 @@ class Environment {
  debugger::Agent debugger_agent_;
 #if HAVE_INSPECTOR
  inspector::Agent inspector_agent_;
+  std::vector<v8_inspector::V8ContextInfo> contexts_;


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.

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

bnoordhuis · 2016-10-25T20:44:42Z

src/env.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<v8::Context> context);


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

sounds good

change context_created to ContextCreated on Environment create node specific class instead of exposing v8_inspector for Context tracking

bmeck · 2016-10-26T20:24:22Z

@bnoordhuis @eugeneo fixed, please review when you find the time :)

eugeneo

Thanks for doing this. I still strongly feel like majority of this code should remain private to the inspector. I wonder if there is a way (or should be one) to be notified of context creation/destruction so the inspector could somehow subscribe to it.

Please notice that I've updates the V8 inspector version, now the API is the same as in inspector that is in the V8 repository.

eugeneo · 2016-10-26T20:28:30Z

src/env.h

@@ -528,6 +529,12 @@ class Environment {
  inline inspector::Agent* inspector_agent() {
    return &inspector_agent_;
  }
+
+  inline void ContextCreated(node::inspector::ContextInfo* info);


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

unclear on what you suggest as an alternative?

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

eugeneo · 2016-10-26T20:32:13Z

src/env.cc

+    new node::inspector::ContextInfo(context(), 1, "NodeJS Main Context"));
+#endif
+
+  isolate()->SetAutorunMicrotasks(false);


Is this related to this change?

unclear how this got in here, should be removed.

eugeneo · 2016-10-26T20:34:11Z

src/env.h

@@ -564,6 +571,7 @@ class Environment {
  debugger::Agent debugger_agent_;
 #if HAVE_INSPECTOR
  inspector::Agent inspector_agent_;
+  std::vector<node::inspector::ContextInfo*> contexts_;


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

disagree since inspector isn't really usable until it starts

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

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

Trott · 2016-10-26T21:15:01Z

@bmeck The "tests and/or benchmarks are included" box is checked, but I'm not seeing any tests or benchmarks. Did they just get omitted accidentally? Or should that box not be checked?

bmeck · 2016-10-26T21:22:44Z

@Trott accident

bnoordhuis · 2016-10-30T09:32:41Z

src/env-inl.h

+      return;
+    }
+  }
+}


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

the erase needs an index not value.

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

bnoordhuis · 2016-10-30T09:33:30Z

src/env-inl.h

+    }
+  }
+}
+#endif


Can you write this as #endif // HAVE_INSPECTOR? Note the two spaces after #endif.

fixing, but copy paste only shows

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

bnoordhuis · 2016-10-30T09:34:32Z

src/env.cc

@@ -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"));


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

bnoordhuis · 2016-10-30T09:35:42Z

src/inspector_agent.cc

-    inspector_->contextCreated(
-        v8_inspector::V8ContextInfo(env->context(), 1, "NodeJS Main Context"));
+    v8::HandleScope handles(env_->isolate());
+    for (auto it : *(env->contexts())) {


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

bnoordhuis · 2016-10-30T09:37:43Z

src/inspector_agent.h

+                       context_.Reset(context->GetIsolate(), context);
+  }
+  ~ContextInfo() {
+    context_.Reset();


Not necessary, ~Persistent() resets by default.

bnoordhuis · 2016-10-30T09:39:21Z

src/inspector_agent.h

+  const char* name() const { return name_; }
+ private:
+  v8::Persistent<v8::Context> context_;
+  int groupId_;


Can you snake_case it to group_id_? If it never changes, consider making it const (same for name_.)

cpojer · 2016-11-16T04:53:57Z

So glad this is being worked on! @bmeck do you have any ETA when this will make it into a version of node? :)

bmeck · 2016-11-17T15:53:45Z

@cpojer it sounds like the inspector might need changes according to @eugeneo so we don't expose some things like manual create of context information. I'm mostly in waiting position on this PR until then.

bnoordhuis

Left some comments, nothing substantial. I think @eugeneo should do the final sign-off because he understands the inspector code better than I do.

bnoordhuis · 2016-11-18T11:04:55Z

src/env-inl.h

+      return;
+    }
+  }
+}


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

bnoordhuis · 2016-11-18T11:06:24Z

src/inspector_agent.cc

-    inspector_->contextCreated(
-        v8_inspector::V8ContextInfo(env->context(), 1, "NodeJS Main Context"));
+    v8::HandleScope handles(env_->isolate());
+    for (auto it : *env->contexts()) {


Somewhat inconsistent use of env vs. env_.

bnoordhuis · 2016-11-18T11:06:59Z

src/inspector_agent.cc

+void AgentImpl::ContextCreated(const node::inspector::ContextInfo* info) {
+  inspector_->contextCreated(info);
+}
+void AgentImpl::ContextDestroyed(v8::Local<v8::Context> context) {


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

bnoordhuis · 2016-11-18T11:07:51Z

src/inspector_agent.cc

+}
+void Agent::ContextDestroyed(v8::Local<v8::Context> context) {
+  impl->ContextDestroyed(context);
+}


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

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

bnoordhuis · 2016-11-18T11:08:16Z

src/inspector_agent.h

@@ -23,6 +25,25 @@ namespace inspector {

 class AgentImpl;

+class ContextInfo {
+ public:
+  explicit ContextInfo(v8::Local<v8::Context> context, const int groupId,


s/groupId/group_id/

bnoordhuis · 2016-11-18T11:08:44Z

src/inspector_agent.h

+  inline v8::Local<v8::Context> context(v8::Isolate* isolate) const {
+    return context_.Get(isolate);
+  }
+  int groupId() const { return group_id_; }


Ditto, idiomatic core code would spell this as group_id().

is there a style guide on this or reason why linting doesn't pick these things up?

jasnell · 2017-03-24T22:36:52Z

Ping @bmeck ... does this still make sense? Is it needed? /cc @eugeneo

bmeck · 2017-03-24T23:52:21Z

Stalled/blocked but still needed

…

On Mar 24, 2017 5:37 PM, "James M Snell" ***@***.***> wrote: Ping @bmeck <https://github.com/bmeck> ... does this still make sense? Is it needed? /cc @eugeneo <https://github.com/eugeneo> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9272 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOUozedBnlrGvn1o0ivT9QOOAlEJuyIks5rpEWkgaJpZM4KgGfw> .

bmeck · 2017-04-25T20:01:52Z

@eugeneo

Can I get updates on :

#9272 (comment)

If the platform_ comment makes sense still.

#9272 (comment)

If the asymmetry is still a blocker.

#9272 (review)

Guessing such an API is not in the works.

eugeneo · 2017-04-25T20:41:26Z

@bmeck please take a look at https://github.com/eugeneo/node/tree/contexts_crashing - this is my suggested implementation, it is very similar to yours. I had to cut a corner or two - but the real problem is that it is crashing in test/parallel/test-vm-run-in-new-context.js

eugeneo · 2017-04-25T20:47:01Z

@bmeck one thing I have been looking into (and I hope to go back to) is for the inspector to have its own WeakCallback on a context so there's no need to report to Inspector contexts going away.

gaearon · 2017-05-25T12:50:04Z

Is there any way React community could help with pushing this through? It’s affecting Jest pretty badly and I’m sure somebody could pick it up if you are busy with other things.

bmeck · 2017-05-25T13:07:27Z

@gaearon if someone wants to merge both my and @eugeneo 's work that would probably be enough.

I don't have time right now sadly T_T

fhinkel · 2017-05-26T19:14:51Z

I'm removing the stalled label. Anybody we can ping to merge these changes together? @eugeneo ?

cdaringe · 2017-06-01T16:17:36Z

Im super excited about this. I haven't written c/c++ since college, but I'd be willing to at least pair as a sounding board for anyone who can keep this going!

This enables inspector support for contexts created using the vm module. Fixes: nodejs#7593 Fixes: nodejs#12096 Refs: nodejs#9272

Based on work by Bradley Farias <[email protected]> and Eugene Ostroukhov <[email protected]>. TODO: V8's console is injected unconditionally, which may be not desirable. Fixes: nodejs#7593 Refs: nodejs#9272

jgoz · 2017-08-11T19:16:01Z

@bmeck, @eugeneo This can be closed now that #14465 is merged, right?

bnoordhuis · 2017-08-17T14:09:23Z

Yes, #14465 should have obviated this one. I'll close it out, thanks.

inspector: add contexts when using vm module

2e4b318

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: nodejs#7593

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 25, 2016

bmeck force-pushed the inspect-contexts branch from 1906d5f to 2e4b318 Compare October 25, 2016 15:05

eugeneo suggested changes Oct 25, 2016

View reviewed changes

mscdex added the inspector Issues and PRs related to the V8 inspector protocol label Oct 25, 2016

bnoordhuis requested changes Oct 25, 2016

View reviewed changes

fhinkel added the vm Issues and PRs related to the vm subsystem. label Oct 25, 2016

Fixes for reviews:

759333f

change context_created to ContextCreated on Environment create node specific class instead of exposing v8_inspector for Context tracking

eugeneo reviewed Oct 26, 2016

View reviewed changes

fix accidental line added during fixes

8f351b2

bnoordhuis requested changes Oct 30, 2016

View reviewed changes

changes for @bnoordhuis

de390f7

bnoordhuis requested changes Nov 18, 2016

View reviewed changes

damonbauer mentioned this pull request Nov 19, 2016

How to properly debug tests without using the command line debugger? jestjs/jest#2119

Closed

jkrems mentioned this pull request Mar 6, 2017

v8_inspector: Support the V8 debugger from within VM contexts #7593

Closed

jasnell added the stalled Issues and PRs that are stalled. label Mar 24, 2017

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:11

fhinkel removed the stalled Issues and PRs that are stalled. label May 26, 2017

gaearon mentioned this pull request May 26, 2017

🎨 add first class debugging support facebook/create-react-app#2041

Closed

jgoz pushed a commit to jgoz/node that referenced this pull request Jun 20, 2017

inspector: support extra contexts

a4d4715

This enables inspector support for contexts created using the vm module. Fixes: nodejs#7593 Fixes: nodejs#12096 Refs: nodejs#9272

jgoz mentioned this pull request Jun 20, 2017

inspector: support extra contexts when connected #13826

Closed

4 tasks

TimothyGu mentioned this pull request Jul 14, 2017

[wip] src, inspector: support opted-in VM contexts #14231

Closed

4 tasks

bnoordhuis closed this Aug 17, 2017

inspector: add contexts when using vm module #9272

inspector: add contexts when using vm module #9272

Conversation

bmeck commented Oct 25, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

jasnell commented Oct 25, 2016

eugeneo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmeck commented Oct 26, 2016

eugeneo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Oct 26, 2016

bmeck commented Oct 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpojer commented Nov 16, 2016

bmeck commented Nov 17, 2016

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Mar 24, 2017

bmeck commented Mar 24, 2017 via email

bmeck commented Apr 25, 2017

eugeneo commented Apr 25, 2017

eugeneo commented Apr 25, 2017

gaearon commented May 25, 2017

bmeck commented May 25, 2017

fhinkel commented May 26, 2017

cdaringe commented Jun 1, 2017

jgoz commented Aug 11, 2017

bnoordhuis commented Aug 17, 2017

bmeck commented Oct 25, 2016 •

edited

Loading