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

VERIFIED FIXED in Firefox 34

Status

()

VERIFIED FIXED
4 years ago
2 years ago

People

(Reporter: marcausl, Assigned: kip)

Tracking

(Depends on: 1 bug, {regression})

34 Branch
mozilla35
All
Windows 7
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox32 unaffected, firefox33 unaffected, firefox34+ verified, firefox35 verified)

Details

(URL)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

4 years ago
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
(Reporter)

Comment 1

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

Comment 2

4 years ago
Confirmed on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0 ID:20140905030206 CSet: db7212847c14
Status: UNCONFIRMED → NEW
status-firefox35: --- → affected
Component: Untriaged → General
Ever confirmed: true
Summary: amazon kindle → Amazon's "Manage Your Content and Devices" fails to render on Nightly
Version: 35 Branch → Trunk

Updated

4 years ago
status-firefox34: --- → affected
Keywords: regression
Version: Trunk → 34 Branch

Comment 4

4 years ago
[Tracking Requested - why for this release]:


The first bad revision is
changeset:   201661:5b105c3b51be
user:        Kearwood (Kip) Gilbert <kgilbert@mozilla.com>
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
Blocks: 1022818
tracking-firefox34: --- → ?
tracking-firefox35: --- → ?

Updated

4 years ago
Component: General → Layout
Product: Firefox → Core

Comment 5

4 years ago
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.
Component: Layout → DOM: Core & HTML

Comment 6

4 years ago
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.
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.
Flags: needinfo?(kgilbert)
(Assignee)

Comment 8

4 years ago
I am implementing a fix now, thanks for the detailed investigation.
Assignee: nobody → kgilbert
Flags: needinfo?(kgilbert)

Updated

4 years ago
Duplicate of this bug: 1064385

Updated

4 years ago
Summary: Amazon's "Manage Your Content and Devices" fails to render on Nightly → Amazon's "Manage Your Content and Devices" fails to render on Nightly (scrollTo float/long issue)
(Assignee)

Comment 10

4 years ago
Created 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

- 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.
Attachment #8486101 - Flags: review?(bzbarsky)
(Assignee)

Comment 11

4 years ago
Created attachment 8486140 [details] [diff] [review]
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.
Attachment #8486140 - Flags: review?(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
Attachment #8486101 - Flags: review?(bzbarsky) → review+
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
Attachment #8486140 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 15

4 years ago
Created 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

- 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.
Attachment #8486101 - Attachment is obsolete: true
(Assignee)

Comment 16

4 years ago
Created attachment 8486595 [details] [diff] [review]
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.
Attachment #8486140 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
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?
Or ToZeroIfNonfinite, maybe.
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Comment 19

4 years ago
(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.
(Assignee)

Comment 20

4 years ago
Created 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

- 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
Attachment #8486592 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
Kearwood, are you going to raise the spec issue, or should I?
Flags: needinfo?(kgilbert)
(Assignee)

Comment 23

4 years ago
I will raise the spec issue and CC the link here.
Flags: needinfo?(kgilbert)
(Assignee)

Comment 24

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

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

Comment 25

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

http://lists.w3.org/Archives/Public/www-style/2014Sep/0155.html
https://hg.mozilla.org/mozilla-central/rev/582045ca275c
https://hg.mozilla.org/mozilla-central/rev/24a4613761b8
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla35

Comment 27

4 years ago
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
Status: RESOLVED → VERIFIED

Comment 28

4 years ago
kip, can you please request approval to uplift this fix to Aurora 34?
status-firefox35: affected → verified
Flags: needinfo?(kgilbert)
(Assignee)

Comment 29

4 years ago
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
Attachment #8486609 - Flags: approval-mozilla-aurora?
Flags: needinfo?(kgilbert)
(Assignee)

Comment 30

4 years ago
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
Attachment #8486595 - Flags: approval-mozilla-aurora?
status-firefox32: --- → unaffected
status-firefox33: --- → unaffected
tracking-firefox34: ? → +
tracking-firefox35: ? → ---
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+
Attachment #8486609 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8486595 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 33

4 years ago
Verified on 

Firefox 34.0a2
BuildID: 20140917004003
status-firefox34: fixed → verified
Depends on: 1274086
You need to log in before you can comment on or make changes to this bug.