1174946 - fontlist causes incorrect font selection for GitHub monospaced font

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Nick Cameron [:nrc]]

Using GitHub, the css font family is "Consolas,"Liberation Mono",Menlo,Courier,monospace". On my system (Ubuntu 15.4) I have Liberation Mono but not Consolas. I would therefore expect to render using Liberation Mono, but Deja Vu Sans is used. Setting gfx.font_rendering.fontconfig.fontlist.enabled to false fixes this bug (rendered using Liberation Mono).

This happens on the current nightly. It is a recent regression, but I'm not sure when the last nightly that worked was because I upgrade semi-regularly.

Comment 1

[John Daggett (:jtd)]

Steps to reproduce:

1. Run latest Nightly
2. Open URL https://github.com/bramstein/fontloader/blob/master/Gruntfile.js

Result: code appears in DejaVu Sans
Expected result: code appears in monospace font

Comment 2

[Jonathan Kew [:jfkthame]]

I'm suspicious that my (second) caching patch from bug 1165693 may be causing this, though I'm not sure why offhand. I'd suggest backing it out to see if that fixes this problem.

Comment 3

[Matt Brubeck (:mbrubeck)]

Locally backing out 0c35e99b2167 (the second patch from bug 1165693) did *not* fix this regression for me.

Comment 4

[Jonathan Kew [:jfkthame]]

(In reply to Matt Brubeck (:mbrubeck) from comment #3)
> Locally backing out 0c35e99b2167 (the second patch from bug 1165693) did
> *not* fix this regression for me.

Interesting; that's the one I thought might be riskier. How about if you back out the first patch there as well?

Comment 5

[Matt Brubeck (:mbrubeck)]

(In reply to Jonathan Kew (:jfkthame) from comment #4)
> (In reply to Matt Brubeck (:mbrubeck) from comment #3)
> > Locally backing out 0c35e99b2167 (the second patch from bug 1165693) did
> > *not* fix this regression for me.
> 
> Interesting; that's the one I thought might be riskier. How about if you
> back out the first patch there as well?

Yes, backing out both patches in bug 1165693 fixes this regression.

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: tentative patch, back out the first patch from bug 1165693

Interesting. Thanks, Matt.

Could you try one more thing, please: this patch in effect backs out the first part of bug 1165693, while leaving the second patch in place. (Untested, as I'm on the wrong system, but I think it should build!) Would you mind testing this and see whether the issue persists?

Comment 7

[Matt Brubeck (:mbrubeck)]

(In reply to Jonathan Kew (:jfkthame) from comment #6)
> Could you try one more thing, please: this patch in effect backs out the
> first part of bug 1165693, while leaving the second patch in place.

Yes, this patch alone also fixes the regression.

Comment 8

[Jonathan Kew [:jfkthame]]

Comment on attachment 8623213 [details] [diff] [review]
tentative patch, back out the first patch from bug 1165693

Sounds like this may be the simple solution here. It should have relatively little effect on perf, now that the mFcSubstituteCache is in place.

Comment 9

[John Daggett (:jtd)]

The concept of the original optimization is good but the execution is off. The code block is creating a pattern, performing substitutions on it, then setting sSentinelFirstFamily to point to a string in the pattern. Once the pattern goes out of scope, the pointer is dangling and so subsequent comparisons always fail, leading to DejaVu Sans getting returned.

Solution is to save the pattern in a static so the pointer doesn't dangle or just cache an FcChar array instead.

Comment 10

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) from comment #9)
> The concept of the original optimization is good but the execution is off.
> The code block is creating a pattern, performing substitutions on it, then
> setting sSentinelFirstFamily to point to a string in the pattern. Once the
> pattern goes out of scope, the pointer is dangling and so subsequent
> comparisons always fail, leading to DejaVu Sans getting returned.

Ah, of course... that makes sense. Duh.

> Solution is to save the pattern in a static so the pointer doesn't dangle
> or just cache an FcChar array instead.

Yes, that should work; though I wonder if it's worth it, given that the mFcSubstituteCache will preempt most calls to this anyway. So I'd be inclined to suggest taking the patch here, and see if the Tp5o needle actually moves.

Comment 11

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5081a6d1bd38

Comment 12

[Guilherme Lima]

Landed on mozilla-central this morning but didn't got marked as such.
https://hg.mozilla.org/mozilla-central/rev/5081a6d1bd38

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

oh, this introduced the performance regression that we backed out a fix for.  At least our system is working :)