-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
Comments
/cc @nodejs/ctc this is a second regression reported from this change. |
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 |
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 |
@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. |
This reverts commit 524f693. Fixes: nodejs#10806 Fixes: nodejs#10492 Ref: nodejs#10227
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`.
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>
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>
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>
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>
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>
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>
Repro:
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
The text was updated successfully, but these errors were encountered: