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 regression: assigning a property the first time does not work; only the second time #10806

Closed
domenic opened this issue Jan 14, 2017 · 6 comments
Labels
vm Issues and PRs related to the vm subsystem.

Comments

@domenic
Copy link
Contributor

domenic commented Jan 14, 2017

  • Version: v7.4.0
  • Platform: Windows 10 64-bit
  • Subsystem: vm

Repro:

"use strict";
const vm = require("vm");
const ctx = vm.createContext({ open() { } });
const window = vm.runInContext("this", ctx);
const other = 123;

console.log(window.open === other);
window.open = other;
console.log(window.open === other);
window.open = other;
console.log(window.open === other);

this outputs false, false, true in v7.4.0, whereas in v7.2.1 it output the expected false, true, true. I am guessing this is another regression (see also #10492) caused by 524f693. Maybe it is even the same issue, but here is a minimal repro.

/cc @fhinkel @bnoordhuis @MylesBorins

Found via jsdom/jsdom#1703

@Fishrock123 Fishrock123 added the vm Issues and PRs related to the vm subsystem. label Jan 14, 2017
@MylesBorins
Copy link
Contributor

/cc @nodejs/ctc this is a second regression reported from this change.

@fhinkel
Copy link
Member

fhinkel commented Jan 14, 2017

Should we revert @bnoordhuis 's change so we don't break behavior? Once V8 5.5 lands, @AnnaMag is working on removing the CopyProperties() hack, maybe then we can tackle the non-writable properties again?

@joyeecheung
Copy link
Member

I think this is the same issue as #10492 . Removing "use strict"; in the repro example would output false, true, true in v7.4.0 just as in v7.2.1.

@bnoordhuis
Copy link
Member

I've been looking at this earlier this week. It might be possible to fix this properly in v6.x/v7.x and maybe v4.x. The assumption behind CopyProperties() is that Object.defineProperty() doesn't result in a call to the global object setter but that hasn't been true for some time.

@addaleax
Copy link
Member

@bnoordhuis The question remains, does it make sense to revert your original changes first?

@bnoordhuis
Copy link
Member

Sure. It didn't even have to be a question; a revert is always justifiable when there is a regression.

addaleax added a commit to addaleax/node that referenced this issue Jan 20, 2017
addaleax added a commit to addaleax/node that referenced this issue Jan 20, 2017
Add the regression test script presented in
nodejs#10806 to `test/parallel` and
re-add the original regression test for
nodejs#10223 in `test/known_issues`.
addaleax added a commit that referenced this issue Jan 25, 2017
Add the regression test script presented in
#10806 to `test/parallel` and
re-add the original regression test for
#10223 in `test/known_issues`.

PR-URL: #10920
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
targos pushed a commit that referenced this issue Jan 28, 2017
This reverts commit 524f693.

Fixes: #10806
Fixes: #10492
Ref: #10227
PR-URL: #10920
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
targos pushed a commit that referenced this issue Jan 28, 2017
Add the regression test script presented in
#10806 to `test/parallel` and
re-add the original regression test for
#10223 in `test/known_issues`.

PR-URL: #10920
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
italoacasas pushed a commit to italoacasas/node that referenced this issue Jan 30, 2017
This reverts commit 524f693.

Fixes: nodejs#10806
Fixes: nodejs#10492
Ref: nodejs#10227
PR-URL: nodejs#10920
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
italoacasas pushed a commit to italoacasas/node that referenced this issue Jan 30, 2017
Add the regression test script presented in
nodejs#10806 to `test/parallel` and
re-add the original regression test for
nodejs#10223 in `test/known_issues`.

PR-URL: nodejs#10920
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
italoacasas pushed a commit to italoacasas/node that referenced this issue Jan 30, 2017
This reverts commit 524f693.

Fixes: nodejs#10806
Fixes: nodejs#10492
Ref: nodejs#10227
PR-URL: nodejs#10920
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
italoacasas pushed a commit to italoacasas/node that referenced this issue Jan 30, 2017
Add the regression test script presented in
nodejs#10806 to `test/parallel` and
re-add the original regression test for
nodejs#10223 in `test/known_issues`.

PR-URL: nodejs#10920
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

No branches or pull requests

7 participants