Node: Node.js' internal scripts should be hidden in the inspector.

[hashseed]

Node.js' internal scripts are not distinguished from user scripts. So when debugging a user script, the inspector is polluted with these internal scripts.

This could be solved either by blackboxing internal scripts by default, or setting a different script kind in V8 internally for these scripts. The latter probably requires an API change in V8.

@ak239 @ofrobots

[bmeck]

@hashseed latter was marked as wontfix a while ago: https://bugs.chromium.org/p/v8/issues/detail?id=1574

[gireeshpunathil]

@hashseed - while (live) debugging a problem at hand, what would be the chances that the user want to focus only on his code? I guess an average program will be a blend of API invocations, thirdparty modules and the user's own code, and the scope of the investigation could span across and thereby equally applicable to all?

When you say blackboxing code regions, do you mean not to provide debug control over those regions, or hiding them from the view alone, or both?

[Fishrock123]

I would at least like to still be able to set some option to expose core code for... core debugging purposes.

[jasnell]

I use inspector personally to help debug core stuff from time to time. Hiding these would not be ideal. Perhaps if there was a better way of organizing them, that would be better.

[hashseed]

Blackboxing by default would still allow the blackbox pattern to be changed to reveal internal scripts.

[bnoordhuis]

That's the setBlackboxPatterns protocol command, correct? Is there a way to distinguish between a built-in script foo.js and one created with new vm.Script(code, { filename: 'foo.js' })?

[hashseed]

I haven't verified, but the command seems correct. Internal scripts have the same script type as normal ones, so I guess by filename alone makes it not fool proof. But maybe having special file names, e.g. node-internal://module.js is sufficient?

[joyeecheung]

I think blackboxing by default is probably enough(it's what people do when writing frontend code with jQuery and stuff), even users can track down a core bug with it if they have the time & patience, that would be very useful for bug reports.

[Trott]

This should remain open?

[bmeck]

I think so

On Jul 29, 2017 11:32 PM, "Rich Trott" [e-mail] wrote:

This should remain open?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/nodejs/node/issues/11893#issuecomment-318877635, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAOUo0zppPdp4ijs_p6nkdyxjC23VdrWks5sTAdHgaJpZM4MgSQm
.

[refack]

@eugeneo @sam-github @jkrems I'd be happy to tackle this, but would appreciate a starting point.
(/cc @segrey @ulitink how do you handle these?)

[jkrems]

I'm not sure I'm a fan of hiding them. I personally treat those scripts just like any other "3rd party" code. It seems fairly arbitrary to hide require('fs') but not require('fs-extra') (yes, there are implementation details that make those two very different - but as a user they are just two modules I happen to use).

As for helpful reactions: I don't think I was even aware of setBlackboxPatterns so others might be of more help. :)

[jasnell]

I'd be happy with blackboxing the Node.js internal scripts by default but would definitely want the option of switching that off by default.

[TimothyGu]

@hashseed Is the Inspector command the only way to get to this feature? Is it possible to do this from C++'s V8Inspector, which would make it significantly easier?

[Trott]

There hasn't seen a comment on this in over a year, so I'm going to close this. Feel free to re-open if it's still something that we want to do. Or (probably better) leave it closed but add it to the feature requests project if it's unlikely to have anyone working on it soon.

FWIW, I'm with @jkrems on this one: Unless there's evidence that this is causing confusion or difficulty for end users, I'd prefer we expose core code in the debugger rather than blackbox it. I suspect (but don't know) that @addaleax might feel the same.

[bcoe]

@Trott @hashseed we're actually leaning on the fact in https://github.com/nodejs/node/pull/25157 that we can cover internal modules for Node.js' coverage; heads up if we do decide to hide these modules from the inspector.

[joyeecheung]

I think blackboxing them by default would be better than hiding them outright - it's probably orthogonal to the coverage if the only behavior change is being hidden by default in the DevTools UI.

I am reopening this. Happy to take look at this now that the builtin modules are all compiled from C++ in one central API.

[joyeecheung]

I gave a try at this in https://github.com/joyeecheung/node/tree/blackbox by prefixing the resource names of internal scripts with node-internal://, that approach does work well with the any inspector client that sends the setBlackboxPatterns command (e.g. Chrome devtools or the test I added, you no longer see them in the Devtools UI by default and will not step into them unless you remove the blackbox pattern in the setting), but that result in observable changes in the error stack traces as well - I am not sure if that is acceptable. It becomes something like:

/Users/joyee/projects/node/error.js:1
(function (exports, require, module, __filename, __dirname) { throw new Error('test');
                                                              ^

Error: test
    at Object.<anonymous> (/Users/joyee/projects/node/error.js:1:69)
    at Module._compile (node-internal://internal/modules/cjs/loader.js:718:30)
    at Object.Module._extensions..js (node-internal://internal/modules/cjs/loader.js:729:10)
    at Module.load (node-internal://internal/modules/cjs/loader.js:617:32)
    at tryModuleLoad (node-internal://internal/modules/cjs/loader.js:560:12)
    at Function.Module._load (node-internal://internal/modules/cjs/loader.js:552:3)
    at Function.Module.runMain (node-internal://internal/modules/cjs/loader.js:771:12)
    at executeUserCode (node-internal://internal/bootstrap/node.js:435:15)
    at startExecution (node-internal://internal/bootstrap/node.js:372:3)
    at startup (node-internal://internal/bootstrap/node.js:306:3)

(Personally this looks odd/inconsistent to me unless we also prefix the file names with file://, but that would be even more breaking)

It is possible to fix up the resource names and restore them to what they use to be when producing the error stack traces, but I'd prefer not to hack on the current code but build on some refactoring like https://github.com/nodejs/node/pull/23926

So my current plan is to see if I can help making any progress on https://github.com/nodejs/node/pull/23926 first.

[piranna]

I like the node-internal:// idea :-) How is it done with URL-based imports? Does they have an http:// scheme? Regarding local files I don't think it's needed to add a file:// scheme since it's the default one...

[joyeecheung]

@piranna If you are talking about ESM import, those are not related to this issue - this is only for core modules which are not ESM, and Node core does not support loading modules from remote either (without custom loaders) so I think http:// is out of scope as well.

I don't think it's needed to add a file:// scheme since it's the default one.

What do you mean by it's the default one? ScriptOrigin? Any other API related to the script compiler? There is no default protocol in any of those as far as I know, at least at the moment in Node core all the compilation calls into V8 do not use any protocol in the ScriptOrigin so those resource names are not even URLs. There isn't a default protocol in the URL spec either.

[piranna]

Yes, I was talking about ESM modules, how are they shown when they are imported from a remote location? Using a scheme? By default I means until ESM all modules has been get from the local filesystem, so I find it like a sane default...

[joyeecheung]

@piranna That's separate from this thread, core modules are not compiled in the same way as user land modules, and they are not ESM.

By default I means until ESM all modules has been get from the local filesystem, so I find it like a sane default...

I don't think the default resides in the URL protocol? If we were to add protocols and make the resource names proper URLs, that protocol should come from the module loader and reflect where the source comes from, not from the compiler - this thread is talking specifically about the compiler of core CJS modules, whose source are built into the binary and are not loaded from anywhere external.

[BridgeAR]

@joyeecheung now that we have better control over prepareStackTrace it should be relatively straight forward to remove the added part from the stack traces again.

[joyeecheung]

@BridgeAR Yes, though I just learned that ESM has a different scheme..I will need to investigate a bit and see what's good for both sides.

[joyeecheung]

I now realize that fixing up the file names for error stack traces is trickier than I thought when the Error.prepareStacktrace hook is used by the user. We then run in to a situation similar to the one faced by the source map support - we could override the .getFilename() method of the call sites though, but it seems a bit fragile..

[BridgeAR]

@joyeecheung if I am not mistaken, it should also be possible to have at least one implementation that hides the internal frames in case Error.prepareStacktrace is not used. That way we would probably already fix the "problem" for most people.

[BridgeAR]

Looking at the other case could be done afterwards as a separate step.

[mmarchini]

Can we keep node-internal:// in the stack trace as well? It's a breaking change but it looks like a welcome one, as it makes it very explicit that those scripts are coming from Node.js and not a third party dependency.

[jkrems]

If we expose it in the stack trace, should we call it node://module since that's now a documented way to import them from modules? It seems unfortunate if we end up showing node-internal://module.js in stack traces but allow people to import 'node://module'.

[mmarchini]

Oh, import 'node://module' is a thing now? When did it land?

I do agree if that's a thing now we should follow the same pattern and have node://module in the stack trace.

[bmeck]

@mmarchini after a few minor back and forths policies and import have converged recently, docs just landed in https://github.com/nodejs/node/pull/35434

[mmarchini]

oh that's node:module and not node://module, but in the end it doesn't matter as we can use it as well here :)

[bmeck]

Yeah, historically // is signalling that a sche has relative url strings
resolve against it ( ./ , ../ , etc. ). Only file: and http(s): do that
still in the WHATWG sense

On Fri, Oct 2, 2020, 4:10 PM mary marchini [e-mail] wrote:

oh that's node:module and not node://module, but in the end it doesn't
matter as we can use it as well here :)

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/nodejs/node/issues/11893#issuecomment-702959238, or
unsubscribe
https://github.com/notifications/unsubscribe-auth/AABZJI574VJI2WIAQELE4VTSIY6TXANCNFSM4DEBEQTA
.

[joyeecheung]

So would it be acceptable to make the builtin modules compiled with node:module as their names? (Those won't be done at the module loader level but instead they will just be names we tell V8 to use in stack traces and inspector commands, and yes as mentioned in https://github.com/nodejs/node/issues/11893#issuecomment-702871172 this will be breaking)

[piranna]

OffTopic:

Yeah, historically // is signalling that a sche has relative url strings resolve against it ( ./ , ../ , etc. ). Only file: and http(s): do that still in the WHATWG sense

And in fact, Tim Berners Lee told in an interview that the // for http was an historical error, because he originally conceived that servers domains would be "mounted" on a higher level agregation (http:/whatever/domain) and you could be able move up and navigate between servers on this upper level agregation and also mount it in your own computer as another filesystem, and if he could he would remove them now, but the // has became an identity of the web and mostly should not be considered : as the schema-domain separator as it's defined in the URL format specs, but instead :// when regarding web schemas (http, https, ws, wss... and don't know if any others :-P).

Take in mind we are talking early 90's, where Gopher and BBSs were the standard hypertext protocol at that time and there were no web search engines, so maybe he was thinking about agregating domains on topics like news, games, software, xxx... similar to how Usenet categories are organized :-) That's why for file: it still does makes sense, because file servers can still be agregated in a higher level structure, like SaMBa organizations and similar things :-)

[mmarchini]

@joyeecheung it's acceptable IMO. It will also make parsing stack traces (both on errors and profiles) easier to find Node.js built-in modules.

[targos]

PR at https://github.com/nodejs/node/pull/35498

[gireeshpunathil]

should this remain open? I see the linked PR (for qualifying core modules) is landed, but not sure if it fully addresses the original proposal (hiding core modules from inspector).

[joyeecheung]

Maybe something can be done from DevTool's side to hide these scripts by default? Though I am not sure if that's desired behavior. We should post something to teach users how to make use of this feature to blackbox the internal scripts, though.

[mmarchini]

Can this be implemented on Node.js by running an inspector command to blackbox internal scripts when DevTools connects to the agent?

[joyeecheung]

Maybe, we can send

{
  "method": "Debugger.setBlackboxPatterns",
  "pattern": { "patterns": ["^node:"] }
}

to the inspector, though we also need to send Debugger.enable first

[mmarchini]

We had a call to volunteers in the diag meeting today, so I'll remove from the agenda.