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

TimothyGu · 2017-07-14T17:37:07Z

Works, doesn't crash~~, but no tests or docs at this moment~~.

V8's console is injected unconditionally upon every attachContext call, which may be not desirable.

Uses an approach inspired by @jgoz in #7593 (comment).

/cc @eugeneo @bmeck

Fixes: #7593
Refs: #9272

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

inspector

Based on work by Bradley Farias <bradley.meck@gmail.com> and Eugene Ostroukhov <eostroukhov@chromium.org>. TODO: V8's console is injected unconditionally, which may be not desirable. Fixes: nodejs#7593 Refs: nodejs#9272

jgoz · 2017-07-14T17:50:14Z

This is pretty close to a solution I started a couple days ago, but mine avoids refactoring Contextify, instead introducing a ContextFromSandbox function that is used by the inspector agent to extract a V8 context.

Anyway, I like your solution better @TimothyGu.

jgoz · 2017-07-14T17:56:28Z

@TimothyGu I'll push my branch and you can lift the test I wrote for it if you like. Same API.

Edit: https://github.com/jgoz/node/blob/inspector-attach-context/test/inspector/test-inspector-contexts.js

refack · 2017-07-14T21:32:29Z

Refs: #13295, #12243
Why can't it be streamlined into vm.runInDebugContext or vm.runInInspectorContext?

TimothyGu · 2017-07-14T22:46:33Z

@refack Debug context is going to be removed by the end of this year. I'm not sure what you are referring to by "Inspector context".

TimothyGu · 2017-07-15T11:59:47Z

@jgoz Thanks, I'll have to look into how the inspector harness works, and your script is a good place to start!

refack · 2017-07-15T12:47:25Z

@refack Debug context is going to be removed by the end of this year. I'm not sure what you are referring to by "Inspector context".

One could argue that instead of removing vm.runInDebugContext we could fix/reimplement based on the mechanism in this PR.
Alternatively you could create the Inspector variant and name it vm.runInInspectorContext and it will be a wrapper that encapsulates the logic needed.

More specifically my question is does the two new endpoint have any use outside of vm associated operations? If not maybe they should be internal/ and published only as vm.context methods?

module.exports = {
    open: (port, host, wait) => open(port, host, !!wait),
    close: process._debugEnd,
    url: url,		    url: url,
 +  Session,
 +  attachContext,
 +  detachContext
  };

Which leads me to the (trivial) observation that this PR would be easier to review if you provided a draft doc and/or some tests... 🤷‍♂️

jgoz · 2017-07-15T17:55:03Z

@refack

One could argue that instead of removing vm.runInDebugContext we could fix/reimplement based on the mechanism in this PR.

Alternatively you could create the Inspector variant and name it vm.runInInspectorContext and it will be a wrapper that encapsulates the logic needed.

More specifically my question is does the two new endpoint have any use outside of vm associated operations? If not maybe they should be internal/ and published only as vm.context methods?

Unless there is another way to create V8 contexts in Node, I think you are correct that these are only useful for vm operations. My original reasoning for adding to the inspector API, rather than vm was as follows:

Avoid coupling vm to inspector/current debugging subsystem (and similarly, avoid future deprecations such as with runInDebugContext)
inspector API is currently designated "unstable", so if a better solution becomes possible (e.g., via support from the V8 team), deprecating this API will not be as painful

Something worth testing would be calling attach at the start of runInContext and then detach at the end. If this allowed debugging during execution of a script while allowing proper GC cleanup of contexts as well as re-use of contexts, it might be a better solution (possibly controlled with an opt-in or opt-out flag).

refack · 2017-07-15T18:42:36Z

Avoid coupling vm to inspector/current debugging subsystem (and similarly, avoid future deprecations such as with runInDebugContext)

inspector API is currently designated "unstable", so if a better solution becomes possible (e.g., via support from the V8 team), deprecating this API will not be as painful

Something worth testing would be calling attach at the start of runInContext and then detach at the end. If this allowed debugging during execution of a script while allowing proper GC cleanup of contexts as well as re-use of contexts, it might be a better solution (possibly controlled with an opt-in or opt-out flag).

A dilemma indeed...

I have some weird ideas like adding an implementation module arg to runInDebugContext so it'll be:

runInDebugContext(code, require('inspector'));

Or adding a scope method to inspector so:

require('inspector').runInScope(context);

Not recommended for multiple runs on the same context.

TimothyGu · 2017-07-17T07:36:21Z

@jgoz That would actually work, but only synchronously. That's still a better default behavior though, so I implemented it and added a big caveat in the documentation.

I've added tests and docs, but still need to figure out the console issue.

As I was talking to @addaleax at JSConf China, when contextCreated is called, V8 unconditionally sets the sandbox's console property to its internal console. This is possibly undesirable for most users of the VM module.

On our side I foresee three options:

ignore Do not make any changes to the sandbox
merge Merge the methods so that inspector and original methods are both called
replace Replace whatever console avaiable in the sandbox with the Inspector version

All three are implementable right now, but the first two are difficult to implement w/o side effects. I would prefer it if V8 adds a flag to disable this feature. I'm working on writing a CL to V8, but Google's custom tools require a bit of a setup.

TimothyGu · 2017-07-17T09:41:04Z

Unfortunately, the console issue is complicated by v8/v8@54271c2.

To @ak239, @hashseed, and the rest of @nodejs/v8:

We are trying to add Inspector support for contexts created through Node.js's VM module. The way we do that can be roughly illustrated as:

const inspector = require('inspector');
const vm = require('vm');
const sandbox = {
  customProp: 42
};
vm.createContext(sandbox);
inspector.attachContext(sandbox);
vm.runInContext('newProp = customProp + 1;', sandbox);
console.log(sandbox);
// Prints { customProp: 42, newProp: 43 }

Now, V8's contextCreated callback unconditionally initializes the global object's console property to an Inspector-aware version, but this new console property would either be added to sandbox even though the user never requested it to be added, or worse, overwrite an existing sandbox.console before the inspector.attachContext call.

This is not really a problem for the main (top) context, as we already provide a global console variable anyway, and we can just merge our console methods with V8's. But it is a problem for VM contexts, whose properties can always be manipulated outside the context.

In the V8 version we currently have, the console setting is done by a context->Global()->Set, so it is reasonably easy to revert that change just for VM. But after v8/v8@54271c2, it is practically impossible to restore the original state.

Please offer some ideas about how we can and should proceed re. this issue. Thanks!

refack · 2017-07-15T12:41:50Z

src/inspector_agent.cc

@@ -43,6 +48,10 @@ using v8_inspector::V8Inspector;
 static uv_sem_t start_io_thread_semaphore;
 static uv_async_t start_io_thread_async;

+// Used in NodeInspectorClient::currentTimeMS() below.
+const int NANOS_PER_MSEC = 1000000;


I've you just moved this line, but maybe do const double NANOS_PER_MSEC = 1E6; since AFAICT it's only used for double arithmetic.

refack

High level LGTM
I'm still -0.5 on increasing the surface area of the public inspector module.

refack · 2017-07-17T12:31:29Z

In the V8 version we currently have, the console setting is done by a context->Global()->Set, so it is reasonably easy to revert that change just for VM. But after v8/v8@54271c2, it is practically impossible to restore the original state.

Please offer some ideas about how we can and should proceed re. this issue. Thanks!

The global assignment line line from v8/v8@54271c2:

JSObject::AddProperty(global, name, console, DONT_ENUM);

AFAICT the property is non enumerable so console.log(sandbox); should not show it. It's also configurable so we could delete global.console

V8 6.1 in Chrome Canary:

TimothyGu · 2017-07-17T13:22:07Z

Okay, the enumerable bit notwithstanding, sandbox might already have a console property defined in a different way. It could be an accessor property, for example, and setting it in any way could wreck hazard because of the side effects of the setter. Plus, one would have to save the property descriptor before going into V8 to actually restore to the correct value, and even though that's still not too complex it would be exponentially so if the console property is on the [[Prototype]] of the sandbox object, or if sandbox is a Proxy, etc.

refack · 2017-07-17T13:46:07Z

Okay, the enumerable bit notwithstanding, sandbox might already have a console property defined in a different way. It could be an accessor property, for example, and setting it in any way could wreck hazard because of the side effects of the setter. Plus, one would have to save the property descriptor before going into V8 to actually restore to the correct value, and even though that's still not too complex it would be exponentially so if the console property is on the [[Prototype]] of the sandbox object, or if sandbox is a Proxy, etc.

Ack. Having the ability to flag V8 not to add it, is best.

hashseed · 2017-07-17T13:55:22Z

Deleting the console property after creating the context sounds like the simplest solution. You could also get it removed in the snapshot by deleting it before creating the snapshot, but then it will be missing in the main context. That could be solved by actually serializing two separate contexts, but that adds to the snapshot size...

V8 includes the console object as builtin precisely to be able to take advantage of the snapshot.

jasnell · 2017-07-17T16:28:45Z

doc/api/vm.md

+(if support for inspector is available in the Node.js build). If not, and if
+`doNotInformInspector` is not specified, this function attaches it to inspector
+using [`inspector.attachContext()`][], and detaches it before returning.
+However, *it is still recommended that you handle context attachment and


Please avoid the use of you in the docs :-)

jasnell · 2017-07-17T16:29:13Z

doc/api/vm.md

+(if support for inspector is available in the Node.js build). If not, and if
+`doNotInformInspector` is not specified, this function attaches it to inspector
+using [`inspector.attachContext()`][], and detaches it before returning.
+However, *it is still recommended that you handle context attachment and


Here too... please avoid you

refack · 2017-07-17T16:31:34Z

@hashseed what would be the implications of adding:

   Handle<JSObject> console = factory->NewJSObject(cons, TENURED);
   DCHECK(console->IsJSObject());
+ if (!global.has(name)) {
   JSObject::AddProperty(global, name, console, DONT_ENUM);
+ }

hashseed · 2017-07-17T16:54:23Z

It doesn't work. We only create one context to capture snapshot from, which acts as template for all contexts.

The sandboxed object is not even copied into the global object, but hooked into it via interceptor, right?

We do have a step where we copy over all properties from the deserialized global object into the global object created from object template. Maybe we can hook that into that place. Iirc the interceptor is disabled during bootstrapping though?

@fhinkel

jgoz · 2017-07-17T17:23:24Z

doc/api/vm.md



 Runs the compiled code contained by the `vm.Script` object within the given
 `contextifiedSandbox` and returns the result. Running code does not have access
 to local scope.

+By default, this function checks if inspector was already aware of the context
+(if support for inspector is available in the Node.js build). If not, and if
+`doNotInformInspector` is not specified, this function attaches it to inspector


Lots of negatives in here, which might get confusing. How about something like this:

This function will attach the context to inspector (in Node.js builds with inspector support) using [inspector.attachContext()][] before running the script and detach it with [inspector.detachContext()][] before returning. The context will only be attached if inspector was not already aware of the context, or if doNotInformInspector is unspecified or false. Note that it is recommended to handle context attachment and detachment manually, since:

jgoz · 2017-07-17T17:26:28Z

doc/api/vm.md


 The `vm.runInContext()` method compiles `code`, runs it within the context of
 the `contextifiedSandbox`, then returns the result. Running code does not have
 access to the local scope. The `contextifiedSandbox` object *must* have been
 previously [contextified][] using the [`vm.createContext()`][] method.

+By default, this function checks if inspector was already aware of the context
+(if support for inspector is available in the Node.js build). If not, and if
+`doNotInformInspector` is not specified, this function attaches it to inspector


Same recommended rewording as my above comment.

jgoz · 2017-07-17T17:32:42Z

@TimothyGu Given the caveats you outlined in the documentation for automatic attachment/detachment in vm.runInContext, would it make more sense for this behaviour to be opt-in, rather than opt-out? I realize that doesn't offer an optimal/"just works" debugging experience, but (especially with the async caveat) the current opt-out behaviour is not optimal either.

eugeneo

I strongly feel like this is not a proper fix as this really needs to be fixed in V8... What will happen to this new API if/when the bug is fixed in V8?

eugeneo · 2017-07-17T22:10:07Z

src/inspector_agent.h

@@ -102,6 +137,7 @@ class Agent {
  std::unique_ptr<NodeInspectorClient> client_;
  std::unique_ptr<InspectorIo> io_;
  v8::Platform* platform_;
+  std::vector<const node::inspector::ContextInfo*> contexts_;


Please move this to NodeInspectorClient

eugeneo · 2017-07-17T22:11:09Z

src/inspector_socket_server.cc

-    session->server_->MessageReceived(session->id_,
-                                      std::string(buf->base, read));
+    std::string str(buf->base, read);
+#if DEBUG_TRANSACTION


Please remove this before committing.

alexkozy · 2017-07-17T23:06:32Z

I think we need to fix it on inspector side and allow V8 embedders not to call contextDestroyed and then we can just call contextCreated when context is created in Node.js.
CL: https://chromium-review.googlesource.com/c/575519

TimothyGu · 2017-07-18T00:35:37Z

@eugeneo, if you guys can fix it on the V8 side -- even better! We still need some sort of API to provide context name (since vm Module Context 56 isn't exactly clear on what it actually is) and origin info (for use cases like jsdom if they want to have first class support in inspector; /cc @domenic), though that can be done in vm.createContext().

@hashseed Right now it is actually both copied and intercepted. Because the copying is done synchronously, async in VM contexts is already not supported perfectly so I'd be fine with leaving it that way (/cc @jgoz). In fact, this copying step has led to a lot of problems. @AnnaMag is doing some work to rectify that situation in #13265. And yes, the interceptors are supposed to be disabled during bootstrapping.

AnnaMag · 2017-07-18T20:09:54Z

@hashseed, @TimothyGu Sandbox object is intercepted, while the copying step happens after and is a filler only for the own properties (in the global context) that were not set on the sandbox via the interceptors.
I believe that #13265 will fix the async issues in VM (am I right about it @fhinkel ?). The patch is on hold pending V8 changes.

eugeneo · 2017-07-18T21:23:31Z

I would appreciate if you checked out https://github.com/eugeneo/node/tree/multicontext-v8-patch, as an alternative to that patch.

That branch includes a pending V8 change backported to the Node. I am testing right now, looks like it mostly works - e.g. no crashes and some context are even getting garbage collected... Remaining issues are:

Console is always injected into new contexts (this is V8 behavior, not sure what could be done on the Node side).
Contexts are retained if "console.log" or "debugger" statements are executed.

I am planning to start a new PR if we manage to resolve issue #2 and if the V8 changes get landed into that tree.

TimothyGu · 2017-07-23T15:16:20Z

@eugeneo I'd be happy to defer to your branch if the V8 issues are resolved.

TimothyGu · 2017-08-10T09:15:46Z

Closed in favor of #14465.

The `auxData` field is not exposed to JavaScript, as DevTools uses it for its `isDefault` parameter, which is implemented faithfully, contributing to the nice indentation in the context selection panel. Without the indentation, when `Target` domain gets implemented (along with a single Inspector for cluster) in #16627, subprocesses and VM contexts will be mixed up, causing confusion. PR-URL: #17720 Refs: #14231 (comment) Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: James M Snell <jasnell@gmail.com>

TimothyGu added 2 commits July 15, 2017 00:05

src: refactor contextify

ff88cf4

[WIP] src, inspector: support opted-in VM contexts

9f12ee8

Based on work by Bradley Farias <bradley.meck@gmail.com> and Eugene Ostroukhov <eostroukhov@chromium.org>. TODO: V8's console is injected unconditionally, which may be not desirable. Fixes: nodejs#7593 Refs: nodejs#9272

TimothyGu requested review from bmeck and eugeneo July 14, 2017 17:37

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jul 14, 2017

ignore already tracked contexts

a08b03e

TimothyGu added 9 commits July 17, 2017 13:20

src: add a macro for debugging inspector WS

2d9272c

test: add some tools to common

f1ca818

bug fixes

a8ebfb5

tests

fe7c4a5

contextAttached

0352a48

contextAttached tests

06f635b

auto attachment from runInContext

fc7d408

Not recommended for multiple runs on the same context.

Add docs

a0c6f68

vm: support binding a script to a context

15a8ff8

refack reviewed Jul 17, 2017

View reviewed changes

jasnell reviewed Jul 17, 2017

View reviewed changes

jgoz reviewed Jul 17, 2017

View reviewed changes

eugeneo reviewed Jul 17, 2017

View reviewed changes

eugeneo mentioned this pull request Jul 25, 2017

inspector: support non-main contexts #14465

Merged

3 tasks

TimothyGu mentioned this pull request Aug 1, 2017

Report async tasks to V8 Inspector #13870

Closed

4 tasks

TimothyGu closed this Aug 10, 2017

TimothyGu added inspector Issues and PRs related to the V8 inspector protocol vm Issues and PRs related to the vm subsystem. labels Dec 17, 2017

TimothyGu deleted the vm-inspector branch December 17, 2017 21:57

TimothyGu mentioned this pull request Dec 17, 2017

vm: allow modifying context name in inspector #17720

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

TimothyGu commented Jul 14, 2017 •

edited

Loading

jgoz commented Jul 14, 2017

jgoz commented Jul 14, 2017 •

edited

Loading

refack commented Jul 14, 2017 •

edited

Loading

TimothyGu commented Jul 14, 2017

TimothyGu commented Jul 15, 2017

refack commented Jul 15, 2017 •

edited

Loading

jgoz commented Jul 15, 2017

refack commented Jul 15, 2017 •

edited

Loading

TimothyGu commented Jul 17, 2017

TimothyGu commented Jul 17, 2017

refack Jul 15, 2017

refack left a comment

refack commented Jul 17, 2017

TimothyGu commented Jul 17, 2017

refack commented Jul 17, 2017

hashseed commented Jul 17, 2017

jasnell Jul 17, 2017

jasnell Jul 17, 2017

refack commented Jul 17, 2017 •

edited by gibfahn

Loading

hashseed commented Jul 17, 2017

jgoz Jul 17, 2017

jgoz Jul 17, 2017

jgoz commented Jul 17, 2017

eugeneo left a comment

eugeneo Jul 17, 2017

eugeneo Jul 17, 2017

alexkozy commented Jul 17, 2017

TimothyGu commented Jul 18, 2017

AnnaMag commented Jul 18, 2017 •

edited

Loading

eugeneo commented Jul 18, 2017

TimothyGu commented Jul 23, 2017

TimothyGu commented Aug 10, 2017

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

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

Conversation

TimothyGu commented Jul 14, 2017 • edited Loading

Checklist

Affected core subsystem(s)

jgoz commented Jul 14, 2017

jgoz commented Jul 14, 2017 • edited Loading

refack commented Jul 14, 2017 • edited Loading

TimothyGu commented Jul 14, 2017

TimothyGu commented Jul 15, 2017

refack commented Jul 15, 2017 • edited Loading

jgoz commented Jul 15, 2017

refack commented Jul 15, 2017 • edited Loading

TimothyGu commented Jul 17, 2017

TimothyGu commented Jul 17, 2017

refack Jul 15, 2017

Choose a reason for hiding this comment

refack left a comment

Choose a reason for hiding this comment

refack commented Jul 17, 2017

TimothyGu commented Jul 17, 2017

refack commented Jul 17, 2017

hashseed commented Jul 17, 2017

jasnell Jul 17, 2017

Choose a reason for hiding this comment

jasnell Jul 17, 2017

Choose a reason for hiding this comment

refack commented Jul 17, 2017 • edited by gibfahn Loading

hashseed commented Jul 17, 2017

jgoz Jul 17, 2017

Choose a reason for hiding this comment

jgoz Jul 17, 2017

Choose a reason for hiding this comment

jgoz commented Jul 17, 2017

eugeneo left a comment

Choose a reason for hiding this comment

eugeneo Jul 17, 2017

Choose a reason for hiding this comment

eugeneo Jul 17, 2017

Choose a reason for hiding this comment

alexkozy commented Jul 17, 2017

TimothyGu commented Jul 18, 2017

AnnaMag commented Jul 18, 2017 • edited Loading

eugeneo commented Jul 18, 2017

TimothyGu commented Jul 23, 2017

TimothyGu commented Aug 10, 2017

TimothyGu commented Jul 14, 2017 •

edited

Loading

jgoz commented Jul 14, 2017 •

edited

Loading

refack commented Jul 14, 2017 •

edited

Loading

refack commented Jul 15, 2017 •

edited

Loading

refack commented Jul 15, 2017 •

edited

Loading

refack commented Jul 17, 2017 •

edited by gibfahn

Loading

AnnaMag commented Jul 18, 2017 •

edited

Loading