820707 - crash in gfxGDIFont::Initialize @ gfxFontFamily::HasItalicFace

Status: VERIFIED FIXED

(Core :: Graphics: Text, defect)

Description

[Scoobidiver (away)]

It began spiking in 20.0a1/20121211 and is currently #4 top crasher in this build. The regression range for the spike is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=725eb8792d27&tochange=4dfe323a663d
It might be a regression from bug 816483.

Stack traces are various:
Frame 	Module 	Signature 	Source
0 	xul.dll 	gfxFontFamily::HasItalicFace 	gfx/thebes/gfxFont.h:665
1 	xul.dll 	gfxGDIFont::Initialize 	gfx/thebes/gfxGDIFont.cpp:293
2 	xul.dll 	gfxGDIFont::ShapeWord 	gfx/thebes/gfxGDIFont.cpp:114
3 	xul.dll 	gfxFont::GetShapedWord<unsigned char> 	gfx/thebes/gfxFont.cpp:2402
4 	xul.dll 	gfxFont::SplitAndInitTextRun<unsigned char> 	gfx/thebes/gfxFont.cpp:2594
5 	xul.dll 	gfxFontGroup::InitScriptRun<unsigned char> 	gfx/thebes/gfxFont.cpp:3754
6 	xul.dll 	gfxFontGroup::InitTextRun<unsigned char> 	gfx/thebes/gfxFont.cpp:3651
7 	xul.dll 	gfxFontGroup::MakeTextRun 	gfx/thebes/gfxFont.cpp:3572
8 	xul.dll 	BuildTextRunsScanner::BuildTextRunForFrames 	layout/generic/nsTextFrameThebes.cpp:2052
...

Frame 	Module 	Signature 	Source
0 	xul.dll 	gfxFontFamily::HasItalicFace 	gfx/thebes/gfxFont.h:665
1 	xul.dll 	gfxGDIFont::Initialize 	gfx/thebes/gfxGDIFont.cpp:293
2 	xul.dll 	nsLayoutUtils::GetFontMetricsForFrame 	layout/base/nsLayoutUtils.cpp:2163
3 	xul.dll 	nsFontMetrics::MaxAscent 	gfx/src/nsFontMetrics.cpp:222
4 	xul.dll 	nsInlineFrame::ReflowFrames 	layout/generic/nsInlineFrame.cpp:646
5 	xul.dll 	nsInlineFrame::Reflow 	layout/generic/nsInlineFrame.cpp:395
6 	xul.dll 	nsLineLayout::ReflowFrame 	layout/generic/nsLineLayout.cpp:782
7 	xul.dll 	nsLineLayout::BeginLineReflow 	layout/generic/nsLineLayout.cpp:215
8 	xul.dll 	nsBlockFrame::ReflowInlineFrames 	layout/generic/nsBlockFrame.cpp:3377
...

Frame 	Module 	Signature 	Source
0 	xul.dll 	gfxFontFamily::HasItalicFace 	gfx/thebes/gfxFont.h:665
1 	xul.dll 	gfxGDIFont::Initialize 	gfx/thebes/gfxGDIFont.cpp:293
2 	xul.dll 	nsStyleContext::DoGetStyleFont 	layout/style/nsStyleStructList.h:47
3 	xul.dll 	ComputeLineHeight 	layout/generic/nsHTMLReflowState.cpp:2390
4 	xul.dll 	xul.dll@0x10f272f 	
5 	xul.dll 	nsBlockReflowState::nsBlockReflowState 	layout/generic/nsBlockReflowState.cpp:113
6 	xul.dll 	nsBlockFrame::Reflow 	layout/generic/nsBlockFrame.cpp:994
...

Frame 	Module 	Signature 	Source
0 	xul.dll 	gfxFontFamily::HasItalicFace 	gfx/thebes/gfxFont.h:665
1 	xul.dll 	gfxGDIFont::Initialize 	gfx/thebes/gfxGDIFont.cpp:293
2 	mozglue.dll 	arena_bin_nonfull_run_get 	memory/mozjemalloc/jemalloc.c:3882
3 	xul.dll 	gfxFont::GetShapedWord<unsigned char> 	gfx/thebes/gfxFont.cpp:2402
4 	xul.dll 	gfxFont::SplitAndInitTextRun<unsigned char> 	gfx/thebes/gfxFont.cpp:2594
5 	gkmedias.dll 	SkDataSet::CreateProc 	gfx/skia/include/core/SkDataSet.h:74
...

Frame 	Module 	Signature 	Source
0 	xul.dll 	gfxFontFamily::HasItalicFace 	gfx/thebes/gfxFont.h:665
1 	xul.dll 	gfxGDIFont::Initialize 	gfx/thebes/gfxGDIFont.cpp:293
2 	xul.dll 	base::Histogram::Accumulate 	ipc/chromium/src/base/histogram.cc:562
3 	xul.dll 	gfxFont::GetShapedWord<unsigned char> 	gfx/thebes/gfxFont.cpp:2402
4 	xul.dll 	BuildTextRunsScanner::BuildTextRunForFrames 	layout/generic/nsTextFrameThebes.cpp:2092
5 	mozjs.dll 	MakeJITScript 	js/src/methodjit/Compiler.cpp:959
...

More reports at:
https://crash-stats.mozilla.com/report/list?signature=gfxFontFamily%3A%3AHasItalicFace%28%29

Comment 1

[Jonathan Kew [:jfkthame]]

Hmm... it's clearly been happening with earlier builds as well; I see crash reports across a range of Firefox versions. But a spike in 20.0a1/20121211 strongly suggests that bug 816483 has made it more common.

The crashes/stacks suggest we're encountering a font entry with NULL family pointer during GDIFont::Initialize, which means we crash when trying to find out whether the family has any italic faces. I can imagine how bug 816483 has made this more common, because the same font entry might now be used by multiple user font families. If the last-created of those families is deleted, it will clear the entry's family pointer - but other, earlier families might still be alive and try to use it. So basically, we can't rely on the family pointer in user font entries being valid/meaningful any more.

We can avoid the crash here by consistently setting the mFamilyHasItalicFace as needed in all GDI font entries (not only for user fonts), so that Initialize() won't need to refer back to the family. (And it makes sense, actually, that instantiating a gfxFont from its associated font entry should be a "standalone" process that doesn't require knowledge of the family, if any, to which that entry belonged.)

This also has the benefit of eliminating the gfxFontFamily::HasItalicFace() method, which only existed to support GDI's needs, from the general gfxFontFamily class and confining the special italic-management logic to the GDI-specific files, where it more properly belongs.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: bug--has-italic try: -b do -p win32 -u all

Comment 3

[Jonathan Kew [:jfkthame]]

Marking this as blocking 816483 because of the spike in crashiness, even though the signature existed at a low level prior to that. I believe the patch here should eliminate it altogether.

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: consistently set familyHasItalic flag on GDI font entries, to avoid referring back to the family during font instantiation.

And now with appropriate typecasts to make it actually compile. :) Carrying forward r=roc.

Comment 5

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f6f0585d019

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0f6f0585d019

Comment 7

[Ioana (away)]

No post-fix crashes in Socorro.