Closed Bug 1218111 Opened 7 years ago Closed 7 years ago

Incorrect property enumeration order with unboxed objects


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox41 --- wontfix
firefox42 --- affected
firefox43 + fixed
firefox44 + fixed
firefox45 + fixed


(Reporter: jandem, Assigned: jandem)



(Keywords: regression)


(1 file)

Attached 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]

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+
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Comment on attachment 8678485 [details] [diff] [review]

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]

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]

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.