Crash when error.name is not a string

[griffinmyers]

• Version: v12.13.0.
• Platform: Darwin will.local 18.7.0 Darwin Kernel Version 18.7.0: Sat Oct 12 00:02:19 PDT 2019; root:xnu-4903.278.12~1/RELEASE_X86_64 x86_64
• Subsystem: internal/util/inspect.js

The following program crashes on node v12.13.0:

error-12.js

const err = new Error('bloop');
err.name = 404;
console.log(err);

with the following logged:

$ node error-12.js
internal/util/inspect.js:880
      name.endsWith('Error') &&
           ^

TypeError: name.endsWith is not a function
    at formatError (internal/util/inspect.js:880:12)
    at formatRaw (internal/util/inspect.js:681:14)
    at formatValue (internal/util/inspect.js:569:10)
    at inspect (internal/util/inspect.js:223:10)
    at formatWithOptions (internal/util/inspect.js:1651:40)
    at Object.Console.<computed> (internal/console/constructor.js:272:10)
    at Object.log (internal/console/constructor.js:282:61)
    at Object.<anonymous> (/Users/williammyers/projects/js/error-12.js:3:9)
    at Module._compile (internal/modules/cjs/loader.js:774:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:785:10)

On node v10.15.1, the program doesn't crash and instead logs:

$ node error-12.js
{ 404: bloop
    at Object.<anonymous> (/Users/williammyers/projects/js/error-12.js:1:75)
    at Module._compile (internal/modules/cjs/loader.js:689:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
    at Module.load (internal/modules/cjs/loader.js:599:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
    at Function.Module._load (internal/modules/cjs/loader.js:530:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
    at startup (internal/bootstrap/node.js:283:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:743:3) name: 404 }

I appreciate that node might not consider this a bug that is theirs to fix (the name property being non-string could be viewed as a userland error), but wanted to file the issue in case it was concerning.

I discovered this doing an upgrade to node 12--it turns out the official aws-sdk will set the name property of HTTP response errors to the numeric HTTP response code.

I suspect this was introduced in e54f237.

[Trott]

Seems like maybe inspect() can make sure to convert .name to a string before doing anything with it? Going to guess that this would be of interest to @BridgeAR.

[griffinmyers]

I should add, if the team thinks this is something node should patch, I'm more than happy to work up a PR.

[addaleax]

I think it’s definitely something that Node.js should patch, so feel free to open a PR.

I also wonder why we have a .endsWith('Error') check in the first place – it’s not clear to me what exactly the code guarded by it does, but the check seems brittle and I wonder if there’s a better option?

You might also want to test the condition that .stack is not a string, I think it should fail similarly.

[BridgeAR]

@addaleax

I wonder why we have a .endsWith('Error') check in the first place

The check identifies subclassed errors without direct own name property. The subclass name will then show up in the output. It tries to be very conservative by only visualizing the subclass name in case the errors name and stack have not been tampered with.

class SubError extends Error {}

console.log(new SubError('foo'))
// SubError: foo
//     at ...

// It will also highlight the actual name and the subclass name in case they are not aligned.
class FooBar extends Error{}

console.log(new FooBar('foo'))
// FooBar [Error]: foo
//     at ...

// Without this check both cases would be printed as:

// Error: foo
//     at ...

the check seems brittle and I wonder if there’s a better option?

Errors are pretty much the most difficult to inspect object type. They can have lots of shapes and it's quite difficult to always provide the best output (e.g., the stack property and the name and message could deviate from each other).
If someone finds a better way to get this right, that would be great!

[griffinmyers]

Thank you, all, for the responsiveness and prompt followup!

[cuyl]

Any plan to ship the patch to v12?

[BridgeAR]

@cuyl it should be published in one of the upcoming releases.