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

Node 10.0.0 Object.values() returns incorrect array #20278

Closed
raymondfeng opened this issue Apr 25, 2018 · 9 comments
Closed

Node 10.0.0 Object.values() returns incorrect array #20278

raymondfeng opened this issue Apr 25, 2018 · 9 comments
Labels
confirmed-bug Issues with confirmed bugs. v8 engine Issues and PRs related to the V8 dependency.

Comments

@raymondfeng
Copy link
Contributor

raymondfeng commented Apr 25, 2018

  • Version: v10.0.0
  • Platform: Darwin Kernel Version 17.5.0: Mon Mar 5 22:24:32 PST 2018; root:xnu-4570.51.1~1/RELEASE_X86_64 x86_64

To reproduce the problem, create the following js files:

  1. values.js
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
class X {
}
exports.X = X;
  1. test.js
const x = require('./values.js');
console.log(x);
console.log(Object.values(x));
  1. test1.js
const x = require('./values.js');
console.log(Object.values(x));

Now node test prints:

{ X: [Function: X] }
[]

Interestingly, node test1 prints:

[ [Function: X] ]

Please note Object.values(x) in test.js returns []. I expect it to print:

{ X: [Function: X] }
[ [Function: X] ]

I tried Node 8.x, both test and test1 work as expected.

@apapirovski
Copy link
Member

@AyushG3112 Are you sure? I can definitely reproduce, on multiple platforms.

@AyushG3112
Copy link
Contributor

AyushG3112 commented Apr 25, 2018

@apapirovski nevermind, looks like my PC had some version conflicts while installing v10.0.0 from nvm. Will try fixing those and will try again. (deleting previous comment till then)

@apapirovski
Copy link
Member

Ouch...

'use strict';
const obj = {};
Object.defineProperty(obj, 'ibreakyourobjects', { value: true });
obj.X = true;
console.log(Object.values(obj));
// [ true ]
Object.keys(obj);
console.log(Object.values(obj));
// []

ping @nodejs/v8 this looks bad to me...

@apapirovski
Copy link
Member

Also, adding enumerable: true on ibreakyourobjects fixes the bug... (setting it to false breaks it again)

@ChALkeR
Copy link
Member

ChALkeR commented Apr 25, 2018

I can definitely reproduce this in Chromium 66.0.3359.117.

@ChALkeR ChALkeR added the v8 engine Issues and PRs related to the V8 dependency. label Apr 25, 2018
@apapirovski
Copy link
Member

apapirovski commented Apr 25, 2018

Also, swapping order of declarations like so:

'use strict';
const obj = {};
obj.X = true;
Object.defineProperty(obj, 'ibreakyourobjects', { value: true });
obj.Y =  true;
console.log(Object.values(obj));
// [ true, true ]
Object.keys(obj);
console.log(Object.values(obj));
// [ true ]

Note how the one before the defineProperty is still there but the one after isn't.

Edit: I can reproduce on 67.0.3396.10

@hashseed
Copy link
Member

I can reproduce in V8, and will bisect.

@targos
Copy link
Member

targos commented Apr 25, 2018

I can reproduce with V8 canary.

Opened https://bugs.chromium.org/p/v8/issues/detail?id=7687 upstream.

@benjamingr benjamingr added the confirmed-bug Issues with confirmed bugs. label Apr 25, 2018
raymondfeng added a commit to loopbackio/loopback-next that referenced this issue Apr 25, 2018
- Replace new Buffer() with Buffer.from()
- Work around nodejs/node#20278
- Upgrade to source-map-support@0.5.5
@xaviergonz
Copy link
Contributor

xaviergonz commented Apr 25, 2018

raymondfeng added a commit to loopbackio/loopback-next that referenced this issue Apr 25, 2018
- Replace new Buffer() with Buffer.from()
- Work around nodejs/node#20278
- Upgrade to source-map-support@0.5.5
targos added a commit to targos/node that referenced this issue Apr 27, 2018
Original commit message:

    Fix Object.entries/.values with non-enumerable properties

    Iterate over all descriptors instead of bailing out early and missing
    enumerable properties later.

    Bug: chromium:836145
    Change-Id: I104f7ea89480383b6b4b9204942a166bdf8e0597
    Reviewed-on: https://chromium-review.googlesource.com/1027832
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Commit-Queue: Camillo Bruni <cbruni@chromium.org>
    Cr-Commit-Position: refs/heads/master@{nodejs#52786}

Refs: v8/v8@76cab5f
Fixes: nodejs#20278
@targos targos closed this as completed in 9c8b479 May 2, 2018
MylesBorins pushed a commit that referenced this issue May 4, 2018
Original commit message:

    Fix Object.entries/.values with non-enumerable properties

    Iterate over all descriptors instead of bailing out early and missing
    enumerable properties later.

    Bug: chromium:836145
    Change-Id: I104f7ea89480383b6b4b9204942a166bdf8e0597
    Reviewed-on: https://chromium-review.googlesource.com/1027832
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Commit-Queue: Camillo Bruni <cbruni@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#52786}

Refs: v8/v8@76cab5f
Fixes: #20278

PR-URL: #20350
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

No branches or pull requests

8 participants