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.13%. Comparing base (e272637) to head (f616bcc).
Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #58196   +/-   ##
=======================================
  Coverage   90.13%   90.13%           
=======================================
  Files         630      630           
  Lines      186780   186782    +2     
  Branches    36653    36651    -2     
=======================================
+ Hits       168347   168364   +17     
+ Misses      11207    11199    -8     
+ Partials     7226     7219    -7     

Files with missing lines	Coverage Δ
lib/internal/util/inspect.js	99.88% <100.00%> (-0.08%)	⬇️

... and 27 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]

Suggested change

	if (error.stack !== error) {
	return formatValue(ctx, error.stack);
	}
	ctx.seen.push(error);
	ctx.indentationLvl += 4;
	const result = formatValue(ctx, error.stack);
	ctx.indentationLvl -= 4;
	ctx.seen.pop();
	return `${ErrorPrototypeToString(error)}\n ${result}`;

This is going to catch all circular issues and it provides additional information to the user. It does require tests to be adjusted accordingly but only a few.