618406 - crash [@ @0x0 | gfxFT2LockedFace::GetUVSGlyph ] from the checkin for bug 569770

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Cork]

Attachment: Crash test

I'm getting a ton of crashes from the checkin for bug 569770:
bp-2c5545a5-b271-4329-b41c-782f92101210
bp-b3f5708e-a1d4-469e-bf6c-250a42101210
bp-4a21b3a2-cb00-49cf-8794-7e2cf2101210
bp-80ae45d6-85e5-479c-9022-105a02101210

The regression range points right at this bug:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9ab76fbcb6d7&tochange=11e328a49e0a

Its only Linux x86_64 systems as far as i can see.

Comment 1

[Joe Drew (not getting mail)]

This is from brand new code, and is a regression since beta 7. It's quite crashy, the #2 topcrasher today on Linux, but that's still only a small number of crashes, 22.

This could still turn into a beta 8 blocker.

Comment 2

[Joe Drew (not getting mail)]

Cork:

Did you submit all 22 of those crashes?

Comment 3

[Cork]

I have 27 as of before i started regression searching (stoped remembered to uncheck it by then).

Comment 4

[Jonathan Kew [:jfkthame]]

Cork: what version of freetype is installed on your system? If it's earlier than 2.4.4, I think this may be a freetype bug that was recently fixed (http://git.savannah.gnu.org/cgit/freetype/freetype2.git/commit/?id=e891e4d6f130408d171724723673472a4e0359f9).

Comment 5

[Cork]

It's 2.4.2, the version used in ubuntu 10.10 and (for the momet at least) 11.04.

Comment 6

[Karl Tomlinson (:karlt)]

I'm having trouble understanding why that patch was necessary.

FT_Face_GetCharVariantIndex will only try to call vcmap->clazz->char_var_index
if find_variant_selector_charmap returns non-NULL ...

http://git.savannah.gnu.org/cgit/freetype/freetype2.git/tree/src/base/ftobjs.c?id=6aee69096f7c04f3c16fa3d1a097180f9fb62154#n3285

... but find_variant_selector_charmap only returns non-NULL when
FT_Get_CMap_Format( charmap ) == 14.

http://git.savannah.gnu.org/cgit/freetype/freetype2.git/tree/src/base/ftobjs.c?id=6aee69096f7c04f3c16fa3d1a097180f9fb62154#n1062

Comment 7

[Karl Tomlinson (:karlt)]

I don't reproduce with freetype 2.4.3, but I can reproduce with
http://security.ubuntu.com/ubuntu/pool/main/f/freetype/libfreetype6_2.4.2-2ubuntu0.1_amd64.deb

Comment 8

[Karl Tomlinson (:karlt)]

In FreeType versions 2.4.0 to 2.4.3, when FT_CONFIG_OPTION_OLD_INTERNALS is defined, find_variant_selector_charmap always returns the first cmap.
Fixed here:

http://git.savannah.gnu.org/cgit/freetype/freetype2.git/commit/?id=ac09390afcfaf2c63b75ffee5c0759e29359f9ac

Comment 9

[Karl Tomlinson (:karlt)]

I don't see any way to access FreeType's preloaded cmap data to use
gfxFontUtils::MapUVSToGlyphFormat14 without making a copy of the whole cmap
table (with all subtables).

FT_Library_Version is available to check for versions that may have the bug,
http://freetype.sourceforge.net/freetype2/docs/reference/ft2-version.html#FT_Library_Version

and for possibly buggy versions presence of the symbol FT_Alloc indicates
FT_CONFIG_OPTION_OLD_INTERNALS and so presence of the bug.

Comment 10

[Karl Tomlinson (:karlt)]

This is in our testsuite as layout/reftests/font-face/ivs-1.html but we should remove random-if(gtk2Widget).

Comment 11

[Karl Tomlinson (:karlt)]

gfxFontUtils has the necessary functions for copying the variant selector
subtable from the cmap table and for performing the lookup.
However, managing the lifetime of the subtable copy gets a bit fiddly,
particularly for gfxFT2Fonts consumers where the FontEntry does not expire.

Functional IVS on systems with broken IVS support doesn't seem an important
enough hoop to jump through given the addition code required, so I suggest a
miminal fix to simply avoid crashy versions of FT_Face_GetCharVariantIndex.

Comment 12

[Karl Tomlinson (:karlt)]

Attachment: avoid crashy versions of FT_Face_GetCharVariantIndex

Comment 13

[Jonathan Kew [:jfkthame]]

Comment on attachment 497198 [details] [diff] [review]
avoid crashy versions of FT_Face_GetCharVariantIndex

Yes, I'd agree: it's not worth managing a whole separate code path to work around this by using the gfxFontUtils code - just detect the buggy FT version and don't support the feature. So this looks fine.

Comment 14

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/rev/2b3626056e13

Comment 15

[LegNeato]

As per today's meeting, beta 9 will be a time-based release. Marking these all betaN+. Please move it back to beta9+ if you believe it MUST be in the next beta (ie: trunk is in an unshippable state without this)