util.inspect can throw when passed a Proxy

[daurnimator]

• Version: v9.3.0
• Platform: Linux daurn-z170 4.14.11-1-ARCH deps: update openssl to 1.0.1j #1 SMP PREEMPT Wed Jan 3 07:02:42 UTC 2018 x86_64 GNU/Linux
• Subsystem: util

> console.log(require("util").inspect(new Proxy({}, {"ownKeys": function() {}})))
TypeError: CreateListFromArrayLike called on non-object
    at Function.getOwnPropertySymbols (<anonymous>)
    at formatValue (util.js:422:24)
    at Object.inspect (util.js:298:10)

As util.inspect is used by console.log in a recursive manner, such a proxy object acts as a "poison pill" for printing an object.

[bnoordhuis]

Node.js 10 will have a {showProxy: true} option that you can pass as the second argument to util.inspect(). That could be back-ported to older releases if there is enough demand. It prints something like this:

> util.inspect(new Proxy({}, {ownKeys() {}}), {showProxy: true})
'Proxy [ {}, { ownKeys: [Function: ownKeys] } ]'

In general though, what you have there is a buggy proxy. Throwing an exception when it doesn't honor the contract is the right thing to do.

[daurnimator]

Node.js 10 will have a {showProxy: true} option that you can pass as the second argument to util.inspect(). That could be back-ported to older releases if there is enough demand.

IMO this new option should be the default for console.log.

In general though, what you have there is a buggy proxy. Throwing an exception when it doesn't honor the contract is the right thing to do.

Right. But how do I track down the bug? By printing the proxy (and things that may contain it)... For other issues related to proxys this is a good answer. But console.log is one of the debugging primitives.

I also think it's bad that such a buggy Proxy can act as a "poison pill" for trying to debug any surrounding code by default.

[benjamingr]

I don't think we've ever made any throw-safety guarantees about util.inspect:

// crash inspect
util.inspect({ [util.inspect.custom]() { throw '^_^'; } })

Is an easy example of how someone might do it. I think the point of proxies is to be able to override behavior like this - and if someone is using them they should know what they're doing when they override a hook like ownKeys.

Since this is util.inspect we should do what users find the most useful - but in terms of overall correctness I like the current (node 10) behavior.

[bnoordhuis]

The exception message could be better but that's a V8 issue. I took a quick look and it's probably not hard to fix but I don't have time. If anyone wants to work on that and needs some pointers, let me know.

Re: switching to {showProxy: true} in console.log - I'd suggest opening a pull request and see how it's received.

Barring further movement, I'll close this in a few days.

[BridgeAR]

One problem with showProxy: true is that is it a huge performance overhead.

I also think that the current behaviour is correct to throw a error.

[TimothyGu]

@bnoordhuis

Re: switching to {showProxy: true} in console.log - I'd suggest opening a pull request and see how it's received.

Ironically, this has already been done by you on master in #16485.

@BridgeAR

I also think that the current behaviour is correct to throw a error.

I agree. Hence, #16485 (review).

[bnoordhuis]

@TimothyGu Only in the repl, not console.log.

One problem with showProxy: true is that is it a huge performance overhead.

Does it? Not saying it doesn't but I haven't noticed it.

[TimothyGu]

One problem with showProxy: true is that is it a huge performance overhead.

Does it? Not saying it doesn't but I haven't noticed it.

It does call into a C++ runtime function. But on the other hand, calling Proxy hooks isn’t too fast either. I’m not sure which one is faster, depends on the circumstances probably.

[bnoordhuis]

No further movement. I'll go ahead and close this out.