util: inspect: do not crash on an Error stack pointing to itself

[SamVerschueren]

See #58195

This avoids a maximum call stack size exceeded crash when the error stack is pointing to the error itself error.stack = error. This bug was introduced by #56573.

[BridgeAR]

LGTM. I am just surprised the regular circular detection does not pick this up. Do we know why?

The output does seem to be better in this case, so it's likely a good idea not to have the circular check trigger.
Should we maybe just add additional information that it's circular? That might be useful for users?

[SamVerschueren]

Open to ideas on how to show the circularity. I think it's a good idea.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.14%. Comparing base (e272637) to head (111b9ca).
Report is 327 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #58196      +/-   ##
==========================================
+ Coverage   90.13%   90.14%   +0.01%     
==========================================
  Files         630      637       +7     
  Lines      186780   188028    +1248     
  Branches    36653    36890     +237     
==========================================
+ Hits       168347   169504    +1157     
- Misses      11207    11279      +72     
- Partials     7226     7245      +19     

Files with missing lines	Coverage Δ
lib/internal/util/inspect.js	99.96% <100.00%> (+<0.01%)	⬆️

... and 185 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[BridgeAR]

@SamVerschueren instead of only returning the ErrorPrototypeToString call result, we could add additional information that the stack is circular similar to the other circular outputs.

[SamVerschueren]

So I checked how it's done with objects where it looks like this

<ref *1> { bar: [Circular *1] }

The problem here though is that it's a method to get the stack string itself, so I don't see a way how to represent it in the same way more or less.

So I just tried some things out

in the case of the following code

const foo = new Error('foo'); 
foo.stack = foo; 

console.log(foo.stack);

1. [[Circular] Error: foo]
2. [<Circular> Error: foo]
3. [[Circular stack] Error: foo]
4. [<Circular stack> Error: foo]
5. [[Circular *stack] Error: foo]
6. [<Circular *stack> Error: foo]

Let me know what any one of you would like to see. Or something completely different, although might be a bit more work.

<ref *1> Error: foo { stack: [Circular *1] }

But this deviates way too much from what it was I feel like.

[BridgeAR]

I had a brief look into this and it does not fully fix the problem.

There are currently two spots where we miss to add adding the circular check (the other spot is for weird function names), since we have to special handle parts of the inspection before getting to the spot where we add the object to our circular check (it is intentionally at that spot to prevent doing it for e.g., empty objects, arrays, etc.).

One of them is here. Just comparing for a simple direct recursion does not prevent other cases, for example:

const error = new Error()
const error2 = new Error()
error.stack = error2
error2.stack = error

[BridgeAR]

@SamVerschueren could you take another look? :)

We could also land this as intermediate improvement and have another PR on top, I just thought it makes sense to combine the points that I looked at.

[SamVerschueren]

Yes I'll definitely look into this. I was out this week so I'll probably do it over the weekend.

[BridgeAR]

Ping @SamVerschueren

[SamVerschueren]

Ugh Sorry. I totally forgot about this. I'll do it tomorrow!

[BridgeAR]

LGTM, thanks for the PR!

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/67429/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/67431/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/67433/

[Ethan-Arrowood]

lgtm

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/67451/

[nodejs-github-bot]

Landed in ea5d37e