1478716 - Tab crashes when attempting to inspect variable fonts on Linux

Status: VERIFIED FIXED

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

Description

[Martin Balfanz [:mbalfanz]]

Crashreport: https://crash-stats.mozilla.com/report/index/9ee6ebf1-b70f-4772-8e20-cca6c0180726

Running the latest Kubuntu and Nightly.

STR:
- visit https://typetools.typenetwork.com/
- right click on any of the variable fonts sample texts
- select "inspect element"
- open the fonts editor in devtools

ER: devtools should be open with the font editor

AR: tab crashes

Comment 1

[Jonathan Kew [:jfkthame]]

Interesting, this doesn't seem to crash for me. But it's possible I have a different FreeType version than you, which might be a factor; can you determine what is installed on your machine? (I'm on FT 2.9.1 here.)

Comment 2

[Jonathan Kew [:jfkthame]]

I'm guessing the problem may be a null 'name' field in the axis information returned by freetype; if so, just adding a null check there may be all we need to do. Here's a try build with a check added; if you could confirm whether this avoids the crash, that'd be great - thanks!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1abb18d55b77725c99341b2e97ad859ccf224f6&selectedJob=190309864

Comment 3

[Martin Balfanz [:mbalfanz]]

Unfortunately that didn't help.  Here is the new crashreport: https://crash-stats.mozilla.com/report/index/390da04f-48e8-416e-af45-1df501180726

I'm on FT version is 2.8.1

Comment 4

[Jonathan Kew [:jfkthame]]

OK, thanks. Guess I'll try downgrading my freetype and see if it reproduces for me then.

Comment 5

[Jonathan Kew [:jfkthame]]

Frustratingly, it refuses to crash for me even after removing ft 2.9.1 and installing 2.8.1 instead.

Lee, can you see if you can reproduce this crasher?

Martin, any chance you can try a local debug build under gdb and see if we can get more details from there?

Comment 6

[Martin Balfanz [:mbalfanz]]

Attachment: gdb-output.txt

Please see the file attached.  If that's not what you were looking for I will need a bit more guidance :-)

Comment 7

[Jonathan Kew [:jfkthame]]

Thanks; but unfortunately that build seems to be lacking debug symbols (see the message "Reading symbols from /home/martin/code/mozilla-unified/obj-x86_64-pc-linux-gnu/dist/bin/firefox...(no debugging symbols found)...done." at the top of the file). I'm not sure offhand why that is....what does your mozconfig look like? How exactly are you building?

Comment 8

[Jonathan Kew [:jfkthame]]

OK, I can reproduce the crash here after installing ubuntu 18.04, which comes with libfreetype6-2.8.1-2ubuntu2. It seems like the FT_MM_Var record we're getting back from freetype is trashed in some way.

But if I install freetype 2.8.1 locally from a freetype tarball, and use that instead, the crash doesn't occur. (Nor does it occur if I update to freetype 2.9.)

Not sure yet exactly what's going wrong with the ubuntu-packaged freetype....

Comment 9

[Lee Salzman [:lsalzman]]

Doesn't seem to reproduce for me on Debian with 2.8.1 installed.

Comment 10

[Jonathan Kew [:jfkthame]]

OK, I think I know what's happening. I believe we're running into a problem because of freetype bug https://savannah.nongnu.org/bugs/?52955, which I believe was introduced just after the 2.8.1 release by the changes in http://git.savannah.gnu.org/cgit/freetype/freetype2.git/commit/?id=3b3cb32dd2340d86d3165961a4bb3dbd44353075 and http://git.savannah.gnu.org/cgit/freetype/freetype2.git/commit/?id=b19cdc9c8164cd81801024b024ce132bcb4a29f1 (and was then reported/fixed in January 2018 in time for 2.9.1). This bug means that freetype returns garbage (with invalid string pointers, among other things) in the FT_MM_Var record when called a second time on a given FT_Face.

Unfortunately, the ubuntu package for freetype-2.8.1 has cherry-picked those two post-2.8.1 commits, and so suffers from the bug.

I'm in the process of testing a patch to work around this (by ensuring we only call FT_Get_MM_Var once for a given font face).

Comment 11

[Jonathan Kew [:jfkthame]]

Martin, could you try the build from https://treeherder.mozilla.org/#/jobs?repo=try&revision=948a85c8d43ebc7c89f0f92c6c8e6398edf6568c and see if this resolves the crash for you? Thanks!

Comment 12

[Martin Balfanz [:mbalfanz]]

That solves the issue for me, great work!

Comment 13

[Jonathan Kew [:jfkthame]]

Attachment: Ensure we only call FT_Get_MM_Var once per face (and cache the result in the font entry), to avoid being bitten by freetype bug 52955 on Ubuntu

The code in gfxFontconfigFontEntry was already doing this in some cases, but unfortunately if gfxFT2FontBase::SetupVarCoords had separately also used the font's FT_MM_Var record, we'd still have problems. So we need to make sure gfxFT2FontBase::SetupVarCoords also relies on the cached record from the font entry, rather than calling FT_Get_MM_Var for itself.

Comment 14

[Lee Salzman [:lsalzman]]

Comment on attachment 8996628 [details] [diff] [review]
Ensure we only call FT_Get_MM_Var once per face (and cache the result in the font entry), to avoid being bitten by freetype bug 52955 on Ubuntu

It is worth noting we will also probably hit this in Moz2D. :(

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aee91e7da807
Ensure we only call FT_Get_MM_Var once per face (and cache the result in the font entry), to avoid being bitten by freetype bug 52955 on Ubuntu. r=lsalzman

Comment 16

[Noemi Erli[:noemi_erli]]

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

Comment 17

[Jonathan Kew [:jfkthame]]

Comment on attachment 8996628 [details] [diff] [review]
Ensure we only call FT_Get_MM_Var once per face (and cache the result in the font entry), to avoid being bitten by freetype bug 52955 on Ubuntu

Although AIUI the new font editor is preffed-off by default in Fx62, the fact that we're shipping variable font support means adventurous users may well experiment with it and run into this crash. So I think we should uplift the fix to beta rather than shipping such a poor experience.

Approval Request Comment
[Feature/Bug causing the regression]: variable fonts + DevTools font editor (when platform FreeType has a specific bug)

[User impact if declined]: crash when opening DevTools font editor

[Is this code covered by automated tests?]: no (variable fonts not supported on CI infrastructure, need updated OS versions)

[Has the fix been verified in Nightly?]: yes

[Needs manual test from QE? If yes, steps to reproduce]: manual testing appreciated; STR in comment 0

[List of other uplifts needed for the feature/fix]: none

[Is the change risky?]: not really

[Why is the change risky/not risky?]: Logically a simple change: instead of calling FT_Get_MM_Var when needed (which is broken for subsequent calls due to a FreeType bug), we call it only once per font entry, and cache the result for re-use. So there is no behavior change, except for avoiding the buggy repeated calls. (Also, patch only affects Linux & Android, NPOTB on Win/Mac.)

[String changes made/needed]: none

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

Marking affected for 62 though we are shipping with this off by default (so that the request will show up in sheriff queries)

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8996628 [details] [diff] [review]
Ensure we only call FT_Get_MM_Var once per face (and cache the result in the font entry), to avoid being bitten by freetype bug 52955 on Ubuntu

Fix for crash with variable fonts. Let's uplift for beta 15.

Comment 20

[Dorel Luca [:dluca]]

https://hg.mozilla.org/releases/mozilla-beta/rev/eaa3a2a13a5d

Comment 21

[Oana Botisan, Desktop QA]

I managed to reproduce the issue using an older version of Nightly (2018-07-26) on Kubuntu 18.04. I opened the Fonts editor and the tab crashed.
I retested everything using latest Nightly 63.0a1 and Beta 62.0b15 and the bug is not reproducing anymore.