821442 - eliminate the mFamily back-pointer in gfxFontEntry because it is troublesome and no longer reliable

Status: RESOLVED FIXED

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

Description

[Jonathan Kew [:jfkthame]]

Currently, the gfxFontEntry objects that represent individual font faces available on the platform have an mFamily member that points back to their owning gfxFontFamily. This has been a source of bugs due to these back-pointers becoming stale/dangling (e.g. bug 721315); if we can eliminate the mFamily pointer, there will no longer be a risk of it being invalidated during manipulation of the font list (e.g. as user font sets are being modified).

Moreover, bug 816483 (just landed on trunk) has actually broken the assumption underlying this pointer - that any given gfxFontEntry belongs to precisely one gfxFontFamily. Now that we cache the entries for activated user fonts and re-use them for subsequent @font-face rules where the source and principal match, this assumption is no longer true: a single entry might be simultaneously serving several separate @font-face-declared families, and hence several gfxFontFamily objects might hold references to it. Which one should its mFamily point to then?

In addition to the risks inherent in maintaining the mFamily pointer, this means that it would now be possible (in obscure cases with strange mixtures of @font-face rules) to actually get incorrect font-matching behavior, because we use the mFamily back-pointer for the gfxFontGroup::TryOtherFamilyMembers method. It would be possible to contrive a testcase where this would end up using a face from a different CSS family that shouldn't have been available to the current fontGroup.

In view of all this, I'm interested in trying to eliminate the mFamily pointer from font entries, so that we have a strictly one-way connection where the family holds references to its members, but the individual members do -not- know about their family.

AFAICT, the main user of the mFamily pointer is the user font set code, which can readily be refactored so as to explicitly pass the family to methods that need it instead of retrieving it from a font entry.

In addition, there are a handful of other users that may be trickier: there's roughly one method in each of font-matching, a11y, mathml, and layout-inspector code that relies on being able to find the family (or the family name) given a specific font face, so some solution will need to be provided in those places. Perhaps some code that currently keeps track of only a gfxFontEntry (or gfxFont instance) will need to maintain a <gfxFontFamily, gfxFontEntry> pair instead.

I think the result of this, if successful, will be a cleaner and more robust structure, as well as fixing the potential font-matching error that bug 816483 has inadvertently introduced.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I guess this needs to be fixed in the same release we ship bug 816483?

Comment 2

[Jonathan Kew [:jfkthame]]

That would be ideal, but TBH the scenario whereby we'd actually do incorrect font-matching is sufficiently obscure that it wouldn't worry me very much if we shipped a release with that bug present.

Basically, the problem would only arise if the user visits multiple pages that (a) use the same webfont resources (URIs and principals match), but (b) collect them into *differently-organized* CSS families; and then (c) during font-matching, we encounter characters that aren't present in the chosen font, so we call TryOtherFamilyMembers; whereupon (d) the family back-pointer mismatch causes us to try a face that shouldn't have been in the current font stack, but happens to include the required character.

I've started on a patch for this, though, so if all goes well we can get it fixed soon.

Comment 3

[Jonathan Kew [:jfkthame]]

One aspect of this looks like being a bit tricky: it impacts the nsIDOMFontFace interface that we added in bug 467669. Specifically, the CSSFamilyName, rule, and srcIndex properties will not be reliably available unless we do extra work during font-matching and add something to the gfxTextRun's GlyphRun records to keep track of them. This is because once a font entry may be shared between multiple @font-face rules that happen to load the same resource, we can no longer rely on locating the rule/srcIndex/CSSFamilyName from the gfxFontEntry, which is currently the only font identifier we have left once we've finished building the textrun.

Comment 4

[Jonathan Kew [:jfkthame]]

So... for purposes of nsIDOMFontFace, how important is it to be able to refer back to the actual @font-face rule that is responsible for loading the font?

E.g., given

@font-face { font-family: A; src(myfont.ttf); }
@font-face { font-family: B; src(myfont.ttf); }
@font-face { font-family: C; src(myfont.ttf); }

<p style="font-family: A">foo <span style="font-family: C">bar</span>

when the user asks for the fonts used in the range including "foo bar", is it important that we're able to identify the two specific rules that were used, even though we're now using the same font instance for the entire range?

In current releases, we recognize these as being distinct (because we load the font twice with separate gfxFontEntry objects), and track them separately, but bug 816483 breaks that.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I guess it probably isn't important --- as long as you can come up with a reasonable and predictable alternative behavior.

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: eliminate the unreliable mFamily back-pointer in gfxFontEntry, and instead pass/track font family explicitly where needed.

Tryserver: https://tbpl.mozilla.org/?tree=Try&rev=284fe411a290.

Comment 7

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/mozilla-central/rev/1b6ab3a080d8

Comment 8

[John Daggett (:jtd)]

(In reply to Jonathan Kew (:jfkthame) from comment #4)

> @font-face { font-family: A; src(myfont.ttf); }
> @font-face { font-family: B; src(myfont.ttf); }
> @font-face { font-family: C; src(myfont.ttf); }
> 
> <p style="font-family: A">foo <span style="font-family: C">bar</span>
> 
> when the user asks for the fonts used in the range including "foo bar", is
> it important that we're able to identify the two specific rules that were
> used, even though we're now using the same font instance for the entire
> range?

It is actually important.  The font-specific feature values rule in CSS3 Fonts is defined per family and distinguishing between family A and B *is* important.  This change actually breaks the implementation for that on bug 549861, *sigh*.

I wish you had cc'd me on this bug...

Comment 9

[Jonathan Kew [:jfkthame]]

Sorry about that. (Do you not watch Layout:Text, to keep an eye on new bugs as they're filed?)

Given that the entry can no longer point back at its family (as that may not be unambiguous), the strategy here is to keep track of the pair <family,entry> during font matching, so that the family can also be passed to any code that needs to know it. So presumably you can pass it down to the shaper if necessary; or would it be better to resolve such rules at a slightly higher level, before actually calling in to the shaper itself?

Comment 10

[John Daggett (:jtd)]

Yeah, I looked at that but it looks like the <family, font> pair is only maintained for the font group and isn't maintained for pref/system fallback fonts.  I think the best thing might be to create some sort of ref counted family name ptr and stick it in the gfxFont (?!?).