1218111 - Incorrect property enumeration order with unboxed objects

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

Noticed this while working on bug 1175111, but it's a pre-existing issue.

The spec requires the following enumeration order: indexed properties, then string properties, then symbol properties. This doesn't work correctly for unboxed objects with expando properties, for instance symbols can be added before strings.

Fixing this in the enumerate hook is annoying, it requires duplicating the logic in EnumerateNativeProperties. We could have a templatized helper function and call that in both places, but the extra boilerplate isn't that great either (I tried this initially).

I think a simpler fix is to special-case unboxed plain objects with expandos and call EnumerateNativeProperties on the expando. The attached patch does this.

Comment 1

[Brian Hackett [Laid off!]]

Comment on attachment 8678485 [details] [diff] [review]
Patch

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

::: js/src/jsiter.cpp
@@ +234,5 @@
>              if (!MergeSort(ids, n, tmp.begin(), SortComparatorIntegerIds))
>                  return false;
>          }
>  
> +        if (unboxed) {

The enumeration logic in this patch is kind of hard to follow, maybe add a comment here like:

If |unboxed| is set then |pobj| is the expando for an unboxed plain object we are enumerating. Add the unboxed properties themselves here since they are all property names that were given to the object before any of the expando's properties.

Comment 2

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8d34c0a05a1f

Comment 3

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8d34c0a05a1f

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8678485 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: Bug 1162199.
[User impact if declined]: Might break some websites.
[Describe test coverage new/current, TreeHerder]: On m-c, adds testcase.
[Risks and why]: Low risk.
[String/UUID change made/needed]: None.

Comment 5

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8678485 [details] [diff] [review]
Patch

Looks like it landed before the merge!

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking since this sounds like a regression from 41.

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8678485 [details] [diff] [review]
Patch

Fix for recent regression, adds tests. Approved for uplift to beta.

Comment 8

[Carsten Book [:Tomcat]]

This doesn't apply cleanly to beta:

grafting 311461:8d34c0a05a1f "Bug 1218111 - Fix property enumeration order of unboxed objects with expando properties. r=bhackett"
merging js/src/jsiter.cpp
warning: conflicts during merge.
merging js/src/jsiter.cpp incomplete! (edit conflicts, then use 'hg resolve --mark')
merging js/src/vm/UnboxedObject.cpp
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)

Comment 9

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/releases/mozilla-beta/rev/842a741af958