Closed Bug 1397238 Opened 7 years ago Closed 7 years ago

Crash in InvalidArrayIndex_CRASH | gfxFontFamily::FindAllFontsForStyle

Categories

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

57 Branch
Unspecified
Windows 10
defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- fixed

People

(Reporter: calixte, Assigned: jfkthame)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression, Whiteboard: [clouseau])

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-1d589bfc-797a-4a61-a46f-b6f450170906.
=============================================================

There are 39 crashes in nightly 57 starting with buildid 20170905100117. In analyzing the backtrace the regression may have been introduced by patch [1] to fix bug 835204.

[1] https://hg.mozilla.org/mozilla-central/rev?node=c31c7c6520240d32ac26741022d89b4f407885a9
Flags: needinfo?(jfkthame)
Puzzling. Yes, this is probably somehow related to the patch from bug 835204, but so far I don't see how it happens.

It looks like we're ending up at the line

  http://searchfox.org/mozilla-central/rev/4d8e389498a08668cce9ebf6232cc96be178c3e4/gfx/thebes/gfxFontEntry.cpp#1282

in a gfxFontFamily where mAvailableFonts has Length==2, and if the code here tries to access face index 2 or 3, we crash.

But we shouldn't be in this situation, because the only way for mIsSimpleFamily to be true is if it was set by gfxFontFamily::CheckForSimpleFamily():

  http://searchfox.org/mozilla-central/rev/4d8e389498a08668cce9ebf6232cc96be178c3e4/gfx/thebes/gfxFontEntry.cpp#1359

And that method sets the mIsSimpleFamily flag in two cases: either mAvailableFonts.Length==1 (but in that case we'd have taken the early return from FindAllFontsForStyle, and the crash reports confirm that we're seeing Length==2 in the crashes), or else we reached the end of the method. But in that case, we've unconditionally done mAvailableFonts.SetLength(4).

So I don't see how we're reaching FindAllFontsForStyle with the combination of mIsSimpleFamily==true and mAvailableFonts.Length==2; that should be impossible.

Clearly there's something I haven't spotted yet...
OK, one possibility: maybe there's a scenario where we do CheckForLegacyFamilyNames on one font family, and as a result we create a new legacy family with a single font face. This family gets the mIsSimpleFamily flag set. Subsequently, we do CheckForLegacyFamilyNames on a different "canonical" family, and find *the same* legacy family name. So we add a new face to the already-existing legacy family: it now has two faces. But its mIsSimpleFamily flag was already set, and so we don't repeat the full CheckForSimpleFamily() and we don't allocate the four standard face entries. That would result in the crash we're seeing here.

AFAICS, hitting this issue would require that there are two separate font families that each include a face with the *same* legacy "styled family" name. While that's a bit of an odd situation, it's certainly possible.

To avoid this risk, we should clear the mIsSimpleFamily flag any time we modify the list of faces in the family, so that we will re-check them before making any assumptions that the mAvailableFonts has four standard entries.
Flags: needinfo?(jfkthame)
This should fix the crash if it is indeed a result of the scenario above; and in any case it's a harmless precaution.
Attachment #8905065 - Flags: review?(cam)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
For information, 71% of the users have an asian locale (ja, ko, zh-cn, zh-tw) and the other have en-US and for them, most of the URLs are following the pattern *.jp.
Adding some odd Linux-specific variants of this signature.
Crash Signature: [@ InvalidArrayIndex_CRASH | gfxFontFamily::FindAllFontsForStyle] → [@ InvalidArrayIndex_CRASH | gfxFontFamily::FindAllFontsForStyle] [@ InvalidArrayIndex_CRASH | gfxFontFamily::FindAllFontsForStyle [clone .cold.282] ] [@ InvalidArrayIndex_CRASH | gfxFontFamily::FindAllFontsForStyle [clone .cold.283] ]
Comment on attachment 8905065 [details] [diff] [review]
Reset the mIsSimpleFamily flag whenever a face is added to a font family, to ensure we will re-check the available faces before making assumptions about face indexes in FindAllFontsForStyle

Review of attachment 8905065 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxFontEntry.h
@@ +635,5 @@
>          }
>          aFontEntry->mSkipDefaultFeatureSpaceCheck = mSkipDefaultFeatureSpaceCheck;
>          mAvailableFonts.AppendElement(aFontEntry);
> +        mIsSimpleFamily = false; // CheckForSimpleFamily may set this later,
> +                                 // but at this point we're not sure

This change looks fine.

I wonder if it might be clearer to have a separate "have we checked whether this is a simple family" and "is this a simple family" flags.  Currently it seems like we rely on the caller gfxPlatformFontList::CheckFamily to avoid calling CheckForSimplyFamily needlessly if we have already done the check but the result is that it's not a simple family.
Attachment #8905065 - Flags: review?(cam) → review+
I'm inclined to agree the management of this flag could be clearer, but for this bug I'd prefer to keep the change as simple as possible, particularly as I haven't reproduced the actual crash locally; we'll be relying on observing a crash-stats change to confirm whether the problem and solution was guessed correctly. Given that, I prefer not to add any extra churn right now, although I can imagine a future refactoring to clarify the code.
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c70e1ffbbbd
Reset the mIsSimpleFamily flag whenever a face is added to a font family, to ensure we will re-check the available faces before making assumptions about face indexes in FindAllFontsForStyle. r=heycam
Let's keep this bug open for a few days, until we see whether the fix here actually does solve the issue.
Keywords: leave-open
Priority: -- → P1
Depends on: 1398458
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Keywords: leave-open
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.