Incorrect property enumeration order with unboxed objects

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

({regression})

unspecified
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 wontfix, firefox42 affected, firefox43+ fixed, firefox44+ fixed, firefox45+ fixed)

Details

Attachments

(1 attachment)

Posted patch PatchSplinter Review
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.
Attachment #8678485 - Flags: review?(bhackett1024)
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.
Attachment #8678485 - Flags: review?(bhackett1024) → review+
https://hg.mozilla.org/mozilla-central/rev/8d34c0a05a1f
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
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.
Attachment #8678485 - Flags: approval-mozilla-beta?
Attachment #8678485 - Flags: approval-mozilla-aurora?
Comment on attachment 8678485 [details] [diff] [review]
Patch

Looks like it landed before the merge!
Attachment #8678485 - Flags: approval-mozilla-aurora?
Tracking since this sounds like a regression from 41.
Comment on attachment 8678485 [details] [diff] [review]
Patch

Fix for recent regression, adds tests. Approved for uplift to beta.
Attachment #8678485 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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)
Flags: needinfo?(jdemooij)
You need to log in before you can comment on or make changes to this bug.