Crash in _cairo_dwrite_font_face_scaled_font_create

VERIFIED FIXED in Firefox 9

Status

()

Core
Graphics
--
critical
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: Scoobidiver (away), Assigned: jfkthame)

Tracking

(Blocks: 2 bugs, {crash, regression, reproducible})

8 Branch
mozilla11
x86
Windows 7
crash, regression, reproducible
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(firefox8- wontfix, firefox9+ verified, firefox10+ verified, firefox11+ verified, status1.9.2 unaffected)

Details

(Whiteboard: [sg:critical][qa!], crash signature, URL)

Attachments

(5 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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
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.
(Reporter)

Comment 2

6 years ago
In every crash reports I checked, D3D9 and D3D10 are disabled.
(Assignee)

Comment 3

6 years ago
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.
John, can you take a look at this & pass it off if you're not comfortable?
Er, whoops, midair.

John, can you take a look at this & pass it off if you're not comfortable?
Assignee: nobody → jdaggett

Comment 6

6 years ago
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
Blocks: 532972
Group: core-security
Whiteboard: [sg:critical]

Updated

6 years ago
status-firefox10: --- → affected
status-firefox11: --- → affected
status-firefox8: --- → wontfix
status-firefox9: --- → affected
tracking-firefox10: --- → +
tracking-firefox11: --- → +
tracking-firefox8: --- → -
tracking-firefox9: --- → +

Comment 7

6 years ago
ditto Beta/9 Windows http://www.sihk.de/share/flip/November2011/files/assets/seo/page39.html
John, this crash is still happening. Any updates on a fix here?
See also Bug 648245 which may give more clues how to find a reproduceable testcase (Japanese Epson printer font?)
See Also: → bug 648245

Comment 10

6 years ago
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

6 years ago
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

6 years ago
Using mozilla-central nightlies:

Bad:  Gecko/20111028
Good: Gecko/20111029
(Reporter)

Comment 13

6 years ago
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
Keywords: reproducible

Comment 14

6 years ago
Changes in gfx during this time:

gfxDrawable.cpp: changeset:   79450:e481b6ffc60b
gfxDrawable.cpp: user:        Adrian Johnson <ajohnson@redneon.com>
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 <jwatt@jwatt.org>
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 <ajohnson@redneon.com>
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 <jwatt@jwatt.org>
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.
(Assignee)

Comment 15

6 years ago
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

6 years ago
(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.

Updated

6 years ago
status1.9.2: --- → unaffected
(Assignee)

Comment 17

6 years ago
Created attachment 578939 [details] [diff] [review]
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.)
Attachment #578939 - Flags: review?(jdaggett)

Comment 18

6 years ago
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

6 years ago
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.
Attachment #578939 - Flags: review?(jdaggett) → review-

Comment 20

6 years ago
Created attachment 579000 [details]
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

6 years ago
Created attachment 579001 [details] [diff] [review]
patch, enforce minimum font size of 1.0

With crashtest.
Attachment #579001 - Flags: review?(jfkthame)
(Assignee)

Comment 22

6 years ago
(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

6 years ago
(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.
(Assignee)

Comment 24

6 years ago
(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).
(Assignee)

Comment 25

6 years ago
(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.
(Assignee)

Comment 26

6 years ago
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.
Attachment #578939 - Flags: review- → review?(jdaggett)
(Assignee)

Updated

6 years ago
Attachment #579001 - Flags: review?(jfkthame) → review-

Comment 27

6 years ago
(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.

Updated

6 years ago
Depends on: 670072
(Assignee)

Comment 28

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
Created attachment 579253 [details] [diff] [review]
alternate patch, invalidate the font and zero the metrics
Attachment #579001 - Attachment is obsolete: true

Comment 32

6 years ago
Created attachment 579254 [details] [diff] [review]
reftests, will only pass in the GDI case

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

Comment 33

6 years ago
Created attachment 579257 [details] [diff] [review]
crashtest extracted for the previous patch
Attachment #579257 - Flags: review?(jfkthame)

Updated

6 years ago
Attachment #579254 - Flags: review?(jfkthame)

Updated

6 years ago
Assignee: jdaggett → jfkthame

Comment 34

6 years ago
Confirmed that the alternate patch fixes the drawing problem on XP.
(Assignee)

Comment 35

6 years ago
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.
(Assignee)

Comment 36

6 years ago
(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

6 years ago
(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).
[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 alex.keybl@gmail.com as soon as we're ready. Thanks!

Comment 39

6 years ago
Created attachment 579553 [details] [diff] [review]
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.
Attachment #579253 - Attachment is obsolete: true
Attachment #579553 - Flags: review?(roc)

Comment 40

6 years ago
Created attachment 579555 [details] [diff] [review]
crashtest, test with list of bitmap fonts at small sizes

crashtest for this
Attachment #579257 - Attachment is obsolete: true
Attachment #579257 - Flags: review?(jfkthame)
Attachment #579555 - Flags: review?(roc)

Updated

6 years ago
Attachment #579254 - Flags: review?(jfkthame)

Updated

6 years ago
Attachment #578939 - Flags: review?(jdaggett)
Attachment #579553 - Flags: review?(roc) → review+
Attachment #579555 - Flags: review?(roc) → review+
(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?

Updated

6 years ago
Blocks: 708162

Comment 42

6 years ago
Landed on trunk:

https://hg.mozilla.org/mozilla-central/rev/365d0a50014a
https://hg.mozilla.org/mozilla-central/rev/86e70adaf190
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Attachment #579553 - Flags: approval-mozilla-beta?
Attachment #579553 - Flags: approval-mozilla-aurora?

Updated

6 years ago
Attachment #579555 - Flags: approval-mozilla-beta?
Attachment #579555 - Flags: approval-mozilla-aurora?
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.
Attachment #579553 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

6 years ago
Attachment #579555 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [sg:critical] → [sg:critical][qa+]

Comment 44

6 years ago
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 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?
Attachment #579553 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

6 years ago
Attachment #579555 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Adding qawanted for the try build.
Keywords: qawanted

Comment 47

6 years ago
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/jdaggett@mozilla.com-99078012adf2

Comment 48

6 years ago
Pushed to beta
https://hg.mozilla.org/releases/mozilla-beta/rev/f4e11a42ed9a
https://hg.mozilla.org/releases/mozilla-beta/rev/eb256dc10994
(Reporter)

Updated

6 years ago
status-firefox10: affected → fixed
status-firefox11: affected → fixed
status-firefox9: affected → fixed
Target Milestone: --- → mozilla11
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

6 years ago
(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).
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
Status: RESOLVED → VERIFIED
Flags: in-testsuite+
Flags: in-litmus-
Keywords: qawanted → verified-aurora, verified-beta
Whiteboard: [sg:critical][qa+] → [sg:critical][qa!]
status-firefox10: fixed → verified
status-firefox11: fixed → verified
status-firefox9: fixed → verified
Keywords: verified-aurora, verified-beta
Whiteboard: [sg:critical][qa!] → [sg:critical][qa-]
Whiteboard: [sg:critical][qa-] → [sg:critical][qa!]
Group: core-security
You need to log in before you can comment on or make changes to this bug.