1291647 - Crash in gfxFontFamily::FindAllFontsForStyle

Status: RESOLVED WORKSFORME

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

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

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

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Yeah, adding a check there sounds like the right thing to do.

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1291647 - Add a null check in GetFTLibrary.

Review commit: https://reviewboard.mozilla.org/r/69514/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69514/

Comment 3

[Jonathan Kew [:jfkthame]]

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....

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 5

[Chris Peterson [:cpeterson]]

There are crash reports for both Linux and macOS in Nightly 57.

https://crash-stats.mozilla.com/signature/?product=Firefox&version=57.0a1&signature=gfxFontFamily%3A%3AFindAllFontsForStyle

Comment 6

[Jonathan Kew [:jfkthame]]

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...

Comment 7

[Jeremy Chen ([e-mail])]

(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

Comment 8

[Jonathan Kew [:jfkthame]]

(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.

Comment 9

[Jeremy Chen ([e-mail])]

(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?

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

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.

Comment 11

[Andrei Purice]

Hello,
Does this crash still occur for you Kan-Ru?

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to Andrei Purice from comment #11)

Does this crash still occur for you Kan-Ru?

It looks like Kan-Ru didn't hit this himself, but was filing this based on incoming crash reports.

And yes, we do still have incoming crashes with this signature, but not a lot. We've got 17 total crashes for this calendar-year (the past 2.5 months):
https://crash-stats.mozilla.org/signature/?signature=gfxFontFamily%3A%3AFindAllFontsForStyle&date=%3E%3D2021-01-01T21%3A03%3A00.000Z&date=%3C2021-03-15T21%3A03%3A00.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_columns=startup_crash&_sort=-date&page=1

The most recent one is bp-447ca8ac-d5f4-4193-bb8d-ccb7a0210312

So, this still seems to be an issue, though fortunately it seems to be low-frequency.

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

Closing because no crashes reported for 12 weeks.