Open Bug 1291647 Opened 4 years ago Updated 2 years ago

Crash in gfxFontFamily::FindAllFontsForStyle

Categories

(Core :: Layout: Text and Fonts, defect, P3, critical)

Unspecified
Linux
defect

Tracking

()

Tracking Status
firefox48 --- affected
firefox-esr45 --- affected
firefox50 --- affected
firefox51 --- affected
firefox57 --- affected

People

(Reporter: kanru, Unassigned)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-6ec029d0-d098-4ff2-a5f6-2ff862160802.
=============================================================

7 crashes from maybe two installation.

Since gfxPlatformFontList::GetDefaultFont() might return nullptr, maybe we should check family and return nullptr here:

https://hg.mozilla.org/mozilla-unified/annotate/ffac2798999c/gfx/thebes/gfxFcPlatformFontList.cpp#l1516
Flags: needinfo?(jd.bugzilla)
Yeah, adding a check there sounds like the right thing to do.
Looking at the places in gfxFcPlatformFontList where it uses GetFTLibrary(), I think we have a deeper problem here. Those call sites pass the result of GetFTLibrary directly to Freetype APIs (FT_New_Face, FT_New_Memory_Face). But AFAICS, those APIs expect to be given a valid FT_Library.

So allowing GetFTLibrary to return nullptr under _any_ circumstances is probably a recipe for disaster. It might be better to MOZ_CRASH immediately, rather than pass a null library handle to Freetype and take a chance on the outcome of that....
Crash volume for signature 'gfxFontFamily::FindAllFontsForStyle':
 - nightly (version 51): 14 crashes from 2016-08-01.
 - aurora  (version 50): 5 crashes from 2016-08-01.
 - beta    (version 49): 0 crashes from 2016-08-02.
 - release (version 48): 3 crashes from 2016-07-25.
 - esr     (version 45): 9 crashes from 2016-05-02.

Crash volume on the last weeks (Week N is from 08-22 to 08-28):
            W. N-1  W. N-2  W. N-3
 - nightly       0       2      12
 - aurora        5       0       0
 - beta          0       0       0
 - release       1       1       1
 - esr           0       0       0

Affected platforms: Windows, Mac OS X, Linux

Crash rank on the last 7 days:
           Browser     Content   Plugin
 - nightly
 - aurora  #403
 - beta
 - release
 - esr
Flags: needinfo?(jd.bugzilla)
Those reports are (all, I think) the macOS/Linux signature of bug 1398458, which was an unrelated regression that we introduced and then fixed fairly promptly.

The issue here would have GetFTLibrary on the stack, which I don't think is the case with any of the recent reports. But the  issue is still potentially there, AFAICS. We should follow up on comment 3 and figure out how to handle it...
(In reply to Jonathan Kew (:jfkthame) from comment #3)
> Looking at the places in gfxFcPlatformFontList where it uses GetFTLibrary(),
> I think we have a deeper problem here. Those call sites pass the result of
> GetFTLibrary directly to Freetype APIs (FT_New_Face, FT_New_Memory_Face).
> But AFAICS, those APIs expect to be given a valid FT_Library.
> So allowing GetFTLibrary to return nullptr under _any_ circumstances is
> probably a recipe for disaster. It might be better to MOZ_CRASH immediately,
> rather than pass a null library handle to Freetype and take a chance on the
> outcome of that....

Hi Jonathan, in Bug 1309205, we don't pass the result of GetFTLibrary directly to Freetype APIs (FT_New_Face, FT_New_Memory_Face) anymore. Instead, we use a private member [1] to store it, and access the member by SetFTLibrary and GetFTLibrary. An assertion is also added in GetFTLibrary [2] to ensure that we'd crash if we try to pass a null library handle to Freetype APIs. Does this somehow fulfill what you mentioned in comment 3?

If so, maybe xidorn's fix is worth landing now, so we can avoid the potential misleading crash signatures (like comment 5) in the future. WDYT?


[1] https://searchfox.org/mozilla-central/rev/15ce5cb2db0c85abbabe39a962b0e697c9ef098f/gfx/2d/Factory.cpp#212
[2] https://searchfox.org/mozilla-central/rev/15ce5cb2db0c85abbabe39a962b0e697c9ef098f/gfx/2d/Factory.cpp#667
Flags: needinfo?(jfkthame)
Priority: -- → P3
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #7)
> An assertion is also added in GetFTLibrary
> [2] to ensure that we'd crash if we try to pass a null library handle to
> Freetype APIs.

That's a debug-only assertion, though. AFAICS, if gfxFcPlatformFontList's GetFTLibrary ever returns null, we'll still end up with null stored in Moz2D's member variable. And in a release build, Factory::GetFTLibrary will return it without complaint -- and so we'd still be passing it on to the FreeType APIs.

Given that we can't safely proceed without a valid FTLibrary, I think we should have a release-mode assertion somewhere that will detect such a failure. Probably not in Factory::GetFTLibrary, as we don't want to add any extra code there, but maybe in Factory::SetFTLibrary? Or in gfxFcPlatformFontList::GetFTLibrary, so that it asserts and kills the process (even in release builds) rather than ever returning null.
Flags: needinfo?(jfkthame)
(In reply to Jonathan Kew (:jfkthame) from comment #8)
> (In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #7)
> > An assertion is also added in GetFTLibrary
> > [2] to ensure that we'd crash if we try to pass a null library handle to
> > Freetype APIs.
> 
> That's a debug-only assertion, though. AFAICS, if gfxFcPlatformFontList's
> GetFTLibrary ever returns null, we'll still end up with null stored in
> Moz2D's member variable. And in a release build, Factory::GetFTLibrary will
> return it without complaint -- and so we'd still be passing it on to the
> FreeType APIs.
> 
> Given that we can't safely proceed without a valid FTLibrary, I think we
> should have a release-mode assertion somewhere that will detect such a
> failure. Probably not in Factory::GetFTLibrary, as we don't want to add any
> extra code there, but maybe in Factory::SetFTLibrary? Or in
> gfxFcPlatformFontList::GetFTLibrary, so that it asserts and kills the
> process (even in release builds) rather than ever returning null.

Sounds reasonable. In this way, we can get crash report with proper signature once we indeed hit this potential issue. xidorn, would you like to keep working on this bug by adding a release-mode assertion as Jonathan suggested?
Flags: needinfo?(xidorn+moz)
I would like... when I have time to finish other higher priority things.

Anyone interested feel free to pick up this, as I may not be able to come to this for a while.
Flags: needinfo?(xidorn+moz)
You need to log in before you can comment on or make changes to this bug.