1803406 - [macOS] Text rendering issues when users have multiple fonts which internally use the same name, and the OS chooses differently among them for different Firefox processes

Status: RESOLVED FIXED

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

Description

[Daniel Holbert [:dholbert]]

We've had several bug reports for a general category of issue, which I'm grouping under this bug.

bug 1771496 comment 39 and nearby comments have some STR. Essentially the setup is:

• Have several fonts that identify themselves as being the same font

In some configurations, our different processes seem to get back a different answer from the OS when we query for the font, though we expect the result to be deterministic / reliable. This can cause substantial problems if the font metrics disagree, as discussed in bug 1771496 comment 44.

(We don't yet fully understand what's going on here or why the OS seems to be returning different values, or what our options are to work around this, but hopefully we can address this somehow; because from a user perspective, it seems to manifest as a Firefox-specific bug.)

Comment 3

[Daniel Holbert [:dholbert]]

(Note RE the "fonts with the same name" setup here - the user-facing font name may differ, but the issue seems to happen when internal hidden fields in the font itself declare it to be the same. E.g. in bug 1771496 comment 45, we have a font that's called "Brady Bunch" but which internally has "Arial" and "ArialMT" in some of its name fields, and that naming [combined with other Arial/ArialMT fonts on the system] is what causes trouble here.)

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

The following field has been copied from a duplicate bug:

Field	Value	Source
Regressed by	bug 1696504	bug 1771496

For more information, please visit auto_nag documentation.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

:glandium, since you are the author of the regressor, bug 1696504, could you take a look?

For more information, please visit auto_nag documentation.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1696504

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1696504

Comment 11

[Jonathan Kew [:jfkthame]]

I've been looking into this, and think I may have a possible solution here.

Currently, on recent macOS versions the gfxPlatformMac::RegisterSupplementalFonts() function, which gives Firefox access to a bunch of fonts for additional language support, only performs the font registration in the parent process. This is enough to get them into the global font list, which is then shared with content processes, and the fonts appear to work fine.

However, what I'm seeing through local testing is that while the content process has no problem using these fonts, the fact that CTFontManagerRegisterFontsForURLs was not called in the content process in the same way it was in the parent means that the font databases in the processes are not identical. And this can result in inconsistent font lookup results even for fonts that weren't part of the "supplemental" collection, like locally-installed Arial and Helvetica versions.

So to ensure that the OS font manager database is consistent between parent and content processes, we need to go ahead and call CTFontManagerRegisterFontsForURLs for the supplemental fonts in all processes, not just in the parent.

Unfortunately, this comes at a considerable cost -- I'm seeing something up to around 300ms locally on my MacBook. So that will be a substantial cpstartup regression. It's less clear to me how user-perceivable it will be, as much of the time we presumably use a preallocated process when the user opens a new tab, in which case the regression may have gone unnoticed in the background. But it does seem likely that in some scenarios it'll be quite bad. :-(

Comment 12

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1803406 - Activate supplementary fonts in all processes, to ensure consistent font lists across macOS processes. r=#gfx-reviewers

Local testing indicates that if we don't do this, there's a risk that Core Text will resolve font names
differently in the content vs parent processes when duplicate fonts are installed (e.g. old versions of
Arial, Helvetica, etc that some users have "inherited" from ancient systems). The mismatched fonts used
for layout (in the content process) vs painting (by the parent) lead to the "garbled text" issue.

Unfortunately, this will impact content-process startup on macOS, so expect a perf regression report.

In my local testing, this appears to prevent the "garbled text" problem when an old version of Arial is
installed, or other similar font mismatch situations. There should be no user-visible behavior change
for systems with a "clean" font installation.

Comment 13

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1803406 - To try and mitigate the startup impact of font initialization, move more work to the RegisterFonts thread. r=#gfx-reviewers

No user-visible behavior change; just trying to move a bit more work onto a secondary thread,
in the hope of improving startup perf (althogh if there's too much contention for disk i/o,
or not enough CPU cores available, it may not help much).

Depends on D170286

Comment 14

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1803406 - Use CTFontManagerRegisterFontURLs if available to register fonts. r=#gfx-reviewers

This replaces CTFontManagerRegisterFontsForURLs, which is deprecated since macOS 10.15.

(No user-visible behavior change.)

Depends on D170287

Comment 15

[Jonathan Kew [:jfkthame]]

Setting perf? flag to get this on the team's radar, as the patch here will result in a substantial cpstartup hit on macOS (see comment 11), which I can't currently see how to avoid. It's unclear to me how user-visible this will be overall, thanks to preallocated content processes, but nevertheless it's obviously a concern.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3e5d60c6826a
Register supplemental macOS fonts in content processes, to maintain font-list consistency with parent. r=gfx-reviewers,lsalzman
https://hg.mozilla.org/integration/autoland/rev/30d9711bf362
To try and mitigate the startup impact of font initialization, move more work to the RegisterFonts thread. r=gfx-reviewers,lsalzman
https://hg.mozilla.org/integration/autoland/rev/d4cfac1ac9c6
Use CTFontManagerRegisterFontURLs if available to register fonts. r=gfx-reviewers,lsalzman

Comment 17

[Iulian Moraru]

Backed out for causing wd failures.

• Push with failures

• Failure log when it fails with webdriver.error.JavascriptErrorException: javascript error (500): JavaScriptError: Document was unloaded
• Failure log when it fails with test_subscribe - AssertionError

• Backout link

Comment 18

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:jfkthame, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit auto_nag documentation.

Comment 19

[Jonathan Kew [:jfkthame]]

I'd like to try re-landing this, but have not yet been able to reliable resolve the test issues that showed up.

Comment 20

[Bas Schouten (:bas.schouten)]

I don't think this qualifies as a perf impact issue itself as this cost is mostly hidden from the user. Thanks for letting us know though so we can keep an eye out!

Comment 22

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1803406 - Bump core-text dependency of wr_glyph_rasterizer to 20.1.0, and vendor the updated version. r=jrmuizel

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9c4a875ec67d
Bump core-text dependency of wr_glyph_rasterizer to 20.1.0, and vendor the updated version. r=jrmuizel,supply-chain-reviewers
https://hg.mozilla.org/integration/autoland/rev/edb458dfa8b8
Ensure consistent font lists across macOS processes, and use the kCTFontOptionsPreferSystemFont option when instantiating Core Text fonts. r=gfx-reviewers,lsalzman
https://hg.mozilla.org/integration/autoland/rev/91f12411aff1
To try and mitigate the startup impact of font initialization, move more work to the RegisterFonts thread. r=gfx-reviewers,lsalzman

Comment 24

[Atila Butkovits]

Backed out for causing WR bustages.

Backout link: https://hg.mozilla.org/integration/autoland/rev/ab3dfad46d1c0953e1c833dcf2b76d987bc50fd2

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=91f12411aff1b2863ca51b62b52d73addba95b71&selectedTaskRun=EhtKdZpERUKNZ7mwDlQsUQ.0

Failure log:
https://treeherder.mozilla.org/logviewer?job_id=430814147&repo=autoland&lineNumber=191
https://treeherder.mozilla.org/logviewer?job_id=430814168&repo=autoland

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/00ebd5f86930
Bump core-text dependency of wr_glyph_rasterizer to 20.1.0, and vendor the updated version. r=jrmuizel,supply-chain-reviewers
https://hg.mozilla.org/integration/autoland/rev/87d83c56fbe5
Ensure consistent font lists across macOS processes, and use the kCTFontOptionsPreferSystemFont option when instantiating Core Text fonts. r=gfx-reviewers,lsalzman
https://hg.mozilla.org/integration/autoland/rev/6a82f1f120aa
To try and mitigate the startup impact of font initialization, move more work to the RegisterFonts thread. r=gfx-reviewers,lsalzman

Comment 26

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/00ebd5f86930
https://hg.mozilla.org/mozilla-central/rev/87d83c56fbe5
https://hg.mozilla.org/mozilla-central/rev/6a82f1f120aa

Comment 27

[Jonathan Kew [:jfkthame]]

Given the problem shown in bug 1858869, I'm going to back out the changes here pending further investigation -- this is clearly still too fragile to be trusted. :-(

Comment 28

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1803406 - Backout 2 changesets (6a82f1f120aa, 87d83c56fbe5) due to regression reported in bug 1858869.

Comment 29

[Jonathan Kew [:jfkthame]]

Actually, to reduce supply-chain churn, I'll leave the core-text version bump (00ebd5f86930) in place, as by itself it should have no effect.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dc6c65169f6e
Backout 2 changesets (6a82f1f120aa, 87d83c56fbe5) due to regression reported in bug 1858869.

Comment 31

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/dc6c65169f6e

Comment 33

[Jonathan Kew [:jfkthame]]

Given the issues we saw in bug 1858869, it appears that passing kCTFontOptionsPreferSystemFont to Core Text does not do the "right thing" (for our purposes, at least.... the documentation is pretty sparse). So I've removed that, leaving just the original fix to make content processes do the same supplemental-font activation as the parent process.

Hoping this time it can land without causing new problems....!

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/48b489f34de2
Activate supplementary fonts in all processes, to ensure consistent font lists across macOS processes. r=gfx-reviewers,lsalzman
https://hg.mozilla.org/integration/autoland/rev/eaf61f658327
To try and mitigate the startup impact of font initialization, move more work to the RegisterFonts thread. r=gfx-reviewers,lsalzman

Comment 35

[Sandor Molnar[:smolnar]]

Backed out for causing OSX failures/leakchecks

Backout link: https://hg.mozilla.org/integration/autoland/rev/68721f0d6aec0facf9c8f271ea2bb2719348014a

Push with failures

Failure log -> TEST-UNEXPECTED-FAIL | leakcheck | tab 8 bytes leaked (nsStringBuffer)

Failure log -> TEST-UNEXPECTED-ERROR | startup_about_home_paint_cached | Could not find report in browser output: [('tsformat', ('__start_report', '__end_report')), ('tpformat', ('__start_tp_report', '__end_tp_report'))]

Comment 36

[Jonathan Kew [:jfkthame]]

Backed out for causing OSX failures/leakchecks

The leak here must be the static sSystemFontName string from the second patch here; it was supposed to be freed on shutdown, but I guess that didn't reliably happen. For now I'm going to try re-landing just the first (main) patch, and see if that sticks.

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/19d87a3a7a39
Activate supplementary fonts in all processes, to ensure consistent font lists across macOS processes. r=gfx-reviewers,lsalzman

Comment 38

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/19d87a3a7a39

Comment 39

[Ryan VanderMeulen [:RyanVM]]

Should we close this bug out and move any follow-up work to a new bug so it's easier to track across cycles?

Comment 40

[Jonathan Kew [:jfkthame]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #39)

Should we close this bug out and move any follow-up work to a new bug so it's easier to track across cycles?

Yes, that'd make sense once we're reasonably confident the patch that landed here is sticking this time.... I'd hoped to land D170287 as well but that needs some further testing, given its apparent tendency to leak a string. So we can take that to a followup (as well as any work on the test issues that have been reported).