Last Comment Bug 749935 - Vertical scrollbar missing on specific sites since upgrading to Firefox 12
: Vertical scrollbar missing on specific sites since upgrading to Firefox 12
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla15
Assigned To: Mats Palmgren (vacation)
:
Mentors:
http://www.microsoft.com/learning/en/...
: 749699 749940 758817 (view as bug list)
Depends on:
Blocks: 665597 750323 751278
  Show dependency treegraph
 
Reported: 2012-04-28 05:25 PDT by Tobias (:Tobbi) Markus
Modified: 2012-07-20 13:56 PDT (History)
17 users (show)
mats: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
+
verified


Attachments
sample html (336 bytes, text/html)
2012-04-28 06:49 PDT, Alice0775 White
no flags Details
wip (1.83 KB, patch)
2012-04-28 10:03 PDT, Mats Palmgren (vacation)
no flags Details | Diff | Splinter Review
fix (9.99 KB, patch)
2012-04-29 17:56 PDT, Mats Palmgren (vacation)
roc: feedback+
Details | Diff | Splinter Review
fix+tests (12.66 KB, patch)
2012-05-13 11:29 PDT, Mats Palmgren (vacation)
roc: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Tobias (:Tobbi) Markus 2012-04-28 05:25:00 PDT
A user in the Support forums reported that the scrollbar is missing on a certain site:
http://www.microsoft.com/learning/en/us/Exam.aspx?ID=70-659#tab2

I can reproduce the issue with the latest trunk. On the Microsoft site, there doesn't seem to be 'overflow-y: hidden' anywhere so I doubt this is actually an issue with the site. 

In addition, the site works correctly in Microsoft Internet Explorer.

Thread for reference:
https://support.mozilla.org/en-US/questions/926245
Comment 1 Tobias (:Tobbi) Markus 2012-04-28 06:11:07 PDT
Last good nightly: 2012-01-17
First bad nightly: 2012-01-18

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=34572943a3e4&tochan
ge=f4049f65efc6
Comment 3 Alice0775 White 2012-04-28 06:49:28 PDT
Created attachment 619291 [details]
sample html

Regression window(m-c)
Works:
http://hg.mozilla.org/mozilla-central/rev/0b4c58200e3a
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120117 Firefox/12.0a1 ID:20120117042508
Fails:
http://hg.mozilla.org/mozilla-central/rev/ff1bedd7d463
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120117 Firefox/12.0a1 ID:20120117072408
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0b4c58200e3a&tochange=ff1bedd7d463


Regression window(m-i)
Works:
http://hg.mozilla.org/integration/mozilla-inbound/rev/7ea3311b5e6c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120116 Firefox/12.0a1 ID:20120116145408
Fails:
http://hg.mozilla.org/integration/mozilla-inbound/rev/3077b729dfd5
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120116 Firefox/12.0a1 ID:20120116153908
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7ea3311b5e6c&tochange=3077b729dfd5


Last good:
L:\trunk\2012\01\bin inbound 16-Jan-2012 1538-5 804c80adda0a
First bad:
L:\trunk\2012\01\bin inbound 16-Jan-2012 1538-6 87536f378706

Triggered by:
87536f378706	Mats Palmgren — Bug 665597 - Use saturating calculations when adding the margin to the scrollable overflow rect. part=4/5 r=roc
Comment 4 Alice0775 White 2012-04-28 07:43:18 PDT
about attachment 619291 [details], it works if I change css 
.hide { position: absolute; left: -999999em; top: -999999em; }
instead of
.hide { position: absolute; left: -99999em; top: -99999em; }

I guess that big value of css is a problems.
Comment 5 Mats Palmgren (vacation) 2012-04-28 09:44:40 PDT
Thanks Alice, yes you're right about the cause.

-999999em in itself is less than nscoord_MIN so it's clamped in the style system
already, then when calculating the scrollable overflow rect for #body, adding
the 1000px height doesn't make a difference since the overflow rect height is
clamped to nscoord_MAX.  Previously we would end up with nscoord_MAX + 1000px.

I don't think there is a way to fix this properly unless we start using floats
(bug 265084).  We can mitigate this by clamping harder in the style system on
specified values, for example clamping to nscoord_MAX / 2 would still allow
for 8947849px.  That way, adding reasonable values to it during layout
shouldn't trigger clamping unless a bunch of these values are added together
but that seems like it would be rare in practice...
Comment 6 Mats Palmgren (vacation) 2012-04-28 10:03:32 PDT
Created attachment 619324 [details] [diff] [review]
wip

something like this...

https://tbpl.mozilla.org/?tree=Try&rev=406b10c4f60b
Comment 7 Mats Palmgren (vacation) 2012-04-28 18:46:46 PDT
*** Bug 749940 has been marked as a duplicate of this bug. ***
Comment 8 Mats Palmgren (vacation) 2012-04-29 17:56:24 PDT
Created attachment 619464 [details] [diff] [review]
fix

I think this is a relatively low-risk fix for this bug,
if indeed we want to try to fix it at all.
What do you think?

https://tbpl.mozilla.org/?tree=Try&rev=9ae39ed8d6a0
Comment 9 David Baron :dbaron: ⌚️UTC+1 (mostly busy through August 4; review requests must explain patch) 2012-05-01 17:46:30 PDT
Comment on attachment 619464 [details] [diff] [review]
fix

>+// CoordStyleClamp clamps to nscoord_MIN/2 .. nscoord_MAX/2 unlike
>+// NSToCoordRoundWithClamp which clamps to nscoord_MIN .. nscoord_MAX.
>+// This is to allow adding/subtracting reasonable values to it during
>+// layout without triggering NSToCoordRoundWithClamp clamping there.

So nscoord_MIN and nscoord_MAX already give us only half the range of 32-bit integers, for exactly this sort of reason.  And now we're restricting it to half again?  I'm not crazy about that idea.

Maybe we should consider doing this nscoord_MIN/2 sort of clamping when unioning rectangles -- just for the x and the y, so that we'd essentially never allow a very small x/y to destroy the useful part of the height data?  Or maybe we should allow the width and height of overflow rects to use the full 32-bit int range?
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-08 15:58:35 PDT
(In reply to David Baron [:dbaron] from comment #9)
> Maybe we should consider doing this nscoord_MIN/2 sort of clamping when
> unioning rectangles -- just for the x and the y, so that we'd essentially
> never allow a very small x/y to destroy the useful part of the height data?

Makes sense to me.
Comment 11 Alice0775 White 2012-05-09 07:01:31 PDT
*** Bug 749699 has been marked as a duplicate of this bug. ***
Comment 12 Mats Palmgren (vacation) 2012-05-13 11:29:11 PDT
Created attachment 623512 [details] [diff] [review]
fix+tests

Make all nsRect Union methods saturating.  If width/height overflows nscoord_MAX then clamp the x/y to nscoord_MIN / 2 and try again.
Comment 13 Mats Palmgren (vacation) 2012-05-13 11:38:48 PDT
https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=23ca42af1058
Comment 15 Ed Morley [:emorley] 2012-05-15 06:46:37 PDT
https://hg.mozilla.org/mozilla-central/rev/8c0f00a6a12f
Comment 16 Mats Palmgren (vacation) 2012-05-15 21:05:31 PDT
Probably worth taking on branches considering the risk/benefit.
Comment 17 Alex Keybl [:akeybl] 2012-05-16 12:50:34 PDT
(In reply to Mats Palmgren [:mats] from comment #16)
> Probably worth taking on branches considering the risk/benefit.

Sounds good - we agree. Please nominate for aurora/beta approval when ready (but before 5/21 to make it into our next beta).
Comment 18 Mats Palmgren (vacation) 2012-05-19 06:14:56 PDT
Comment on attachment 623512 [details] [diff] [review]
fix+tests

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 665597
User impact if declined: missing scrollbars on pages that use excessively large CSS length values
Testing completed (on m-c, etc.): baked on m-c since 2012-05-15
Risk to taking this patch (and alternatives if risky): The patch makes the code slightly more branchy in a relatively hot path, so there's a (low) risk for performance regressions in layout heavy work loads.
String or UUID changes made by this patch: none
Comment 19 Mats Palmgren (vacation) 2012-05-20 21:36:59 PDT
Landed on Aurora per verbal approval in comment 17:
https://hg.mozilla.org/releases/mozilla-aurora/rev/5c69f909484c
still waiting for the mozilla-beta tree to open...
Comment 20 Mats Palmgren (vacation) 2012-05-21 23:13:21 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/22cfcc320512
Comment 21 bob 2012-05-26 02:07:04 PDT
*** Bug 758817 has been marked as a duplicate of this bug. ***
Comment 22 Ioana (away) 2012-05-30 06:08:41 PDT
Verified as fixed with the link from comment 0 and the sample.html on:
Mozilla/5.0 (X11; Linux i686; rv:13.0) Gecko/20100101 Firefox/13.0
Mozilla/5.0 (Windows NT 6.1; rv:13.0) Gecko/20100101 Firefox/13.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:13.0) Gecko/20100101 Firefox/13.0
BuildID: 20120528154913
Comment 23 Ioana (away) 2012-06-06 07:30:52 PDT
Verified as fixed with the link from comment 0 and the sample.html on:
Mozilla/5.0 (X11; Linux i686; rv:14.0) Gecko/20100101 Firefox/14.0
Mozilla/5.0 (Windows NT 6.1; rv:14.0) Gecko/20100101 Firefox/14.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:14.0) Gecko/20100101 Firefox/14.0 BuildID: 20120605113340
Comment 24 hiryu 2012-07-20 13:48:09 PDT
Maybe I didn't understood what this bug is but could someone help me with this?
http://i50.tinypic.com/e96rg0.jpg
I think it's the same issue you're talking about but I've got on FF 14.0.1 :/
Comment 25 Mats Palmgren (vacation) 2012-07-20 13:56:08 PDT
hiryu, that looks like a different problem.  Please file a new bug if you
can reproduce it using a fresh profile.  Thanks.

Note You need to log in before you can comment on or make changes to this bug.