src: fix vm enumerator callbacks #22836

addaleax · 2018-09-13T08:32:18Z

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), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

@nodejs/vm

nodejs-github-bot · 2018-09-13T08:32:19Z

@addaleax build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/875/pipeline

addaleax · 2018-09-13T08:44:12Z

Looks like this also fixes a known-issues test? :)

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

BridgeAR · 2018-09-13T08:45:11Z

test/parallel/test-vm-object-keys.js

+
+assert.deepStrictEqual(
+  Object.keys(proxied).sort(),
+  ['0', '1', 'not', '0.5', '-1'].sort());


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.

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)

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.

Would you be so kind and add a comment that this is against the spec and should be fixed?

@BridgeAR does b9ec6b825844 sound okay?

jdalton · 2018-09-13T15:11:05Z

@addaleax

What had to be changed was that inherited properties are no longer
included;

No longer included in/from what?
Is this related to #22390 and the V8 change (@camillobruni) or the backport (@TimothyGu)?

jdalton

The property order issue raised is an indicator that there's a bug hiding somewhere.

addaleax · 2018-09-13T20:56:43Z

@jdalton Just to be clear, the current behavior is to hard-crash.

BridgeAR · 2018-09-13T21:00:55Z

test/parallel/test-vm-object-keys.js

+
+assert.deepStrictEqual(
+  Object.keys(proxied).sort(),
+  ['0', '1', 'not', '0.5', '-1'].sort());


Would you be so kind and add a comment that this is against the spec and should be fixed?

addaleax · 2018-09-17T15:25:58Z

Ping @jdalton – is your objection standing, even though I’d say this is strictly an improvement over crashing the process?

jdalton · 2018-09-17T15:38:34Z

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?

addaleax · 2018-09-17T15:45:51Z

@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.

jdalton · 2018-09-17T17:18:34Z

@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

ryzokuken · 2018-10-27T14:26:40Z

@addaleax is this abandoned? Do you want someone else to pick this up?

fhinkel · 2019-10-28T23:31:57Z

I'm cleaning out a few old PRs 🧹. I'm closing this due to inactivity. Please re-open if needed!

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 Sep 13, 2018

addaleax requested review from devsnek and TimothyGu September 13, 2018 08:32

addaleax mentioned this pull request Sep 13, 2018

Abort during Object.keys after vm.runInContext #22723

Closed

BridgeAR reviewed Sep 13, 2018

View reviewed changes

jasnell approved these changes Sep 13, 2018

View reviewed changes

devsnek approved these changes Sep 13, 2018

View reviewed changes

jdalton suggested changes Sep 13, 2018

View reviewed changes

BridgeAR approved these changes Sep 13, 2018

View reviewed changes

addaleax mentioned this pull request Sep 17, 2018

Revert "src: implement query callbacks for vm" #22911

Closed

2 tasks

TimothyGu and others added 2 commits September 18, 2018 15:50

addaleax force-pushed the fix-22723 branch from b9ec6b8 to 276ad5e Compare September 18, 2018 13:51

addaleax added the wip Issues and PRs that are still a work in progress. label Sep 24, 2018

addaleax added the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label Nov 15, 2018

Trott force-pushed the master branch from 1ecc406 to 49cf67e Compare September 17, 2019 16:52

fhinkel closed this Oct 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: fix vm enumerator callbacks #22836

src: fix vm enumerator callbacks #22836

addaleax commented Sep 13, 2018

nodejs-github-bot commented Sep 13, 2018

addaleax commented Sep 13, 2018

BridgeAR Sep 13, 2018

addaleax Sep 13, 2018

BridgeAR Sep 13, 2018

BridgeAR Sep 13, 2018

addaleax Sep 13, 2018

BridgeAR Sep 13, 2018

jdalton commented Sep 13, 2018 •

edited

Loading

jdalton left a comment

addaleax commented Sep 13, 2018

BridgeAR Sep 13, 2018

addaleax commented Sep 17, 2018

jdalton commented Sep 17, 2018 •

edited

Loading

addaleax commented Sep 17, 2018

jdalton commented Sep 17, 2018

ryzokuken commented Oct 27, 2018

fhinkel commented Oct 28, 2019

src: fix vm enumerator callbacks #22836

src: fix vm enumerator callbacks #22836

Conversation

addaleax commented Sep 13, 2018

Checklist

nodejs-github-bot commented Sep 13, 2018

addaleax commented Sep 13, 2018

BridgeAR Sep 13, 2018

Choose a reason for hiding this comment

addaleax Sep 13, 2018

Choose a reason for hiding this comment

BridgeAR Sep 13, 2018

Choose a reason for hiding this comment

BridgeAR Sep 13, 2018

Choose a reason for hiding this comment

addaleax Sep 13, 2018

Choose a reason for hiding this comment

BridgeAR Sep 13, 2018

Choose a reason for hiding this comment

jdalton commented Sep 13, 2018 • edited Loading

jdalton left a comment

Choose a reason for hiding this comment

addaleax commented Sep 13, 2018

BridgeAR Sep 13, 2018

Choose a reason for hiding this comment

addaleax commented Sep 17, 2018

jdalton commented Sep 17, 2018 • edited Loading

addaleax commented Sep 17, 2018

jdalton commented Sep 17, 2018

ryzokuken commented Oct 27, 2018

fhinkel commented Oct 28, 2019

jdalton commented Sep 13, 2018 •

edited

Loading

jdalton commented Sep 17, 2018 •

edited

Loading