-
-
Notifications
You must be signed in to change notification settings - Fork 35.4k
util: fix inspecting of proxy objects #6465
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
Changes from 1 commit
86daa71
ec16e91
b2ebb7b
f630602
6115df5
842a8e4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
In certain conditions, inspecting a Proxy object can lead to a max call stack error. Avoid that by detecting the Proxy object and outputting information about the Proxy object itself. Also adds util.isProxy() Fixes: #6464
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ using v8::FunctionCallbackInfo; | |
| using v8::Local; | ||
| using v8::Object; | ||
| using v8::Private; | ||
| using v8::Proxy; | ||
| using v8::String; | ||
| using v8::Value; | ||
|
|
||
|
|
@@ -22,6 +23,7 @@ using v8::Value; | |
| V(isMap, IsMap) \ | ||
| V(isMapIterator, IsMapIterator) \ | ||
| V(isPromise, IsPromise) \ | ||
| V(isProxy, IsProxy) \ | ||
| V(isRegExp, IsRegExp) \ | ||
| V(isSet, IsSet) \ | ||
| V(isSetIterator, IsSetIterator) \ | ||
|
|
@@ -37,6 +39,21 @@ using v8::Value; | |
| VALUE_METHOD_MAP(V) | ||
| #undef V | ||
|
|
||
| static void GetProxyDetails(const FunctionCallbackInfo<Value>& args) { | ||
| Environment* env = Environment::GetCurrent(args); | ||
|
|
||
| // Return nothing if it's not a proxy. | ||
| if (!args[0]->IsProxy()) | ||
| return; | ||
|
|
||
| Local<Proxy> proxy = args[0].As<Proxy>(); | ||
|
|
||
| Local<Object> ret = Object::New(env->isolate()); | ||
| ret->Set(env->handler_string(), proxy->GetHandler()); | ||
| ret->Set(env->target_string(), proxy->GetTarget()); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Aside, it might be a little cheaper to return an array instead of a dictionary object.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it still (even) cheaper to pass an array/object from js?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, can do. |
||
|
|
||
| args.GetReturnValue().Set(ret); | ||
| } | ||
|
|
||
| static void GetHiddenValue(const FunctionCallbackInfo<Value>& args) { | ||
| Environment* env = Environment::GetCurrent(args); | ||
|
|
@@ -84,6 +101,7 @@ void Initialize(Local<Object> target, | |
|
|
||
| env->SetMethod(target, "getHiddenValue", GetHiddenValue); | ||
| env->SetMethod(target, "setHiddenValue", SetHiddenValue); | ||
| env->SetMethod(target, "getProxyDetails", GetProxyDetails); | ||
| } | ||
|
|
||
| } // namespace util | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| 'use strict'; | ||
|
|
||
| require('../common'); | ||
| const assert = require('assert'); | ||
| const util = require('util'); | ||
| const processUtil = process.binding('util'); | ||
|
|
||
| const target = {}; | ||
| const handler = { | ||
| get: function() { throw new Error('Getter should not be called'); } | ||
| }; | ||
| const proxyObj = new Proxy(target, handler); | ||
|
|
||
| assert.strictEqual(util.isProxy(proxyObj), true); | ||
|
|
||
| // Inspecting the proxy should not actually walk it's properties | ||
| assert.doesNotThrow(() => util.inspect(proxyObj)); | ||
|
|
||
| const details = processUtil.getProxyDetails(proxyObj); | ||
| assert.deepStrictEqual(target, details.target); | ||
| assert.deepStrictEqual(handler, details.handler); | ||
|
|
||
| assert.strictEqual(util.inspect(proxyObj), | ||
| 'Proxy { handler: { get: [Function] }, target: {} }'); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you line up the arguments? |
||
|
|
||
| // Using getProxyDetails with non-proxy returns undefined | ||
| assert.strictEqual(processUtil.getProxyDetails({}), undefined); | ||
| // isProxy with non-Proxy returns false | ||
| assert.strictEqual(util.isProxy({}), false); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you export this, it becomes public API, and should be documented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 ... Forgot to add the doc for it. Will add soon!
On Apr 29, 2016 6:29 AM, "Colin Ihrig" notifications@github.com wrote: