448281 - font-size sometimes gets calculated wrong

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Radek 'sysKin' Czyz]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1a2pre) Gecko/2008072803 Firefox/3.1a2pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1a2pre) Gecko/2008072803 Minefiled/3.1a2pre

Starting with yesterday's nightly, some fonts are *sometimes* too big on some websites. DOM inspector reveals that calculated font-size is indeed incorrect, being too big by a random (but consistent on one website) factor. It happens randomly.

Reproducible: Sometimes

Steps to Reproduce:
1. visit http://forums.whirlpool.net.au/ or http://forums.mozillazine.org/index.php
2. reload it
Actual Results:  
Example what you *might* see on mozillazine: http://members.ii.net/~syskin/fontproblem.png and after reload, you get more familiar http://members.ii.net/~syskin/fontproblem2.png

No font zoom, regular zoom or OS dpi change was made. Fonts are bigger by consistent 60%.

Example from whirlpool:
Looking fine: http://members.ii.net/~syskin/wp2.png
After reload: http://members.ii.net/~syskin/wp1.png

Some fonts are reliably bigger by exactly 25%, others are unaffected. They have nothing obviously in common with regards to font-family. They have identical CSS rules according to DOMi, but different CSS computed style.


Expected Results:  
Consistency :)


Discussion at http://forums.mozillazine.org/viewtopic.php?f=23&t=768225

Whirlpool problem was confirmed on OSX as well.

I have a strong feeling I've seen such problems for ages (long before FF 3) but VERY occasionally. I remember a bugzilla bug being suddenly too large with a reload fixing it.

After saving whirlpool page to disk, I can't reproduce the problem at all. Randomness might be a timing issue.

Comment 1

[u88484]

Confirming based on comments on mozillazine.org

Comment 2

[Radek 'sysKin' Czyz]

Thanks to mozillazine's smsmith, new better steps to reproduce were found.

Reproducible: Kinda often Not Always

1. Open mozillazine forums index
2. Ensure you have full page zoom selected, and tap ctrl+ OR ctrl- to change zoom away from 100%
3. Reload
4. (optional) change zoom back to 100% by pressing ctrl0

Actual results: 
* Fonts are bigger after step 3 than they were after step 2
* Fonts are bigger after step 4 than they were after step 1

Expected results: fonts are consistent for any fixed zoom level

Comment 3

[Radek 'sysKin' Czyz]

And now, some fun: do steps above but for this bug page.

Fonts don't change, but observe "Attachments" panel - it grows larger for no reason! :)

So it's probably not just fonts, any random website element might be rendered bigger than it should be.

Comment 4

[Bill Gianopoulos [:WG9s]]

Regression range 20080726 - no issue
                 20080727 - this issue exists.

Comment 5

[Bill Gianopoulos [:WG9s]]

I have verified this a regression from bug 156716 via source pulls by changeset identifier.

Comment 6

[Bill Gianopoulos [:WG9s]]

In the steps to reproduce it should also say to ensure you do NOT have "View -> Zoom -> Zoom Text Only" selected.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #5)
> I have verified this a regression from bug 156716 via source pulls by changeset
> identifier.

If you did that, did you happen to notice which changeset?

Comment 8

[Ryan VanderMeulen [:RyanVM]]

This should be blocking 1.9.1 due to it being an ugly regression.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #5)
> I have verified this a regression from bug 156716 via source pulls by changeset
> identifier.

Also, which testcases did you verify were a regression?

Comment 10

[Bill Gianopoulos [:WG9s]]

(In reply to comment #9)
> (In reply to comment #5)
> > I have verified this a regression from bug 156716 via source pulls by changeset
> > identifier.
> 
> Also, which testcases did you verify were a regression?
> 
The case of visiting http://forums.mozillazine.org/viewforum.php?f=23, doing a ctrl + to increase the zoom level then doing a reload of the page and seeing some section get much larger on the reload, then doing a ctrl 0 and not having the page return to normal until doing a reload.

Everything worked normally on a clone specifying the changeset immediately prior to when the first code for this bug was pushed, and not so good on a build form an hg clone specifying the last changeset pushed for this bug.

I was planning to try to narrow it down to one of the individual checkins for this bug unless you feel this is not necessary.

Comment 11

[Bill Gianopoulos [:WG9s]]

I have not been able to reproduce the issue in comment #0, so I cannot speak to a regression bug for that.  The issues raised in comment #2 and comment #3 both appear to have regressed with the checkin of the following changeset:

dbaron
Sat Jul 26 16:15:36 2008 +0000

8050b146979b

Implement Media Queries, part 4: infrastructure for dynamic
change handling at the pres context level. (Bug 156716)
r+sr=bzbarsky

Comment 12

[philippe (part-time)]

I can confirm Bill's regression range with http://forums.whirlpool.net.au/ linked from comment 0.
STR
1. load site
2. pay attention to the font-size of the subheadings (blue foreground-colour, grey background)
3. reload the page, watch the size of those sub headlines shrink

Now the puzzling thing I found when it was posted in the forums:
a. view source, fetch the url to the stylesheet, paste it in the same tab
b. back-button to the html page
c. the subheadings are bigger again.

Comment 13

[Dave Townsend [:mossop]]

I see this frequently on google reader too.

Comment 15

[Derrick Austin]

We did alot of testing of this in Bug 449126 before we realized the duplicate bug you might read through it. (Screenshots of the bug are in it)

I was experiencing the problem at:
http://www.newegg.com
and the member's pages at http://www.icanhascheezburger.com

Confirmed with 3 setups, one Vista, 2 XP Pro.

Comment 16

[Martijn Wargers (dead)]

This reminds me a lot of bug 436717. An indeed, it can be triggered with the testcase in that bug too, when following the steps to reproduce in comment 2 of this bug.
More evidence it is related, the urls mentioned in this bug are all in strict mode.
I guess there is something in the site specific preferences code that triggers it now.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

My gut feeling is that there are two bugs here:

 1) something causing the MQ dynamic change handling to detect that things need to change when they actually don't

 2) an underlying bug (pre-MQ) where bugs like this happen when we rebuild all style data

All the changeset that causes this bug does is cause us to rebuild all style data in additional cases; this normally shouldn't change anything unless styles have actually changed.  See http://hg.mozilla.org/mozilla-central/index.cgi/rev/8050b146979b

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And a simplified testcase could be useful here.

Comment 19

[Martijn Wargers (dead)]

It happens for me with https://bugzilla.mozilla.org/attachment.cgi?id=323241 , which is the testcase of bug 436717.

Comment 20

[Radek 'sysKin' Czyz]

It is perhaps worth pointing out that all affected elements (including width of 'attachments' panel in this bug) have their size specified in em.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #19)
> It happens for me with https://bugzilla.mozilla.org/attachment.cgi?id=323241 ,
> which is the testcase of bug 436717.

In that testcase, when the bug occurs, the table has rules from quirks.css applied to it (which should not happen).

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

OK, what's going on is that during the page load process, we first enable the quirk style sheet and then disable it.  (And if we didn't enable it first, it would be enabled by default anyway.)

What's happening here is that we're now triggering the building of the rule cascade before the quirk style sheet is disabled, and then we're not doing anything to clear the cached data once it is disabled:

#0  nsCSSRuleProcessor::RefreshRuleCascade (this=0x91aef0, 
    aPresContext=0x1a722d0)
    at /home/dbaron/builds/mozilla-central/mozilla/layout/style/nsCSSRuleProcessor.cpp:2399
#1  0x00007f876dbc5b13 in nsCSSRuleProcessor::MediumFeaturesChanged (
    this=0x91af08, aPresContext=0x1a722d0, aRulesChanged=0x17c0ab0)
    at /home/dbaron/builds/mozilla-central/mozilla/layout/style/nsCSSRuleProcessor.cpp:2094
#2  0x00007f876dc10d5c in nsStyleSet::MediumFeaturesChanged (this=0xf18b10, 
    aPresContext=0x1a722d0)
    at /home/dbaron/builds/mozilla-central/mozilla/layout/style/nsStyleSet.cpp:995
#3  0x00007f876dab6021 in nsPresContext::MediaFeatureValuesChanged (
    this=0x1a722d0, aCallerWillRebuildStyleData=1)
    at /home/dbaron/builds/mozilla-central/mozilla/layout/base/nsPresContext.cpp:1454
#4  0x00007f876dab6754 in nsPresContext::SetFullZoom (this=0x1a722d0, 
    aZoom=1.10000002)
    at /home/dbaron/builds/mozilla-central/mozilla/layout/base/nsPresContext.cpp:1168

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2: assert if quirk style sheet enabled/disabled after building rule cascade

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 1: don't rebuild rule cascades if they're aren't currently any

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 332497 [details] [diff] [review]
patch 1: don't rebuild rule cascades if they're aren't currently any

>+  // early (e.g., before whether we know if the quirk style sheet should

s/if/whether/

r+sr=bzbarsky.

Any sane way to test this?

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I landed these:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/ac776b1e0206
http://hg.mozilla.org/mozilla-central/index.cgi/rev/93b1b9aa2831

It's probably possible to test this with chrome privileges, though it's also a good bit of work to write the test... I don't have time right now.


People should test that the various bugs duped to this one are all fixed; I've only looked at Martijn's testcase, since it was simple.

Comment 27

[Radek 'sysKin' Czyz]

Verified fixed, all's good with all websites mentioned in this bug.

Comment 28

[Derrick Austin]

Verified fixed here too.

Comment 29

[[e-mail]]

Comment on attachment 332496 [details] [diff] [review]
patch 2: assert if quirk style sheet enabled/disabled after building rule cascade

I can trigger this assertion using the following steps:

1. Turn on SSL warnings.
2. Open an SSL page in a background tab.
3. OK the SSL warning.

The pres shell is trying to change from standards to almost standards mode, so we shouldn't be hitting this code. I don't know if these steps will trigger an assertion when the page really should be in quirks mode.

Comment 30

[[e-mail]]

Attachment: Stack for assertion

The assertion happens with a quirks mode page too.

Comment 31

[Joshua Mitchell (Inactive)]

Issue is Resolved - removing QA-Wanted Keywords - QA-Wanted query clean-up task