Closed Bug 1246860 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: JavaScript Engine, defect)

46 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox46 + fixed
firefox47 + fixed

People

(Reporter: jaws, Assigned: mrrrgn)

References

Details

(Keywords: regression, site-compat)

Attachments

(4 files, 2 obsolete files)

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.
I also tried disabling tracking protection but that didn't fix it.
[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
Blocks: 715181
I used mozregression and narrowed this down to bug 715181. Can you take a look at this Morgan?
Flags: needinfo?(winter2718)
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;
})
```
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' });
Assignee: nobody → winter2718
Flags: needinfo?(winter2718)
Assignee: winter2718 → nobody
Oops.
Assignee: nobody → winter2718
Status: NEW → ASSIGNED
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
Attached patch holes.diff (obsolete) — Splinter Review
Apologies. The sorting algorithm was not preserving holes, instead they were being converted to void(0) which always evaluates to undefined.
Attachment #8717521 - Flags: review?(till)
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.
Confirmation screenshot.
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.
Attachment #8717521 - Flags: review?(till) → review+
Component: Desktop → JavaScript Engine
Product: Tech Evangelism → Core
https://hg.mozilla.org/mozilla-central/rev/3821b8259c65
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
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.
Flags: needinfo?(winter2718)
(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
Flags: needinfo?(winter2718)
Depends on: 1247465
Does this affect 46 as well?
Flags: needinfo?(winter2718)
(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.
Flags: needinfo?(winter2718)
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.
Yes, the patches associated with Bug 1246860 and Bug 1247283 solve this issue.
OK, we should uplift those patches to 46 then.... Untracking here and tracking in the bugs you mention.
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....
Flags: needinfo?(winter2718)
Flags: needinfo?(winter2718)
Target Milestone: mozilla47 → mozilla46
Attached patch holes.diff (obsolete) — Splinter Review
Attachment #8717521 - Attachment is obsolete: true
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]:
Attachment #8723429 - Flags: approval-mozilla-aurora?
(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.
Target Milestone: mozilla46 → mozilla47
(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.
Attached patch holes.diffSplinter Review
Attachment #8723429 - Attachment is obsolete: true
Attachment #8723429 - Flags: approval-mozilla-aurora?
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]:
Attachment #8723434 - Flags: approval-mozilla-aurora?
Comment on attachment 8723434 [details] [diff] [review]
holes.diff

Fix for a regression from 46, includes tests, please uplift to aurora
Attachment #8723434 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1253898
Version: unspecified → 46 Branch
You need to log in before you can comment on or make changes to this bug.