-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
src: fix vm enumerator callbacks #22836
Conversation
Looks like this also fixes a known-issues test? :) |
|
||
assert.deepStrictEqual( | ||
Object.keys(proxied).sort(), | ||
['0', '1', 'not', '0.5', '-1'].sort()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the sorting necessary? Object.keys()
and Object.getOwnPropertyNames()
should always return the properties in the order you already expect them to be. Everything else is likely a bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It fails if I don’t do this, even though this is the order in which they were declared. I’m not sure on the “why” of that, though. (i.e. this is also a question for @nodejs/vm)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By spec the return value is:
- all indices (integers >= 0) in ascending order
- all other non-symbol properties in their insertion order
- all symbol properties in their insertion order
So if something else is returned, we should probably look into that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you be so kind and add a comment that this is against the spec and should be fixed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BridgeAR does b9ec6b825844 sound okay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes :)
No longer included in/from what? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The property order issue raised is an indicator that there's a bug hiding somewhere.
@jdalton Just to be clear, the current behavior is to hard-crash. |
|
||
assert.deepStrictEqual( | ||
Object.keys(proxied).sort(), | ||
['0', '1', 'not', '0.5', '-1'].sort()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you be so kind and add a comment that this is against the spec and should be fixed?
Ping @jdalton – is your objection standing, even though I’d say this is strictly an improvement over crashing the process? |
I believe my objection stands. If I understand correctly the VM enumerator callback patch was reverted because of issues this PR is attempting to fix. So there is no crash in production or stable bits? Now you're presenting a fix but one that is buggy and asking if I think it's better than the crash scenario that isn't in production or a pending release? |
@jdalton The VM enumerator callback patch was not reverted at this point, no – but we could still do that. Would you prefer to do that, then for me to rebase this PR on top of it and wait until we’ve ironed everything out? That would be okay with me, just indicate your preference. |
@addaleax Ah ok! Thank you for clarifying things for me. I think that it would be best to revert the patch, and then iron out things to a greater extent, before pushing it out in a release. |
This reverts commit 85c356c from PR nodejs#22390. See the discussion in the (proposed) fix at nodejs#22836. Refs: nodejs#22836 Refs: nodejs#22390 Fixes: nodejs#22723
This reverts commit 85c356c from PR #22390. See the discussion in the (proposed) fix at #22836. Refs: #22836 Refs: #22390 Fixes: #22723 PR-URL: #22911 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: John-David Dalton <john.david.dalton@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
This is a re-land of a commit landed as part of nodejs#22390. --- This allows using a Proxy object as the sandbox for a VM context. Refs: nodejs#22390 Fixes: nodejs#17480 Fixes: nodejs#17481 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: James M Snell <jasnell@gmail.com>
Some of the choices here are odd, including that symbols are missing. However, that matches previous behaviour. What had to be changed was that inherited properties are no longer included; the alternative would be to also refactor the descriptor callbacks to provide data for inherited properties. Fixes: nodejs#22723 Refs: nodejs#22390
@addaleax is this abandoned? Do you want someone else to pick this up? |
I'm cleaning out a few old PRs 🧹. I'm closing this due to inactivity. Please re-open if needed! |
Some of the choices here are odd, including that symbols are missing.
However, that matches previous behaviour.
What had to be changed was that inherited properties are no longer
included; the alternative would be to also refactor the descriptor
callbacks to provide data for inherited properties.
Fixes: #22723
Refs: #22390
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes@nodejs/vm