Closed Bug 1175111 Opened 4 years ago Closed 4 years ago

Wrong property order in Object.getOwnPropertyNames(string) and others

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox41 --- affected
firefox44 --- fixed

People

(Reporter: 446240525, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

js> Object.getOwnPropertyNames("123")
["length", "0", "1", "2"] // should be [ "0", "1", "2", "length" ], per spec 9.4.3.3
Blocks: es6
Rules about own property ordering affect many builtins:
- Object.keys
- Object.getOwnPropertyNames
- Object.assign
- JSON.stringify
- Reflect.ownKeys
- Reflect.enumerate (not yet implemented, bug 1185493)

There may also be aspects of behavior exposed by the for-in loop alone that we get wrong.
Summary: The order of properties returned by Object.getOwnPropertyNames(string) is wrong → Wrong property order in Object.getOwnPropertyNames(string) and others
Attached patch PatchSplinter Review
This fixes for-in, Object.getOwnPropertyNames etc to sort integer properties and put them before other properties.

The only problem I see is that the standard defines an integer property as any integer >= 0 and < 2**53, whereas our "indexed id" range is much smaller.

This seems to be a more general issue though and I think fixing this requires changes to IdIsIndex etc.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8676399 - Flags: review?(jorendorff)
Comment on attachment 8676399 [details] [diff] [review]
Patch

Review of attachment 8676399 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: js/src/jsiter.cpp
@@ +144,5 @@
> +{
> +    uint32_t indexA, indexB;
> +    DebugOnly<bool> isIndexA = IdIsIndex(a, &indexA);
> +    DebugOnly<bool> isIndexB = IdIsIndex(b, &indexB);
> +    MOZ_ASSERT(isIndexA && isIndexB);

You can use MOZ_ALWAYS_TRUE for these.

@@ +165,2 @@
>          for (size_t i = 0; i < initlen; ++i, ++vp) {
>              if (!vp->isMagic(JS_ELEMENTS_HOLE)) {

Maybe swap the two arms of this if-else now, and remove the `!`.
Attachment #8676399 - Flags: review?(jorendorff) → review+
Blocks: 1218111
https://hg.mozilla.org/mozilla-central/rev/45dd451ce4ed
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Depends on: 1219363
Duplicate of this bug: 443817
I'm sure the array part of this is my fault -- sorry!
You need to log in before you can comment on or make changes to this bug.