process: improve handling of formatted error stack

[Hakerh400]

Fatal errors are logged from C++ directly to stderr.
If colored stack trace is used, ANSI color escape sequences
are literally included in the stack trace string. It works
in console on Unix-like systems, but does not work on
Windows or in inspector's DevTools. This PR disables colored
stack trace for fatal errors.

Fixes #28287

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

[Hakerh400]

Ready for review

[BridgeAR]

@Hakerh400 would it not be better to improve the color detection instead? It seems like a bug in the color detection and not about the output itself.

Some Windows terminals and versions do support ANSI codes. It is just not that easy to determine which ones do and which don't.

[Hakerh400]

@BridgeAR Libuv can already emulate colors on Windows, so Windows is not the problem. The main problem is that fatal errors are delegated to C++ and logged directly to stderr, avoiding color emulation on consoles that do not support colors natively.

Improving color support detection on Windows may be a good idea too, but that would not solve this problem, it would rather bring an inconsistency in the output between terminals (colors would not be displayed in some terminals, while libuv has capability to do so).

One solution I can think of is to log the error using console.error (which do perform color emulation) instead of coloring the stack trace string directly. The problem is that V8Inspector::exceptionThrown also displays the file name, line number, etc, for which I couldn't find a reliable way to obtain in the JS land (and fatal error message format is used in a lot of tests that literally check how the output looks).

Still one problem would remain: if the inspector is attached, we should either not color the stack trace, or adapt the colors to the inspector (Chrome DevTools support setting background and foreground color of text).

So, I think the two main questions here are 1) Is it a good idea to use console.error to display the error and if yes, how to reliably obtain file name, line number and line position and 2) should we ignore colors if inspector is attached, or should we convert each ANSI color to the corresponding HTML color (that is used in DevTools) before sending to the inspector?

[joyeecheung]

Doesn't this patch just remove the highlight all the time? If I have to pick, not having colors at all is worse than having ANSI escape sequence in inspector output.

[Hakerh400]

@joyeecheung This patch currently only removes highlighting of fatal (uncaught) errors, while console.log(new Error()) still displays the colors. But, as said, I agree that this can probably be improved, alternatives are outlined in #28291 (comment)

[addaleax]

I’m okay with just removing this as a bit of a design smell (we’re performing colour detection without the error only ending up in stdout).

However: I think we can improve the situation without fully removing colour support; e.g. we could not override er.stack, but instead store it on a separate property (a private one, if we want to fully hide this), and then use that for printing to stderr while at the same time keeping e.g. the inspector fully intact.

[joyeecheung]

Alternative fix in #28308 which does not remove the highlighting support. That was closer to what I meant by pass a different stack trace to V8Inspector::exceptionThrown() in #28287 - it adds two hooks for manipulating the stack trace before and after the inspector is notified.

[joyeecheung]

On the Windows issue - that should be fairly trivial to fix by using uv_write instead of vfprintf directly. (Similar to the solution in #27819 (comment) )

(In fact node::errors::TriggerUncaughtException() can almost be implemented entirely in JavaScript, the only caveat is that when you pass an exception into a JavaScript try-catch you'll lost the original metadata for the source line, which is partially hacked around by AppendExceptionLine so I left a TODO there in #28308)