693143 - Crash in _cairo_dwrite_font_face_scaled_font_create

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Scoobidiver (away)]

It's #35 top crasher in 8.0b1 while it is only #115 in 7.0.1 and #188 in 6.0.2.

There are two kinds of stack traces:
Frame 	Module 	Signature [Expand] 	Source
0 	xul.dll 	_cairo_dwrite_font_face_scaled_font_create 	gfx/cairo/cairo/src/cairo-dwrite-font.cpp:462
1 	xul.dll 	_moz_cairo_scaled_font_create 	gfx/cairo/cairo/src/cairo-scaled-font.c:1054
2 	xul.dll 	DCFromContext::DCFromContext 	gfx/thebes/gfxWindowsPlatform.h:79
3 	xul.dll 	HBGetGlyphAdvance 	gfx/thebes/gfxHarfBuzzShaper.cpp:266
4 	xul.dll 	hb_ot_shape_execute_internal 	gfx/harfbuzz/src/hb-ot-shape.cc:344
5 	xul.dll 	gfxHarfBuzzShaper::InitTextRun 	gfx/thebes/gfxHarfBuzzShaper.cpp:888
6 	xul.dll 	gfxGDIFont::InitTextRun 	gfx/thebes/gfxGDIFont.cpp:166
7 	xul.dll 	gfxFont::SplitAndInitTextRun 	gfx/thebes/gfxFont.cpp:1513
8 	xul.dll 	gfxFontGroup::InitScriptRun 	gfx/thebes/gfxFont.cpp:2524
9 	xul.dll 	gfxFontGroup::InitTextRun 	gfx/thebes/gfxFont.cpp:2484
10 	xul.dll 	gfxFontGroup::MakeTextRun 	gfx/thebes/gfxFont.cpp:2418
11 	xul.dll 	TextRunWordCache::MakeTextRun 	gfx/thebes/gfxTextRunWordCache.cpp:850

Frame 	Module 	Signature [Expand] 	Source
0 	xul.dll 	_cairo_dwrite_font_face_scaled_font_create 	gfx/cairo/cairo/src/cairo-dwrite-font.cpp:462
1 	xul.dll 	_moz_cairo_scaled_font_create 	gfx/cairo/cairo/src/cairo-scaled-font.c:1054
2 	xul.dll 	_cairo_gstate_ensure_scaled_font 	gfx/cairo/cairo/src/cairo-gstate.c:1811
3 	xul.dll 	GetRoundOffsetsToPixels 	gfx/thebes/gfxHarfBuzzShaper.cpp:932
4 	xul.dll 	gfxHarfBuzzShaper::SetGlyphsFromRun 	gfx/thebes/gfxHarfBuzzShaper.cpp:1020
5 	xul.dll 	gfxHarfBuzzShaper::InitTextRun 	gfx/thebes/gfxHarfBuzzShaper.cpp:897
6 	xul.dll 	gfxGDIFont::InitTextRun 	gfx/thebes/gfxGDIFont.cpp:166
7 	xul.dll 	gfxFont::SplitAndInitTextRun 	gfx/thebes/gfxFont.cpp:1513
8 	xul.dll 	gfxFontGroup::InitScriptRun 	gfx/thebes/gfxFont.cpp:2524
9 	xul.dll 	gfxFontGroup::InitTextRun 	gfx/thebes/gfxFont.cpp:2484
10 	xul.dll 	gfxFontGroup::MakeTextRun 	gfx/thebes/gfxFont.cpp:2436
11 	xul.dll 	TextRunWordCache::MakeTextRun 	gfx/thebes/gfxTextRunWordCache.cpp:717
12 	xul.dll 	MakeTextRun 	layout/generic/nsTextFrameThebes.cpp:551
...

More reports at:
https://crash-stats.mozilla.com/report/list?signature=_cairo_dwrite_font_face_scaled_font_create&version=Firefox%3A8.0

Comment 1

[Jeff Muizelaar [:jrmuizel]]

I've I had to guess at this I'd guess that font_face contains uninitialized memory, but it's not obvious to me what would be going on here.

Comment 2

[Scoobidiver (away)]

In every crash reports I checked, D3D9 and D3D10 are disabled.

Comment 3

[Jonathan Kew [:jfkthame]]

The problem here is that we're using GDI fonts (see frame 6), but the cairo font didn't get initialized at the appropriate time and then internally cairo has tried to create a fallback font using the DWrite backend (see frame 0). That mixture doesn't work. We fixed something like this previously in bug 544617, but there must be some subtly different conditions here that are leading to a similar problem.

Comment 4

[Joe Drew (not getting mail)]

John, can you take a look at this & pass it off if you're not comfortable?

Comment 5

[Joe Drew (not getting mail)]

Er, whoops, midair.

John, can you take a look at this & pass it off if you're not comfortable?

Comment 6

[Bob Clary [:bc] (inactive)]

1. http://lizbraswell.com/
2. Crash Firefox 8 and Beta/9 at address 0xcdcdcdcd	IDWriteFontFace *

If I understand this correctly, this is susceptible to arbitrary code execution? 

-		&font_face	0x0017217c	_cairo_dwrite_font_face * *
-			0x042b1928 {base={...} font=0x01935110 dwriteface=0xcdcdcdcd }	_cairo_dwrite_font_face *
+		base	{hash_entry={...} status=CAIRO_STATUS_SUCCESS ref_count={...} ...}	_cairo_font_face
+		font	0x01935110	IDWriteFont *
+		dwriteface	0xcdcdcdcd	IDWriteFontFace *


>	xul.dll!_cairo_dwrite_font_face_scaled_font_create(void * abstract_face=0x042b1928, const _cairo_matrix * font_matrix=0x6831ad38, const _cairo_matrix * ctm=0x6831ada0, const _cairo_font_options * options=0x00172384, _cairo_scaled_font * * font=0x001721b4)  Line 462 + 0x10 bytes	C++
 	xul.dll!_moz_cairo_scaled_font_create(_cairo_font_face * font_face=0x042b1928, const _cairo_matrix * font_matrix=0x6831ad38, const _cairo_matrix * ctm=0x6831ada0, const _cairo_font_options * options=0x00172384)  Line 1054 + 0x22 bytes	C
 	xul.dll!_cairo_gstate_ensure_scaled_font(_cairo_gstate * gstate=0x6831ace8)  Line 1811 + 0x21 bytes	C
 	xul.dll!_cairo_gstate_get_scaled_font(_cairo_gstate * gstate=0x6831ace8, _cairo_scaled_font * * scaled_font=0x001723bc)  Line 1685 + 0x9 bytes	C
 	xul.dll!_moz_cairo_get_scaled_font(_cairo * cr=0x6831acc8)  Line 3260 + 0x10 bytes	C
 	xul.dll!DCFromContext::DCFromContext(gfxContext * aContext=0x044a2cf8)  Line 79 + 0xe bytes	C++
 	xul.dll!gfxUniscribeShaper::InitTextRun(gfxContext * aContext=0x044a2cf8, gfxTextRun * aTextRun=0x0797c578, const wchar_t * aString=0x0017306c, unsigned int aRunStart=0, unsigned int aRunLength=1, int aRunScript=0)  Line 471	C++
 	xul.dll!gfxGDIFont::InitTextRun(gfxContext * aContext=0x044a2cf8, gfxTextRun * aTextRun=0x0797c578, const wchar_t * aString=0x0017306c, unsigned int aRunStart=0, unsigned int aRunLength=1, int aRunScript=0, int aPreferPlatformShaping=0)  Line 185 + 0x3f bytes	C++
 	xul.dll!gfxFont::SplitAndInitTextRun(gfxContext * aContext=0x044a2cf8, gfxTextRun * aTextRun=0x0797c578, const wchar_t * aString=0x0017306c, unsigned int aRunStart=0, unsigned int aRunLength=1, int aRunScript=0)  Line 1584 + 0x2d bytes	C++
 	xul.dll!gfxFontGroup::InitScriptRun(gfxContext * aContext=0x044a2cf8, gfxTextRun * aTextRun=0x0797c578, const wchar_t * aString=0x0017306c, unsigned int aTotalLength=1, unsigned int aScriptRunStart=0, unsigned int aScriptRunEnd=1, int aRunScript=0)  Line 2606 + 0x20 bytes	C++
 	xul.dll!gfxFontGroup::InitTextRun(gfxContext * aContext=0x044a2cf8, gfxTextRun * aTextRun=0x0797c578, const wchar_t * aString=0x0017306c, unsigned int aLength=1)  Line 2556	C++
 	xul.dll!gfxFontGroup::MakeTextRun(const unsigned char * aString=0x680bd385, unsigned int aLength=1, const gfxTextRunFactory::Parameters * aParams=0x00173130, unsigned int aFlags=35)  Line 2491	C++
 	xul.dll!gfxTextRun::SetSpaceGlyph(gfxFont * aFont=0x07a797f0, gfxContext * aContext=0x044a2cf8, unsigned int aCharIndex=0)  Line 4347 + 0x22 bytes	C++
 	xul.dll!TextRunWordCache::MakeTextRun(const unsigned char * aText=0x00174c00, unsigned int aLength=16, gfxFontGroup * aFontGroup=0x07c714d0, const gfxTextRunFactory::Parameters * aParams=0x00174b8c, unsigned int aFlags=22282528)  Line 824	C++
 	xul.dll!gfxTextRunWordCache::MakeTextRun(const unsigned char * aText=0x00174c00, unsigned int aLength=16, gfxFontGroup * aFontGroup=0x07c714d0, const gfxTextRunFactory::Parameters * aParams=0x00174b8c, unsigned int aFlags=22282528)  Line 1047	C++
 	xul.dll!MakeTextRun(const unsigned char * aText=0x00174c00, unsigned int aLength=16, gfxFontGroup * aFontGroup=0x07c714d0, const gfxTextRunFactory::Parameters * aParams=0x00174b8c, unsigned int aFlags=22282528)  Line 584 + 0x19 bytes	C++
 	xul.dll!BuildTextRunsScanner::BuildTextRunForFrames(void * aTextBuffer=0x00174c10)  Line 1984 + 0x1f bytes	C++
 	xul.dll!BuildTextRunsScanner::FlushFrames(int aFlushLineBreaks=1, int aSuppressTrailingBreak=0)  Line 1399 + 0x17 bytes	C++
 	xul.dll!BuildTextRuns(gfxContext * aContext=0x044a2cf8, nsTextFrame * aForFrame=0x070100b0, nsIFrame * aLineContainer=0x0700ffe8, const nsLineList_iterator * aForFrameLine=0x001766d4)  Line 1331	C++

Reproducible in debug Beta/9 builds on Windows 7 but not on Aurora/10, or Nightly/11

Reproducible in Firefox 8 bp-65ec9a6e-3ef7-4ef5-91a2-d505e2111110

See also bug 648245 but I could not reproduce with that url http://www.tamasoft.co.jp/ja/general-info/unicode.html

Comment 7

[Bob Clary [:bc] (inactive)]

ditto Beta/9 Windows http://www.sihk.de/share/flip/November2011/files/assets/seo/page39.html

Comment 8

[Johnny Stenback (:jst)]

John, this crash is still happening. Any updates on a fix here?

Comment 9

[Daniel Veditz [:dveditz]]

See also Bug 648245 which may give more clues how to find a reproduceable testcase (Japanese Epson printer font?)

Comment 10

[John Daggett (:jtd)]

Using the URL in comment 7 I can reproduce this with Firefox 8
Build identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0) Gecko/20100101 Firefox/8.0

Steps to reproduce:
1. about:config -> set 'gfx.direct2d.disabled' to true (this simulates running on a machine with a blacklisted driver)
2. Click on http://www.sihk.de/share/flip/November2011/files/assets/seo/page39.html

Result: crash occurs

Also occurs with:
Build identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0) Gecko/20100101 Firefox/9.0

Does not occur with:
Build identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a2) Gecko/20111201 Firefox/10.0a2
Build identifier: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:11.0a1) Gecko/20111201 Firefox/11.0a1

Comment 11

[John Daggett (:jtd)]

Analyizing crashes from the past week, confirmed the behavior Jonathan noted in comment 3:

Total dumps: 1167

gfxGDIFont::InitTextRun 1166
  HBGetGlyphAdvance 791
  GetRoundOffsetsToPixels 337
    _cairo_dwrite_font_face_scaled_font_create 1167

Next step is to isolate when this got "fixed" and try and figure out the details so we can land the fix on beta/release.

Comment 12

[John Daggett (:jtd)]

Using mozilla-central nightlies:

Bad:  Gecko/20111028
Good: Gecko/20111029

Comment 13

[Scoobidiver (away)]

There are still crashes in 10.0a2:
https://crash-stats.mozilla.com/report/list?version=Firefox%3A10.0a2&range_value=4&range_unit=weeks&signature=_cairo_dwrite_font_face_scaled_font_create

(In reply to John Daggett (:jtd) from comment #12)
> Bad:  Gecko/20111028
> Good: Gecko/20111029
The working range for this STR is :
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=322354df233d&tochange=dfbe9a0fbf97

Comment 14

[John Daggett (:jtd)]

Changes in gfx during this time:

gfxDrawable.cpp: changeset:   79450:e481b6ffc60b
gfxDrawable.cpp: user:        Adrian Johnson <[e-mail]>
gfxDrawable.cpp: date:        Sat Oct 29 19:36:19 2011 +1030
gfxDrawable.cpp: summary:     Bug 691061 - Don't use EXTEND_PAD on printing surfaces. r=jmuizelaar
gfxDrawable.cpp: 
gfxGDIFontList.cpp: changeset:   79383:21f2d6c4c976
gfxGDIFontList.cpp: user:        Jonathan Watt <[e-mail]>
gfxGDIFontList.cpp: date:        Fri Oct 28 19:33:28 2011 +0100
gfxGDIFontList.cpp: summary:     Bug 695303 - Add a mozilla::clamped function to replace NS_CLAMP (so side affects of args are evaluated no more than once) and NS_MIN(max, NS_MAX(val, min)) (to make code clearer). r=bsmedberg.
gfxGDIFontList.cpp: 
gfxGDIFontList.cpp: changeset:   79367:96278b6d8c92
gfxGDIFontList.cpp: user:        Adrian Johnson <[e-mail]>
gfxGDIFontList.cpp: date:        Fri Oct 28 09:58:49 2011 -0400
gfxGDIFontList.cpp: summary:     Bug 454532. Substitute "Courier" with "Courier New". r=jdagget
gfxGDIFontList.cpp: 
gfxQuaternion.h: changeset:   79383:21f2d6c4c976
gfxQuaternion.h: user:        Jonathan Watt <[e-mail]>
gfxQuaternion.h: date:        Fri Oct 28 19:33:28 2011 +0100
gfxQuaternion.h: summary:     Bug 695303 - Add a mozilla::clamped function to replace NS_CLAMP (so side affects of args are evaluated no more than once) and NS_MIN(max, NS_MAX(val, min)) (to make code clearer). r=bsmedberg.
gfxQuaternion.h: 

I suspect this has something to do with the substitution of Courier with Courier New, I confirmed that the page does include a "Courier,sans-serif" font list.  Next step is to confirm this with a local build.

Comment 15

[Jonathan Kew [:jfkthame]]

Sounds like this is an issue with old .FON-format bitmap fonts, then. You can probably reproduce it on current trunk by specifying a different bitmap font, as we're now force-substituting Courier.

Comment 16

[John Daggett (:jtd)]

(In reply to Jonathan Kew (:jfkthame) from comment #15)
> Sounds like this is an issue with old .FON-format bitmap fonts, then. You
> can probably reproduce it on current trunk by specifying a different bitmap
> font, as we're now force-substituting Courier.

It's the end of my work week, I may have time tomorrow evening to look at this again.  My next step was to stub out the substitution code on trunk and try to reproduce it there.  Feel free to give it a whirl if you have a Windows 7 machine sitting idle.

Comment 17

[Jonathan Kew [:jfkthame]]

Attachment: patch, mark GDI font as invalid on cairo backend failure

The crash occurs when the cairo scaled_font associated with the gfxGDIFont we're trying to use is in an error state. This leads to cairo creating a "toy" font internally using its DW backend, resulting in a mismatch with our code which assumes it's using a GDI font.

It seems that with bitmap .fon fonts such as Courier (prior to the forced Courier New substitution), the call to cairo_scaled_font_create in gfxGDIFont::Initialize() may fail (although I haven't yet traced it to figure out exactly why it's failing). In this case, we print a warning (in debug builds), but still proceed to try and use the font - which is a Bad Thing. We should be marking it as invalid.

There may be some deeper issue to investigate in cairo, as it's not clear to me why cairo_scaled_font_create is failing in this case, but in any case we need to handle the possibility of failure more robustly.

(Tested on a trunk build with the Courier->Courier New substitution backed out, and confirmed that this fixes the crash we were seeing on the page mentioned here.)

Comment 18

[John Daggett (:jtd)]

Bug appears to be caused by very small font sizes, the failure occurs when the size on the gfxFont is 0.083px.

Simplified testcase:

data:text/html,<body><div%20style="font-family:%20Courier;%20font-size:%201px;"><span%20style="font-size:%208.3%;">very%20small</span></div></body>

Comment 19

[John Daggett (:jtd)]

Comment on attachment 578939 [details] [diff] [review]
patch, mark GDI font as invalid on cairo backend failure

Causes a leak:

WARNING: Textrun cache not empty!: 'mCache.Count() == 0', file c:/builds/mozcentral/gfx/thebes/gfxTextRunWordCache.cpp, line 88
WARNING: Fonts still alive while shutting down gfxFontCache: 'mFonts.Count() == 0', file c:\builds\mozcentral\obj-debug\dist\include\gfxFont.h, line 658
Assertion failed at c:/builds/mozcentral/gfx/cairo/cairo/src/cairo-hash.c:196: hash_table->live_entries == 0

Unfortunately, I think we probably need to dig deeper into the cairo code to error out sooner.

Comment 20

[John Daggett (:jtd)]

Attachment: testcase, test a set of bitmap fonts

Refined testcase, uses crappy bitmap fonts other than Courier such that this crashes on trunk (yay!).

Comment 21

[John Daggett (:jtd)]

Attachment: patch, enforce minimum font size of 1.0

With crashtest.

Comment 22

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) from comment #19)
> Comment on attachment 578939 [details] [diff] [review] [diff] [details] [review]
> patch, mark GDI font as invalid on cairo backend failure
> 
> Causes a leak:
> 
> WARNING: Textrun cache not empty!: 'mCache.Count() == 0', file
> c:/builds/mozcentral/gfx/thebes/gfxTextRunWordCache.cpp, line 88
> WARNING: Fonts still alive while shutting down gfxFontCache: 'mFonts.Count()
> == 0', file c:\builds\mozcentral\obj-debug\dist\include\gfxFont.h, line 658
> Assertion failed at
> c:/builds/mozcentral/gfx/cairo/cairo/src/cairo-hash.c:196:
> hash_table->live_entries == 0

I'm not seeing this happening on my local build.

I've run across these warnings occasionally (on various platforms) in the past. I don't see why this change would trigger them, though. Marking a gfxFont as invalid just causes it to return FALSE from HasCharacter() for all character codes, so that we will always skip it and fall back to something else, but doesn't affect lifetime of the object.

Comment 23

[John Daggett (:jtd)]

(In reply to Jonathan Kew (:jfkthame) from comment #22)
> I'm not seeing this happening on my local build.
> 
> I've run across these warnings occasionally (on various platforms) in the
> past. I don't see why this change would trigger them, though. Marking a
> gfxFont as invalid just causes it to return FALSE from HasCharacter() for
> all character codes, so that we will always skip it and fall back to
> something else, but doesn't affect lifetime of the object.

Interesting.  Stepping through the cairo_scaled_font_create code, it looks like the code initializes a data structure and puts it into a hash table and that seems to be the same hash table in the warning message.

The revised patch doesn't hit this code because it doesn't call cairo code to create a size=0 font.

Comment 24

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) from comment #21)
> Created attachment 579001 [details] [diff] [review] [diff] [details] [review]
> patch, enforce minimum font size of 1.0

This will regress bug 670072, which is currently fixed on Win7/GDI, though it is still a problem on XP (and hence is not marked as resolved).

Comment 25

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) from comment #23)
> (In reply to Jonathan Kew (:jfkthame) from comment #22)
> > I'm not seeing this happening on my local build.
> > 
> > I've run across these warnings occasionally (on various platforms) in the
> > past. I don't see why this change would trigger them, though. Marking a
> > gfxFont as invalid just causes it to return FALSE from HasCharacter() for
> > all character codes, so that we will always skip it and fall back to
> > something else, but doesn't affect lifetime of the object.
> 
> Interesting.  Stepping through the cairo_scaled_font_create code, it looks
> like the code initializes a data structure and puts it into a hash table and
> that seems to be the same hash table in the warning message.
> 
> The revised patch doesn't hit this code because it doesn't call cairo code
> to create a size=0 font.

The warning can still happen, though, as it's not specific to that size-zero font. I just encountered it on a build that has your patch (and not mine). It seems to like to show up (together with a bunch of messages about leaked URLs), particularly if I shut down the browser immediately after starting up and loading a bugzilla page.

However, this happens equally readily with or without the patches here; it's an unrelated issue. Maybe in certain circumstances we don't reliably shut things down in quite the right sequence. I don't have consistent STR, though; I think it's timing-dependent or something like that.

Comment 26

[Jonathan Kew [:jfkthame]]

Comment on attachment 578939 [details] [diff] [review]
patch, mark GDI font as invalid on cairo backend failure

Re-requesting review on this, as (a) I don't believe it's responsible for the warning/possible leak that sometimes occurs, as noted above; and (b) even if we do something else to avoid problems with small font sizes, we should still handle the possibility of failure here. Currently, we detect the cairo failure, print a warning (in debug builds), and then mark the font as "valid" and continue; that's just wrong.

Comment 27

[John Daggett (:jtd)]

(In reply to Jonathan Kew (:jfkthame) from comment #24)
> (In reply to John Daggett (:jtd) from comment #21)
> > Created attachment 579001 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > patch, enforce minimum font size of 1.0
> 
> This will regress bug 670072, which is currently fixed on Win7/GDI, though
> it is still a problem on XP (and hence is not marked as resolved).

So you that ROUND is there expressly to make size < 1.0 ==> size = 0?

If that's the case, I think we should back out your fix for 670072 and come up with something that doesn't crash or leak.

Comment 28

[Jonathan Kew [:jfkthame]]

Attachment 578939 [details] [diff] doesn't crash or leak, AFAICS. It would be correct to make gfx respect the status returned by cairo there, even if we weren't seeing a specific problem that results from ignoring it. Is there any reason _not_ to land it?

Comment 29

[John Daggett (:jtd)]

I looked into the root cause of the cairo font error today.  With existing code, when font size is zero, the call to cairo_scaled_font_create within gfxGDIFont::Initialize fails when the font is a bitmap font but is fine when the font is an OpenType font.

I traced through the code and the problem is that due to the way the font matrix is set up in the size == 0 case, a slightly different code path is taken within the Win32 cairo code for this case.  The sequence of steps is this:

1. _win32_scaled_font_create calls _compute_transform (f, &scale)

2. _compute_transform sets preserve_axes based on the contents of
   the font matrix.  Normal case is true, in the size == 0 case
   it's set to false.

3. _win32_scaled_font_create calls _cairo_win32_scaled_font_set_metrics

4. Within _cairo_win32_scaled_font_set_metrics the pseudo-logic below is used:

      if (preserve_axes)
        call GetTextMetrics ==> fine for all fonts
      else
        call _cairo_win32_scaled_font_select_unscaled_font
          call _win32_scaled_font_get_unscaled_hfont
            call GetOutlineTextMetrics ==> fails for non-ttf fonts

5. The error ripples out

I'm not quite sure what the intent of preserve_axes is and why the code path is different for the two cases but the right fix (as opposed to the band-aid) would be to fix up the cairo code so that it doesn't error out in the size == 0 case.

We actually never should have landed the fix for bug 670072 that caused this in the first place because it's only "fixing" the problem for the GDI case.  If size < 0.5px is going to imply effectively size == 0 with no effect on layout, then we need to make this work consistently on all platforms.  That's actually trickier than it sounds because we have all sorts of assumptions that the size is *not* zero and both the font-size-adjust and OSX codepaths clamp the lower bound at 1px.  Plus we have assertions in out code that mFUnitsConvFactor is not zero, which it will be in the size == 0.0 case.

In short, the right fix is going to take a lot more work than ROUND(size) in the original patch and certainly a lot more testing across platforms and font types.

Comment 30

[John Daggett (:jtd)]

Another problem with the current code is that although the size is rounded down to zero, non-zero metrics are still returned.  Windows appears to have a lower limit on font size, roughly 2px for OpenType fonts and 6px for non-OpenType fonts.  Any size below that is ignored.  For the 'font-size: 0px' case the existing code whacks the metrics within SanitizeMetrics so I think the way to correctly get the metrics zero'd out is to change the condition in SanitizeMetrics to look at the adjusted size, not the size in the style.  Without this, gfxFont::Draw is called when mAdjustedSize is zero but with it it's never called in that case.  I have a feeling this will fix the XP case but I haven't tested that yet.

Comment 31

[John Daggett (:jtd)]

Attachment: alternate patch, invalidate the font and zero the metrics

Comment 32

[John Daggett (:jtd)]

Attachment: reftests, will only pass in the GDI case

Tests to confirm that font sizes < 0.5px don't affect layout

Comment 33

[John Daggett (:jtd)]

Attachment: crashtest extracted for the previous patch

Comment 34

[John Daggett (:jtd)]

Confirmed that the alternate patch fixes the drawing problem on XP.

Comment 35

[Jonathan Kew [:jfkthame]]

Comment on attachment 579253 [details] [diff] [review]
alternate patch, invalidate the font and zero the metrics

Review of attachment 579253 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxFont.cpp
@@ +1831,5 @@
>  gfxFont::SanitizeMetrics(gfxFont::Metrics *aMetrics, bool aIsBadUnderlineFont)
>  {
>      // Even if this font size is zero, this font is created with non-zero size.
>      // However, for layout and others, we should return the metrics of zero size font.
> +    if (mAdjustedSize == 0.0) {

I think this may break things on Linux and Android (and probably OS/2), as the FT2 backend doesn't appear to set mAdjustedSize at all AFAICS.

Comment 36

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) from comment #29)
> We actually never should have landed the fix for bug 670072 that caused this
> in the first place because it's only "fixing" the problem for the GDI case. 
> If size < 0.5px is going to imply effectively size == 0 with no effect on
> layout, then we need to make this work consistently on all platforms. 

I don't think that's necessarily true, nor is it critical to fixing _this_ bug. I don't see any reason why size < 0.5px should necessarily map to size = 0 on all platforms. It does under GDI because GDI doesn't support tiny fractional font sizes, but that's no reason other platforms have to be bound by the same constraint. If an author tries to render some text with a 0.1px font, that's what they should get - within the limitations of the platform capabilities, which in some cases may mean rounding the size to 0px.

I'd argue that we should fix this bug right now, by landing the patch that marks a font as invalid if cairo_create_scaled_font fails (for any reason - there might potentially be other failure modes besides zero-sized fonts). If we want to consider other changes to how tiny font sizes are handled across various platforms, that's a distinct topic and should be a separate bug.

Comment 37

[John Daggett (:jtd)]

(In reply to Jonathan Kew (:jfkthame) from comment #36)
> I don't think that's necessarily true, nor is it critical to fixing _this_
> bug. I don't see any reason why size < 0.5px should necessarily map to size
> = 0 on all platforms. It does under GDI because GDI doesn't support tiny
> fractional font sizes, but that's no reason other platforms have to be bound
> by the same constraint. If an author tries to render some text with a 0.1px
> font, that's what they should get - within the limitations of the platform
> capabilities, which in some cases may mean rounding the size to 0px.

It should have never landed because it makes rendering inconsistent across platforms/os.  Handling fractional sizes is a *separate* issue from how subpixel sizes are dealt with.  If we decide that text < 0.5px means invisible text (modulo min font size), then that behavior should be consistent.

I agree that we should fix the crash first but a simple way of doing that is backing out the original patch which causes it and which turned out not to be a complete fix anyways (i.e. only in the GDI/Win7 case).

Comment 38

[Alex Keybl [:akeybl]]

[Triage Comment]
We either need to get an r+'d patch or a backout of 670072 in today before we go to build. Please nominate for aurora/beta and email me at [e-mail] as soon as we're ready. Thanks!

Comment 39

[John Daggett (:jtd)]

Attachment: alternate patch, invalidate the font and zero the metrics (GDI only)

This includes Jonathan's patch and adds the logic set up null metrics so that nothing is drawn in the XP case.  This will only affect the GDI font case (i.e. XP or Win7/Vista with no hw acceleration).

I'll log a separate bug for fixing the inconsistency across platform/os of < 0.5px fonts.

Comment 40

[John Daggett (:jtd)]

Attachment: crashtest, test with list of bitmap fonts at small sizes

crashtest for this

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to John Daggett (:jtd) from comment #37)
> It should have never landed because it makes rendering inconsistent across
> platforms/os.  Handling fractional sizes is a *separate* issue from how
> subpixel sizes are dealt with.  If we decide that text < 0.5px means
> invisible text (modulo min font size), then that behavior should be
> consistent.

Rendering is never perfectly consistent across platforms. The fact that GDI explodes when we try to pass too small a font size, so we need to make text disappear, doesn't mean we need to replicate the fix on all platforms.

Having said that, I think it would probably be fine to make all platforms draw nothing for font sizes < 1px. The only thing I'd worry about is if someone draws, say, 0.5px text with a large scale-up transform. But wouldn't hinting and spacing be quite broken anyway?

Comment 42

[John Daggett (:jtd)]

Landed on trunk:

https://hg.mozilla.org/mozilla-central/rev/365d0a50014a
https://hg.mozilla.org/mozilla-central/rev/86e70adaf190

Comment 43

[Alex Keybl [:akeybl]]

Comment on attachment 579553 [details] [diff] [review]
alternate patch, invalidate the font and zero the metrics (GDI only)

[Triage Comment]
This ended up missing our go-to-build last night, so let's first take this on aurora and land on beta early next week after further testing on m-c/aurora.

Comment 44

[John Daggett (:jtd)]

For QA use:

Steps to reproduce

1. Set up a profile on a Windows 7 machine
2. about:config, set gfx.direct2d.disabled to true
3. Restart
4. Open URL

FF8/9/10 ==> crash, trunk code ==> tra la!

Comment 45

[Alex Keybl [:akeybl]]

Comment on attachment 579553 [details] [diff] [review]
alternate patch, invalidate the font and zero the metrics (GDI only)

[Triage Comment]
Approving for Beta as well because this is a high value backout. Can we get a try build of beta+693143 to pass off to QA ahead of our beta 6 go to build?

Comment 46

[Alex Keybl [:akeybl]]

Adding qawanted for the try build.

Comment 47

[John Daggett (:jtd)]

Pushed to aurora
https://hg.mozilla.org/releases/mozilla-aurora/rev/2453d5384fea
https://hg.mozilla.org/releases/mozilla-aurora/rev/8532e30e372a

Pushed beta patches to try
http://hg.mozilla.org/try/pushloghtml?changeset=99078012adf2
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-99078012adf2

Comment 48

[John Daggett (:jtd)]

Pushed to beta
https://hg.mozilla.org/releases/mozilla-beta/rev/f4e11a42ed9a
https://hg.mozilla.org/releases/mozilla-beta/rev/eb256dc10994

Comment 49

[u279076]

Using the tryserver build on Windows 7 and the following steps...

1. Started Try server build with a new profile.
2. Built an amount of history items, bookmarked pages, grouped a few tabs in Panorama, installed Download Helper add-on. The profile had most popular plugins installed: Real Player, Quick Time, Shockwave, Java.
3. Set gfx.direct2d.disabled to true in about:config.
4. Restarted Firefox.
5. Opened http://people.mozilla.org/~jdaggett/tests/bitmapfonts-waterfall.html

...we only see blue text; no red text and no crash. The try build seems good enough, though I will let John and Alex make that call.

Comment 50

[John Daggett (:jtd)]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #49)
> ...we only see blue text; no red text and no crash. The try build seems good
> enough, though I will let John and Alex make that call.

The key is you didn't crash! You should see blue text but no red text in both the default profile case (DirectWrite) and the no-D2D case (GDI).

Comment 51

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed with Tinderbox builds and the both test cases (crash + blue letters) on Windows 7:

Mozilla/5.0 (Windows NT 6.1; rv:11.0a1) Gecko/20111212 Firefox/11.0a1
Mozilla/5.0 (Windows NT 6.1; rv:10.0a2) Gecko/20111208 Firefox/10.0a2
Mozilla/5.0 (Windows NT 6.1; rv:9.0) Gecko/20111208 Firefox/9.0