407415 - NaNs in gfx*Font with font-size:0 and font-size-adjust

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Karl Tomlinson (:karlt)]

Attachment: testcase

The only reason

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/gfx/thebes/src/gfxPangoFonts.cpp&rev=1.117&root=/cvsroot&mark=330-341#327

doesn't loop infinitely is that PR_MAX(0.0/0.0,1.0) happens to be 1.0:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/gfx/thebes/public/gfxFont.h&rev=1.93&mark=112#108

Comment 1

[Karl Tomlinson (:karlt)]

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

[Mats Palmgren (inactive)]

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

Comment 3

[Karl Tomlinson (:karlt)]

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

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

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

Changing platform to All

Comment 7

[Karl Tomlinson (:karlt)]

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

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

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

This seems to be turning all the windows unit test tinderboxen orange.

Comment 11

[Karl Tomlinson (:karlt)]

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

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

So what should we do here?

Comment 14

[Karl Tomlinson (:karlt)]

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.