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

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

Open
hashseed opened this issue Mar 17, 2017 · 54 comments
Open

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

hashseed opened this issue Mar 17, 2017 · 54 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 module Issues and PRs related to the module subsystem.

Comments

@hashseed
Copy link
Member

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

screenshot from 2017-03-17 09 02 09

@mscdex mscdex added inspector Issues and PRs related to the V8 inspector protocol module Issues and PRs related to the module subsystem. labels Mar 17, 2017
@bmeck
Copy link
Member

bmeck commented Mar 17, 2017

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

@gireeshpunathil
Copy link
Member

@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
Copy link
Contributor

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

@jasnell
Copy link
Member

jasnell commented Mar 17, 2017

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
Copy link
Member Author

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

@bnoordhuis
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

joyeecheung commented Mar 20, 2017

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
Copy link
Member

Trott commented Jul 30, 2017

This should remain open?

@bmeck
Copy link
Member

bmeck commented Jul 30, 2017 via email

@refack
Copy link
Contributor

refack commented Jul 30, 2017

@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?)

@refack refack self-assigned this Jul 30, 2017
@jkrems
Copy link
Contributor

jkrems commented Jul 30, 2017

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
Copy link
Member

jasnell commented Jul 31, 2017

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
Copy link
Member

@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
Copy link
Member

Trott commented Dec 24, 2018

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.

@Trott Trott closed this as completed Dec 24, 2018
@bcoe
Copy link
Contributor

bcoe commented Dec 24, 2018

@Trott @hashseed we're actually leaning on the fact in #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
Copy link
Member

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 joyeecheung reopened this Dec 24, 2018
@joyeecheung joyeecheung self-assigned this Dec 24, 2018
@joyeecheung
Copy link
Member

joyeecheung commented Jan 8, 2019

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 #23926

So my current plan is to see if I can help making any progress on #23926 first.

@piranna
Copy link
Contributor

piranna commented Jan 8, 2019

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
Copy link
Member

joyeecheung commented Jan 8, 2019

@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
Copy link
Contributor

piranna commented Jan 8, 2019

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
Copy link
Member

joyeecheung commented Jan 8, 2019

@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.

@mmarchini
Copy link
Contributor

@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 added a commit to targos/node that referenced this issue Oct 4, 2020
This change allows for easier recognition of builtin modules in stack
traces.

Refs: nodejs#11893
@targos
Copy link
Member

targos commented Oct 4, 2020

PR at #35498

targos added a commit that referenced this issue Oct 7, 2020
This change allows for easier recognition of builtin modules in stack
traces.

Refs: #11893

PR-URL: #35498
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
@gireeshpunathil
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor

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

@joyeecheung
Copy link
Member

joyeecheung commented Nov 4, 2020

Maybe, we can send

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

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

@mmarchini
Copy link
Contributor

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

@mmarchini mmarchini removed the diag-agenda Issues and PRs to discuss during the meetings of the diagnostics working group. label Nov 25, 2020
@RafaelGSS
Copy link
Member

Hey!

I've taken a look at it today and I wondering if it should be done either on the Node.js or Dev tools. Based on the message above seems that we just need to send this message through WebSocket on the debugger startup.

Besides sending the pattern via DevTools, would be good to change it in the inspector as well, I think here specifically.

Not sure if I understood correctly, but if yes, I can help as I can on node-inspect.

joesepi pushed a commit to joesepi/node that referenced this issue Jan 8, 2021
This change allows for easier recognition of builtin modules in stack
traces.

Refs: nodejs#11893

PR-URL: nodejs#35498
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
@RedYetiDev
Copy link
Member

It's been a few years since any activity on this issue, but I'd like to give my opinion given the amount of input it's received.

I'm firmly against this, as the internal scripts are extremely helpful in the inspector.

When debugging a new feature (or fixing an old one) in Node.js, I find it very helpful to see where in the internal systems everything is happening.


From a triage perspective, there hasn't been any activity on this issue in a while, and if it's not going to be pursued, please close it. Nonetheless, your efforts are appreciated, and we are happy to review future issues :-).

@joyeecheung
Copy link
Member

joyeecheung commented Apr 22, 2024

Personally I would still be happy to see this option implemented in the inspector clients (well, the sever can send the commands if necessary) especially for the async hooks - since DevTools sets maximum async stack depth by default (maybe making that optional would be another great feature), not black boxing the internal scripts means that step-debugging every Promise or async/await would get you tripped on the promise hooks several times and it’s quite annoying (basically, what #36022 was about). I have to blackbox the scripts manually using the right click menu whenever I need to debug async code.

@breautek
Copy link

Just wanted to add that #36022 was decided to be closed in favour of this issue. The issue isn't resolved, we just thought it would make more sense to bring the discussion under one thread since what #36022 described is really caused by this issue.

I have simple reproduction steps that demonstrates the issue on v22.5.1 at #36022 (comment). It also demonstrates that the behaviour is exclusive to NodeJS environments. Debugging the same code in a Chrome browser environment for example does not have these "internal" js files that gets stepped into.

I'm not sure if the blockbox idea is still being considered but it sounds like it would address this issue. To summarise my thoughts, I'm sure there are use cases for stepping into node internals, but those use cases are likely limited to NodeJS contributors developing NodeJS. Most users trying to debug their applications likely aren't interested in what the internals is doing, and is more interested what their app-code or potentially imported user libraries are doing.

So given that, I think it would make sense if by default NodeJS will ignore debugging any node internal js files, and perhaps a flag to enable debugging internal js files for NodeJS contributors and/or anyone with an edge case who wants to step into nodejs internals.

Just expressing my opinions as an user of NodeJS.

@RedYetiDev
Copy link
Member

So given that, I think it would make sense if by default NodeJS will ignore debugging any node internal js files, and perhaps a flag to enable debugging internal js files for NodeJS contributors and/or anyone with an edge case who wants to step into nodejs internals.

Or perhaps a flag to disable it, something like:
--debug-hide-internals
--debug-expose-internals


I've also applied the "Feature Request" label to this, as it sounds like a request rather than a bug.

@RedYetiDev RedYetiDev added the feature request Issues that request new features to be added to Node.js. label Jul 23, 2024
@joyeecheung
Copy link
Member

joyeecheung commented Jul 23, 2024

Technically this should be a feature request for chromium DevTools instead of Node.js. We do have a pattern for hiding internals (Node.js only implements the server), but the DevTools client is not sending the command to hide them (in the DevTools UI, you can hide certain scripts and there are checkboxes you can use for toggling, it just doesn’t have a preset for Node.js internals by default). Any other inspector client can send the command if they want to implement a better UX. Using a CLI flag in Node.js is a bit awkward compared to toggling it in the DevTools UI.

@piranna
Copy link
Contributor

piranna commented Jul 23, 2024

👍🏻 to have internals hidden by default, and have a flag to show them.

@joyeecheung
Copy link
Member

joyeecheung commented Jul 23, 2024

In DevTools can already ignore the internals by using settings -> ignore list and add the following pattern

Screenshot 2024-07-23 at 17 24 18

After configuring this, the Node.js internal frames will be skipped and by default the debugger will step through them. There is a toggle you can use to restore them.

Screenshot 2024-07-23 at 17 26 22

You can still deliberately step into them and the DevTools will show you this if you do that

Screenshot 2024-07-23 at 17 28 39

This is technically a setting that should be done by DevTools, not Node.js - I don't think Node.js can set this itself and make the DevTools remember the setting, so if the requirement is that this pattern is configured by default, the fix probably needs to submitted to Chrome DevTools's repository, instead of here (Chromium already configures the ignore list for its extensions by default, it should do the same thing for Node.js internals in the Node.js DevTools).

@joshuakb2
Copy link

@joyeecheung using the ignore list feature in Dev Tools does not fix the problem, in my view. When I "step into" an async function call, I expect to be stepping into my function definition, not the node internals. But even with the ignore list configured like you showed above, "step into" steps into the internals instead. This is not the behavior you get when you "step into" a library function call that is ignore-listed. In that case, you end up stepping over the call unless some of your own code is executed by the library.

@joyeecheung
Copy link
Member

joyeecheung commented Jul 23, 2024

That sounds like a Chromium DevTools bug, instead of a Node.js bug?

Note: the Node.js DevTools in Chromium is not maintained by the contributors here, or most people here have no idea how to work with that code base. If you run into an issue in Chromium DevTools, please submit a ticket at https://issues.chromium.org/components/1457055/edit

@joshuakb2
Copy link

joshuakb2 commented Jul 23, 2024

@joyeecheung gotcha. I'm just a user so it's not clear to me which project is responsible for this behavior. I had been following #36022 until today.

Edit: After re-reading the comments on that other issue, others over there were convinced that this was a node bug and not a chromium dev tools bug. They characterized a related bug fix in VS Code as more of a workaround. But I'm not qualified to draw the distinction.

@breautek
Copy link

breautek commented Jul 24, 2024

In DevTools can already ignore the internals by using settings -> ignore list and add the following pattern

I tried this as well with my reproduction code and yes it doesn't prevent stepping into the node: code, though it does hide it in the call stack as shown in the screenshots. But yes, the ignore list behaviour does seem to be problem with the Dev Tools itself, considering the verbiage used suggesting that those files matching will not stop the debugger.

If I find time later I'm going to experiment with more with different ignore term regexes to try to isolate/scope the dev tools bug and I'll follow up with a bug report against chromium.

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 module Issues and PRs related to the module subsystem.
Projects
Development

No branches or pull requests