749935 - Vertical scrollbar missing on specific sites since upgrading to Firefox 12

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Tobias (:Tobbi) Markus]

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]

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 2

[Tobias (:Tobbi) Markus]

Proper Pushlog URL:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=34572943a3e4&tochange=f4049f65efc6

Comment 3

[Alice0775 White]

Attachment: 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]

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 (inactive)]

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 (inactive)]

Attachment: wip

something like this...

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

Comment 8

[Mats Palmgren (inactive)]

Attachment: 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-4, no longer working on Mozilla)]

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) (email my personal email if necessary)]

(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 12

[Mats Palmgren (inactive)]

Attachment: 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 (inactive)]

https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=23ca42af1058

Comment 14

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c0f00a6a12f

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8c0f00a6a12f

Comment 16

[Mats Palmgren (inactive)]

Probably worth taking on branches considering the risk/benefit.

Comment 17

[Alex Keybl [:akeybl]]

(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 (inactive)]

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 (inactive)]

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 (inactive)]

https://hg.mozilla.org/releases/mozilla-beta/rev/22cfcc320512

Comment 22

[Ioana (away)]

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)]

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]

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 (inactive)]

hiryu, that looks like a different problem.  Please file a new bug if you
can reproduce it using a fresh profile.  Thanks.