975704 - "ASSERTION: font entry not found in family!" with always_use_cmaps, @font-face fallback

Status: RESOLVED WORKSFORME

(Core :: Graphics: Text, defect)

Description

[Jesse Ruderman]

Attachment: testcase (see comment 0)

With:
  user_pref("gfx.font_rendering.fallback.always_use_cmaps", true);
  user_pref("security.fileuri.strict_origin_policy", false);

###!!! ASSERTION: font entry not found in family!: 'i < numFonts', file gfxUserFontSet.h, line 125

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: remove incorrect assertion from ReplaceFontEntry.

This is reproducible with default prefs, too; it doesn't require the specific settings in comment 0. However, it's timing-sensitive; e.g. it doesn't generally fire if the testcase is loaded as part of restoring a saved session, but force-reloading will then usually hit it. Anyhow, I believe the assertion here is incorrect (and the condition it detects is harmless). When styles are being modified dynamically, we may remove a proxy from its family while it's in the process of loading; in that case, we just want to ignore it here.

Comment 3

[John Daggett (:jtd)]

Comment on attachment 8380681 [details] [diff] [review]
remove incorrect assertion from ReplaceFontEntry.

I don't think this is the right thing to change here. Why is that proxy font entry hanging around if the userfont set has already been rebuilt? At a minimum, the current code leaks the new font entry in this case so we need to address that but I suspect there's some other logic in the userfont set rebuild code that's not quite right.

Comment 4

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) from comment #3)
> Comment on attachment 8380681 [details] [diff] [review]
> remove incorrect assertion from ReplaceFontEntry.
> 
> I don't think this is the right thing to change here. Why is that proxy font
> entry hanging around if the userfont set has already been rebuilt? 

The proxy can be held alive across the font set rebuild by the nsFontFaceLoader that was kicked off to fetch one of the url sources. (We don't eagerly cancel loaders because it's quite common for pages to dynamically modify styles, triggering a font-set rebuild, yet end up using many of the same @font-face rules after the change. So we want any downloads that have been initiated to continue, not cancel and force them to restart.)

> At a
> minimum, the current code leaks the new font entry in this case so we need
> to address that

Yes, it looks like there's a possible leak; the problem is that gfxUserFontSet::LoadNext and gfxUserFontSet::LoadFont don't claim a strong reference to the entry that's created, and so if it doesn't get added to a family, its refcount never changes. Hitting this in a testcase would be quite timing-sensitive, so I'm not surprised we haven't noticed it, but we should fix anyway.

> but I suspect there's some other logic in the userfont set
> rebuild code that's not quite right.

An alternative "fix" would be to aggressively existing cancel loaders when we need to update the user font set, but that would mean that in the case where the same rules end up still being present after the update, we have to start over instead of benefiting from downloads that may already be well on their way.

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: patch 2 - plug potential font-entry leaks in gfxUserFontSet.

Comment 6

[Jonathan Kew [:jfkthame]]

Comment on attachment 8380681 [details] [diff] [review]
remove incorrect assertion from ReplaceFontEntry.

Resetting r? here, as I think this change is legitimate as per comment 4. The potential leak is a separate issue, fixed by the second patch.

Comment 7

[John Daggett (:jtd)]

Comment on attachment 8381288 [details] [diff] [review]
patch 2 - plug potential font-entry leaks in gfxUserFontSet.

Looks good. One of the reasons we didn't catch this earlier is the lack of explicit MOZ_COUNT_CTOR/DTOR counting for gfxFontEntry. r+ with the addition of those.

Comment 8

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) from comment #7)
> Comment on attachment 8381288 [details] [diff] [review]
> patch 2 - plug potential font-entry leaks in gfxUserFontSet.
> 
> Looks good. One of the reasons we didn't catch this earlier is the lack of
> explicit MOZ_COUNT_CTOR/DTOR counting for gfxFontEntry. r+ with the addition
> of those.

OK, that may help. But we still need the first patch as well, to remove the assertion...

Comment 9

[John Daggett (:jtd)]

(In reply to Jonathan Kew (:jfkthame) from comment #8)
> OK, that may help. But we still need the first patch as well, to remove the
> assertion...

I haven't overlooked that patch, I'd like to do some more testing here first.

Comment 10

[Jonathan Kew [:jfkthame]]

:jtd - review ping? (for the assertion removal)

Comment 11

[John Daggett (:jtd)]

Did some testing this week, need to do some more. I'm really uncomfortable with the notion that a proxy font could be associated with a family even after the font entries for that family have been cleared out. It just seems like we're leaving things in an inconsistent state that's hard to figure out when a bug occurs.  If we're going to have orphan proxy font entries it seems like we should be canceling the loads whenever they are orphaned.

Comment 12

[John Daggett (:jtd)]

Comment on attachment 8380681 [details] [diff] [review]
remove incorrect assertion from ReplaceFontEntry.

Clearing the review flag here as the structure of this code has changed with the landing of the patches on bug 1062058.

Comment 13

[Jonathan Kew [:jfkthame]]

Closing this as WFM; the code involved here has been substantially modified since this was filed, and the assertion no longer occurs.