1062406 - Amazon's "Manage Your Content and Devices" fails to render on Nightly (scrollTo float/long issue)

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Marc Auslander]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20140903030206

Steps to reproduce:

visit https://www.amazon.com/mn/dcw/myx.html?ie=UTF8&ref_=nav_youraccount_myk#/home/content/booksAll/titleAsc/
to see my kindle books


Actual results:

no book entries appear


Expected results:

should see my books listed

fails with emtpy profile
works with chrome so it's not (completely) an amazon problem

Comment 1

[Marc Auslander]

Last good revision: dc352a7bf234 (2014-08-26)
First bad revision: 0753f7b93ab7 (2014-08-27)

Comment 2

[alex_mayorga]

Confirmed on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0 ID:20140905030206 CSet: db7212847c14

Comment 3

[Ekanan Ketunuti]

Last good: mozilla-inbound changeset a6832b6e110d
First bad: mozilla-inbound changeset 912de3bb85ce
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a6832b6e110d&tochange=912de3bb85ce

Comment 4

[Ekanan Ketunuti]

[Tracking Requested - why for this release]:


The first bad revision is
changeset:   201661:5b105c3b51be
user:        Kearwood (Kip) Gilbert <[e-mail]>
date:        Tue Aug 05 15:01:32 2014 -0700
files:       content/base/public/Element.h content/base/src/Element.cpp content/html/content/src/nsGenericHTMLElement.h dom/base/nsGlobalWindow.cpp dom/base/nsGlobalWindow.h dom/webidl/Element.webidl dom/webidl/Window.webidl
description:
Bug 1022818 - Part 1: Update webidl interfaces. r=bz

Comment 5

[Jeff Walden [:Waldo]]

Things break on this line of code:

  win.scrollTo(!i ? val : jQuery(win).scrollLeft(),
               i ? val : jQuery(win).scrollTop());

In my testing |i === 1|.  jQuery(win).scrollLeft() returns 0.  |val| is an object, some sort of jQuery wrapper around a document.

At a guess, maybe when there's only one scrollTop overload that object converts to a number of the kind scrollTop previously took, and when there are two conversion from object can't select just one and so complains?  The stack here is jQuery-nonsense-inscrutable, so I can't actually tell whether the object there is supposed to be there, or is really a site bug or something.

Comment 6

[Jeff Walden [:Waldo]]

Or, no, looking back at the diff it appears this *added* a scrollTop method that takes more than one argument.  Previously this code probably hit the old overload that took just one argument, and |val| was ignored.  But now we try to convert to number, and because +val is nonsense you get NaN or so and thus the error happens.

Whether that means Amazon (jQuery?) is doing something dumb, or this overload should just ignore errors if the second number argument is non-finite, or something else, I dunno.

Comment 7

[Boris Zbarsky [:bzbarsky]]

What actually happened here is that the old scrollTo method looked like this:

  void scrollTo(long x, long y);

The new one looks like this:

  void scrollTo(double x, double y, optional ScrollOptions options);

The important part here is the change from long to double.  "long" uses the ToInteger conversion, which will happily convert NaN to 0.  "double" uses ToNumber and then throws on non-finite numbers.

What we should probably do here is change the API to use unrestricted double and coerce non-finite ones to 0 in the C++, to keep backwards compat.  And raise a spec issue to this effect, of course.

Comment 8

[:kip (Kearwood Gilbert)]

I am implementing a fix now, thanks for the detailed investigation.

Comment 10

[:kip (Kearwood Gilbert)]

Attachment: Bug 1062406: Part 1 - Change x and y parameters of window.scroll* CSSOM-View DOM calls from double to unrestricted double

- WebIDL updated so that x and y parameters of window.scroll, window.scrollTo,
  and window.ScrollBy are changed from "double" to "unrestricted double".
- Updated nsGlobalWindow::Scroll, ScrollTo, and ScrollBy methods so that they
  replace non-finite numbers with 0.

Comment 11

[:kip (Kearwood Gilbert)]

Attachment: Bug 1062406: Part 2 - Tests

- Implemented a mochitest to ensure that non-finite values passed to
window.scroll, window.scrollTo, and window.scrollBy are interpreted
as 0.

Comment 12

[:kip (Kearwood Gilbert)]

Pushed to try:

https://tbpl.mozilla.org/?tree=Try&rev=ee215ef1ccf1

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8486101 [details] [diff] [review]
Bug 1062406: Part 1 - Change x and y parameters of window.scroll* CSSOM-View DOM calls from double to unrestricted double

> mozilla::IsFinite(aXScroll) ? aXScroll : 0.0f

It might be nice to have a helper function for this, either in this file or in mozilla::FloatingPoint; we have some other in-tree consumers that want this pattern.  Doing it in this file is simpler, I guess.

>+nsGlobalWindow::ScrollBy(double aXScrollDif, double aYScrollDif,
>+    CSSIntPoint scrollDif(mozilla::IsFinite(aXScrollDif) ? aXScrollDif : 0.0f,
>+                           mozilla::IsFinite(aYScrollDif) ? aYScrollDif : 0.0f);

Please fix the indent.

Please add a test; something as simple as scrollTo/By passed NaN not throwing should do.

r=me

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8486140 [details] [diff] [review]
Bug 1062406: Part 2 - Tests

Whoops, I missed the second attachment when commenting on the first one.

Maybe move the "/** Test ..." comment to before the test code?  And put the large div (which should have "px" on its width/height) before the <pre>, so there's somewhere to actually scroll to when the script runs?

r=me

Comment 15

[:kip (Kearwood Gilbert)]

Attachment: Bug 1062406: Part 1 - Change x and y parameters of window.scroll* CSSOM-View DOM calls from double to unrestricted double (V2 Patch), r=bz

- WebIDL updated so that x and y parameters of window.scroll, window.scrollTo,
and window.ScrollBy are changed from "double" to "unrestricted double".
- Implemented mozilla::ConformFinite
- Updated nsGlobalWindow::Scroll, ScrollTo, and ScrollBy methods so that they
replace non-finite numbers with 0.

V2 Patch: Updated with feedback from review.

Comment 16

[:kip (Kearwood Gilbert)]

Attachment: Bug 1062406: Part 2 - Tests (V2 Patch), r=bz

- Implemented a mochitest to ensure that non-finite values passed to
window.scroll, window.scrollTo, and window.scrollBy are interpreted
as 0.

V2 Patch: Updated with feedback from review.

Comment 17

[Jeff Walden [:Waldo]]

Comment on attachment 8486592 [details] [diff] [review]
Bug 1062406: Part 1 - Change x and y parameters of window.scroll* CSSOM-View DOM calls from double to unrestricted double (V2 Patch), r=bz

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

::: mfbt/FloatingPoint.h
@@ +192,5 @@
> + * otherwise, the float/double is returned.
> + */
> +template<typename T>
> +static MOZ_ALWAYS_INLINE T
> +ConformFinite(T aValue)

"Conform" is a somewhat strange (and not precise) verb to use here.  What about ToZeroIfNotFinite?

Comment 18

[Jeff Walden [:Waldo]]

Or ToZeroIfNonfinite, maybe.

Comment 19

[:kip (Kearwood Gilbert)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #18)
> Or ToZeroIfNonfinite, maybe.
I had some trouble naming that function.  I like yours.  Updating the patch, then will re-apply checkin flag.

Comment 20

[:kip (Kearwood Gilbert)]

Attachment: Bug 1062406: Part 1 - Change x and y parameters of window.scroll* CSSOM-View DOM calls from double to unrestricted double (V3 Patch), r=bz

- WebIDL updated so that x and y parameters of window.scroll, window.scrollTo,
and window.ScrollBy are changed from "double" to "unrestricted double".
- Implemented mozilla::ToZeroIfNonfinite
- Updated nsGlobalWindow::Scroll, ScrollTo, and ScrollBy methods so that they
replace non-finite numbers with 0.

V3 Patch:
Renamed mozilla::ConformFinite to mozilla:ToZeroIfNonfinite

Comment 21

[Carsten Book [:Tomcat]]

landed in https://hg.mozilla.org/integration/mozilla-inbound/rev/24a4613761b8 and  https://hg.mozilla.org/integration/mozilla-inbound/rev/582045ca275c

Comment 22

[Boris Zbarsky [:bzbarsky]]

Kearwood, are you going to raise the spec issue, or should I?

Comment 23

[:kip (Kearwood Gilbert)]

I will raise the spec issue and CC the link here.

Comment 24

[:kip (Kearwood Gilbert)]

I have submitted a bug at w3.org to raise the issue:

https://www.w3.org/Bugs/Public/show_bug.cgi?id=26775

Comment 25

[:kip (Kearwood Gilbert)]

I have also started a thread on the w3.org mailing list:

http://lists.w3.org/Archives/Public/www-style/2014Sep/0155.html

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/582045ca275c
https://hg.mozilla.org/mozilla-central/rev/24a4613761b8

Comment 27

[alex_mayorga]

I can see the list of books on https://www.amazon.com/mn/dcw/myx.html?ie=UTF8&ref_=nav_youraccount_myk#/home/content/booksAll/titleAsc/ now with Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0 ID:20140911030204 CSet: bc7deafdac4b

Comment 28

[Ekanan Ketunuti]

kip, can you please request approval to uplift this fix to Aurora 34?

Comment 29

[:kip (Kearwood Gilbert)]

Comment on attachment 8486609 [details] [diff] [review]
Bug 1062406: Part 1 - Change x and y parameters of window.scroll* CSSOM-View DOM calls from double to unrestricted double (V3 Patch), r=bz

Approval Request Comment
[Feature/regressing bug #]: Bug #1022818 included a change to scrolling related WebIDL which changed scroll offset parameters from integer to double units.
[User impact if declined]: Existing web sites were dependent on the inherent casting of non-finite numbers to 0 for integer WebIDL parameters.  Javascript will fail to execute correctly if a NaN, Infinity, or -Infinity value are passed to a scroll offset parameter.
[Describe test coverage new/current, TBPL]: Was pushed to try (https://tbpl.mozilla.org/?tree=Try&rev=ee215ef1ccf1) and landed in mozilla-central.  The fix has been confirmed for the affected site (Amazon's "Manage Your Content and Services") page.
[Risks and why]: There is risk of regressions related to CSSOM-View DOM scrolling API's and handling of casting of data types for scroll offset parameters.
[String/UUID change made/needed]: none

Comment 30

[:kip (Kearwood Gilbert)]

Comment on attachment 8486595 [details] [diff] [review]
Bug 1062406: Part 2 - Tests (V2 Patch), r=bz

Approval Request Comment
[Feature/regressing bug #]: Bug #1022818 included a change to scrolling related WebIDL which changed scroll offset parameters from integer to double units.
[User impact if declined]: Existing web sites were dependent on the inherent casting of non-finite numbers to 0 for integer WebIDL parameters.  Javascript will fail to execute correctly if a NaN, Infinity, or -Infinity value are passed to a scroll offset parameter.
[Describe test coverage new/current, TBPL]: Was pushed to try (https://tbpl.mozilla.org/?tree=Try&rev=ee215ef1ccf1) and landed in mozilla-central.  The fix has been confirmed for the affected site (Amazon's "Manage Your Content and Services") page.
[Risks and why]: There is risk of regressions related to CSSOM-View DOM scrolling API's and handling of casting of data types for scroll offset parameters.
[String/UUID change made/needed]: none

Comment 31

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8486609 [details] [diff] [review]
Bug 1062406: Part 1 - Change x and y parameters of window.scroll* CSSOM-View DOM calls from double to unrestricted double (V3 Patch), r=bz

Let's start with this fix for a known Web regression and see if anything else falls out. Aurora+

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/885dcc785dd4
https://hg.mozilla.org/releases/mozilla-aurora/rev/c143c4fb1627

Comment 33

[Ekanan Ketunuti]

Verified on 

Firefox 34.0a2
BuildID: 20140917004003