FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

vm: fix a variety of bugs, using V8 4.3 APIs by domenic · Pull Request #1773 · nodejs/node · GitHub

/ node Public

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

Closed
domenic wants to merge 3 commits into
nodejs:nextfrom
domenic:fix-vm-propdescs
ConversationCommits (3)ChecksFiles changed
Closed

vm: fix a variety of bugs, using V8 4.3 APIs#1773
domenic wants to merge 3 commits into
nodejs:nextfrom
domenic:fix-vm-propdescs

Conversation

domenic commented May 22, 2015

Copy link
Copy Markdown
Contributor

See individual commits for details.

brendanashworth added the vm Issues and PRs related to the vm subsystem. label May 22, 2015
domenic reviewed May 22, 2015
View reviewed changes
Comment thread src/node_contextify.cc Outdated

domenic May 22, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 mentioned this pull request May 22, 2015
Closed

domenic commented May 23, 2015

Copy link
Copy Markdown
Contributor Author

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

domenic commented May 23, 2015

Copy link
Copy Markdown
Contributor Author

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

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
targos mentioned this pull request May 24, 2015
Closed
brendanashworth added the c++ Issues and PRs that require attention from people who are familiar with C++. label May 25, 2015
domenic reviewed May 26, 2015
View reviewed changes
Comment thread src/node_contextify.cc Outdated

domenic May 26, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

rvagg force-pushed the next branch 2 times, most recently from d85231d to 70716fd Compare May 27, 2015 04:03
domenic mentioned this pull request May 27, 2015
Closed

domenic commented Jun 1, 2015

Copy link
Copy Markdown
Contributor Author

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

R=@bnoordhuis, @indutny

indutny reviewed Jun 1, 2015
View reviewed changes
Comment thread src/node_contextify.cc Outdated

indutny Jun 1, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Jun 1, 2015

Copy link
Copy Markdown
Member

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

domenic commented Jun 1, 2015

Copy link
Copy Markdown
Contributor Author

@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.

bnoordhuis reviewed Jun 1, 2015
View reviewed changes
Comment thread src/node_contextify.cc Outdated

bnoordhuis Jun 1, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

EDIT: Here and everywhere.

domenic Jun 1, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Fixed

domenic added 3 commits June 1, 2015 12:19
vm: fix property descriptors of sandbox properties
99104df 
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 #885; fixes #864.
vm: remove unnecessary access checks
6443054 
No reason to install access checks if they're always going to return
true.
vm: fix symbol access
5f69cda 
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.

domenic commented Jun 1, 2015

Copy link
Copy Markdown
Contributor Author

@bnoordhuis fixed nit, ready for merge if LGTY

bnoordhuis commented Jun 1, 2015

Copy link
Copy Markdown
Member

LGTM. CI: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/748/

domenic mentioned this pull request Jun 2, 2015
Closed
domenic added a commit that referenced this pull request Jun 17, 2015
vm: fix symbol access
84950bc 
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
vm: fix property descriptors of sandbox properties
37a8a54 
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
vm: remove unnecessary access checks
222c8a5 
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
vm: fix symbol access
81f12be 
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 mentioned this pull request Jul 22, 2015
Closed
domenic added a commit that referenced this pull request Jul 24, 2015
vm: fix property descriptors of sandbox properties
d019cb3 
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
vm: remove unnecessary access checks
bf4f6ca 
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
vm: fix symbol access
bbbc552 
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
vm: fix property descriptors of sandbox properties
65dd834 
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
vm: remove unnecessary access checks
29556eb 
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
vm: fix symbol access
416cf7c 
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
vm: fix property descriptors of sandbox properties
9e45e54 
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
vm: remove unnecessary access checks
cceb279 
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
vm: fix symbol access
78a0a15 
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
vm: fix property descriptors of sandbox properties
9b29e24 
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
vm: remove unnecessary access checks
5dcfaa3 
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
vm: fix symbol access
8047015 
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
vm: fix property descriptors of sandbox properties
450ba88 
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
vm: remove unnecessary access checks
5ca9579 
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
vm: fix symbol access
7767b9d 
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
vm: fix property descriptors of sandbox properties
659dadd 
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
vm: remove unnecessary access checks
7b81e4b 
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
vm: fix symbol access
3b021ef 
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 mentioned this pull request Aug 4, 2015
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL