1458003 - Crash in InvalidArrayIndex_CRASH | gfxFontFamily::CheckForLegacyFamilyNames

Status: RESOLVED FIXED

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

Description

[[:philipp]]

This bug was filed from the Socorro interface and is
report bp-53403bf0-b8b1-4dd9-aabc-b78850180430.
=============================================================

Top 10 frames of crashing thread:

0 libmozglue.dylib MOZ_CrashPrintf mfbt/Assertions.cpp:50
1 XUL InvalidArrayIndex_CRASH xpcom/ds/nsTArray.cpp:26
2 XUL gfxFontFamily::CheckForLegacyFamilyNames xpcom/ds/nsTArray.h:1029
3 XUL gfxPlatformFontList::FindAndAddFamilies gfx/thebes/gfxPlatformFontList.cpp:802
4 XUL gfxMacPlatformFontList::FindAndAddFamilies gfx/thebes/gfxMacPlatformFontList.mm:1574
5 XUL gfxFontGroup::AddPlatformFont gfx/thebes/gfxTextRun.cpp:1863
6 XUL gfxFontGroup::BuildFontList gfx/thebes/gfxTextRun.cpp:1818
7 XUL gfxFontGroup::gfxFontGroup gfx/thebes/gfxTextRun.cpp:1799
8 XUL gfxPlatformMac::CreateFontGroup gfx/thebes/gfxPlatformMac.cpp:152
9 XUL nsFontMetrics::nsFontMetrics gfx/src/nsFontMetrics.cpp:143

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

this low volume crash on macos has regressed in firefox 57, probably due to bug 835204.

according to affected users, the issue happens repeatedly on particular sites, see https://support.mozilla.org/en-US/questions/1215824...

Comment 1

[Jonathan Kew [:jfkthame]]

Somehow we're running into an array indexing error here. :\ We handle it by "safely" crashing, but clearly we need to fix this.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: speculative fix: make a local copy of the face list in CheckForLegacyFamilyNames, in case mAvailableFonts is mutated during this function

I haven't been able to reproduce a crash like this; guessing it's related to particular fonts that some users have installed. Currently the only scenario I can think of (aside from the mAvailableFonts array being corrupted somehow) is that during the iteration here, we end up modifying the current family's list of faces, and this breaks the range-based iteration. To avoid the risk of this, I propose making a local copy of the array and iterating over that; let's see if this puts a stop to the crashes.

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: speculative fix: make a local copy of the face list in CheckForLegacyFamilyNames, in case mAvailableFonts is mutated during this function

This time attaching a patch that actually builds.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/511276b31ce9
speculative fix: make a local copy of the face list in CheckForLegacyFamilyNames, in case mAvailableFonts is mutated during this function. r=lsalzman

Comment 5

[Jonathan Kew [:jfkthame]]

Let's keep this open for now, until we see whether it actually makes a difference to the crashes.

Comment 6

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/511276b31ce9

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Hard to tell what impact this had. I can see that Beta61 is still hitting this from time to time, though.

Comment 8

[Jonathan Kew [:jfkthame]]

AFAICT, the (speculative) patch here has only landed on 62, so it makes sense we'd still see Beta61 crashes. I don't see any from 62, but it's possible the Nightly user population is too small to show it; we'll have to see over the next few weeks whether Beta62 crashes start to appear.

Comment 9

[Release mgmt bot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:jfkthame, maybe it's time to close this bug?

Comment 10

[Jonathan Kew [:jfkthame]]

I don't see any crash reports from versions later than 61.0.2, so I'm inclined to resolve this as fixed.

Comment 11

[Lee Salzman [:lsalzman]]

Since the reports are mostly coming from 60 ESR, should we just uplift this to clean those up too? Seems like the patch would graft cleanly.

Comment 12

[Jonathan Kew [:jfkthame]]

That sounds like a good idea, yes. I don't see any significant risk here, and it does appear to have stopped the crashes.

Comment 13

[Jonathan Kew [:jfkthame]]

Comment on attachment 8974022 [details] [diff] [review]
speculative fix: make a local copy of the face list in CheckForLegacyFamilyNames, in case mAvailableFonts is mutated during this function

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: Crashes (low frequency, but continuing) seen on esr60

User impact if declined: possible crashes during font lookup

Fix Landed on Version: 62

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Minimal change just makes a local copy of a (small) possibly-mutable array before iterating over it.

String or UUID changes made by this patch: none

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8974022 [details] [diff] [review]
speculative fix: make a local copy of the face list in CheckForLegacyFamilyNames, in case mAvailableFonts is mutated during this function

Looks simple enough and is plenty well-baked on release. Approved for 60.5.0esr.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/10b03e611a42