1260673 - Stop using _DefineDataProperty in MergeSort

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[Morgan Phillips [:mrrrgn]]

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."

Comment 1

[Morgan Phillips [:mrrrgn]]

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.

Comment 2

[Jason Orendorff [:jorendorff]]

I don't think there's a way around it, and I think that's OK!

Comment 3

[Jason Orendorff [:jorendorff]]

anba/es6draft and node 4.4 both throw with that test case (I took out the `assertThrows` line.)

Comment 4

[Morgan Phillips [:mrrrgn]]

Attachment: definedataproperty.diff

Comment 5

[Jason Orendorff [: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).

Comment 6

[Jason Orendorff [:jorendorff]]

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.

Comment 7

[Morgan Phillips [:mrrrgn]]

(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.

Comment 8

[Morgan Phillips [:mrrrgn]]

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

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d2dd90e33f4

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0d2dd90e33f4

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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!

Comment 12

[Morgan Phillips [:mrrrgn]]

Doh, I need to throw up a test with a sealed object (with holes).

Comment 13

[Jason Orendorff [:jorendorff]]

(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.

Comment 14

[Morgan Phillips [:mrrrgn]]

(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.

Comment 15

[Morgan Phillips [:mrrrgn]]

I vote for the first option. "* uplift this patch - minor risk"

Comment 16

[Morgan Phillips [:mrrrgn]]

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 17

[Morgan Phillips [:mrrrgn]]

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.

Comment 18

[Jason Orendorff [:jorendorff]]

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.

Comment 19

[Morgan Phillips [:mrrrgn]]

Attachment: definedataproperty2.diff

Okay, I'll request uplift for this and the previous patch if it looks okay.

Comment 20

[Morgan Phillips [:mrrrgn]]

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.

Comment 21

[Morgan Phillips [:mrrrgn]]

Attachment: definedataproperty2.diff

Comment 22

[Jason Orendorff [:jorendorff]]

Comment on attachment 8737975 [details] [diff] [review]
definedataproperty2.diff

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

Great!

Comment 23

[Morgan Phillips [:mrrrgn]]

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

Comment 24

[Morgan Phillips [:mrrrgn]]

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

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c59b45a041d2

Comment 26

[Tom S]

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.

Comment 27

[Morgan Phillips [:mrrrgn]]

(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/

Comment 28

[Morgan Phillips [:mrrrgn]]

Attachment: definedataproperty2.diff

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c8c35258999

Comment 30

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8736959 [details] [diff] [review]
definedataproperty.diff

Clearing out the uplift nomination request flag until the fix is ready.

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c59b45a041d2
https://hg.mozilla.org/mozilla-central/rev/2c8c35258999

Comment 32

[Morgan Phillips [:mrrrgn]]

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

Comment 33

[Morgan Phillips [:mrrrgn]]

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

Comment 34

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8738241 [details] [diff] [review]
definedataproperty2.diff

test-only patch, Beta47+

Comment 35

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8736959 [details] [diff] [review]
definedataproperty.diff

Recent regression, Beta47+

Comment 36

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

I'm hitting conflicts uplifting this to beta.

Comment 37

[Morgan Phillips [:mrrrgn]]

(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. :(

Comment 38

[Jason Orendorff [:jorendorff]]

resetting ni?mrrrgn to bug Wes on irc (to make sure we don't drop this)

Comment 39

[Morgan Phillips [:mrrrgn]]

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?

Comment 40

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

https://hg.mozilla.org/releases/mozilla-beta/rev/20321aaa882d
https://hg.mozilla.org/releases/mozilla-beta/rev/b54446178c96

Comment 41

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

Okay, now it works just fine without me changing anything...