Closed Bug 1062406 Opened 10 years ago Closed 10 years ago

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

Categories

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

34 Branch
All
Windows 7
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla35
Tracking Status
firefox32 --- unaffected
firefox33 --- unaffected
firefox34 + verified
firefox35 --- verified

People

(Reporter: marcausl, Assigned: kip)

References

(Depends on 1 open bug, )

Details

(Keywords: regression)

Attachments

(2 files, 3 obsolete files)

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
Last good revision: dc352a7bf234 (2014-08-26) First bad revision: 0753f7b93ab7 (2014-08-27)
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
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
Keywords: regression
Version: Trunk → 34 Branch
[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
Component: General → Layout
Product: Firefox → Core
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
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)
I am implementing a fix now, thanks for the detailed investigation.
Assignee: nobody → kgilbert
Flags: needinfo?(kgilbert)
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)
- 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)
Attached patch Bug 1062406: Part 2 - Tests (obsolete) — Splinter Review
- 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+
- 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
- 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
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.
Keywords: checkin-needed
(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.
- 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
Keywords: checkin-needed
Kearwood, are you going to raise the spec issue, or should I?
Flags: needinfo?(kgilbert)
I will raise the spec issue and CC the link here.
Flags: needinfo?(kgilbert)
I have submitted a bug at w3.org to raise the issue: https://www.w3.org/Bugs/Public/show_bug.cgi?id=26775
I have also started a thread on the w3.org mailing list: http://lists.w3.org/Archives/Public/www-style/2014Sep/0155.html
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
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
kip, can you please request approval to uplift this fix to Aurora 34?
Flags: needinfo?(kgilbert)
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)
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?
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+
Verified on Firefox 34.0a2 BuildID: 20140917004003
Depends on: 1274086
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: