New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 4118 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2015
Cc:
Components:
HW: ----
NextAction: ----
OS: ----
Priority: 2
Type: Bug



Sign in to add a comment

Object.getOwnPropertyNames returns names in wrong order

Project Member Reported by arv@chromium.org, May 15 2015

Issue description

Comment 2 by arv@chromium.org, May 15 2015

To be clear:

  var str = new String("abc");
  str[5] = "de";

  var expected = ["0", "1", "2", "5", "length"];
  var actual = Object.getOwnPropertyNames(str);

our actual is

  ["5", "0", "1", "2", "length"];

I assume this is because "5" becomes a non indexed property?

Comment 3 by adamk@chromium.org, Jul 14 2015

Cc: -arv@chromium.org rossberg@chromium.org
Owner: adamk@chromium.org
Status: Started
5 is an indexed property, but the mapped characters are only virtually present. I guess we mix those in at the wrong time. I presume the same is true for arguments objects with added or reconfigured elements, given that also there there are 2 elements backing stores; and those are even more complex to merge. In both cases there's always a single correct holder though, so we don't need to check for duplicates. (I fixed some bad cornercases wrt to string wrappers obtaining duplicates recently).
Project Member

Comment 5 by bugdroid1@chromium.org, Jul 15 2015

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/1e146c07088d4e0f5c25177be6b2245b562cb929

commit 1e146c07088d4e0f5c25177be6b2245b562cb929
Author: adamk <adamk@chromium.org>
Date: Wed Jul 15 07:31:26 2015

[es6] JSObject::GetOwnElementKeys should collect String wrapper keys first

This makes Object.getOwnPropertyNames() return the integer keys in the
proper order, following the spec:

http://www.ecma-international.org/ecma-262/6.0/#sec-ordinary-object-internal-methods-and-internal-slots-ownpropertykeys

BUG= v8:4118 
LOG=n

Review URL: https://codereview.chromium.org/1228803006

Cr-Commit-Position: refs/heads/master@{#29667}

[modify] http://crrev.com/1e146c07088d4e0f5c25177be6b2245b562cb929/src/objects.cc
[modify] http://crrev.com/1e146c07088d4e0f5c25177be6b2245b562cb929/test/test262-es6/test262-es6.status

Comment 6 by adamk@chromium.org, Jul 15 2015

Cc: verwa...@chromium.org
Status: Fixed
I don't see any issue with arguments:

d8> var f = function() { arguments[5] = 4; return Object.getOwnPropertyNames(arguments) }
undefined
d8> f(1, 2)
["0", "1", "5", "length", "callee"]
d8> f = function() { Object.defineProperty(arguments, 75, {enumerable: false, value: 42}); return Object.getOwnPropertyNames(arguments) }
function () { Object.defineProperty(arguments, 75, {enumerable: false, value: 42}); return Object.getOwnPropertyNames(arguments) }
d8> f(1, 2, 3)
["0", "1", "2", "75", "length", "callee"]

Closing. Toon, feel free to reopen if you have a test case that doesn't do the right thing WRT arguments.
No that was written from my phone before looking at the code. It doesn't seem pretty, but seems to work afaict. Thanks for fixing.
I am seeing strange ordering for getOwnPropertyNames as well:

var t = {}; 'abcdefghijklmnopqrs'.split('').forEach(function (letter) { t[letter] = letter; }); Object.getOwnPropertyNames(t);
> ["a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k", "l", "m", "n", "o", "p", "q", "r", "s"]

var t = {}; 'abcdefghijklmnopqrst'.split('').forEach(function (letter) { t[letter] = letter; }); Object.getOwnPropertyNames(t);
> ["g", "b", "h", "a", "t", "c", "f", "r", "k", "e", "p", "l", "q", "n", "s", "m", "o", "j", "i", "d"]

There seems to be a specific object size where the properties get shifted around.

Comment 9 by adamk@chromium.org, Aug 18 2015

Status: Assigned
Thanks for the report; this is due to normalization from fast properties to dictionary mode.

Apparently we don't do the proper magic in getOwnPropertyNames to order the keys by insertion order. Should be relatively easy to fix, since we do the right thing for for..in.

Comment 11 by adamk@chromium.org, Aug 18 2015

Status: Fixed
Closing this in preference to  issue 3056  (as this issue was originally about indexed properties, which are fixed).
Labels: Priority-2

Sign in to add a comment