Support pausing the debugger on script load

[roblourens]

There is a Node debugging scenario that doesn't work very well in vscode (or chrome devtools), but does in debugging Chrome, and I want to start a discussion about how we can improve this.

Typically when debugging a Node project with sourcemaps, the scripts are transpiled to disk, vscode's launch config points at these files with the "outFiles" parameter, then vscode preloads the sourcemaps so it can set breakpoints in the correct locations before the scripts are actually loaded.

But there is a scenario where scripts are transpiled on demand, so vscode can't preload their sourcemaps, and can't set breakpoints at the right spots in those scripts until some time after they are loaded. This means that the breakpoints may not be hit, due to the race between running the code and setting the breakpoint at the same time. The best way for vscode to deal with this would be to have node pause execution each time a script is loaded, giving it a chance to load its sourcemaps and set the breakpoint before running the code in the script.

The Chrome devtools protocol for Chrome actually gives us a way to pause execution each time a script is loaded, via https://chromedevtools.github.io/devtools-protocol/1-3/DOMDebugger#method-setEventListenerBreakpoint. But this is in the DOMDebugger domain which doesn't exist in Node.

So what do you think it would take to get an api like this for node as well? Can nodejs implement a subset of the DOMDebugger domain, does a new domain need to be defined? Or maybe someone has an idea for another solution entirely.

[mike-kaufman]

/cc @ofrobots, @eugeneo - I think you two have context here on the CrDP protocol integration into Node?

[alexkozy]

I believe that this method should be called setInstrumentationBreakpoint and should be part of Debugger domain, so it should be implemented in V8 and can be backported to Node 10.
It could be even smarter and be something like scriptWithSourceMapParsed to reduce performance overhead.

[hashseed]

Seems like we should just expose the after-compile event to the devtools protocol?

[alexkozy]

After compile events are exposed as Debugger.scriptParsed and Debugger.scriptFailedToParse events. In this use case it is important to not just send event but to pause JavaScript execution as well to give DevTools or VSCode frontends chance to fetch source map and set breakpoints based on this source map.

[alexkozy]

DOMDebugger.setEventListenerBreakpoint for pause at first line should actually be moved all together to Debugger.setInstrumentationBreakpoint. It will help with crbug.com/724793 as well.

[hashseed]

So I guess could expose an option that can be enabled which causes the Debugger.scriptParsed event to behave like a debugger pause that needs manual resume?

[hashseed]

Do you envision this breakpoint to break at parsing/compiling, or upon execution?

[alexkozy]

I prefer to emit regular Debugger.paused event, implementation wise it should be easier, with reason - instrumentationBreakpoint and instrumentationType inside data object, it should be less 'surprising' for different protocol clients and better aligned with existing DOMDebugger behavior.
If this breakpoint is set we schedule pause on next function call using v8::debug::SetBreakOnNextFunctionCall. Ideally we should get another callback from V8 when top level function is finished to clear requested break if no javascript was executed but I am not sure that this case is possible.
I believe that this breakpoint should trigger break at first script line.

[roblourens]

Fixing http://crbug.com/724793 would be very helpful too!

I think the break should happen immediately before execution. It should happen after Debugger.scriptParsed, because we need the sourcemapURL that comes with that event. And in some cases we would need the script id to set breakpoints.

+1 for

I prefer to emit regular Debugger.paused event, implementation wise it should be easier, with reason - instrumentationBreakpoint and instrumentationType inside data object, it should be less 'surprising' for different protocol clients and better aligned with existing DOMDebugger behavior.

[hashseed]

Iirc SetBreakOnNextFunctionCall would pause before we even enter the top-level function. Sgtm.

[alexkozy]

We can actually provide sourceMappingURL and sourceURL inside data param of Debugger.paused to make it easier.

[hashseed]

Now that I think about this, triggering the pause at the after-compile event is probably more straightforward to implement. Otherwise we would have to schedule a pause and remember until that pause for what reason we paused (and what script URL to pass).

[alexkozy]

We already have infrastructure for this use case - V8DebuggerAgentImpl::schedulePauseOnNextStatement method. So we need to call this method from V8DebuggeragentImpl::didParseSource, we just need to add another flag to this method to distinguish call to this method from V8 and from V8DebuggerAgentImpl::enable method.

[alexkozy]

I uploaded patch for V8: CL. It looks good to me but definitely requires accurate review 😄

[roblourens]

That was fast! This looks great, scriptWithSourceMapParsed is a good idea and will help a ton with perf.

One question - what happens when this is enabled and there is a breakpoint on the first line of the script? Ideally, the debugger can

• Hit the instrumentation breakpoint
• Continue
• Hit the user breakpoint on (1,1)

This API in Chrome used to work that way. Then at some point the behavior changed such that if we continue from the instrumentation bp, it will skip the user bp at 1,1, so we now have to check manually whether the user set a bp there.

[mike-kaufman]

Any update here? Seems like the v8 CL is still open with some pending comments?

[mhdawson]

Kevin was going to follow up since person who opened left Google.

[alexkozy]

I addressed comments and I hope to get review and land my CL soon. It was very busy December and later I forgot about this thread - sorry!

[alexkozy]

My commit was finally landed. Test might be good starting point to see how to use this new API.
I am wondering should we backport it to Node 12, don't we?

[mhdawson]

Just waiting on a backport to Node.js 12.x and then we can close

[alexkozy]

Could someone approve backport #27720 please? :)

[github-actions]

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

[github-actions]

There has been no activity on this feature request and it is being closed. If you feel closing this issue is not the right thing to do, please leave a comment.

For more information on how the project manages feature requests, please consult the feature request management document.