1246860 - stockx.com listing pages are blank on Firefox but show full listing on Chrome

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

STR:
Visit https://stockx.com/jordan-11-retro-72-10

ER:
See details of shoe for sale

AR:
Large blank area in the middle of the page. See attached screenshots for comparison.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

I also tried disabling tracking protection but that didn't fix it.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Screenshot on Firefox Nightly 47.0a1 (2016-02-08)

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Screenshot on Chrome Canary Version 50.0.2631.0 canary (64-bit)

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

[Tracking Requested - why for this release]: notable regression in webcompat, looks like the issue is somewhere in React which is heavily used by web developers

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

I used mozregression and narrowed this down to bug 715181. Can you take a look at this Morgan?

Comment 6

[4esn0k]

Do you have "Uncaught TypeError: Cannot read property 'subtitle' of undefined" ?

This patch uses sparse arrays and sorts them on line 118832 of https://stockx.com/skin/frontend/grails/default/public/js/script.js?v=c7bed6b7588fd1536c5070c3f4e5f3df 
Current impl. patches holes in sparse arrays:
```
[,,,,,,,,,,,,,,,,,,,,{size: 1},{size: 2}].sort(function (a, b) {
  return +a.size - +b.size;
})
```

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

No, I get `TypeError: option is undefined` at script.js line 118837.

> 118834 options.sort(function (a, b) {
> 118835     return +a.size - +b.size;
> 118836 }).forEach(function (option) {
> 118837     var hasVal = option.subtitle && +option.subtitle > 0;
> 118838     var optionClasses = (0, _classnames2['default'])('select-option', { 'no-val': !hasVal }, { 'active': option.selected }, { 'all': option.title == 'All' || option.title == 'Size' });

Comment 8

[Kohei Yoshino [:kohei]]

Oops.

Comment 9

[Morgan Phillips [:mrrrgn]]

Working up a patch for this today. Custom comparators now trigger a different sorting algorithm, and it's not handling holes properly: 

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

js> [,,,,,,,,,,,,,,,,,,,,{size: 1},{size: 2}].sort().forEach((o)=>{print(o)});
[object Object]
[object Object]
js> [,,,,,,,,,,,,,,,,,,,,{size: 1},{size: 2}].sort((a, b)=>{+a.size - +b.size}).forEach((o)=>{print(o)});
[object Object]
[object Object]
undefined
undefined
undefined
undefined
undefined
undefined
undefined
undefined
undefined
undefined
undefined
undefined
undefined
undefined
undefined
undefined
undefined
undefined
undefined
undefined

Comment 10

[Morgan Phillips [:mrrrgn]]

Attachment: holes.diff

Apologies. The sorting algorithm was not preserving holes, instead they were being converted to void(0) which always evaluates to undefined.

Comment 11

[Morgan Phillips [:mrrrgn]]

Comment on attachment 8717521 [details] [diff] [review]
holes.diff

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

::: js/src/builtin/Sorting.js
@@ +76,2 @@
>          lBuffer[i] = array[start + i];
> +    }

Unnecessary leftover, removed locally.

Comment 12

[Morgan Phillips [:mrrrgn]]

Attachment: Capture d’écran 2016-02-09 à 12.08.26 PM.png

Confirmation screenshot.

Comment 13

[Till Schneidereit [:till]]

Comment on attachment 8717521 [details] [diff] [review]
holes.diff

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

r=me with feedback addressed.

::: js/src/builtin/Sorting.js
@@ +79,2 @@
>          rBuffer[j] = array[mid + 1 + j];
> +    }

Here too, I assume.

@@ +108,5 @@
>  }
>  
> +// Helper function for overwriting a sparce array with a
> +// dense array, filling remaining slots with holes.
> +function MoveHoles(sparce, sparceLen, dense, denseLen) {

s/sparce/sparse/g

@@ +112,5 @@
> +function MoveHoles(sparce, sparceLen, dense, denseLen) {
> +    for (var i in dense)
> +        sparce[i] = dense[i];
> +    for (var j = denseLen; j < sparceLen; j++)
> +        delete sparce[j];

I wonder if it'd be faster to shrink and then re-grow the array. Would only be valid on genuine Arrays, of course, but might be worth it.

@@ +121,5 @@
> +    // To save effort we will do all of our work on a dense array,
> +    // then create holes at the end.
> +    var denseArray = [];
> +    var denseLen = 0;
> +    for (var i in array)

This needs braces.

@@ +123,5 @@
> +    var denseArray = [];
> +    var denseLen = 0;
> +    for (var i in array)
> +        if (i in array)
> +            denseArray[denseLen++] = array[i];

This needs to use _DefineDataProperty instead of brackets. Otherwise a setter on Array.prototype can observe and change behavior here. Please add a test that would've caught this.

@@ +144,2 @@
>      var mid, end, endOne, endTwo;
> +    for (var windowSize = 1; windowSize < denseLen; windowSize = 2*windowSize) {

Pre-existing, but because I just see it: spaces around the * please.

::: js/src/tests/ecma_6/Array/sort_holes.js
@@ +1,2 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/licenses/publicdomain/ */

No need to remove it but for future reference: PD is the default license for tests, so they don't need a header at all.

Comment 14

[Morgan Phillips [:mrrrgn]]

Pushing after the try push looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=58aa065e2c25

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3821b8259c65

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3821b8259c65

Comment 17

[Till Schneidereit [:till]]

This seems to have caused an 11% regression on ss-tagcloud:
More details: http://arewefastyet.com/regressions/#/regression/1802490

Morgan, is there perhaps a way to speed this up / only do the work if it's required? We have an intrinsic for checking if an array is dense, might be worth using that here.

Comment 18

[Morgan Phillips [:mrrrgn]]

(In reply to Till Schneidereit [:till] from comment #17)
> This seems to have caused an 11% regression on ss-tagcloud:
> More details: http://arewefastyet.com/regressions/#/regression/1802490
> 
> Morgan, is there perhaps a way to speed this up / only do the work if it's
> required? We have an intrinsic for checking if an array is dense, might be
> worth using that her

I really did a number on Array.sort, yikes. I have some ideas for speeding this up. Plan on working on it in bug: 1247283

Comment 19

[Morgan Phillips [:mrrrgn]]

* bug 1247283

Comment 20

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

Does this affect 46 as well?

Comment 21

[Morgan Phillips [:mrrrgn]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #20)
> Does this affect 46 as well?

Oy, just checked on my dev edition. It does indeed.

Comment 22

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

Tracking for 46 since this is a regression from 46. 
Did you sort out the performance issue? Once you have let's uplift that work to 46.

Comment 23

[Morgan Phillips [:mrrrgn]]

Yes, the patches associated with Bug 1246860 and Bug 1247283 solve this issue.

Comment 24

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

OK, we should uplift those patches to 46 then.... Untracking here and tracking in the bugs you mention.

Comment 25

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

Wait, one of the bugs you mention is this bug. Can you request uplift on the patches in this bug, and bug 1247283, to aurora, so they'll be fixed in 46 when it releases? 
Tracking this bug again for 46....

Comment 26

[Morgan Phillips [:mrrrgn]]

Attachment: holes.diff

Comment 27

[Morgan Phillips [:mrrrgn]]

Comment on attachment 8723429 [details] [diff] [review]
holes.diff

Approval Request Comment
[Feature/regressing bug #]: 1246860
[User impact if declined]: Many sites broken (web frameworks affected).
[Describe test coverage new/current, TreeHerder]: Unit tests are included. See the bug description for an example of an affected web page.
[Risks and why]: Breaks the web. 
[String/UUID change made/needed]:

Comment 28

[Morgan Phillips [:mrrrgn]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #25)
> Wait, one of the bugs you mention is this bug. Can you request uplift on the
> patches in this bug, and bug 1247283, to aurora, so they'll be fixed in 46
> when it releases? 
> Tracking this bug again for 46....

I've requested uplift for the patch here (holes.diff) and the one on bug 1247283 (arrayperf.diff) along with a note specifying that they should be applied in the order: holes.diff then arrayperf.diff

Let me know if I need to take further actions.

Comment 29

[Morgan Phillips [:mrrrgn]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #25)
> Wait, one of the bugs you mention is this bug. Can you request uplift on the
> patches in this bug, and bug 1247283, to aurora, so they'll be fixed in 46
> when it releases? 
> Tracking this bug again for 46....

I've requested uplift for the patch here (holes.diff) and the one on bug 1247283 (arrayperf.diff) along with a note specifying that they should be applied in the order: holes.diff then arrayperf.diff

Let me know if I need to take further actions.

Comment 30

[Morgan Phillips [:mrrrgn]]

Attachment: holes.diff

Comment 31

[Morgan Phillips [:mrrrgn]]

Comment on attachment 8723434 [details] [diff] [review]
holes.diff

Approval Request Comment
[Feature/regressing bug #]: 1246860
[User impact if declined]: Many sites broken (web frameworks affected).
[Describe test coverage new/current, TreeHerder]: Unit tests are included. See the bug description for an example of an affected web page.
[Risks and why]: 
[String/UUID change made/needed]:

Comment 32

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

Comment on attachment 8723434 [details] [diff] [review]
holes.diff

Fix for a regression from 46, includes tests, please uplift to aurora

Comment 33

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/4f3bb934fb37