Partial fix for #864 - report property attributes from sandbox object - #885
Partial fix for #864 - report property attributes from sandbox object#885smikes wants to merge 1 commit into
Conversation
Sorry, something went wrong.
|
Please do not merge this in while the test is still failing. |
Sorry, something went wrong.
GlobalPropertyQueryCallback() was returning `None` as attributes for all properties, without regard to actual properties settings. This patch looks up attributes for sandbox-defined properties and returns them. Partial fix for nodejsgh-864 Add test for nodejsgh-864, non-default settings of object properties (e.g., `enumerable: false`) are not visible when object is used as a sandbox
|
Test is no longer failing; a change to node_contextify.cc is a partial fix for #864 and fixes the behavior tested in test-vm-preserves-property.js. |
Sorry, something went wrong.
|
Although this PR was originally inspired by #864, it is complete in itself and reports sandbox property attributes correctly. The full fix for #864 may involve changes to v8 APIs upstream (see @bnoordhuis comments in #884). In the meantime, I believe this PR is now ready for review. |
Sorry, something went wrong.
There was a problem hiding this comment.
Shouldn't we also add if (in_proxy_global) { attr = proxy_global->GetPropertyAttributes(property); } as the fallback?
Sorry, something went wrong.
There was a problem hiding this comment.
I tried that. It caused infinite recursion on proxy_global. I think @bnoordhuis has a patch into v8 to add a new api to support this.
That's why this change is "partial fix"
Sorry, something went wrong.
There was a problem hiding this comment.
Got it. Also given that per #855 I want to at least attempt to rip out proxy global entirely that might end up being unnecessary.
Probably want to leave a TODO(smikes) in the source code though for later.
Sorry, something went wrong.
|
LGTM if you add a TODO. (I am not an official LGTM-er though.) |
Sorry, something went wrong.
|
I will try to finish this this weekend. |
Sorry, something went wrong.
|
Apropos that V8 patch, it landed in v8/v8@726eb05 and should probably be back-ported. That makes it a semver-minor thing because it's a new public API. It's also going to break building against a shared libv8 that doesn't carry the patch. :-/ |
Sorry, something went wrong.
|
@smikes any chance of landing this on next, with the updates? |
Sorry, something went wrong.
There was a problem hiding this comment.
Why not HasRealNamedProperty ?
Sorry, something went wrong.
|
Actually I think I can take this over. |
Sorry, something went wrong.
The GlobalPropertyQueryCallback was changed in 2010 to return an integer instead of a boolean: https://groups.google.com/forum/#!topic/v8-users/OOjHJrix-cU This integer communicates the property descriptors of the property, instead of just its presence or absence. However, the original contextify code was probably written before this change, and it was not updated when porting to Node.js. Credit to @smikes for the test and the original PR of #885. Fixes: #864 Fixes: #885 PR-URL: #1773 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
The GlobalPropertyQueryCallback was changed in 2010 to return an integer instead of a boolean: https://groups.google.com/forum/#!topic/v8-users/OOjHJrix-cU This integer communicates the property descriptors of the property, instead of just its presence or absence. However, the original contextify code was probably written before this change, and it was not updated when porting to Node.js. Credit to @smikes for the test and the original PR of #885. Fixes: #864 Fixes: #885 PR-URL: #1773 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
The GlobalPropertyQueryCallback was changed in 2010 to return an integer instead of a boolean: https://groups.google.com/forum/#!topic/v8-users/OOjHJrix-cU This integer communicates the property descriptors of the property, instead of just its presence or absence. However, the original contextify code was probably written before this change, and it was not updated when porting to Node.js. Credit to @smikes for the test and the original PR of #885. Fixes: #864 Fixes: #885 PR-URL: #1773 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
The GlobalPropertyQueryCallback was changed in 2010 to return an integer instead of a boolean: https://groups.google.com/forum/#!topic/v8-users/OOjHJrix-cU This integer communicates the property descriptors of the property, instead of just its presence or absence. However, the original contextify code was probably written before this change, and it was not updated when porting to Node.js. Credit to @smikes for the test and the original PR of #885. Fixes: #864 Fixes: #885 PR-URL: #1773 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
The GlobalPropertyQueryCallback was changed in 2010 to return an integer instead of a boolean: https://groups.google.com/forum/#!topic/v8-users/OOjHJrix-cU This integer communicates the property descriptors of the property, instead of just its presence or absence. However, the original contextify code was probably written before this change, and it was not updated when porting to Node.js. Credit to @smikes for the test and the original PR of #885. Fixes: #864 Fixes: #885 PR-URL: #1773 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
The GlobalPropertyQueryCallback was changed in 2010 to return an integer instead of a boolean: https://groups.google.com/forum/#!topic/v8-users/OOjHJrix-cU This integer communicates the property descriptors of the property, instead of just its presence or absence. However, the original contextify code was probably written before this change, and it was not updated when porting to Node.js. Credit to @smikes for the test and the original PR of #885. Fixes: #864 Fixes: #885 PR-URL: #1773 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
The GlobalPropertyQueryCallback was changed in 2010 to return an integer instead of a boolean: https://groups.google.com/forum/#!topic/v8-users/OOjHJrix-cU This integer communicates the property descriptors of the property, instead of just its presence or absence. However, the original contextify code was probably written before this change, and it was not updated when porting to Node.js. Credit to @smikes for the test and the original PR of #885. Fixes: #864 Fixes: #885 PR-URL: #1773 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
The GlobalPropertyQueryCallback was changed in 2010 to return an integer instead of a boolean: https://groups.google.com/forum/#!topic/v8-users/OOjHJrix-cU This integer communicates the property descriptors of the property, instead of just its presence or absence. However, the original contextify code was probably written before this change, and it was not updated when porting to Node.js. Credit to @smikes for the test and the original PR of #885. Fixes: #864 Fixes: #885 PR-URL: #1773 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
The GlobalPropertyQueryCallback was changed in 2010 to return an integer instead of a boolean: https://groups.google.com/forum/#!topic/v8-users/OOjHJrix-cU This integer communicates the property descriptors of the property, instead of just its presence or absence. However, the original contextify code was probably written before this change, and it was not updated when porting to Node.js. Credit to @smikes for the test and the original PR of #885. Fixes: #864 Fixes: #885 PR-URL: #1773 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Here's a failing test for issue #864