Closed Bug 1478716 Opened 4 years ago Closed 4 years ago
Tab crashes when attempting to inspect variable fonts on Linux
45.70 KB, text/plain
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
8.42 KB, patch
|Details | Diff | Splinter Review|
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
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.)
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
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
OK, thanks. Guess I'll try downgrading my freetype and see if it reproduces for me then.
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?
Please see the file attached. If that's not what you were looking for I will need a bit more guidance :-)
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?
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....
Doesn't seem to reproduce for me on Debian with 2.8.1 installed.
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).
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!
That solves the issue for me, great work!
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.
Attachment #8996628 - Flags: review?(lsalzman)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
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. :(
Attachment #8996628 - Flags: review?(lsalzman) → review+
Pushed by email@example.com: 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 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
Attachment #8996628 - Flags: approval-mozilla-beta?
Marking affected for 62 though we are shipping with this off by default (so that the request will show up in sheriff queries)
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.
Attachment #8996628 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
You need to log in before you can comment on or make changes to this bug.