1493743 - Crash in gfxFontGroup::BuildFontList

Status: VERIFIED FIXED

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

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is
report bp-13e170e2-018f-4751-b0fd-2f0f80180924.
=============================================================

Seen while looking at crash stats. Small volume Mac crash seems to be only confined to 10.14 at the moment: https://bit.ly/2PVHsiE. Several of the users crashed more than once. 

There are several Windows crashes, but they are not on 64 nightly.

Sorry wasn't sure if this is Graphics |Text or Texts and Fonts...

Top 10 frames of crashing thread:

0 XUL gfxFontGroup::BuildFontList gfx/thebes/gfxTextRun.cpp:1889
1 XUL gfxFontGroup::gfxFontGroup gfx/thebes/gfxTextRun.cpp:1814
2 XUL gfxPlatformMac::CreateFontGroup gfx/thebes/gfxTextRun.cpp:1810
3 XUL nsFontMetrics::nsFontMetrics gfx/src/nsFontMetrics.cpp:143
4 XUL nsFontCache::GetMetricsFor gfx/src/nsFontMetrics.cpp:123
5 XUL nsLayoutUtils::GetFontMetricsForComputedStyle gfx/src/nsDeviceContext.cpp:245
6 XUL mozilla::ReflowInput::CalcLineHeight layout/generic/ReflowInput.cpp:2871
7 XUL mozilla::BlockReflowInput::BlockReflowInput layout/generic/ReflowInput.cpp:2883
8 XUL nsBlockFrame::Reflow layout/generic/BlockReflowInput.cpp:50
9 XUL nsContainerFrame::ReflowChild layout/generic/nsContainerFrame.cpp:951

=============================================================

Comment 1

[Marcia Knous [:marcia]]

Randell looked at the crashes in triage that weren't 64 and they seem to be a different issue. Let's keep this bug about the Mac crashes showing up in Mac nightly on 10.14.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Can you figure out who should look at this?  Thanks

Comment 3

[Jonathan Kew [:jfkthame]]

I looked at this a bit to try and understand a scenario that could lead to a null-deref in here, but without much success.

There are a few Windows crashes which point to https://hg.mozilla.org/mozilla-central/annotate/bae2e21a9312/gfx/thebes/gfxTextRun.cpp#l1833, which suggests the name.mName atom may be null. That shouldn't be the case when name.IsNamed() is true, but if we somehow have a default-constructed FontFamilyName, then it could happen. I think we should change the default FontFamilyName constructor to avoid that possibility, even though I haven't figured out how it would happen.

The macOS crashes point to https://hg.mozilla.org/mozilla-central/annotate/856103837d4dda0e176706cc64927546459d510c/gfx/thebes/gfxTextRun.cpp#l1889, which looks more like we have a null aFamily. But I can't see where that would come from.

If someone has a machine running 10.14 (Lee, Markus -- either of you have 10.14 yet?) and can reproduce this, we'd probably be able to figure out where it's coming from, but without that it's hard to see a way forward.

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: speculative patch - Set type to None rather than Named in default FontFamilyName constructor, as no name is provided

I think we should do this, to avoid the risk of an inconsistent state, though I don't know if it will actually help anything here.

Comment 5

[Markus Stange [:mstange]]

I don't have 10.14 yet. Stephen, have you seen this crash before?

Comment 6

[Lee Salzman [:lsalzman]]

Comment on attachment 9014106 [details] [diff] [review]
speculative patch - Set type to None rather than Named in default FontFamilyName constructor, as no name is provided

It's worth a try, at least.

I don't have 10.14 unfortunately. Only 10.13.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f34f0818f82d
speculative patch - Set type to None rather than Named in default FontFamilyName constructor, as no name is provided. r=lsalzman

Comment 8

[Jonathan Kew [:jfkthame]]

Let's keep this open for now, until we see whether this makes any difference; I'm not at all confident it's the real or only problem.

Comment 9

[Stephen A Pohl [:spohl]]

(In reply to Markus Stange [:mstange] from comment #5)
> I don't have 10.14 yet. Stephen, have you seen this crash before?

No, I haven't. If we find steps to reproduce I'm happy to investigate or verify a fix.

Comment 10

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/f34f0818f82d

Comment 11

[Marcia Knous [:marcia]]

23 crashes/5 installations in the last week. Most of the crashes are on 10.14 Mac.

Comment 12

[David Durst [:ddurst]]

NI for another look

Comment 13

[Jonathan Kew [:jfkthame]]

The stack in reports like https://crash-stats.mozilla.com/report/index/e3949730-6a4d-4f44-9b3b-7e1700181019 makes it appear that perhaps we're getting a null gfxFontFamily pointer passed to gfxFontGroup::AddFamilyToFontList. That isn't supposed to be possible, but perhaps there's some circumstance where it happens. (We're clearly not seeing it in automation with debug builds, as there's an assertion that would be firing.)

I suggest we revise the code here to be safer in the face of a null aFamily argument, and see if that helps eliminate these crashes. In the worst case, I guess this could mean we end up with an empty font list, but we've got other code that kills the process if there's no usable font available; more likely, there's some kind of broken font present, and if we skip that we'll just end up using the next in the list, or a generic fallback.

Comment 14

[Jonathan Kew [:jfkthame]]

Attachment: Make AddFamilyToFontList return safely if passed a null font-family

I suggest we try this and see if some/all of these crashes stop. While I haven't tracked down exactly how a null pointer could arise here, it's clear that nothing good can come of attempting to use it.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eff78d899e01
Make AddFamilyToFontList return safely if passed a null font-family. r=lsalzman

Comment 16

[Jonathan Kew [:jfkthame]]

Let's keep the leave-open keyword for now, until we see if this makes any difference.

Comment 17

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/eff78d899e01

Comment 18

[Jonathan Kew [:jfkthame]]

https://crash-stats.mozilla.com/report/index/a30ac042-1631-4328-a346-058e20181027 says we still have some kind of problem here.

In this case, it looks like a null nsAtom pointer is being used; not sure how that arises, though.

Comment 19

[Jonathan Kew [:jfkthame]]

Attachment: Make BuildFontList robust against an invalid FontFamilyName record with no name atom

I'm guessing this is the atom dereference that's crashing, though I haven't figured out how we can end up with a null pointer in the list here, and no idea how to reproduce. Anyhow, skipping the bad FontFamilyName should be safe enough, we'll just end up using a default font if no valid names are found here.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f6bc2bf601b
Make BuildFontList robust against an invalid FontFamilyName record with no name atom. r=lsalzman

Comment 21

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/3f6bc2bf601b

Comment 22

[Jonathan Kew [:jfkthame]]

I don't see any reports from trunk builds later than 20181026220839; I think we can call this fixed.

There's a steady trickle of crashes on beta, though, so we should probably uplift these patches.

Comment 23

[Jonathan Kew [:jfkthame]]

Comment on attachment 9020729 [details] [diff] [review]
Make BuildFontList robust against an invalid FontFamilyName record with no name atom

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: unknown

User impact if declined: Crashes (mostly on macOS, also some Windows), exact cause unknown but may be related to a bad font of some kind

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce: no known STR, just crash reports from the wild

List of other uplifts needed: recommend taking all 3 patches on this bug

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): All 3 patches here just add safe handling of unexpectedly-null/invalid objects (missing name, broken font?) so that we'll end up falling back to a default font, as if the bad entry were not present, rather than crashing.

String changes made/needed: none

Comment 24

[Jonathan Kew [:jfkthame]]

Comment on attachment 9019760 [details] [diff] [review]
Make AddFamilyToFontList return safely if passed a null font-family

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None

User impact if declined: 

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: Yes

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): 

String changes made/needed:

Comment 25

[Jonathan Kew [:jfkthame]]

Comment on attachment 9014106 [details] [diff] [review]
speculative patch - Set type to None rather than Named in default FontFamilyName constructor, as no name is provided

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None

User impact if declined: 

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: Yes

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): 

String changes made/needed:

Comment 26

[Jonathan Kew [:jfkthame]]

Hmm, trying to set the approval-beta? flag on the other two patches without providing additional comments -- as comment 23 was intended to cover all three -- turns out to partially fill in the approval form with some unexpected defaults. Comments 24-25 should be ignored.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9014106 [details] [diff] [review]
speculative patch - Set type to None rather than Named in default FontFamilyName constructor, as no name is provided

This landed on m-c when it was still tracking 64.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9019760 [details] [diff] [review]
Make AddFamilyToFontList return safely if passed a null font-family

[Triage Comment]
Fixes font-related crashes on OSX and Windows. Approved for 64.0b7.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/da525e924591
https://hg.mozilla.org/releases/mozilla-beta/rev/42154e18f97c

Comment 30

[Bogdan Maris, Desktop Test Engineering]

Setting qe-verify+ in order to check for Socorro data Friday in beta 8, since there are no known str.

Comment 31

[Stefan [:StefanG_QA]]

Based on crash-stats (https://bit.ly/2JXnD97), this crash did not occur on builds with the fix: Beta 64.0b7 (11.05.2018) or after and Nightly 65.0a1 builds created after 11.05.2018.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Given that bug 1497563 is showing up on ESR60 with decent volume and this is a pretty small set of patches, I think this would is worth uplift consideration. Please nominate unless you feel strongly otherwise :)

Comment 33

[Jonathan Kew [:jfkthame]]

Yes, I think it'd be worth uplifting these -- they're all totally safe, just hardening the code so it handles unexpected error conditions safely.

(FWIW, I notice there are still some crash reports from Fx64 and 65 builds after these patches landed, so the issue hasn't entirely gone away, though it does seem to be much improved.)

Uplift request is intended to refer to all three patches (though I think the third one turned out to be the most helpful).

Comment 34

[Jonathan Kew [:jfkthame]]

Comment on attachment 9020729 [details] [diff] [review]
Make BuildFontList robust against an invalid FontFamilyName record with no name atom

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: Potential crashes when building font lists (this signature or bug 1497563)

User impact if declined: continued crashes

Fix Landed on Version: 64

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Just simple changes to safely handle unexpected error conditions

String or UUID changes made by this patch: none

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9020729 [details] [diff] [review]
Make BuildFontList robust against an invalid FontFamilyName record with no name atom

Low-risk fix for a crash showing up on ESR60 at moderate volumes. Approved for 60.5.0esr.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/b765f5e2444f
https://hg.mozilla.org/releases/mozilla-esr60/rev/ae6e52dedf97
https://hg.mozilla.org/releases/mozilla-esr60/rev/1e6656f49a2a

Comment 37

[Ryan VanderMeulen [:RyanVM]]

These needed to be backed out due to Rust panics caused by rev b765f5e2444f (confirmed on Try).
https://treeherder.mozilla.org/logviewer.html#?job_id=219807258&repo=mozilla-esr60

https://hg.mozilla.org/releases/mozilla-esr60/rev/29ad50003204b225561a651b16ed1be46d438c07

Comment 38

[Jonathan Kew [:jfkthame]]

It looks like we should just omit the first patch here when landing on esr60, to avoid the risk of the Rust panic. I've pushed a try run to double-check that it's happier without that:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e25ea502a0fc732890cc778e06439abf12315cf

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/fe810e4d9777
https://hg.mozilla.org/releases/mozilla-esr60/rev/421de5ff5bcb