Error "message" property enumerability change on 9.7.0+

[alexjeffburke]

• Version: 9.7.0+
• Platform: macOS 10.12.6
• Subsystem: errors

Background
There appears to be a change in the enumerability of the "message" property on at least errors coming up from the getaddrinfo system call that were noticed when a number of tests for the Unexpected project and a number of it's plugins started failing. After looking into and rolling out code changes I was able to beset node versions, and it seems this issue was introduced between 9.6.1 and 9.7.0.

The follwing code should demonstrate the issue:

const dns = require('dns');

dns.lookup('asdfasdfasdfasdfasdfasdfasdf.zzz', (err) => {
    if (!err) return;
    var enumerableKeys = Object.keys(err);
    var hasMessage = enumerableKeys.indexOf('message') > -1;
    console.log(`${err.name} with ${enumerableKeys.length} enumerable keys ${hasMessage ? 'WITH' : 'WITHOUT'} "message"`);
 });

Expected outcome
Both version of node report the same number of enumerable keys and message is not included.

Actual outcome
9.6.1: 'Error with 4 enumerable keys WITHOUT "message"'
9.7.0: 'Error with 5 enumerable keys WITH "message"'

Summary
I hope the above is enough to explain the issue. Based on the workaround we applied, it seems likely that these errors were previously instantiated by passing the message into the constructor but were changed so the message was attached after the fact.

If there's anything else I can provide please let me know.

Thanks

[mscdex]

If I had to guess, it may be 1246902. One difference is that no message is being passed to the Error() constructor anymore, but the .message property is being manually set now. That might cause this change?

[joyeecheung]

@alexjeffburke Yes that's the intended behavior, because most errors coming out of Node.js or the JS engine does not have a enumerable error.message since it's spec'ed that messages passed during instantiation should not be enumerable. Could not find a link but I believe transforming the errors into a more uniformed manner triggered a test failure because it didn't expect a message out of a child process printingJSON.stringify(err).

[joyeecheung]

Also, to test against errors coming out of Node.js v8 and above, a better way is to test against (a subset of) their properties, e.g. syscall, code and host, instead of strictly matching against messages. That's also what we started to do in the tests of Node.js core.

[joyeecheung]

Oops, I think I misread the OP, it was talking about dnsException, not uvException that I mentioned in #19716 (comment) , this should be fixed by #19719

[alexjeffburke]

@mscdex yeah I think it is exactly that - within the commit you linked it's this ending up as that.

@joyeecheung yep it's a defined quirk in the spec. I know checking those properties or perhaps soon a standard error code is the more correct way, but pretty sure this change was unintentional and it does rather subtly change behaviour.

@BridgeAR thanks for jumping on it.