Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

util: fix inspecting of proxy objects #6465

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions benchmark/util/inspect-proxy.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
'use strict';

const util = require('util');
const common = require('../common.js');

const bench = common.createBenchmark(main, {
v: [1, 2],
n: [1e6]
});

function twoDifferentProxies(n) {
// This one should be slower between we're looking up multiple proxies.
const proxyA = new Proxy({}, {get: () => {}});
const proxyB = new Proxy({}, {get: () => {}});
bench.start();
for (var i = 0; i < n; i += 1)
util.inspect({a: proxyA, b: proxyB}, {showProxy: true});
bench.end(n);
}

function oneProxy(n) {
// This one should be a bit faster because of the internal caching.
const proxy = new Proxy({}, {get: () => {}});
bench.start();
for (var i = 0; i < n; i += 1)
util.inspect({a: proxy, b: proxy}, {showProxy: true});
bench.end(n);
}

function main(conf) {
const n = conf.n | 0;
const v = conf.v | 0;

switch (v) {
case 1:
oneProxy(n);
break;
case 2:
twoDifferentProxies(n);
break;
default:
throw new Error('Should not get to here');
}
}
4 changes: 4 additions & 0 deletions doc/api/util.md
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,10 @@ formatted string:
- `customInspect` - if `false`, then custom `inspect(depth, opts)` functions
defined on the objects being inspected won't be called. Defaults to `true`.

- `showProxy` - if `true`, then objects and functions that are Proxy objects
will be introspected to show their `target` and `hander` objects. Defaults to
`false`.

Example of inspecting all properties of the `util` object:

```js
Expand Down
42 changes: 41 additions & 1 deletion lib/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ function inspect(obj, opts) {
if (ctx.depth === undefined) ctx.depth = 2;
if (ctx.colors === undefined) ctx.colors = false;
if (ctx.customInspect === undefined) ctx.customInspect = true;
if (ctx.showProxy === undefined) ctx.showProxy = false;
if (ctx.colors) ctx.stylize = stylizeWithColor;
return formatValue(ctx, obj, ctx.depth);
}
Expand Down Expand Up @@ -241,6 +242,46 @@ function inspectPromise(p) {


function formatValue(ctx, value, recurseTimes) {
if (ctx.showProxy &&
((typeof value === 'object' && value !== null) ||
typeof value === 'function')) {
var proxy = undefined;
var proxyCache = ctx.proxyCache;
if (!proxyCache)
proxyCache = ctx.proxyCache = new Map();
// Determine if we've already seen this object and have
// determined that it either is or is not a proxy.
if (proxyCache.has(value)) {
// We've seen it, if the value is not undefined, it's a Proxy.
proxy = proxyCache.get(value);
} else {
// Haven't seen it. Need to check.
// If it's not a Proxy, this will return undefined.
// Otherwise, it'll return an array. The first item
// is the target, the second item is the handler.
// We ignore (and do not return) the Proxy isRevoked property.
proxy = binding.getProxyDetails(value);
if (proxy) {
// We know for a fact that this isn't a Proxy.
// Mark it as having already been evaluated.
// We do this because this object is passed
// recursively to formatValue below in order
// for it to get proper formatting, and because
// the target and handle objects also might be
// proxies... it's unfortunate but necessary.
proxyCache.set(proxy, undefined);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you storing the array that .getProxyDetails() returns as a key? I would expect this to be proxyCache.set(value, proxy). The comment seems wrong as well. This whole if statement seems like it could be dropped, actually.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look at lines 273 and 276.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the logic but I don't think the 'necessary' is accurate; if you skipped adding proxy to the cache here, the algorithm would still work, only less efficiently. The next iteration would check the cache, miss, call .getProxyDetails() for the details array, and add the undefined return value to the cache a few lines below.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Necessary in the sense of avoiding an extraneous call to getProxyDetails... Which I would consider to be a bug since the point is to only check when we don't know for sure. ;)

}
// If the object is not a Proxy, then this stores undefined.
// This tells the code above that we've already checked and
// ruled it out. If the object is a proxy, this caches the
// results of the getProxyDetails call.
proxyCache.set(value, proxy);
}
if (proxy) {
return 'Proxy ' + formatValue(ctx, proxy, recurseTimes);
}
}

// Provide a hook for user-specified inspect functions.
// Check that value is an object with an inspect function on it
if (ctx.customInspect &&
Expand Down Expand Up @@ -785,7 +826,6 @@ exports.isPrimitive = isPrimitive;

exports.isBuffer = Buffer.isBuffer;


function pad(n) {
return n < 10 ? '0' + n.toString(10) : n.toString(10);
}
Expand Down
16 changes: 16 additions & 0 deletions src/node_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,13 @@
namespace node {
namespace util {

using v8::Array;
using v8::Context;
using v8::FunctionCallbackInfo;
using v8::Local;
using v8::Object;
using v8::Private;
using v8::Proxy;
using v8::String;
using v8::Value;

Expand All @@ -37,6 +39,19 @@ using v8::Value;
VALUE_METHOD_MAP(V)
#undef V

static void GetProxyDetails(const FunctionCallbackInfo<Value>& args) {
// Return undefined if it's not a proxy.
if (!args[0]->IsProxy())
return;

Local<Proxy> proxy = args[0].As<Proxy>();

Local<Array> ret = Array::New(args.GetIsolate(), 2);
ret->Set(0, proxy->GetTarget());
ret->Set(1, proxy->GetHandler());

args.GetReturnValue().Set(ret);
}

static void GetHiddenValue(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
Expand Down Expand Up @@ -84,6 +99,7 @@ void Initialize(Local<Object> target,

env->SetMethod(target, "getHiddenValue", GetHiddenValue);
env->SetMethod(target, "setHiddenValue", SetHiddenValue);
env->SetMethod(target, "getProxyDetails", GetProxyDetails);
}

} // namespace util
Expand Down
84 changes: 84 additions & 0 deletions test/parallel/test-util-inspect-proxy.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
'use strict';

require('../common');
const assert = require('assert');
const util = require('util');
const processUtil = process.binding('util');
const opts = {showProxy: true};

const target = {};
const handler = {
get: function() { throw new Error('Getter should not be called'); }
};
const proxyObj = new Proxy(target, handler);

// Inspecting the proxy should not actually walk it's properties
assert.doesNotThrow(() => util.inspect(proxyObj, opts));

// getProxyDetails is an internal method, not intended for public use.
// This is here to test that the internals are working correctly.
const details = processUtil.getProxyDetails(proxyObj);
assert.strictEqual(target, details[0]);
assert.strictEqual(handler, details[1]);

assert.strictEqual(util.inspect(proxyObj, opts),
'Proxy [ {}, { get: [Function] } ]');

// Using getProxyDetails with non-proxy returns undefined
assert.strictEqual(processUtil.getProxyDetails({}), undefined);

// This will throw because the showProxy option is not used
// and the get function on the handler object defined above
// is actually invoked.
assert.throws(
() => util.inspect(proxyObj)
);

// Yo dawg, I heard you liked Proxy so I put a Proxy
// inside your Proxy that proxies your Proxy's Proxy.
const proxy1 = new Proxy({}, {});
const proxy2 = new Proxy(proxy1, {});
const proxy3 = new Proxy(proxy2, proxy1);
const proxy4 = new Proxy(proxy1, proxy2);
const proxy5 = new Proxy(proxy3, proxy4);
const proxy6 = new Proxy(proxy5, proxy5);
const expected0 = '{}';
const expected1 = 'Proxy [ {}, {} ]';
const expected2 = 'Proxy [ Proxy [ {}, {} ], {} ]';
const expected3 = 'Proxy [ Proxy [ Proxy [ {}, {} ], {} ], Proxy [ {}, {} ] ]';
const expected4 = 'Proxy [ Proxy [ {}, {} ], Proxy [ Proxy [ {}, {} ], {} ] ]';
const expected5 = 'Proxy [ Proxy [ Proxy [ Proxy [Object], {} ],' +
' Proxy [ {}, {} ] ],\n Proxy [ Proxy [ {}, {} ]' +
', Proxy [ Proxy [Object], {} ] ] ]';
const expected6 = 'Proxy [ Proxy [ Proxy [ Proxy [Object], Proxy [Object]' +
' ],\n Proxy [ Proxy [Object], Proxy [Object] ] ],\n' +
' Proxy [ Proxy [ Proxy [Object], Proxy [Object] ],\n' +
' Proxy [ Proxy [Object], Proxy [Object] ] ] ]';
assert.strictEqual(util.inspect(proxy1, opts), expected1);
assert.strictEqual(util.inspect(proxy2, opts), expected2);
assert.strictEqual(util.inspect(proxy3, opts), expected3);
assert.strictEqual(util.inspect(proxy4, opts), expected4);
assert.strictEqual(util.inspect(proxy5, opts), expected5);
assert.strictEqual(util.inspect(proxy6, opts), expected6);
assert.strictEqual(util.inspect(proxy1), expected0);
assert.strictEqual(util.inspect(proxy2), expected0);
assert.strictEqual(util.inspect(proxy3), expected0);
assert.strictEqual(util.inspect(proxy4), expected0);
assert.strictEqual(util.inspect(proxy5), expected0);
assert.strictEqual(util.inspect(proxy6), expected0);

// Just for fun, let's create a Proxy using Arrays.
const proxy7 = new Proxy([], []);
const expected7 = 'Proxy [ [], [] ]';
assert.strictEqual(util.inspect(proxy7, opts), expected7);
assert.strictEqual(util.inspect(proxy7), '[]');

// Now we're just getting silly, right?
const proxy8 = new Proxy(Date, []);
const proxy9 = new Proxy(Date, String);
const expected8 = 'Proxy [ [Function: Date], [] ]';
const expected9 = 'Proxy [ [Function: Date], [Function: String] ]';
assert.strictEqual(util.inspect(proxy8, opts), expected8);
assert.strictEqual(util.inspect(proxy9, opts), expected9);
assert.strictEqual(util.inspect(proxy8), '[Function: Date]');
assert.strictEqual(util.inspect(proxy9), '[Function: Date]');