[go: up one dir, main page]

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

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

Closed
ide opened this issue Jul 7, 2016 · 24 comments
Closed

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

ide opened this issue Jul 7, 2016 · 24 comments
Assignees
Labels
feature request Issues that request new features to be added to Node.js. inspector Issues and PRs related to the V8 inspector protocol vm Issues and PRs related to the vm subsystem.

Comments

@ide
Copy link
Contributor
ide commented Jul 7, 2016

This is a feature request to support Node 6.3.0's V8 inspector capability (node --inspect) from within the vm module's contexts. For example:

vm.runInNewContext(code, sandbox, {
  inspect: 9999, // same as --inspect=9999
});

If it's not possible to make individual contexts inspectable, it'd also meet our needs to make other contexts inspectable if the parent Node process is running with --inspect. Currently code that runs in other contexts isn't debuggable (ex: debugger statements are ignored).

@ide ide changed the title v8_inspector: Support the V8 debugger from with VM contexts v8_inspector: Support the V8 debugger from within VM contexts Jul 7, 2016
@mscdex mscdex added inspector Issues and PRs related to the V8 inspector protocol vm Issues and PRs related to the vm subsystem. feature request Issues that request new features to be added to Node.js. labels Jul 7, 2016
@cpojer
Copy link
cpojer commented Oct 24, 2016

node --inspect also doesn't stop for debugger; statements.

Repro:

var vm = require('vm');
new vm.Script('debugger;').runInContext(vm.createContext());
debugger;

@ide
Copy link
Contributor Author
ide commented Oct 24, 2016

@cpojer are you running Node with the --debug-brk option? You need that so that Node waits for you to connect the Blink Inspector since debugger statements are no-ops unless there is a debugger actively attached.

@cpojer
Copy link
cpojer commented Oct 24, 2016

@ide yes, please see jestjs/jest#1652 for more details.

@jkrems
Copy link
Contributor
jkrems commented Oct 24, 2016

Quoting @bmeck (https://twitter.com/bradleymeck/status/790571493579116546):

basically you need to setup

void V8InspectorImpl::contextCreated(const V8ContextInfo& info)
properly to work w/ vm module

@bmeck
Copy link
Member
bmeck commented Oct 24, 2016

can be based upon https://github.com/bmeck/node/tree/inspect-contexts , don't really have time to figure out segfaults currently but would need tests for sure to ensure GC properly works on this.

@ofrobots ofrobots added the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label Oct 24, 2016
@ofrobots
Copy link
Contributor

The branch from @bmeck looks like a good starting point. This would be a really neat contribution.

/cc @nodejs/v8-inspector.

@bmeck
Copy link
Member
bmeck commented Oct 25, 2016

i'll take this over, it seems like there is also a bug with the inspector not being synced the contexts on connect which makes this less trivial since we need to rejig the handshaking.

@bmeck bmeck self-assigned this Oct 25, 2016
@bmeck bmeck removed the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label Oct 25, 2016
bmeck added a commit to bmeck/node that referenced this issue Oct 25, 2016
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
@oldrich-s
Copy link

Are there any news on this issue?

@jkrems
Copy link
Contributor
jkrems commented Mar 6, 2017

@czb There is an open PR (#9272), linked above. You can subscribe to it to get notified on updates. Doesn't look like there has been progress in the last few months.

@eugeneo
Copy link
Contributor
eugeneo commented Mar 6, 2017

@czb this is blocked on deprecating the legacy debugger. Only one "debugger" can connect to V8 - so either the inspector or the legacy debugger. Current node versions support starting up legacy debugger at any time by sending the signal - which means we cannot provide inspector API that are always available.

@eugeneo
Copy link
Contributor
eugeneo commented Apr 25, 2017

I spent some time trying to add context reporting to Inspector and feel like I hit the wall (for now).

The core problem is that the Node contexts do not behave in the same way as V8 Inspector (that is coming from the browser) was designed. Browser contexts lifetime is tied to likes of frames and workers so the Inspector can be notified when the context is going away (e.g. while the context is still fully alive). Node context "dies" when it is collected by GC so the Inspector in its current form cannot work with the context at that point.

What it boils down to is that this assert is triggered on GC - https://github.com/nodejs/node/blob/master/deps/v8/src/inspector/inspected-context.cc#L28 :)

There is a chance that this issue will need to be fixed upstream before the Node side can be implemented.

@bmeck do you mind me taking over this issue?

@bmeck
Copy link
Member
bmeck commented Apr 25, 2017

@eugeneo does that mean I can continue #9272 / can you comment if the problems still apply?

@eugeneo
Copy link
Contributor
eugeneo commented Apr 25, 2017

The problems still apply... What I am trying to do is similar to what you have there - only difference is that I am relying on inspector agent to always be there so there's no need to have a vector of contexts.

@bmeck
Copy link
Member
bmeck commented Apr 25, 2017

@eugeneo see comment in that PR

@tnrich
Copy link
tnrich commented May 25, 2017

Any news on this? I got here from jestjs/jest#1652 (comment)
Thank you for the hard work!

@jgoz
Copy link
jgoz commented Jun 21, 2017

Given that Node and V8-Inspector have incompatible expectations about context lifecycles (Node expects GC cleanup, while Inspector expects explicit cleanup), perhaps we can approach this issue differently.

Rather than automatically attaching every VM context to the inspector, we could expose an API for attaching/detaching manually. I'm thinking something like the following:

var inspector = require('inspector');
var vm = require('vm');

var context = vm.createContext();

inspector.attachContext(context); // this will prevent GC cleanup of 'context'
// do something with 'context'
inspector.detachContext(context); // allow GC cleanup again

The use case I had in mind was library authors who use VM contexts for executing user code (e.g., test runners) because context lifecycles are finite and well-defined: create context, attach context, execute test, detach context.

But this could also be generally useful for debugging sessions when VM contexts are involved. Inserting an inspector.attachContext statement is somewhat analogous to inserting a console.log statement to print a value during a debugging session. The difference is that the former could cause memory leaks if not cleaned up.

The documentation would need to be very clear about the potential for memory leaks, but I think the "experimental" designation on the inspector module is advantageous here. It implies "here be dragons, pay attention".

Any thoughts on this approach?

@michahell
Copy link

ping. This issue is still causing JEST tests to not trigger on debugger; statements: jestjs/jest#1652

@aschrijver
Copy link
aschrijver commented Jul 12, 2017

I just referred to this issue from stackoverflow after spending more than a day on setting up debugging, without results! FYI: https://stackoverflow.com/questions/45056952/debugging-jest-unit-tests-with-breakpoints-in-vs-code-with-react-native

@aschrijver
Copy link
aschrijver commented Jul 12, 2017

Are there any debuggers besides Chrome and VS Code that I can use to debug using V8 or should I downgrade and bide my time?

[UPDATE: I decided to downgrade to node 7 and now debugging in VS Code is a breeze! See details...]

@TimothyGu TimothyGu self-assigned this Jul 14, 2017
TimothyGu added a commit to TimothyGu/node that referenced this issue Jul 14, 2017
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
@wangxuepeng
Copy link

Downgrade to 7.10.1 to work-around the issue. Keep watching on this.

@hakunin
Copy link
hakunin commented Jul 26, 2017

@cpojer Maybe FB should donate a developer to the node project for a few months so we can get somewhere :D

@jgoz
Copy link
jgoz commented Aug 11, 2017

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

@ide
Copy link
Contributor Author
ide commented Aug 17, 2017

Closing this since Node 8.4.0 supports attaching to vm contexts out of the box.

@ide ide closed this as completed Aug 17, 2017
@jbeard4
Copy link
jbeard4 commented Aug 24, 2017

Source maps are ignored in chrome dev tools for code that is evaluated using vm.runInContext.

jcollum added a commit to jcollum/jest that referenced this issue Mar 14, 2018
I was trying to get jest debugging working and ran into issues. Switched to Node 9.7.1 and the issue went away, breakpoints were hit. Related: nodejs/node#7593
SimenB pushed a commit to jestjs/jest that referenced this issue Mar 15, 2018
* Update Troubleshooting.md

I was trying to get jest debugging working and ran into issues. Switched to Node 9.7.1 and the issue went away, breakpoints were hit. Related: nodejs/node#7593

* Update Troubleshooting.md
jbeard4 added a commit to jbeard4/SCION that referenced this issue Jun 23, 2018
Copy executionContext into global variable. This is ugly, because it
means that identically-named properties of different execution contexts
can step on each other in subsequent calls to model.prepare. This is
ok-ish, because SCION support for node --inspect is a development-only
feature. This will not affect production uses of SCION.

One day I may patch require to accept an executionContext argument, and
use vm.runInContext inside of Module._compile.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. inspector Issues and PRs related to the V8 inspector protocol vm Issues and PRs related to the vm subsystem.
Projects
None yet