Closed Bug 1260673 Opened 4 years ago Closed 4 years ago

Stop using _DefineDataProperty in MergeSort

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox46 --- affected
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: mrrrgn, Assigned: mrrrgn)

Details

Attachments

(2 files, 2 obsolete files)

From the following comment: https://bugzilla.mozilla.org/show_bug.cgi?id=1259600#c16

"https://tc39.github.io/ecma262/#sec-array.prototype.sort says:
> The following steps are taken:
>
> 1.  Perform an implementation-dependent sequence of calls to the [[Get]] and [[Set]]
>     internal methods of obj, to the DeletePropertyOrThrow and HasOwnProperty abstract
>     operation with obj as the first argument, and to SortCompare (described below),
>     such that:

...followed by a bunch more text, but basically the spec says we are only permitted to do
property gets, sets, deletes, and existence checks, and call the comparator.

So we're not allowed to use DefineProperty here. So if our MoveHoles routine is doing that,
it has a bug, and it would be observable when using sort() on any sealed object with holes,
not just typed arrays."
Assignee: nobody → winter2718
So this is a bummer because without using this I don't see a way to prevent a negative impact of users overriding the Array setter. For instance, this test will fail. 

// Ensure that the array setter is touch touched during sorting.

Object.defineProperty(Array.prototype, "0", {
    set: (value) => {throw "Illegally touched the array's setter!"},
    configurable: true
});

assertThrows(() => {o[1] = 11;});

let o = [,,,,,,,,,,,,,,,,,,,,{size: 1},{size: 2}];
o.sort((a, b) => {+a.size - +b.size});

Trying to think of a way around this.
I don't think there's a way around it, and I think that's OK!
anba/es6draft and node 4.4 both throw with that test case (I took out the `assertThrows` line.)
Attachment #8736959 - Flags: review?(jorendorff)
Comment on attachment 8736959 [details] [diff] [review]
definedataproperty.diff

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

Thanks for seeing this through.

::: js/src/tests/ecma_6/Array/sort_holes.js
@@ -60,5 @@
> -    set: (value) => {throw "Illegally touched the array's setter!"},
> -    configurable: true
> -});
> -
> -assertThrows(() => {o[1] = 11;});

Pre-existing: this last line here really did not make a lot of sense... you get a TDZ error, right?

@@ -63,5 @@
> -
> -assertThrows(() => {o[1] = 11;});
> -
> -let o = [,,,,,,,,,,,,,,,,,,,,{size: 1},{size: 2}];
> -o.sort((a, b) => {+a.size - +b.size});

Let's keep this test, and assert that this sort() call throws the expected string. sort() has a really strange specification, so who knows, but I can't think of any other legal outcome.

    assertThrowsValue(() => o.sort((a, b) => a.size - b.size), "setter was called");

Note that if you an arrow function has curly braces, it needs an explicit `return` statement or else it's returning undefined. Also, I think the unary + operator is unnecessary there since subtraction converts its operands to numbers anyway (and these .size values are all numbers to begin with).
Attachment #8736959 - Flags: review?(jorendorff) → review+
Morgan, this is a regression, right? Which releases is the bug in? If it's a regression and it's in Aurora, we should consider fixing it there, especially since the fix is so straightforward.
Flags: needinfo?(winter2718)
(In reply to Jason Orendorff [:jorendorff] from comment #6)
> Morgan, this is a regression, right? Which releases is the bug in? If it's a
> regression and it's in Aurora, we should consider fixing it there,
> especially since the fix is so straightforward.

I'll request uplift.
Flags: needinfo?(winter2718)
Comment on attachment 8736959 [details] [diff] [review]
definedataproperty.diff

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Array.sort will fail on sealed objects with holes. Could break web pages.
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:
Attachment #8736959 - Flags: approval-mozilla-beta?
Attachment #8736959 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/0d2dd90e33f4
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Hi Jason, could you please help me assess the risk of taking this fix in Aurora 47? Also would you be able to comment on the test coverage for this? Are there automated tests that would test the fix and catch any obvious regressions? I see that the patch has an automated test that was changed. Thanks!
Flags: needinfo?(jorendorff)
Doh, I need to throw up a test with a sealed object (with holes).
(In reply to Ritu Kothari (:ritu) from comment #11)
> Hi Jason, could you please help me assess the risk of taking this fix in
> Aurora 47? Also would you be able to comment on the test coverage for this?
> Are there automated tests that would test the fix and catch any obvious
> regressions? I see that the patch has an automated test that was changed.
> Thanks!

Three possible courses:

* uplift this patch - minor risk
* do nothing - it's a regression, moderate risk of breaking some web sites
* back out the patch that caused the regression - Morgan, is this possible?

The test should have gone in with the patch - an oversight.
Flags: needinfo?(jorendorff) → needinfo?(winter2718)
(In reply to Jason Orendorff [:jorendorff] from comment #13)
> (In reply to Ritu Kothari (:ritu) from comment #11)
> > Hi Jason, could you please help me assess the risk of taking this fix in
> > Aurora 47? Also would you be able to comment on the test coverage for this?
> > Are there automated tests that would test the fix and catch any obvious
> > regressions? I see that the patch has an automated test that was changed.
> > Thanks!
> 
> Three possible courses:
> 
> * uplift this patch - minor risk
> * do nothing - it's a regression, moderate risk of breaking some web sites
> * back out the patch that caused the regression - Morgan, is this possible?
> 
> The test should have gone in with the patch - an oversight.

Backing out the patch that caused this wouldn't be good. I think it makes more sense to just write a test.
Flags: needinfo?(winter2718)
I vote for the first option. "* uplift this patch - minor risk"
Honestly, looking at this again I think more work needs to go into making Array.sort work with sealed arrays (with holes). We should I'm now more in favor of not uplifting this.
Comment on attachment 8736959 [details] [diff] [review]
definedataproperty.diff

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

I don't believe this should be uplifted. It's OK to leave in, but doesn't solve the problem that would make it worth uplifting. | let a = [3, 2, , , 5]; Object.seal(a); a.sort(); | will still fail.
Attachment #8736959 - Flags: approval-mozilla-beta?
Attachment #8736959 - Flags: approval-mozilla-aurora?
I think leaving this regression in Aurora is the worst option. We can either uplift the fix or back out the regression.

(In reply to Morgan Phillips [:mrrrgn] from comment #17)
> I don't believe this should be uplifted. It's OK to leave in, but doesn't
> solve the problem that would make it worth uplifting. | let a = [3, 2, , ,
> 5]; Object.seal(a); a.sort(); | will still fail.

I understand why that fails, and it's OK for it to fail. I think the spec basically requires it to fail.

Another situation where this is observable is if you use Array#sort to sort a Proxy with only get/set/delete/getOwnPropertyDescriptor handlers implemented, and *not* defineProperty.
Attached patch definedataproperty2.diff (obsolete) — Splinter Review
Okay, I'll request uplift for this and the previous patch if it looks okay.
Attachment #8737922 - Flags: review?(jorendorff)
Comment on attachment 8737922 [details] [diff] [review]
definedataproperty2.diff

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

Oh hold up, I'm going to do a test case for
> Another situation where this is observable is if you use Array#sort to sort
> a Proxy with only get/set/delete/getOwnPropertyDescriptor handlers
> implemented, and *not* defineProperty.
Attachment #8737922 - Flags: review?(jorendorff)
Attached patch definedataproperty2.diff (obsolete) — Splinter Review
Attachment #8737922 - Attachment is obsolete: true
Attachment #8737975 - Flags: review?(jorendorff)
Comment on attachment 8737975 [details] [diff] [review]
definedataproperty2.diff

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

Great!
Attachment #8737975 - Flags: review?(jorendorff) → review+
Comment on attachment 8736959 [details] [diff] [review]
definedataproperty.diff

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Possible broken web pages. Sealed arrays are unsortable.
[Describe test coverage new/current, TreeHerder]: Tests have been added in a follow up patch. Object.seal([5, 3, 2, , ,]).sort((x, y) => 1 * x - y) will fail in current builds and pass when the patch is applied.
[Risks and why]: 
[String/UUID change made/needed]:
Attachment #8736959 - Flags: approval-mozilla-aurora?
Comment on attachment 8737975 [details] [diff] [review]
definedataproperty2.diff

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Possible broken web pages.
[Describe test coverage new/current, TreeHerder]: This patch includes tests, to ensure that sealed objects are sortable.
[Risks and why]: Minimal to none.
[String/UUID change made/needed]:
Attachment #8737975 - Flags: approval-mozilla-aurora?
Comment on attachment 8737975 [details] [diff] [review]
definedataproperty2.diff

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

::: js/src/tests/ecma_6/Array/sort_proxy.js
@@ +26,5 @@
> +    proxy.sort((x, y) => 1 * x - y);
> +    arr.sort((x, y) => 1 * x - y);
> +
> +    for (let i of arr)
> +        assertEq(arr[i], proxy[i]);

I think this is wrong. Your are iterating over values and not indexes.
(In reply to Tom Schuster [:evilpie] from comment #26)
> Comment on attachment 8737975 [details] [diff] [review]
> definedataproperty2.diff
> 
> Review of attachment 8737975 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/tests/ecma_6/Array/sort_proxy.js
> @@ +26,5 @@
> > +    proxy.sort((x, y) => 1 * x - y);
> > +    arr.sort((x, y) => 1 * x - y);
> > +
> > +    for (let i of arr)
> > +        assertEq(arr[i], proxy[i]);
> 
> I think this is wrong. Your are iterating over values and not indexes.

Oy, yes, s/of/in/
Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:
Attachment #8737975 - Attachment is obsolete: true
Attachment #8737975 - Flags: approval-mozilla-aurora?
Attachment #8738241 - Flags: approval-mozilla-aurora?
Comment on attachment 8736959 [details] [diff] [review]
definedataproperty.diff

Clearing out the uplift nomination request flag until the fix is ready.
Attachment #8736959 - Flags: approval-mozilla-aurora?
Comment on attachment 8736959 [details] [diff] [review]
definedataproperty.diff

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

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Array.sort will fail on sealed objects with holes. Could break web pages.
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:
Attachment #8736959 - Flags: approval-mozilla-beta?
Comment on attachment 8738241 [details] [diff] [review]
definedataproperty2.diff

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

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Array.sort will not have proper test coverage.
[Describe test coverage new/current, TreeHerder]: Spidermonkey tests are included in the patch.
[Risks and why]: 
[String/UUID change made/needed]:
Attachment #8738241 - Flags: approval-mozilla-beta?
Comment on attachment 8738241 [details] [diff] [review]
definedataproperty2.diff

test-only patch, Beta47+
Attachment #8738241 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8736959 [details] [diff] [review]
definedataproperty.diff

Recent regression, Beta47+
Attachment #8736959 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I'm hitting conflicts uplifting this to beta.
Flags: needinfo?(winter2718)
(In reply to Wes Kocher (:KWierso) from comment #36)
> I'm hitting conflicts uplifting this to beta.

I'm not getting the merge conflicts. hmmmm I'll need to bug you on irc

Morgans-MacBook-Pro:projects mrrrgn$ cd mozilla-beta/
Morgans-MacBook-Pro:mozilla-beta mrrrgn$ hg import /tmp/p.diff
applying /tmp/p.diff
Morgans-MacBook-Pro:mozilla-beta mrrrgn$ hg import /tmp/p2.diff
applying /tmp/p2.diff

where p.diff and p2.diff are the patches attached to this bug. :(
Flags: needinfo?(winter2718)
resetting ni?mrrrgn to bug Wes on irc (to make sure we don't drop this)
Flags: needinfo?(winter2718)
Wes is having problems applying the patch because one of these new files seems to exist already. I'm not sure how this would be possible. Maybe it's a problem with his local copy? I checked out mozilla-beta and applied the patches successfully:

Morgans-MacBook-Pro:src mrrrgn$ hg import hg import -e https://bugzilla.mozilla.org/attachment.cgi?id=8736959
Morgans-MacBook-Pro:src mrrrgn$ hg import -e https://bugzilla.mozilla.org/attachment.cgi?id=8736959
applying https://bugzilla.mozilla.org/attachment.cgi?id=8736959
Morgans-MacBook-Pro:src mrrrgn$ https://bugzilla.mozilla.org/attachment.cgi?id=8738241
Morgans-MacBook-Pro:src mrrrgn$ hg import -e https://bugzilla.mozilla.org/attachment.cgi?id=8738241
applying https://bugzilla.mozilla.org/attachment.cgi?id=8738241

Wes, am I checking out the wrong repo? Which are you using?
Flags: needinfo?(winter2718) → needinfo?(wkocher)
Okay, now it works just fine without me changing anything...
Flags: needinfo?(wkocher)
You need to log in before you can comment on or make changes to this bug.