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.

Sign up for GitHub

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

Jump to bottom

src: fix delete operator on vm context #11266

Closed
wants to merge 1 commit into from
Closed

src: fix delete operator on vm context #11266

wants to merge 1 commit into from

Conversation

fhinkel
Copy link
Member

@fhinkel fhinkel commented Feb 9, 2017

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 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
addaleax approved these changes Feb 9, 2017
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM

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

src/node_contextify.cc Outdated
@@ -456,7 +456,7 @@ class ContextifyContext {

Maybe<bool> success = ctx->sandbox()->Delete(ctx->context(), property);

if (success.IsJust())
if (!success.IsJust() || !success.FromJust())
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. Done.

bnoordhuis
bnoordhuis approved these changes Feb 10, 2017
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

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 fhinkel changed the title src: fix delete in vm module src: fix delete operator in vm module Feb 10, 2017
@fhinkel fhinkel changed the title src: fix delete operator in vm module src: fix delete operator on vm context Feb 10, 2017
@fhinkel
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
Copy link
Member Author

fhinkel commented Feb 10, 2017

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

jasnell pushed a commit that referenced this pull request Feb 11, 2017
@fhinkel @jasnell
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
Copy link
Member

jasnell commented Feb 11, 2017

Landed in 0237198

@jasnell jasnell closed this Feb 11, 2017
italoacasas pushed a commit that referenced this pull request Feb 13, 2017
@fhinkel @italoacasas
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 italoacasas mentioned this pull request Feb 13, 2017
Merged
@fhinkel fhinkel mentioned this pull request Feb 13, 2017
Closed
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 14, 2017
@fhinkel @italoacasas
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
@fhinkel @krydos
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
Copy link
Member

jasnell commented Mar 7, 2017

A backport PR is required for this to land in v4

jasnell pushed a commit that referenced this pull request Mar 7, 2017
@fhinkel @jasnell
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
@fhinkel @MylesBorins
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 MylesBorins mentioned this pull request Mar 9, 2017
Merged
@aaronabramov aaronabramov mentioned this pull request Mar 17, 2017
Closed
gdborton added a commit to gdborton/enzyme that referenced this pull request Mar 23, 2017
@gdborton
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 gdborton mentioned this pull request Mar 23, 2017
Merged
gdborton added a commit to gdborton/enzyme that referenced this pull request Mar 24, 2017
@gdborton
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 joyeecheung mentioned this pull request Mar 28, 2017
Closed
@brettz9 brettz9 mentioned this pull request Apr 24, 2017
Closed
@jasonLaster jasonLaster mentioned this pull request Apr 25, 2017
Closed
@MylesBorins
Copy link
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
Copy link
Contributor

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

@fhinkel
Copy link
Member Author

fhinkel commented Apr 26, 2017

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

@thymikee thymikee mentioned this pull request Oct 5, 2017
Closed
@domenic domenic mentioned this pull request Feb 2, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis 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
@fhinkel @jasnell @MylesBorins @bnoordhuis @addaleax @nodejs-github-bot