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

src: fix delete operator on vm context by fhinkel · Pull Request #11266 · nodejs/node · GitHub

/ node Public

src: fix delete operator on vm context - #11266

Closed
fhinkel wants to merge 1 commit into
nodejs:masterfrom
fhinkel:vm-fix-delete
ConversationCommits (1)ChecksFiles changed
Closed

src: fix delete operator on vm context#11266
fhinkel wants to merge 1 commit into
nodejs:masterfrom
fhinkel:vm-fix-delete

Conversation

fhinkel commented Feb 9, 2017

Copy link
Copy Markdown
Contributor

In the implementation of the vm module, if a property is successfully deleted
on the sandbox, we also need to delete it on the global_proxy object. Therefore, we
must not call args.GetReturnValue().Set().

We only intercept, i.e., call args.GetReturnValue().Set(), in the
DeleterCallback, if Delete() failed, e.g. because
the property was read only.

This is a breaking change, it fixes one of our known issues. But it does not break any
tests in Jest.

Fixes #6287

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

src

If anybody has a better way to word the commit message, I'll happily take it!

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. vm Issues and PRs related to the vm subsystem. labels Feb 9, 2017
addaleax approved these changes Feb 9, 2017
View reviewed changes

addaleax left a comment

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

LGTM

The commit message seems okay … maybe “delete on vm context”? I think it’s clear what it refers to anyway.

Comment thread src/node_contextify.cc Outdated

addaleax Feb 9, 2017

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

Maybe if (success.FromMaybe(false)) is a bit more readable?

fhinkel Feb 10, 2017

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

Thanks. Done.

bnoordhuis approved these changes Feb 10, 2017
View reviewed changes

bnoordhuis left a comment

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

LGTM. A propos the commit log, my first thought was that it was fixing a bad delete C++ statement. Maybe you can reword it a little, perhaps s/delete/delete operator/?

fhinkel changed the title src: fix delete in vm module src: fix delete operator in vm module Feb 10, 2017
fhinkel changed the title src: fix delete operator in vm module src: fix delete operator on vm context Feb 10, 2017
src: fix delete operator on vm context
7c00321 
In the implementation of the vm module,
if a property is successfully deleted
on the sandbox, we also need to delete it
on the global_proxy object. Therefore, we
must not call args.GetReturnValue().Set().

We only intercept, i.e., call
args.GetReturnValue().Set(), in the
DeleterCallback, if Delete() failed, e.g. because
the property was read only.

Fixes nodejs#6287

fhinkel commented Feb 10, 2017

Copy link
Copy Markdown
Contributor Author

CI https://ci.nodejs.org/job/node-test-pull-request/6336/

jasnell pushed a commit that referenced this pull request Feb 11, 2017
src: fix delete operator on vm context
0237198 
In the implementation of the vm module,
if a property is successfully deleted
on the sandbox, we also need to delete it
on the global_proxy object. Therefore, we
must not call args.GetReturnValue().Set().

We only intercept, i.e., call
args.GetReturnValue().Set(), in the
DeleterCallback, if Delete() failed, e.g. because
the property was read only.

PR-URL: #11266
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

jasnell commented Feb 11, 2017

Copy link
Copy Markdown
Member

Landed in 0237198

jasnell closed this Feb 11, 2017
italoacasas pushed a commit that referenced this pull request Feb 13, 2017
src: fix delete operator on vm context
c1a24a5 
In the implementation of the vm module,
if a property is successfully deleted
on the sandbox, we also need to delete it
on the global_proxy object. Therefore, we
must not call args.GetReturnValue().Set().

We only intercept, i.e., call
args.GetReturnValue().Set(), in the
DeleterCallback, if Delete() failed, e.g. because
the property was read only.

PR-URL: #11266
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
italoacasas mentioned this pull request Feb 13, 2017
Merged
fhinkel mentioned this pull request Feb 13, 2017
Closed
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 14, 2017
src: fix delete operator on vm context
308df11 
In the implementation of the vm module,
if a property is successfully deleted
on the sandbox, we also need to delete it
on the global_proxy object. Therefore, we
must not call args.GetReturnValue().Set().

We only intercept, i.e., call
args.GetReturnValue().Set(), in the
DeleterCallback, if Delete() failed, e.g. because
the property was read only.

PR-URL: nodejs#11266
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
krydos pushed a commit to krydos/node that referenced this pull request Feb 25, 2017
src: fix delete operator on vm context
ff07006 
In the implementation of the vm module,
if a property is successfully deleted
on the sandbox, we also need to delete it
on the global_proxy object. Therefore, we
must not call args.GetReturnValue().Set().

We only intercept, i.e., call
args.GetReturnValue().Set(), in the
DeleterCallback, if Delete() failed, e.g. because
the property was read only.

PR-URL: nodejs#11266
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

jasnell commented Mar 7, 2017

Copy link
Copy Markdown
Member

A backport PR is required for this to land in v4

jasnell pushed a commit that referenced this pull request Mar 7, 2017
src: fix delete operator on vm context
fe9bf31 
In the implementation of the vm module,
if a property is successfully deleted
on the sandbox, we also need to delete it
on the global_proxy object. Therefore, we
must not call args.GetReturnValue().Set().

We only intercept, i.e., call
args.GetReturnValue().Set(), in the
DeleterCallback, if Delete() failed, e.g. because
the property was read only.

PR-URL: #11266
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
src: fix delete operator on vm context
587857e 
In the implementation of the vm module,
if a property is successfully deleted
on the sandbox, we also need to delete it
on the global_proxy object. Therefore, we
must not call args.GetReturnValue().Set().

We only intercept, i.e., call
args.GetReturnValue().Set(), in the
DeleterCallback, if Delete() failed, e.g. because
the property was read only.

PR-URL: #11266
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins mentioned this pull request Mar 9, 2017
Merged
boujeepossum mentioned this pull request Mar 17, 2017
Closed
gdborton added a commit to gdborton/enzyme that referenced this pull request Mar 23, 2017
Fix an issue w/ cleaning up global.document.
cc05b8a 
Except in the most recent versions of node, developers are unable to delete
things from global when running code in a node vm.  This means that enzyme is
failing to clean up after itself when temporarily defining global.document for
setState calls.

This papers over the issue by explicitly setting global.document to undefined
if it failed to be deleted.

Relevant jest issue - jestjs/jest#3152

Relevant node pr - nodejs/node#11266
gdborton mentioned this pull request Mar 23, 2017
Merged
gdborton added a commit to gdborton/enzyme that referenced this pull request Mar 24, 2017
Fix an issue w/ cleaning up global.document.
9634269 
Except in the most recent versions of node, developers are unable to delete
things from global when running code in a node vm.  This means that enzyme is
failing to clean up after itself when temporarily defining global.document for
setState calls.

This papers over the issue by explicitly setting global.document to undefined
before attempting to delete. If the delete succeeds, the undefined value will
be removed.

Relevant jest issue - jestjs/jest#3152

Relevant node pr - nodejs/node#11266
joyeecheung mentioned this pull request Mar 28, 2017
Closed
brettz9 mentioned this pull request Apr 24, 2017
Closed
jasonLaster mentioned this pull request Apr 25, 2017
Closed

MylesBorins commented Apr 25, 2017

Copy link
Copy Markdown
Contributor

So it seems that this is causing a failure over at firefox-devtools/debugger#2728

I'm not sure if this is a behavior change on our side, or if the repo in question was relying on the broken behavior

/cc @fhinkel

MylesBorins commented Apr 25, 2017

Copy link
Copy Markdown
Contributor

Also worth mentioning... this changed behavior, should we back this out of v6 LTS?

fhinkel commented Apr 26, 2017

Copy link
Copy Markdown
Contributor Author

Let's leave it out of v6 because it's a breaking change.

thymikee mentioned this pull request Oct 5, 2017
Closed
domenic mentioned this pull request Feb 2, 2018
Closed
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

bnoordhuis bnoordhuis approved these changes
jasnell jasnell approved these changes
addaleax addaleax approved these changes

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.

deleting property in vm context has no effect

6 participants

Back | FazBrowse Home | New Git URL