Last Comment Bug 407415 - NaNs in gfx*Font with font-size:0 and font-size-adjust
: NaNs in gfx*Font with font-size:0 and font-size-adjust
Status: RESOLVED FIXED
[not needed for 1.9]
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: mozilla1.9
Assigned To: Karl Tomlinson (back Dec 13 :karlt)
:
: Milan Sreckovic [:milan]
Mentors:
Depends on: 407352
Blocks: 363410
  Show dependency treegraph
 
Reported: 2007-12-07 13:38 PST by Karl Tomlinson (back Dec 13 :karlt)
Modified: 2011-12-04 14:53 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (156 bytes, text/html)
2007-12-07 13:38 PST, Karl Tomlinson (back Dec 13 :karlt)
no flags Details
don't repeat on zero size [checked-in for Pango] (950 bytes, patch)
2007-12-07 13:43 PST, Karl Tomlinson (back Dec 13 :karlt)
roc: review+
dsicore: approval1.9+
Details | Diff | Splinter Review
check for zero xHeight [checked-in for Atsui] (6.43 KB, patch)
2007-12-13 20:53 PST, Karl Tomlinson (back Dec 13 :karlt)
roc: review+
roc: superreview+
Details | Diff | Splinter Review

Comment 1 Karl Tomlinson (back Dec 13 :karlt) 2007-12-07 13:43:29 PST
Created attachment 292139 [details] [diff] [review]
don't repeat on zero size [checked-in for Pango]

Bug 407352 will hopefully mean this code path is not hit for this case, but let's protect anyway.
Comment 3 Karl Tomlinson (back Dec 13 :karlt) 2007-12-09 14:01:40 PST
(In reply to comment #2)
> Do we have the same problem on Windows/Mac?
> http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/gfx/thebes/src/gfxWindowsFonts.cpp&rev=1.160&root=/cvsroot&mark=192-193,262,245-246,253#189

Not sure.  Can mMetrics->emHeight be zero here?
Does bug 404112 comment 37 indicate that this is usually non-zero even for zero-size fonts?

Even if it can be zero, I can't see any looping here that could risk being infinite.

But I hear Windows sometimes crashes on NaNs, so it might be good to ensure we don't divide 0 by 0.

> http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/gfx/thebes/src/gfxAtsuiFonts.cpp&rev=1.76&root=/cvsroot&mark=130-131,168-169,171-172,175#112

Yes, same kind of thing as Pango with zeros in slightly different places.
Comment 4 Mats Palmgren (:mats) 2007-12-10 10:55:30 PST
(In reply to comment #3)
> Not sure.  Can mMetrics->emHeight be zero here?

In branch builds some (buggy?) fonts resulted in zero ascent and descent.
I haven't tested trunk with said fonts so I don't know if it still occurs.
Seems like a good idea to make the code robust against it, just in case...
See bug 279032 and dupes.  (roc@ suggests in 279032 comment 17 we should
return all-zero metrics in this case)

> Does bug 404112 comment 37 indicate that this is usually non-zero even for
> zero-size fonts?

Yes, all three platforms returns some fallback font it seems.
(Windows/Mac had small values which I'm guessing is the minimal size
for that font.)   We should probably return all-zero metrics on all
platforms for a zero size font, for consistency (filed bug 407765).

> Even if it can be zero, I can't see any looping here that could risk being
> infinite.

On second reading it looks ok to me too.
Comment 5 Karl Tomlinson (back Dec 13 :karlt) 2007-12-11 14:50:11 PST
Comment on attachment 292139 [details] [diff] [review]
don't repeat on zero size [checked-in for Pango]

Requesting approval for 1.9.  There are currently no known bugs that this fixes, but this makes things safer wrt changes elsewhere in the code.
Comment 6 Karl Tomlinson (back Dec 13 :karlt) 2007-12-13 20:37:13 PST
Changing platform to All
Comment 7 Karl Tomlinson (back Dec 13 :karlt) 2007-12-13 20:53:56 PST
Created attachment 293072 [details] [diff] [review]
check for zero xHeight [checked-in for Atsui]


Check for zero xHeight in Windows and Atsui code to avoid divide by zero in GetAdjustedSize().

Also makes changes to Windows/Atsui/Pango to make it clearer that these are floats not integers.

I haven't tested Windows/Atsui but the changes are simple.
Comment 8 Robert O'Callahan (:roc) (email my personal email if necessary) 2007-12-18 01:13:20 PST
Comment on attachment 293072 [details] [diff] [review]
check for zero xHeight [checked-in for Atsui]


Should you also check that emHeight is nonzero in gfxWindowsFonts to ensure we don't divide by zero on it? I guess it should be equal to the requested font size which should be nonzero, but I'm not sure if we can guarantee that.

If you want to do that, you can add it to this patch or make a new patch.
Comment 9 Karl Tomlinson (back Dec 13 :karlt) 2007-12-18 13:36:41 PST
(In reply to comment #8)
> Should you also check that emHeight is nonzero in gfxWindowsFonts to ensure we
> don't divide by zero on it?

I don't think we need to guarantee that emHeight is non-zero.
IIUC dividing a non-zero by zero is not such an issue now that we use floats rather than ints.

"aspect = mMetrics->xHeight / 0.0f" would be +/-inf depending on the sign of mMetrics->xHeight, which is now not going to be 0.0f.

"size*(sizeAdjust/aspect)" would then be +/-0.0f provided size and sizeAdjust are not +/-inf (or NaN).
Comment 10 Bill Gianopoulos [:WG9s] 2007-12-19 14:02:52 PST
This seems to be turning all the windows unit test tinderboxen orange.
Comment 11 Karl Tomlinson (back Dec 13 :karlt) 2007-12-19 16:28:25 PST
Comment on attachment 293072 [details] [diff] [review]
check for zero xHeight [checked-in for Atsui]


This was checked in, then in gfxWindowsFont 0.0f was changed to 0.0 and the xHeight test was backed out as the MozillaAliveTest failed.

The 0.0f to 0.0 change was because I saw that gfxFloat is a double not a float.  This should probably be changed for Pango and Atsui too.

I suspect the MozillaAliveTest failed because mMetrics was null.  ("exited with status 5" is the only info I have.)  I haven't yet worked out the logic behind when to throw out the metrics,

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/gfx/thebes/src/gfxWindowsFonts.cpp&rev=1.160&mark=252-253,257-259#245

so I'm not sure of the right way to avoid a 0.0/0.0.  But this is not a top priority right now.
Comment 12 Joe Drew (not getting mail) 2008-05-09 10:56:01 PDT
Comment on attachment 293072 [details] [diff] [review]
check for zero xHeight [checked-in for Atsui]


Looks like we don't want to land this.
Comment 13 Benoit Jacob [:bjacob] (mostly away) 2011-12-02 13:33:48 PST
So what should we do here?
Comment 14 Karl Tomlinson (back Dec 13 :karlt) 2011-12-04 14:53:25 PST
I'd guess the analysis in this bug is out of date and so there's not much point keeping it open.  Marking fixed because something landed for Pango fonts.

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