vm regression: assigning a property the first time does not work; only the second time

@fhinkel

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

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

@bnoordhuis

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?

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.

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.

@bnoordhuis

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

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