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

vm: fix a variety of bugs, using V8 4.3 APIs #1773

Closed
wants to merge 3 commits into from

Conversation

domenic
Copy link
Contributor

@domenic domenic commented May 22, 2015

See individual commits for details.

@brendanashworth brendanashworth added the vm Issues and PRs related to the vm subsystem. label May 22, 2015
if (!in_sandbox || !in_proxy_global) {
if (sandbox->HasRealNamedProperty(property)) {
PropertyAttribute propAttr =
sandbox->GetRealNamedPropertyAttributes(property).FromJust();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The .FromJust() here feels bad. I am not sure what the right way to handle these new maybes is.

Maybe if they're empty I should return None?

@domenic
Copy link
Contributor Author

domenic commented May 23, 2015

Added another commit while I was in there; kept it separate.

@domenic
Copy link
Contributor Author

domenic commented May 23, 2015

Morphing this into a general "fix all the vm bugs" thread.

@domenic domenic changed the title vm: fix property descriptors of sandbox properties vm: fix a variety of bugs, using V8 4.3 APIs May 23, 2015
@brendanashworth brendanashworth added the c++ Issues and PRs that require attention from people who are familiar with C++. label May 25, 2015
if (!in_sandbox || !in_proxy_global) {
args.GetReturnValue().Set(None);
bool in_sandbox =
sandbox->HasRealNamedProperty(ctx->context(), property).FromMaybe(false);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to rewrite this to be a little less crazy now that, as of v8/v8@7b24219, GetRealNamedProperty returns Nothing for absent, which allows us to only use GetRealNamedProperty instead of both HasRealNamedProperty and GetRealNamedProperty.

@domenic
Copy link
Contributor Author

domenic commented Jun 1, 2015

Rebased on latest next branch, and with some better Maybe-hygeine in the last commit. Would love to land this.

R=@bnoordhuis, @indutny

if (!in_sandbox || !in_proxy_global) {
args.GetReturnValue().Set(None);

Maybe<PropertyAttribute> maybePropAttr =
Copy link
Member

Choose a reason for hiding this comment

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

May I ask you to use underscores instead of camel case? This way it will be more consistent with the rest of the core and this file in particular ;)

@indutny
Copy link
Member

indutny commented Jun 1, 2015

Few nits otherwise LGTM! Thank you! Guess we need to land V8 update first? ;)

@domenic
Copy link
Contributor Author

domenic commented Jun 1, 2015

@indutny fixed nits and lint errors; thanks for that. The next branch has a floating V8 backport patch for now so this works on top of the existing next.

Local<Value> rv = sandbox->GetRealNamedProperty(property);
if (rv.IsEmpty()) {
MaybeLocal<Value> maybe_rv =
sandbox->GetRealNamedProperty(ctx->context(), property);
Copy link
Member

Choose a reason for hiding this comment

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

Femto-nit: can you indent line continuations with four spaces?

EDIT: Here and everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

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 nodejs#885.

Fixes nodejs#885; fixes nodejs#864.
No reason to install access checks if they're always going to return
true.
By using the new SetHandler API instead of SetNamedPropertyHandler, we can
intercept symbols now. This forces us to use Maybes and MaybeLocals more,
since this new API does not have a non-maybe variant.

Fixes nodejs#884.
domenic added a commit that referenced this pull request Jun 17, 2015
By using the new SetHandler API instead of SetNamedPropertyHandler, we can
intercept symbols now. This forces us to use Maybes and MaybeLocals more,
since this new API does not have a non-maybe variant.

Fixes: #884
PR-URL: #1773
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
domenic added a commit that referenced this pull request Jul 22, 2015
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>
domenic added a commit that referenced this pull request Jul 22, 2015
No reason to install access checks if they're always going to return
true.

PR-URL: #1773
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
domenic added a commit that referenced this pull request Jul 22, 2015
By using the new SetHandler API instead of SetNamedPropertyHandler, we can
intercept symbols now. This forces us to use Maybes and MaybeLocals more,
since this new API does not have a non-maybe variant.

Fixes: #884
PR-URL: #1773
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@rvagg rvagg mentioned this pull request Jul 22, 2015
domenic added a commit that referenced this pull request Jul 24, 2015
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>
domenic added a commit that referenced this pull request Jul 24, 2015
No reason to install access checks if they're always going to return
true.

PR-URL: #1773
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
domenic added a commit that referenced this pull request Jul 24, 2015
By using the new SetHandler API instead of SetNamedPropertyHandler, we can
intercept symbols now. This forces us to use Maybes and MaybeLocals more,
since this new API does not have a non-maybe variant.

Fixes: #884
PR-URL: #1773
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
domenic added a commit that referenced this pull request Jul 30, 2015
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>
domenic added a commit that referenced this pull request Jul 30, 2015
No reason to install access checks if they're always going to return
true.

PR-URL: #1773
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
domenic added a commit that referenced this pull request Jul 30, 2015
By using the new SetHandler API instead of SetNamedPropertyHandler, we can
intercept symbols now. This forces us to use Maybes and MaybeLocals more,
since this new API does not have a non-maybe variant.

Fixes: #884
PR-URL: #1773
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
domenic added a commit that referenced this pull request Aug 1, 2015
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>
domenic added a commit that referenced this pull request Aug 1, 2015
No reason to install access checks if they're always going to return
true.

PR-URL: #1773
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
domenic added a commit that referenced this pull request Aug 1, 2015
By using the new SetHandler API instead of SetNamedPropertyHandler, we can
intercept symbols now. This forces us to use Maybes and MaybeLocals more,
since this new API does not have a non-maybe variant.

Fixes: #884
PR-URL: #1773
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
domenic added a commit that referenced this pull request Aug 3, 2015
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>
domenic added a commit that referenced this pull request Aug 3, 2015
No reason to install access checks if they're always going to return
true.

PR-URL: #1773
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
domenic added a commit that referenced this pull request Aug 3, 2015
By using the new SetHandler API instead of SetNamedPropertyHandler, we can
intercept symbols now. This forces us to use Maybes and MaybeLocals more,
since this new API does not have a non-maybe variant.

Fixes: #884
PR-URL: #1773
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
domenic added a commit that referenced this pull request Aug 4, 2015
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>
domenic added a commit that referenced this pull request Aug 4, 2015
No reason to install access checks if they're always going to return
true.

PR-URL: #1773
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
domenic added a commit that referenced this pull request Aug 4, 2015
By using the new SetHandler API instead of SetNamedPropertyHandler, we can
intercept symbols now. This forces us to use Maybes and MaybeLocals more,
since this new API does not have a non-maybe variant.

Fixes: #884
PR-URL: #1773
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
domenic added a commit that referenced this pull request Aug 4, 2015
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>
domenic added a commit that referenced this pull request Aug 4, 2015
No reason to install access checks if they're always going to return
true.

PR-URL: #1773
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
domenic added a commit that referenced this pull request Aug 4, 2015
By using the new SetHandler API instead of SetNamedPropertyHandler, we can
intercept symbols now. This forces us to use Maybes and MaybeLocals more,
since this new API does not have a non-maybe variant.

Fixes: #884
PR-URL: #1773
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@rvagg rvagg mentioned this pull request Aug 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants