Closed Bug 1458003 Opened 4 years ago Closed 3 years ago

Crash in InvalidArrayIndex_CRASH | gfxFontFamily::CheckForLegacyFamilyNames

Categories

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

57 Branch
Unspecified
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- fixed
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: philipp, Assigned: jfkthame)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file, 1 obsolete file)

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...
Somehow we're running into an array indexing error here. :\ We handle it by "safely" crashing, but clearly we need to fix this.
Priority: -- → P2
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.
Attachment #8974019 - Flags: review?(lsalzman)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Attachment #8974019 - Attachment is obsolete: true
Attachment #8974019 - Flags: review?(lsalzman)
Attachment #8974022 - Flags: review?(lsalzman) → review+
Pushed by jkew@mozilla.com:
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
Let's keep this open for now, until we see whether it actually makes a difference to the crashes.
Keywords: leave-open
Hard to tell what impact this had. I can see that Beta61 is still hitting this from time to time, though.
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.
The leave-open keyword is there and there is no activity for 6 months.
:jfkthame, maybe it's time to close this bug?
Flags: needinfo?(jfkthame)
I don't see any crash reports from versions later than 61.0.2, so I'm inclined to resolve this as fixed.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Flags: needinfo?(jfkthame)
Resolution: --- → FIXED
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.
Flags: needinfo?(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.
Flags: needinfo?(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
Attachment #8974022 - Flags: approval-mozilla-esr60?
Target Milestone: --- → mozilla62
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.
Attachment #8974022 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
You need to log in before you can comment on or make changes to this bug.