Closed Bug 1174946 Opened 4 years ago Closed 4 years ago

fontlist causes incorrect font selection for GitHub monospaced font

Categories

(Core :: Graphics: Text, defect)

41 Branch
All
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: nrc, Assigned: jtd)

References

()

Details

(Keywords: perf, regression, Whiteboard: [talos_regression])

Attachments

(1 file)

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.
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
Assignee: nobody → jdaggett
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.
Keywords: regression
Version: unspecified → 41 Branch
Locally backing out 0c35e99b2167 (the second patch from bug 1165693) did *not* fix this regression for me.
(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?
Flags: needinfo?(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.
Blocks: 1165693
Flags: needinfo?(mbrubeck)
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?
Flags: needinfo?(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.
Flags: needinfo?(mbrubeck)
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.
Attachment #8623213 - Flags: review?(jdaggett)
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.
Attachment #8623213 - Flags: review?(jdaggett)
(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.
Attachment #8623213 - Flags: review+
Landed on mozilla-central this morning but didn't got marked as such.
https://hg.mozilla.org/mozilla-central/rev/5081a6d1bd38
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
oh, this introduced the performance regression that we backed out a fix for.  At least our system is working :)
Blocks: 1165349
Keywords: perf
Whiteboard: [talos_regression]
Duplicate of this bug: 1173826
You need to log in before you can comment on or make changes to this bug.