console.log never finishes on Proxy with getPrototypeOf pointing to itself

[kevinkassimo]

• Version:
  v11.10.0

• Platform:
  Darwin (MacOS High Sierra)

(This is a pretty rare case: I encountered this when I was playing around with Proxy to build a dummy that always silently do nothing on any function call/value assignment given any name)

Example: console.log(p) never finishes running given the following code (potentially in an infinite loop):

const o = {};
const handler = {};
const p = new Proxy(o, handler);
handler.getPrototypeOf = () => p;
console.log(p);

In comparison, on Chrome 71 console.log would immediately finishes with output

Proxy {}

[Hakerh400]

That is probably working as expected. Object having itself as prototype usually doesn't make sense and handling it as a special case may not be worth it. Note that there is showProxy inspect option which doesn't cause infinite loop, but since its default value is false suggests that this is probably intended.

[joyeecheung]

Reduced test case:

const util = require('util');
const p = new Proxy({}, {
  getPrototypeOf() {
    return p;
  }
});
util.inspect(p);

I'd expect util.inspect to be clever enough to print a summary properly though, since the v8 inspector is clever enough to generate a proper preview for it.

[joyeecheung]

The cause is this loop which does not take care of the case when Object.getPrototypeOf(obj) === obj

node/lib/internal/util/inspect.js

Lines 334 to 345 in e51da1f

	while (obj) {
	const descriptor = Object.getOwnPropertyDescriptor(obj, 'constructor');
	if (descriptor !== undefined &&
	typeof descriptor.value === 'function' &&
	descriptor.value.name !== '') {
	return descriptor.value.name;
	}
	obj = Object.getPrototypeOf(obj);
	if (firstProto === undefined) {
	firstProto = obj;
	}

[BridgeAR]

V8 has the advantage of not triggering the proxy handlers when inspecting the proxy. That is the reason why it works there but not with util.inspect(). We can work around some of these proxy issues but this is a never ending story as we can not work around all of them. It will also cause the code to become more complex and slower for the common cases.

[BridgeAR]

@joyeecheung that would not be sufficient. If we handle that case there, it would cause a maximum call stack size error instead.

[BridgeAR]

I don't think we should fix this. The only real way to fix proxy inspection is to use some internal V8 magic which does not trigger the proxy handlers.

[joyeecheung]

Funny enough, adding a memo in the loop reveals another bug(?):

const p = new Proxy({}, {
  getPrototypeOf() {
    return p;
  }
});
p instanceof Error; // RangeError: Maximum call stack size exceeded

[Hakerh400]

The cause is this loop which does not take care of the case when Object.getPrototypeOf(obj) === obj

Taking care of that case is not enough, as proxy can have cyclic prototype with length more than 1 (obj has proto obj1, which has proto obj and so on). Storing them in an array or Set and breaking the loop on first recursion is also not enough, as proxy can construct another proxy in the trap function itself, which will eventually cause out-of-memory. Also, if trap function never returns, there's no way to deal with it from JS land.

[joyeecheung]

Upstream bug in https://bugs.chromium.org/p/v8/issues/detail?id=8884 for #26231 (comment)

@BridgeAR

We can work around some of these proxy issues but this is a never ending story as we can not work around all of them.

Why are we attempting to display the proxy itself instead of doing showProxy by default? Aren't util.inspect supposed to be side-effect-free?

[cjihrig]

I looked into this a bit last night and found the same issues. The following diff seemed to solve the infinite looping:

diff --git a/lib/internal/util/inspect.js b/lib/internal/util/inspect.js
index 5246ed89b7..66c7194d1c 100644
--- a/lib/internal/util/inspect.js
+++ b/lib/internal/util/inspect.js
@@ -330,8 +330,9 @@ function getEmptyFormatArray() {
 }
 
 function getConstructorName(obj, ctx) {
+  const localSeen = [];
   let firstProto;
-  while (obj) {
+  while (localSeen.indexOf(obj) === -1) {
     const descriptor = Object.getOwnPropertyDescriptor(obj, 'constructor');
     if (descriptor !== undefined &&
         typeof descriptor.value === 'function' &&
@@ -339,6 +340,7 @@ function getConstructorName(obj, ctx) {
       return descriptor.value.name;
     }
 
+    localSeen.push(obj);
     obj = Object.getPrototypeOf(obj);
     if (firstProto === undefined) {
       firstProto = obj;
@@ -349,10 +351,14 @@ function getConstructorName(obj, ctx) {
     return null;
   }
 
-  return `<${inspect(firstProto, {
+  ctx.seen.push(firstProto);
+  const result = `<${inspect(firstProto, {
     ...ctx,
     customInspect: false
   })}>`;
+  ctx.seen.pop();
+
+  return result;
 }
 
 function getPrefix(constructor, tag, fallback) {

Unfortunately, we call isError(), which has the problematic p instanceof Error expression, which causes the max call stack size error. I don't have the latest Chrome, but the same error is seen in Chrome 72. We could add a try...catch to isError(), but that seems like a hack to me.

[BridgeAR]

I actually found a way to completely resolve all our proxy issues without changing the behavior (no trap is called anymore). The main problem I have with this is just that it does a C++ call on each object. I'll open a PR for it and see what others think about it.

@joyeecheung

Why are we attempting to display the proxy itself instead of doing showProxy by default? Aren't util.inspect supposed to be side-effect-free?

There are different opinions about proxies in general. We even got complains that the repl sets showProxy to true by default as some people say it should not be possible to inspect proxies ever. I personally think it's a good to have this possibility.

[jdalton]

I'm a fan of using a Set instead of an array.

[BridgeAR]

I opened #26241 to fix general proxy inspection.