524462 - startup crash [@ gfxWindowsFontGroup::WhichFontSupportsChar(nsTArray<nsRefPtr<FontEntry> > const&, unsigned int)]

Status: VERIFIED FIXED

(Core :: Graphics, defect, P1)

Description

[Wayne Mery (:wsmwk)]

regression?

crash [@ gfxWindowsFontGroup::WhichFontSupportsChar(nsTArray<nsRefPtr<FontEntry> > const&, unsigned int)] appears as top of stack for both FF and TB.  No FF crashes prior to 3.1 aka 3.5, so perhaps a regression?  

For thunderbird, about 1/3-1/2 look to be crashes during startup.
99% windows, but here are two Mac crashes:
 TB bp-324d1f10-ed11-4e33-9830-94dc92090717
 SM bp-c17887a3-fb93-44fc-bff3-81adf2091002

bp-02466c92-dc16-42a0-aa23-46e352091025
0	thunderbird.exe	gfxWindowsFontGroup::WhichFontSupportsChar	 gfx/thebes/src/gfxWindowsFonts.cpp:2383
1	thunderbird.exe	gfxWindowsFontGroup::WhichPrefFontSupportsChar	gfx/thebes/src/gfxWindowsFonts.cpp:2513
2	thunderbird.exe	gfxFontGroup::FindFontForChar	gfx/thebes/src/gfxFont.cpp:1129
3	thunderbird.exe	gfxFontGroup::ComputeRanges	gfx/thebes/src/gfxFont.cpp:1182
4	thunderbird.exe	gfxWindowsFontGroup::InitTextRunUniscribe	gfx/thebes/src/gfxWindowsFonts.cpp:2607
5	thunderbird.exe	gfxWindowsFontGroup::MakeTextRun	gfx/thebes/src/gfxWindowsFonts.cpp:1444
6	thunderbird.exe	TextRunWordCache::MakeTextRun	gfx/thebes/src/gfxTextRunWordCache.cpp:683
7	thunderbird.exe	gfxTextRunWordCache::MakeTextRun	gfx/thebes/src/gfxTextRunWordCache.cpp:992
8	thunderbird.exe	gfxTextRunCache::MakeTextRun	gfx/thebes/src/gfxTextRunCache.cpp:89
9	thunderbird.exe	nsThebesFontMetrics::AutoTextRun::AutoTextRun	gfx/src/thebes/nsThebesFontMetrics.h:171
10	thunderbird.exe	nsThebesFontMetrics::GetWidth	gfx/src/thebes/nsThebesFontMetrics.cpp:340
11	thunderbird.exe	nsThebesRenderingContext::GetWidthInternal	gfx/src/thebes/nsThebesRenderingContext.cpp:869
12	thunderbird.exe	nsRenderingContextImpl::GetWidth	gfx/src/shared/nsRenderingContextImpl.cpp:170
13	thunderbird.exe	nsLayoutUtils::GetStringWidth	layout/base/nsLayoutUtils.cpp:2487
14	thunderbird.exe	nsTreeBodyFrame::AdjustForCellText	layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp:1521
15	thunderbird.exe	nsTreeBodyFrame::PaintText	layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp:3618
16	thunderbird.exe	nsTreeBodyFrame::PaintCell	layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp:3306
17	thunderbird.exe	nsTreeBodyFrame::PaintRow	layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp:3097
18	thunderbird.exe	nsTreeBodyFrame::PaintTreeBody	layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp:2900
19	thunderbird.exe	PaintTreeBody	layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp:2828
20	thunderbird.exe	nsDisplayGeneric::Paint	layout/base/nsDisplayList.h:875
21	thunderbird.exe	nsDisplayList::Paint	layout/base/nsDisplayList.cpp:313
22	thunderbird.exe	nsDisplayClip::Paint	layout/base/nsDisplayList.cpp:978
23	thunderbird.exe	nsDisplayList::Paint	layout/base/nsDisplayList.cpp:313
24	thunderbird.exe	nsLayoutUtils::PaintFrame	layout/base/nsLayoutUtils.cpp:1114
25	thunderbird.exe	PresShell::Paint	layout/base/nsPresShell.cpp:5775
26	thunderbird.exe	nsViewManager::RenderViews	view/src/nsViewManager.cpp:648 

FF crashes with comments:
 bp-14bd7ab9-18db-41c3-89e7-db8202091017
 bp-4b194b29-ceb9-4324-8afe-dfed62090904

Also appears in SM 2.0 http://crash-stats.mozilla.com/report/index/88e0b210-3e72-453b-b107-c5a592091012

Comment 1

[Bas Schouten (:bas.schouten)]

From a quick look at this bug I'd argue the problem could be gfxWindowsFontGroup::FamilyListToArrayList, gfxWindowsFonts.cpp:1325, doesn't actually check if it actually finds the FontEntry, it just adds it to the array. That probably needs an if (fe.get()) around it. I'm not sure if this works but QA could try to put fonts in the pref font list which they know don't exist, and see if it chokes on that. This function would get called if you enter a character which doesn't exist in the normal font, and it starts looking through the pref and system fonts to see which character does support it. A good symbol for this case is the unicode 'Comet' http://www.fileformat.info/info/unicode/char/2604/index.htm, which only exists in a few fonts.

Comment 2

[Jesse Ruderman]

This is topcrash #3 in 3.5.4.

Comment 3

[Samuel Sidler (old account; do not CC)]

I bet Joe wants to own this.

Comment 4

[Joe Drew (not getting mail)]

I so don't! But I know someone who might.

Comment 5

[Samuel Sidler (old account; do not CC)]

If URLs would be helpful here, we can dig them out. Please let us know.

Comment 6

[John Daggett (:jtd)]

Sam, URL's would definitely help to try and reproduce this somehow.

Comment 7

[Bas Schouten (:bas.schouten)]

(In reply to comment #1)
> From a quick look at this bug I'd argue the problem could be
> gfxWindowsFontGroup::FamilyListToArrayList, gfxWindowsFonts.cpp:1325, doesn't
> actually check if it actually finds the FontEntry, it just adds it to the
> array. That probably needs an if (fe.get()) around it. I'm not sure if this
> works but QA could try to put fonts in the pref font list which they know don't
> exist, and see if it chokes on that. This function would get called if you
> enter a character which doesn't exist in the normal font, and it starts looking
> through the pref and system fonts to see which character does support it. A
> good symbol for this case is the unicode 'Comet'
> http://www.fileformat.info/info/unicode/char/2604/index.htm, which only exists
> in a few fonts.
Hrm, played around with this a bit. In theory this should never happen since gfxWindowsPlatform::ResolveFontName should prevent any non existant font from ever beind added to the string list in FamilyListToArray. 
This the only way a null pointer could ever get into the list is if gfxFontFamily::FindFontForStyle returns nsnull.

Comment 9

[Samuel Sidler (old account; do not CC)]

Since it hasn't been mentioned in this bug yet...

We didn't take many changes to this area of code in 1.9.1.4. The only two I can think of are bug 516709 and bug 496573.

Comment 10

[Bas Schouten (:bas.schouten)]

(In reply to comment #9)
> Since it hasn't been mentioned in this bug yet...
> We didn't take many changes to this area of code in 1.9.1.4. The only two I can
> think of are bug 516709 and bug 496573.

So, bug 516709 is very interesting here. 

And I have a theory on what could happen. Although you'd need to introduce some font with a corrupted CMAP table somewhere to reproduce this, and I haven't quite figured out where. If some font family would be corrupted, CreateFontEntry will now return nsnull. Where it never used to. The FontFamily::FamilyAddStylesProc could then in theory add no font styles to the FontFamily (see gfxWindowsFonts.cpp:283). When this happens a call to FindFontForStyle could hit the assertion conditions at gfxFont.cpp:147 and then at gfxFont.cpp:298 or gfxFont.cpp:212, which obviously wouldn't go off, and it would return nsnull. This is behavior which is not allowed. Will propegate and cause gfxWindowsPlatform::FindFontEntry to return nsnull, and cause FamilyListToArray list to add a null font to the font entry list. Which would later cause this crash as it tries to traverse that list.

Comment 11

[Bas Schouten (:bas.schouten)]

Attachment: Proposed patch for preventing null items from entering list

Since every other call to FindFontEntry null-checks the return value. It's probably a good idea to do that at this location too. Added a proposed patch for this.

Comment 13

[John Daggett (:jtd)]

As Bas notes, this looks like a regression from 516709.  Crap, crap, crap.  Probably needs to block 3.6b1.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

I agree that we should get it fixed on mozilla-1.9.2 right-quick, but also think we can go to beta without it. Our current plan is to update the beta as early as next week, actually, so the fix should roll to users quickly.

Comment 15

[Bas Schouten (:bas.schouten)]

Attachment: Font with malformed CMAP

I've been able to reproduce the issue using the following (quite farfetched) steps. Ofcourse this is unlikely to be what is happening in the wild.

1. Install the Swiss Neue font with corrupt CMAP table.
2. Add the following lines to your prefs.js:
user_pref("font.default.x-western", "swiss-neue");
user_pref("font.name.swiss-neue.x-western", "Swiss Neue");
3. Load the attached html which uses a unicode character.

Comment 16

[Bas Schouten (:bas.schouten)]

Attachment: Simple unicode lookup trigger

Comment 17

[John Daggett (:jtd)]

So I think the patch here is correct, it will fix the problem.  Running through the crash URLs, there are a large number of Chinese pages and many other non-English pages.  Of the fonts listed as default pref fonts in all.js, only a subset of these are available with an English version of Windows.  Specifically, I'd really like to confirm that the Chinese font "MS Song" has a valid cmap.

Comment 18

[Bas Schouten (:bas.schouten)]

Attachment: Improved bugfix patch

Improved patch that uses proper whitespacing. The old one was using Cairo's modeline and had tabs in it.

Comment 19

[John Daggett (:jtd)]

Comment on attachment 409278 [details] [diff] [review]
Proposed patch for preventing null items from entering list

This will fix the crashes, we should land this ASAP.

We should still track down which pref fonts are causing the problem and confirm that the cmap checking code isn't rejecting fonts unnecessarily but that can happen after this patch has landed.

Comment 20

[John Daggett (:jtd)]

Comment on attachment 409398 [details] [diff] [review]
Improved bugfix patch

Not sure what's changed but looks good.

Comment 21

[Samuel Sidler (old account; do not CC)]

Restoring flags... again.

Comment 22

[John Daggett (:jtd)]

Landed on trunk:
http://hg.mozilla.org/mozilla-central/rev/0eb54d1ad4e0

Comment 23

[John Daggett (:jtd)]

Cheng took a call from a user reporting this problem.  He sent us his fonts and I went through and looked at them.  There's one font that had a cmap structure problem, a duplicate cmap range.  For some reason, the user had set this as his default font:

user_pref("font.name.serif.x-western", "Arial MT Black");
user_pref("font.size.variable.x-western", 18);

The font says it's an Arial face but it looks like a hacked up version of the real version.  The copyright string in the name table had:

呹灥晡捥₩⁔桥⁍潮潴祰攠䍯牰潲慴楯渠灬挮⁄慴愠ꤠ周攠䵯湯瑹灥⁃潲灯牡瑩潮⁰汣⽔祰攠卯汵瑩潮猠䥮挮‱㤹〭ㄹ㤲⸠䅬氠剩杨瑳⁒敳敲癥搀 

It would be interesting to know where this font came from.  There were other funky fonts on the user's system but this was the only one with a cmap problem.

Comment 24

[Samuel Sidler (old account; do not CC)]

John: Were you able to reproduce the crash with that font?

Any chance we can get this on 1.9.2 and 1.9.1 today? Is the patch ready (or is it the same one)? I'll be around to approve it.

Comment 25

[Samuel Sidler (old account; do not CC)]

Comment on attachment 409398 [details] [diff] [review]
Improved bugfix patch

Approved for 1.9.1.5. a=ss

(This doesn't need 1.9.2 approval since it blocks.)

Comment 26

[John Daggett (:jtd)]

Pushed to 1.9.1
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/00e5f186048d

Yes, with the funky font from the user set as the default x-western sans-serif font, the browser crashes almost immediately.  With the fix it does not.

The patch is simply enough that it applies to any branch without change.

Comment 27

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

Patches already landed on trunk and 1.9.1. Marking as fixed.

Comment 28

[Samuel Sidler (old account; do not CC)]

This needs to land on GECKO1914_20091006_RELBRANCH for 1.9.1.5.

Comment 29

[John Daggett (:jtd)]

Landed on 1.9.1 RELBRANCH
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c6d27d9d1e62

Comment 30

[Daniel Veditz [:dveditz]]

I can has test plz?

Comment 31

[John Daggett (:jtd)]

(In reply to comment #30)
The actual fix here is in pref font handling code.  We can change the pref font in mochitest but that pref font (the bad cmap font) has to be installed on the system for the test to really be valid.

Comment 32

[Al Billings [:abillings - ex-MoCo]]

(In reply to comment #15)
> Created an attachment (id=409341) [details]
> Font with malformed CMAP
> 
> I've been able to reproduce the issue using the following (quite farfetched)
> steps. Ofcourse this is unlikely to be what is happening in the wild.
> 
> 1. Install the Swiss Neue font with corrupt CMAP table.
> 2. Add the following lines to your prefs.js:
> user_pref("font.default.x-western", "swiss-neue");
> user_pref("font.name.swiss-neue.x-western", "Swiss Neue");
> 3. Load the attached html which uses a unicode character.

Verified fixed in the 1.9.1 branch with last night's nightly build (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.5pre) Gecko/20091102 Shiretoko/3.5.5pre (.NET CLR 3.5.30729)) and the testcase above. I will mark it as verified for .5 when we get release branch builds.

Comment 33

[Daniel Veditz [:dveditz]]

Does this bug still need to be hidden at this point?

Comment 34

[John Daggett (:jtd)]

Landed on 1.9.2
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/51741697c83a

Comment 35

[Al Billings [:abillings - ex-MoCo]]

Verified with the official 1.9.1.5 build 1 (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.5) Gecko/20091102 Firefox/3.5.5 (.NET CLR 3.5.30729)). This is fixed.

Comment 37

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

Verified fixed on trunk and 1.9.2 with:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20091105 Minefield/3.7a1pre (.NET CLR 3.5.30729) ID:20091105045542

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2b2pre) Gecko/20091105 Namoroka/3.6b2pre (.NET CLR 3.5.30729) ID:20091105045233