1450209 - Crash when opening some variable-fonts demo pages [including https://developer.microsoft.com/en-us/microsoft-edge/testdrive/demos/variable-fonts/, https://v-fonts.com/]

Status: RESOLVED FIXED

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

Description

[laszlo.bialis]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180329222832

[Affected versions]:
Nightly 61.0a1

[Affected platforms]:
Mac 10.13

1.Launch Nightly 61.0a1 with a new profile
2.Set pref gfx.downloadable_fonts.otl_validation = false
3.Restart
4.Open https://developer.microsoft.com/en-us/microsoft-edge/testdrive/demos/variable-fonts/

Expected result:
The page should be displayed correctly.

Actual result:
Crash: https://crash-stats.mozilla.com/report/index/ea6ac35a-cfb0-4f10-8e32-33b8a0180330

Note: Could not repro on Win 10

Comment 1

[Jonathan Kew [:jfkthame]]

If you set gfx.downloadable_fonts.keep_variation_tables = false before loading the page, does that prevent the crash?

If you set layout.css.font-variations.enabled = false before loading the page, does that prevent the crash?

Does the page load OK in Safari on the same system?

Comment 2

[laszlo.bialis]

(In reply to Jonathan Kew (:jfkthame) from comment #1)

If Set pref gfx.downloadable_fonts.otl_validation = false and
> If you set gfx.downloadable_fonts.keep_variation_tables = false before
> loading the page, does that prevent the crash? ---> Works

> If you set layout.css.font-variations.enabled = false before loading the
> page, does that prevent the crash? ---> Works
> 
> Does the page load OK in Safari on the same system? ---> Safari looks good, no crash, on basic settings.

Comment 3

[Jonathan Kew [:jfkthame]]

One more test: I've pushed a try build at https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe355ea1e3f12a013b375d75372d784b3e485398. When this is ready, could you please see if the page still crashes that build (with default prefs)? Thanks!

Comment 4

[Jonathan Kew [:jfkthame]]

BTW, what exact version of macOS is this? (10.13.???)

FWIW, the microsoft demo page doesn't crash, and seems to work fine for me on 10.12.6.

Comment 5

[laszlo.bialis]

(In reply to Jonathan Kew (:jfkthame) from comment #3)
> One more test: I've pushed a try build at
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=fe355ea1e3f12a013b375d75372d784b3e485398. When this
> is ready, could you please see if the page still crashes that build (with
> default prefs)? Thanks!

Tried with Build ID:20180330121558, but the crash is reproduced.
(In reply to Jonathan Kew (:jfkthame) from comment #4)
> BTW, what exact version of macOS is this? (10.13.???). 
  10.13.1 (17B1003)
 
> FWIW, the microsoft demo page doesn't crash, and seems to work fine for me
> on 10.12.6. 
The crash occurs when opening the exact link https://developer.microsoft.com/en-us/microsoft-edge/testdrive/demos/variable-fonts/ the page is displayed for max one second before the crash happens.

Comment 6

[Jonathan Kew [:jfkthame]]

On 10.12.6, I can open that link with no problem. Is there someone who could test with the latest update (10.13.4) to see if the crash still occurs there?

Comment 8

[Jonathan Kew [:jfkthame]]

:mstange confirmed this still crashes on 10.13.4. Another site that crashes similarly is https://v-fonts.com/.

Comment 9

[Markus Stange [:mstange]]

Attachment: lldb debugging sessions

I caught this in the debugger.

The glyph ID is 4 and the font is "Sitka FV Regular".

The crash happens with this stack:
> GetUnscaledAdvances(TFont const&, unsigned short const*, CGSize*, long, CTFontOrientation) + 76
> TFont::GetAdvancesForGlyphs(unsigned short const*, CGSize*, long, CTFontOrientation) const + 96
> CTFontGetAdvancesForGlyphs + 213
> gfxMacFont::GetGlyphWidth(this=0x000000015e4eaae0, aDrawTarget=0x000000010887bb80, aGID=4) at gfxMacFont.cpp:469

CTFontGetAdvancesForGlyphs gets a non-null TFont* from a field in the CTFont object at offset 0x28.
GetUnscaledAdvances gets a pointer from a field in the TFont object at offset 0x148. GetUnscaledAdvances dereferences this pointer without null-checking, but the pointer is null, so we crash.

So my guess is that either something goes wrong during the construction of the TFont, leaving it with a null field in a place that should always be non-null, or some code that is supposed to lazy-initialize a field in the TFont isn't being called in time for our call to CTFontGetAdvancesForGlyphs.

Out of curiousity, I set a breakpoint on GetUnscaledAdvances to check whether it was being called for other fonts with non-null values in the relevant field, and yes, there were other fonts for which this field was non-null.

Comment 10

[Jonathan Kew [:jfkthame]]

Thanks for looking into this, Markus. It looks to me very much like a Core Text bug; if we could create a reduced testcase, it would be worth reporting to Apple. If I update my machine to 10.13, I may take a stab at that.

Meanwhile, though, we'll need to work around this, probably by avoiding the use of CTFontGetAdvancesForGlyphs. :( I'm looking into a couple of possible approaches.

Comment 11

[Jonathan Kew [:jfkthame]]

I've pushed try jobs with a couple of different patches that I hope might avoid the issue here:

(a) https://treeherder.mozilla.org/#/jobs?repo=try&revision=694413ba9c087640b8d75efdde247fa187bcf4cf

(b) https://treeherder.mozilla.org/#/jobs?repo=try&revision=7be668e991aad283415d738ec47ae81439afdea9

As I don't currently have a 10.13.x machine, these are untested (the crash doesn't occur on 10.12). Laszlo, Markus: if you could try these builds and confirm whether either/both of them do resolve the crash, that would be really helpful - thanks!

Comment 12

[laszlo.bialis]

(In reply to Jonathan Kew (:jfkthame) from comment #11)
> I've pushed try jobs with a couple of different patches that I hope might
> avoid the issue here:
> 
> (a)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=694413ba9c087640b8d75efdde247fa187bcf4cf
> 
> (b)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=7be668e991aad283415d738ec47ae81439afdea9
> 
> As I don't currently have a 10.13.x machine, these are untested (the crash
> doesn't occur on 10.12). Laszlo, Markus: if you could try these builds and
> confirm whether either/both of them do resolve the crash, that would be
> really helpful - thanks!

Tried with both patches and couldn't reproduce the crash on either of the pages, both links open successfully in both patches (a)and(b).

However in case of both patches and in case of the link https://v-fonts.com/ although no crash occurs, still some fonts are not displayed (ex. the second font on the page Venn VF). I've also tried to open the link in Safari/Chrome and in both cases all the fonts from the page are displayed correctly. 
Is this intended? 

Tried also on Win 10 with https://v-fonts.com/ in Nightly (Version 61.0a1 and Build ID 20180401220058) same fonts are missing as on MacOS 10.13.

Comment 13

[Jonathan Kew [:jfkthame]]

Attachment: Avoid use of CTFontGetAdvancesForGlyphs with variation fonts due to possible Core Text crash

Comment 14

[Jonathan Kew [:jfkthame]]

(In reply to laszlo.bialis from comment #12)
> However in case of both patches and in case of the link https://v-fonts.com/
> although no crash occurs, still some fonts are not displayed (ex. the second
> font on the page Venn VF). I've also tried to open the link in Safari/Chrome
> and in both cases all the fonts from the page are displayed correctly. 
> Is this intended? 
> 
> Tried also on Win 10 with https://v-fonts.com/ in Nightly (Version 61.0a1
> and Build ID 20180401220058) same fonts are missing as on MacOS 10.13.

I think this must be an unrelated problem. All the fonts on https://v-fonts.com/ show up as expected for me in Nightly (both with and without patches here) on macOS 10.12, and also on Win10.

If the same fonts failed for you on both macOS and Win10, where the font back-end in firefox is entirely different, that suggests maybe a temporary issue with the site or with your connection, such that they failed to download correctly.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/95342072369b
Avoid use of CTFontGetAdvancesForGlyphs with variation fonts due to possible Core Text crash. r=mstange

Comment 16

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/95342072369b

Comment 17

[laszlo.bialis]

The issue still reproduces on the latest Nightly, accessing https://v-fonts.com/ causes an instant crash.

Report: https://crash-stats.mozilla.com/report/index/1775fa57-2d08-4a21-983d-3be260180416

Should I reopen this issue?

Comment 18

[Jonathan Kew [:jfkthame]]

We fixed the issue at the original location, but it's now crashing within a Core Text call made by Skia. :(  We have bug 1454094 about that, so no need to re-open this.