process: Cast promise rejection reason to string

[apexskier]

The unhandled promise rejection warning uses a template literal and
prints the reason a promise was rejected. If rejecting with a symbol,
the symbol failed to convert to a string and the process crashed. Now,
symbols are casted to strings and the process does not crash.

Fixes: #11637

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• [ ] documentation is changed or added
• commit message follows commit guidelines

Affected core subsystem(s)

process

[cjihrig]

I don't think we need a message test for this. A "normal" test in test/parallel/ that uses common.expectWarning() should do.

[Fishrock123]

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

[jasnell]

Definitely agree that all this needs is a simple common.expectsWarning() check.

[apexskier]

From the source and my tests, it looks like common.expectWarning() will only work if a single warning is generated in the test.

A test like the following generates two warnings with different names, so I can't ensure the warning matches. I could

'use strict';// ensure this doesn't crashPromise.reject(Symbol());

(node:95244) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): Symbol() (node:95244) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code. 

I've got a change that allows expectWarning to take a map of names to message(s) or the original parameter types.

[cjihrig]

Can you split the changes to test/common.js into a separate commit.

[addaleax]

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

request to not have a message test has been addressed

[addaleax]

Landed in 765be6c...a3132b0, thanks for the PR!

[bakkot]

Symbol is only one example among many of something which throws when trying to call .toString. If I read this correctly, this PR doesn't fix the core issue, and will presumably still crash on e.g.

Promise.reject({toString(){throw0;}})

[sam-github]

Should this land on 6.x?

It sounds like it fixes a crash, so should be, but it also sounds like a change in error messages.

[gibfahn]

Should this land on 6.x?

ping

cc/ @addaleax

[addaleax]

Should this land on 6.x?

@gibfahn Yes, it should. :)

[bakkot]

people backporting this: see my above comment, which I just opened a new issue for at #13771. The fix in this PR doesn't actually handle the full extent of the problem.

[gibfahn]

The fix in this PR doesn't actually handle the full extent of the problem.

Does it make things worse? As long as it doesn't regess things (and hopefully is an improvement) then it should be fine to backport.

[bakkot]

It's definitely an improvement, just not a complete solution. And I'd expect a complete solution to subsume this one.

[gibfahn]

It's definitely an improvement, just not a complete solution. And I'd expect a complete solution to subsume this one.

Sure, but any "complete solution" will be a commit on top of this one, so if we don't backport this first we'll get merge conflicts when we try to cherry-pick the complete solution.

[gibfahn]

Is anyone willing to backport this to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.