Closed Bug 1245811 Opened 9 years ago Closed 9 years ago

fontconfig <prefer> aliases cause the original font to be missed altogether [was: Liberation Mono font is not picked up when using Firefox 44]

Categories

(Core :: Graphics: Text, defect)

Product:
Core
Component:
Graphics: Text
Platform:
Unspecified
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla48
Tracking Flags:
Tracking Status
firefox44 --- wontfix
firefox45 - wontfix
firefox46 + wontfix
firefox47 + fixed
firefox48 --- fixed

People

(Reporter: jtd, Assigned: jfkthame)

References

Details

(Keywords: regression, Whiteboard: [gfx-noted])

Attachments

(4 files, 2 obsolete files)

fonts.tar
150.00 KB, application/x-tar
Details
MozReview Request: Bug 1245811 - Make gfxPlatformFontList::FindFamily return a list of families, fixing fontconfig prioritization. r?karlt
58 bytes, text/x-review-board-request
karlt
: feedback+
Details
part 1 (based on patch by Andrew Comminos) - Replace gfxPlatformFontList::FindFamily with FindAndAddFamilies to allow for the possibility of the implementation returning multiple font families (e.g. when fontconfig has 'prefer' aliases)
26.44 KB, patch
karlt
: review+
ritu
: approval-mozilla-aurora+
Details | Diff | Splinter Review
part 2 (based on patch by Andrew Comminos) - Let gfxFcPlatformFontList return multiple families for a given name once fontconfig substitutions have been applied
6.38 KB, patch
karlt
: review+
ritu
: approval-mozilla-aurora+
Details | Diff | Splinter Review
From bug 1119062, comment 32: https://bugzilla.mozilla.org/show_bug.cgi?id=1119062#c32 > Seems that after release 44 some font families are not matched correctly in Linux platforms. > > For example "Liberation Mono" seems to be ignored even if is installed correctly in the system: > > $ fc-match 'Liberation Mono' > LiberationMono-Regular.ttf: "Liberation Mono" "Regular" > > This didn't happen in previous releases. Is it related to unicode-range? The URL below shows DejaVu Serif: data:text/html,<body style="font-family: Liberation Mono,serif">burp > Arch Linux here, Linux x86_64 > > fontconfig version: 2.11.1-2 > fontconfig configuration: system provided/untouched > > I noticed "Liberation Mono" stopping working after upgrade from 43 to 44
Michel, could you attach the output of the command below on your system: fc-list -v "Liberation Mono" The '-v' option will dump all the gory details of the font. I'm puzzled why you would see this and I don't, using a very similar version of fontconfig. Also, what do you see when you dump out: fc-match monospace Thanks!
Flags: needinfo?(michelesr)
FWIW, I'm guessing it's more likely this was triggered by bug 1180560 than bug 1119062.
> Michel, could you attach the output of the command below on your system: > fc-list -v "Liberation Mono" http://sprunge.us/KfWi > Also, what do you see when you dump out: > fc-match monospace DejaVuSansMono.ttf: "DejaVu Sans Mono" "Book"
Flags: needinfo?(michelesr)
After deleting all the entries in /etc/fonts/conf.d/ the "Liberation Mono" font-family is matched correctly. Seems like my default fontconfig configuration is messed up. I'll open a thread in Arch Linux forum.
However still I don't understand why this issue raised up with FF44.
(In reply to michelesr from comment #5) > However still I don't understand why this issue raised up with FF44. FF44 radically changed how font lookup/matching is implemented, and apparently the new implementation doesn't work well with something about that Arch Linux configuration. I think we should try to figure out why, to understand whether this is likely to impact other fonts/distros/whatever or if it's an isolated case. Did you keep a copy of the fontconfig entries you deleted (I hope so!)? Could you attach here so we can take a look at what's in there.... thanks!
Attached file fonts.tar —
> Did you keep a copy of the fontconfig entries you deleted (I hope so!)? > Could you attach here so we can take a look at what's in there.... thanks! The tarball contains my /ect/fonts/ directory. All the entries in conf.d/ are symlinks from conf.avail/
Whiteboard: [gfx-noted]
Hmm... I don't know much about fontconfig setup, but if you remove only the file 10-powerline-symbols.conf from /etc/fonts/conf.d/, does that resolve the issue for you?
Flags: needinfo?(michelesr)
>if you remove only the file 10-powerline-symbols.conf from /etc/fonts/conf.d/, does that resolve the issue for you? Yes, it does. Thank You!
Flags: needinfo?(michelesr)
OK, it sounds to me like the new code is mishandling fontconfig aliases. That 10-powerline-symbols.conf file includes entries like <alias> <family>Liberation Mono</family> <prefer><family>PowerlineSymbols</family></prefer> </alias> which according to the fontconfig manual, should prepend the "prefer" family (or families: there could be a list) before the specified one (i.e. "Liberation Mono" becomes, in effect, "PowerlineSymbols, Liberation Mono"); but apparently it is replacing it altogether and dropping the originally-specified name.
Flags: needinfo?(jd.bugzilla)
Confirmed affecting Fedora in a similar way. Installing Open Sans and setting that as the interface font will instead show Comfortaa. Removing /etc/fonts/conf.d/60p-opens-sans resolves the issue.
Keywords: regression
Summary: Liberation Mono font is not picked up when using Firefox 44 → fontconfig <prefer> aliases cause the original font to be missed altogether [was: Liberation Mono font is not picked up when using Firefox 44]
[Tracking Requested - why for this release]: It looks like this breaks the usage of some common font families on major Linux distros, because they are effectively "hidden" by fontconfig alias entries.
status-firefox44: --- → affected
status-firefox45: --- → affected
status-firefox46: --- → affected
tracking-firefox45: --- → ?
tracking-firefox46: --- → ?
tracking-firefox47: --- → ?
Adding ni? for karlt to bring this to his attention, as he knows much more about fontconfig in general.
Flags: needinfo?(karlt)
Blocks: 1056479
The FindFamily method only returns a single font family. In the case of gfxFcPlatformFontList::FindFamily it returns the first family found after doing substitutions. The solution to the problem here is to append all families found before the sentinel name in the fontlist. This will respect all <accept> and <prepend> aliases but it won't handle <default> settings. Handling those in a performant manner is going to be tricky, as the list of names grows very long. Under Ubuntu 14.04 the list of names appended after the sentinel by FcConfigSubstitute runs past 100 entries.
Flags: needinfo?(jd.bugzilla)
Thanks for ni. The approach in comment 15 sounds sensible. I expect <accept> and <prepend> won't be too costly. Handling <default> would require quite a different approach, one that considers all families at once, but <default> is not the problem here.
Flags: needinfo?(karlt)
Tracking because it is a regression from a tier-1 OS. Karl, John, do you know who could help with this? Thanks
status-firefox44: affected → wontfix
tracking-firefox45: ? → +
tracking-firefox46: ? → +
tracking-firefox47: ? → +
Flags: needinfo?(karlt)
Flags: needinfo?(jd.bugzilla)
(In reply to Sylvestre Ledru [:sylvestre] from comment #17) > Tracking because it is a regression from a tier-1 OS. > > Karl, John, do you know who could help with this? Thanks Yeah, I'm working on patch for this.
Flags: needinfo?(jd.bugzilla)
Jonathan, is this something you can take on?
Assignee: nobody → jfkthame
Flags: needinfo?(jfkthame)
(In reply to Milan Sreckovic [:milan] from comment #19) > Jonathan, is this something you can take on? In principle, but how urgently? I don't really have the bandwidth to dig into this until we've stabilized the DPI-support stuff (bug 890156 and its regressions) a bit further....
Flags: needinfo?(jfkthame)
Thanks Jonathan. Good point, those are important bugs. They appear to be 46 and later, from what I can tell, right? Where this bug is tracked for 45, as a regression in 44. If the fix for this is difficult and would take too long to fit into 45 even if prioritized, then we should mark it wontfix for 45. What about mentoring somebody to a solution, is this a complicated thing?
Flags: needinfo?(jfkthame)
AIUI, comment 15 gives the general outline of what we should do. It sounds straightforward enough, but I suspect that the need to cache stuff (to avoid regressing performance) may make it a bit more complex than it seems at first glance. We currently cache a single fontconfig substitution for a given family name, but it looks like that mechanism won't be sufficient any longer. I guess jdaggett's patch (comment 18) didn't come to fruition; John, do you have some work-in-progress you could attach as a basis for someone else to finish, even if it's not quite ready at this point? Otherwise, the best person to fix this (if he has spare time, which he probably doesn't!), or to mentor someone, would probably be :karlt, as he's the most familiar of us with fontconfig issues, I believe.
Flags: needinfo?(jfkthame) → needinfo?(jd.bugzilla)
[Tracking Requested - why for this release]: Sounds like a wontfix for 45, we just can't fit something this complex. Otherwise, Andrew, if you have some time, take a look and maybe we can get somewhere.
tracking-firefox45: + → ?
Flags: needinfo?(andrew)
Sure, I can give this a shot- the solution in comment 15 looks good to me.
Assignee: jfkthame → andrew
Status: NEW → ASSIGNED
Flags: needinfo?(andrew)
Flags: needinfo?(karlt)
I don't think MozReview supports feedback? yet, so I just sent the patch as a review request; when you get a chance, it would be great if you could over this Karl. The patch just records the list of families encountered until reaching the sentinel, keeping with the trend of FindFamily performing substitution behaviour. Thanks!
(In reply to Andrew Comminos [:acomminos] from comment #25) > Created attachment 8723958 [details] > MozReview Request: Bug 1245811 - Make gfxPlatformFontList::FindFamily return > a list of families, fixing fontconfig prioritization. r?karlt > > Review commit: https://reviewboard.mozilla.org/r/36803/diff/#index_header > See other reviews: https://reviewboard.mozilla.org/r/36803/ I haven't tested this but the approach looks fine. This needs actual testing in practice because the gotcha here is that if any .conf file appends or prepends a generic into the list, you will end up with the kitchen sink in the fontlist (i.e. 100 families in the list). If this happens a lot, it may be necessary to rework the code in gfxFontGroup::BuildFontList to do the family lookups more lazily this many loookups will almost never be needed in practice.
Flags: needinfo?(jd.bugzilla)
(In reply to John Daggett (:jtd) from comment #27) > This needs actual testing > in practice because the gotcha here is that if any .conf file appends or > prepends a generic into the list, you will end up with the kitchen sink in > the fontlist (i.e. 100 families in the list). I don't expect that in normal configurations, but, yes, it is possible. Perhaps terminate at the first generic. That may also help with the problem we have is that it is difficult to distinguish strong and weak families. It's hard to guess which heuristics will work best here, but I think this would be reasonable.
Comment on attachment 8723958 [details] MozReview Request: Bug 1245811 - Make gfxPlatformFontList::FindFamily return a list of families, fixing fontconfig prioritization. r?karlt https://reviewboard.mozilla.org/r/36803/#review34283 Yes, I think this is the logical approach. A few nsTArray<gfxFontFamily*> declations can be changed to use AutoTArray to save on memory allocations. ::: gfx/thebes/gfxFcPlatformFontList.h:286 (Diff revision 1) > - nsRefPtrHashtable<nsCStringHashKey, gfxFontFamily> mFcSubstituteCache; > + nsClassHashtable<nsCStringHashKey, nsTArray<gfxFontFamily*>> mFcSubstituteCache; This is changing a strong gfxFontFamily pointer to weak pointers, so please document what keeps the gfxFontFamily alive and what clears these pointers when they become invalid. I expect nsDataHashtable should work here, and save a memory allocation for each entry. I assume nsBaseHashtableET is inheriting ALLOW_MEMMOVE = true, but I expect nsTArray is fine with memmove. AutoTArray would not be. ::: gfx/thebes/gfxFcPlatformFontList.cpp:1212 (Diff revision 1) > - return (*prefFonts)[0]; > + aOutput.AppendElement((*prefFonts)[0]); I suspect we should rethink the situation with generics, but that's probably better handled separately anyway. This is fine for this patch. ::: gfx/thebes/gfxFcPlatformFontList.cpp:1234 (Diff revision 1) > - gfxFontFamily* cached = mFcSubstituteCache.GetWeak(familyToFind); > - if (cached) { > - return cached; > + > + // Create a new cache entry for the given family, or fetch cached font families. > + auto families = mFcSubstituteCache.LookupOrAdd(familyToFind); > + if (families->Length() > 0) { > + aOutput.AppendElements(*families); > + return; Ideally, this lookup would know whether no families were found the previous time, and skip looking again. As much as I like LookupOrAdd(), bool Get(KeyType aKey, UserDataType* aData) const may suit us better here. This is not something the previous code provided, but moving away from Get()/Put() may be moving this code in the wrong direction. ::: gfx/thebes/gfxPlatformFontList.h:132 (Diff revision 1) > - virtual gfxFontFamily* > - FindFamily(const nsAString& aFamily, gfxFontStyle* aStyle = nullptr, > - gfxFloat aDevToCssSize = 1.0); > + virtual void > + FindFamily(const nsAString& aFamily, nsTArray<gfxFontFamily*>& aOutput, > + gfxFontStyle* aStyle = nullptr, gfxFloat aDevToCssSize = 1.0); Please use pointers rather than references to parameter-list objects that may be modified by the method. Please document that results are appended to aOutput. Perhaps rename the function or variable name to indicate this. Perhaps FindAndAddFamily(, aAccumulator). ::: gfx/thebes/gfxPlatformFontList.cpp:690 (Diff revision 1) > - gfxFontFamily *familyEntry = FindFamily(aFamily); > + gfxFontFamily* family = FindFamilyByCanonicalName(aFamily); Please document the canonical name requirement of FindFontForFamily(). ::: gfx/thebes/gfxPlatformFontList.cpp:743 (Diff revision 1) > - gfxFontFamily *ff = FindFamily(aFontName); > - if (!ff) { > + > + gfxFontFamily* family = FindFamilyByCanonicalName(aFontName); I don't think this change is right. GetStandardFamilyName() is trying to lookup from non-standard names. ("standard" is not necessarily "canonical".) ::: gfx/thebes/gfxPlatformFontList.cpp:827 (Diff revision 1) > - if (family) { > - bool notFound = true; > - for (const gfxFontFamily* f : *aGenericFamilies) { > + for (gfxFontFamily* f : families) { > + if (!aGenericFamilies->Contains(f)) { > + aGenericFamilies->AppendElement(f); Indentation. ::: gfx/thebes/gfxPlatformFontList.cpp:1438 (Diff revision 1) > if (mOtherNamesMissed) { > for (auto it = mOtherNamesMissed->Iter(); !it.Done(); it.Next()) { > - if (FindFamily(it.Get()->GetKey())) { > + if (FindFamilyByCanonicalName(it.Get()->GetKey())) { Looks like mOtherNamesMissed are not canonical.
Attachment #8723958 - Flags: review?(karlt)
Too late for 45, however, if you think it is a critical issue, we could take a patch for 45.1esr
status-firefox45: affected → wontfix
tracking-firefox45: ? → -
See Also: → 1254245
Andrew, are you continuing to work on this bug? If you don't have time to tackle it at the moment, I might try to look into it further. But if you have an update at all, that would be awesome - thanks!
Flags: needinfo?(andrew)
Thanks, Jonathan; I probably won't have a chance to work on shipping this until later this month due to exams. If you want to hack away on it, that would be great!
Flags: needinfo?(andrew)
https://reviewboard.mozilla.org/r/36803/#review41555 ::: gfx/thebes/gfxFcPlatformFontList.cpp:1212 (Diff revision 1) > // fontconfig generics? use fontconfig to determine the family for lang > if (isDeprecatedGeneric || > mozilla::FontFamilyName::Convert(familyName).IsGeneric()) { > PrefFontList* prefFonts = FindGenericFamilies(familyName, language); > if (prefFonts && !prefFonts->IsEmpty()) { > - return (*prefFonts)[0]; > + aOutput.AppendElement((*prefFonts)[0]); I think we actually want to copy all entries from prefFonts[] here, not only the first. With that change, this patch will resolve bug 1254245, whereas as it stands, it fails to fix that case.
Blocks: 1254245
I've split this into two parts: this first patch introduces the new FindAndAddFamilies method on gfxPlatformFontList, and updates the various implementations accordingly; it should cause no change in actual behavior on any platform, I believe. Then the second patch will update the gfxFcPlatformFontList implementation such that it actually returns multiple results from fontconfig substitutions.
Attachment #8739688 - Flags: review?(karlt)
Assignee: andrew → jfkthame
Comment on attachment 8739689 [details] [diff] [review] part 2 (based on patch by Andrew Comminos) - Let gfxFcPlatformFontList return multiple families for a given name once fontconfig substitutions have been applied Thanks, for taking this Jonathan. The parts that should be in this patch are good, but marking r- just based on something in a part that should be in the other patch. I don't know why we seem to have two different paths for looking up generics, nor what the difference is, but that is orthogonal to changes here. > gfxFcPlatformFontList::~gfxFcPlatformFontList() > { >+ // Clear the substitute cache, in case there's ever an attempt to do a >+ // family lookup during the font list tear-down process (seems unlikely!), >+ // as its gfxFontFamily pointers will become invalid when the font list >+ // is cleared. >+ mFcSubstituteCache.Clear(); Please remove this. It is not necessary because the destructor is only called when nothing is using the gfxFcPlatformFontList. Cancelling the updates timer won't cause a look up, I assume, and mFcSubstituteCache won't exist after that point. >+ nsTArray<gfxFontFamily*> cachedFamilies; >+ if (mFcSubstituteCache.Get(familyToFind, &cachedFamilies)) { AutoTArray, which is an nsTArray. > NS_ConvertUTF8toUTF16 mappedGenericName(ToCharPtr(mappedGeneric)); >- gfxFontFamily* genericFamily = >- gfxPlatformFontList::FindFamily(mappedGenericName); >- if (genericFamily && !prefFonts->Contains(genericFamily)) { >- prefFonts->AppendElement(genericFamily); >+ AutoTArray<gfxFontFamily*,10> genericFamilies; >+ gfxPlatformFontList::FindAndAddFamilies(mappedGenericName, >+ &genericFamilies); >+ if (!genericFamilies.IsEmpty()) { >+ prefFonts->AppendElements(genericFamilies); This should be in the other patch, right? The !prefFonts->Contains(genericFamily) was important because this is looking through a list of fonts and so it is likely that several consecutive fonts will be in the same family. gfxPlatformFontList::FindAndAddFamilies() will only ever return one family, and so AutoTArray<gfxFontFamily*,1> is fine. Knowing that makes it easier to keep the !prefFonts->Contains(genericFamily). I think it would make life easier for us if the name of the method for finding families by real name differed from that of the method for finding families by alias, but that is an existing issue. For now, assuming one family (and maybe asserting) is fine. (Adding random fonts here regardless of language is wrong, but that is an existing bug.)
Attachment #8739689 - Flags: review?(karlt) → review-
Comment on attachment 8739688 [details] [diff] [review] part 1 (based on patch by Andrew Comminos) - Replace gfxPlatformFontList::FindFamily with FindAndAddFamilies to allow for the possibility of the implementation returning multiple font families (e.g. when fontconfig has 'prefer' aliases) >+ FindAndAddFamilies(const nsAString& aFamily, >+ nsTArray<gfxFontFamily*>* aOutput, Indentation. In other definitions/declarations/overrides/call-sites also. Otherwise this is good, except for missing the change in FindGenericFamilies. (If we had different method names for finding families from aliases, that would have been more obvious.)
Attachment #8739688 - Flags: review?(karlt)
Attachment #8739688 - Attachment is obsolete: true
Attachment #8739689 - Attachment is obsolete: true
Attachment #8739875 - Flags: review?(karlt) → review+
Attachment #8739876 - Flags: review?(karlt) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d8ed605fdd500fe3c613ac7a7df6968d640d4a0 Bug 1245811 - part 1 (based on patch by Andrew Comminos) - Replace gfxPlatformFontList::FindFamily with FindAndAddFamilies to allow for the possibility of the implementation returning multiple font families (e.g. when fontconfig has 'prefer' aliases). r=karlt https://hg.mozilla.org/integration/mozilla-inbound/rev/7b9f67c46b91da21612c2837b6aca7358533a1ac Bug 1245811 - part 2 (based on patch by Andrew Comminos) - Let gfxFcPlatformFontList return multiple families for a given name once fontconfig substitutions have been applied. r=karlt
https://hg.mozilla.org/mozilla-central/rev/0d8ed605fdd5 https://hg.mozilla.org/mozilla-central/rev/7b9f67c46b91
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment on attachment 8739875 [details] [diff] [review] part 1 (based on patch by Andrew Comminos) - Replace gfxPlatformFontList::FindFamily with FindAndAddFamilies to allow for the possibility of the implementation returning multiple font families (e.g. when fontconfig has 'prefer' aliases) Approval Request Comment [Feature/regressing bug #]: 1180560 [User impact if declined]: Depending on fontconfig setup, wrong fonts may be used (incorrectly ignoring page-specified and/or user-preference fonts) on desktop Linux systems. [Describe test coverage new/current, TreeHerder]: Tested manually (depends on specific fontconfig configurations); fix on Nightly confirmed by reporter in bug 1254245. Existing reftests etc confirm that simple font use is unaffected. [Risks and why]: Pretty low - although cross-platform code is touched here (to extend internal APIs), there should be no impact on non-Linux platforms; only the fontconfig backend actually takes advantage of the extended behavior. [String/UUID change made/needed]: none
Attachment #8739875 - Flags: approval-mozilla-aurora?
Attachment #8739876 - Flags: approval-mozilla-aurora?
Comment on attachment 8739875 [details] [diff] [review] part 1 (based on patch by Andrew Comminos) - Replace gfxPlatformFontList::FindFamily with FindAndAddFamilies to allow for the possibility of the implementation returning multiple font families (e.g. when fontconfig has 'prefer' aliases) Regression since 44 (on linux only), fix was verified on Nightly, Aurora47+
Attachment #8739875 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8739876 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
No longer blocks: 1254245
See Also: 1254245 →
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: