inspector: --inspect-brk for ES6 modules #17360

eugeneo · 2017-11-27T22:45:10Z

Fixes: #17340

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)

ES6 module loader, test: added a hook to break on module init and a test

bmeck · 2017-11-28T00:11:24Z

Should this be during eval instead of instantiate?

eugeneo · 2017-11-28T00:14:36Z

I did not dig deep, only did manual test. What I saw was that breaking during eval will actually break later then desired - e.g. for this example (console.log) it will actually break in the getter for the console. Variable declarations will also be skipped. Breaking during initialize worked as expected.

guybedford · 2017-11-28T08:42:45Z

Nice to see this.

@eugeneo it may be worth trying some alternative wrapper functions as instantiate does sound fairly arbitrary as it does no module execution. Would job.run() at https://github.com/nodejs/node/pull/17360/files#diff-e6f2798544dae1f6ec863621e745489aR141 work perhaps?

The other thing that seems non-ideal is having that extra propery from within the ModuleJob class, it would be nice to be able to attach something from outside that way like:

initWrapper(moduleJob.run, moduleJob)

Let me know if any of these adjustments might work.

eugeneo · 2017-11-28T17:02:18Z

The main problem is that the breakpoint set from the C++ code will be hit when next JS instruction is executed. This means that we need to call this immediately before the user code is invoked. Otherwise breakpoint will be hit in the Node.js JS code and confuse/scare the user :)

E.g., replacing process.binding('inspector').callAndPauseOnStart(this.module.instantiate, this.module) with process.binding('inspector').callAndPauseOnStart(() => this.module.instantiate(), null) results in a break in the arrow function, revealing gritty implementation details.

From V8 and inspector pov, there is no difference between framework code (e.j. ModuleJob#run) and user code. But DevTools really want to break in the user code.

TimothyGu · 2017-11-28T17:36:34Z

@eugeneo Is there a way to blackbox the inspector scripts by default?

eugeneo · 2017-11-28T17:53:57Z

Is there a way to blackbox the inspector scripts by default?

Chrome does not use blackboxing in scenarios like this so it had been designed to be controlled by the front-end. Currently blackboxing is per-session and is only available through the inspector protocol. E.g. Node would have to intercept Debugger.enable call and insert a bunch of new messages into the message stream. It is doable, but it might be risky and prone to breakages and may have sideffects.

guybedford · 2017-11-28T18:59:39Z

@eugeneo thanks for the clarification, I guess I'm just completely not understanding how instantiate could be working here then, because instantiate does not do any execution, apart from initializing bindings and function declarations.

eugeneo · 2017-11-28T19:33:32Z

One of the tests I did was this script:

const a = 1;
const b = a + 1;

export default function() {
	console.log(10);
}

Breaking on initialization correctly steps over initialization of the a and b, allowing to see values. It also allows setting a breakpoint in the default export function.

bmeck · 2017-11-28T19:37:49Z

@eugeneo I think that might be because you were breaking on the top level instead of the deepest leaf. Since ESM evaluates in post order traversal, the deepest leaf evaluates first.

eugeneo · 2017-11-28T19:45:23Z

This break is supposed to break once before any code was executed. There should be no modules loaded but the main one.

bmeck · 2017-11-28T20:19:46Z

@eugeneo compiled locally and played around, I think this timing is the most consistent to how CJS --inspect-brk works. LGTM

cjihrig

LGTM with some test nits.

cjihrig · 2017-11-28T21:41:32Z