Closed Bug 693143 Opened 13 years ago Closed 13 years ago

Crash in _cairo_dwrite_font_face_scaled_font_create

Categories

(Core :: Graphics, defect)

Product:
Core
Component:
Graphics
Version:
8 Branch
Platform:
x86
Windows 7
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla11
Tracking Flags:
Tracking Status
firefox8 - wontfix
firefox9 + verified
firefox10 + verified
firefox11 + verified
status1.9.2 --- unaffected

People

(Reporter: scoobidiver, Assigned: jfkthame)

References

(Blocks 1 open bug,
URL
)

Details

(Keywords: crash, regression, reproducible, Whiteboard: [sg:critical][qa!])

Crash Data

Attachments

(5 files, 3 obsolete files)

patch, mark GDI font as invalid on cairo backend failure
934 bytes, patch
Details | Diff | Splinter Review
testcase, test a set of bitmap fonts
603 bytes, text/html
Details
reftests, will only pass in the GDI case
4.29 KB, patch
Details | Diff | Splinter Review
alternate patch, invalidate the font and zero the metrics (GDI only)
13.76 KB, patch
roc
: review+
akeybl
: approval-mozilla-aurora+
akeybl
: approval-mozilla-beta+
Details | Diff | Splinter Review
crashtest, test with list of bitmap fonts at small sizes
1.34 KB, patch
roc
: review+
akeybl
: approval-mozilla-aurora+
akeybl
: approval-mozilla-beta+
Details | Diff | Splinter Review
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.
In every crash reports I checked, D3D9 and D3D10 are disabled.
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
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
Group: core-security
Whiteboard: [sg:critical]
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: → 648245
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
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.
Using mozilla-central nightlies: Bad: Gecko/20111028 Good: Gecko/20111029
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.
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.
(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.
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)
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 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-
Refined testcase, uses crappy bitmap fonts other than Courier such that this crashes on trunk (yay!).
With crashtest.
Attachment #579001 - Flags: review?(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.
(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.
(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).
(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 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)
Attachment #579001 - Flags: review?(jfkthame) → review-
(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.
Depends on: 670072
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?
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.
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.
Attachment #579001 - Attachment is obsolete: true
Tests to confirm that font sizes < 0.5px don't affect layout
Attachment #579257 - Flags: review?(jfkthame)
Attachment #579254 - Flags: review?(jfkthame)
Assignee: jdaggett → jfkthame
Confirmed that the alternate patch fixes the drawing problem on XP.
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.
(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.
(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!
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)
crashtest for this
Attachment #579257 - Attachment is obsolete: true
Attachment #579257 - Flags: review?(jfkthame)
Attachment #579555 - Flags: review?(roc)
Attachment #579254 - Flags: review?(jfkthame)
Attachment #578939 - Flags: review?(jdaggett)
(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?
Blocks: 708162
Landed on trunk: https://hg.mozilla.org/mozilla-central/rev/365d0a50014a https://hg.mozilla.org/mozilla-central/rev/86e70adaf190
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Attachment #579553 - Flags: approval-mozilla-beta?
Attachment #579553 - Flags: approval-mozilla-aurora?
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+
Attachment #579555 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [sg:critical] → [sg:critical][qa+]
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!
URL: http://people.mozilla.org/~jdaggett/t...
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+
Attachment #579555 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Adding qawanted for the try build.
Keywords: qawanted
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.
(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.

Attachment

General

Creator:
Created:
Updated:
Size: