1239176 - OTS font sanitizer rejects fonts opening a new Google docs document

Status: RESOLVED WORKSFORME

(Core :: Graphics: Text, defect)

Description

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Firefox 44.0b8 BuildId: 20160111185352 windows 8

STR:
1. Open drive.google.com
2. Click on "New" -> "Google doc"
3. A new "untitled document" opens up and in a few seconds a warning "Some fonts could not be loaded. Try reloading. Dimiss"

Actual Results: I get the "...fonts could not be loaded" warning on every google doc I open/create that are authored by me or anyone else.

Expected Results: What fonts is it trying to load on a new google document?!

Comment 1

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

[Tracking Requested - why for this release]: This warning usually slows down the browser loading up the google doc (new or existing). And what fonts is it looking for?

Comment 2

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Attachment: font could not be loaded.jpg

Screen snapshot

Comment 3

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Jtd, I know we uplifted a few changes with font family search in Beta44 and wondering if that is the reason why I am running into this bug? Could you please help investigate?

Comment 4

[John Daggett (:jtd)]

Ritu, I don't get this error under FF43 on OSX, so you'll need to be more specific about the platform you're on. Having the user agent string in the description would be helpful ;)

There are two basic steps to diagnose this:

1. Look at the console, I'm guessing you'll see an error there. If we can track it down to a funky font error we can report that to the Google fonts folks.

2. Run with userfont logging:

  export NSPR_LOG_MODULES=userfonts:5
  export NSPR_LOG_FILE=/path/to/userfonts.log

The logging will give you all sorts of nitty gritty about what loaded and what failed. I'm guessing one of the fonts on the fonts menu isn't loading correctly.

Note: with e10s, you may see multiple log files, one for each process. Grep on 'userfonts' to get logging for the content process.

Comment 5

[John Daggett (:jtd)]

Ah, Windows 8. Didn't see that!

Comment 6

[John Daggett (:jtd)]

When loading the fonts for the font menu, the Merriweather fonts are failing to load due to problems with the GSUB table, the sanitizer rejects these with the following messages:

downloadable font: Layout: DFLT table doesn't satisfy the spec. for script tag DFLT (font-family: "Merriweather" style:italic weight:normal stretch:normal src index:2) source: https://fonts.gstatic.com/s/merriweather/v8/So5lHxHT37p2SS4-t60SlGfrnYWAzH6tTbHZfcsRIsM.woff2 <unknown>
downloadable font: Layout: Failed to parse script table 0 (font-family: "Merriweather" style:italic weight:normal stretch:normal src index:2) source: https://fonts.gstatic.com/s/merriweather/v8/So5lHxHT37p2SS4-t60SlGfrnYWAzH6tTbHZfcsRIsM.woff2 <unknown>
downloadable font: GSUB: Failed to parse script list table (font-family: "Merriweather" style:italic weight:normal stretch:normal src index:2) source: https://fonts.gstatic.com/s/merriweather/v8/So5lHxHT37p2SS4-t60SlGfrnYWAzH6tTbHZfcsRIsM.woff2 <unknown>
downloadable font: rejected by sanitizer (font-family: "Merriweather" style:italic weight:normal stretch:normal src index:2) source: https://fonts.gstatic.com/s/merriweather/v8/So5lHxHT37p2SS4-t60SlGfrnYWAzH6tTbHZfcsRIsM.woff2 <unknown>

These messages appear on the console, once for the regular face and again for the bold face.

This occurs with Nightly on both Windows and OSX.

Comment 7

[John Daggett (:jtd)]

Sent in a message to some Google Fonts folks to see if they can verify whether the fonts are funky or if this is an OTS bug.

Comment 8

[John Daggett (:jtd)]

Note that this is due to stricter validation of GSUB/GPOS tables in the latest OTS update. See bug 1193050 for details.

Comment 9

[John Daggett (:jtd)]

Response from Google:

"Jonathan Kew contacted us about this last week, as FF44 will actually stop rendering the fonts. We will push new fonts that fix fatal ots errors this and next week :)"

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

John, does this mean there is no real fix for this issue from our side? I don't see any point in tracking if that is the case. Keep in mind, as is, this is a terrible experience and I am really surprised no one else is complaining about it.

Comment 11

[John Daggett (:jtd)]

Right, nothing we can do other than disable OTS, which is not acceptable from a security standpoint. I think there has been some discussion about the particulars of how OTS is validating GSUB/GPOS tables, Behdad has pointed out places where what OTS is doing is not completely correct. But those should be tackled as individual issues I think, if there's a need.

I agree completely that this is an issue of concern. I did notice that the message didn't always appear when I loaded a new Google docs document.

Comment 12

[Dave Crossland]

I think there is something folks at Mozilla can do - contribute to OTS :) 

Earlier today, Behdad Esfahbod suggested at https://github.com/khaledhosny/ots/pull/88 that ots be changed to not reject such fonts. That is labour that Mozilla staff can perform; contracting or indeed hiring Khaled would be a great idea if you guys are short on hands for this, and right now is a good opportunity to do so, before he is snapped up by someone else :) 

https://github.com/khaledhosny/ots/issues/74 is the issue where Behdad suggested dropping fonts if their tables are not healable. 

https://github.com/khaledhosny/ots/issues/18 is the original sentiment from Behdad on healing tables where possible.

https://github.com/khaledhosny/ots/issues/27 is also relevant.

Comment 13

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) from comment #11)
> Right, nothing we can do other than disable OTS, which is not acceptable
> from a security standpoint. 

Well, one thing we could do would be to make OTS pass the OpenType layout tables (GDEF/GPOS/GSUB) through untouched, rather than validating them. That would be Behdad's recommendation, I think, on the basis that Harfbuzz performs its own "sanitize" operation when loading the tables, and is designed to be robust against bad or malicious fonts.

This would be a three-line code change (just add those three table tags to the condition in gfxOTSContext::GetTableAction), but I don't know if we'd want to consider doing that on Beta this close to release?

(FWIW, I think chromium has switched to allowing these tables through unchecked, and relying on harfbuzz to be robust. But IMO a much better outcome would be for font vendors/distributors such as Google Fonts to fix such out-of-spec fonts.)

> I think there has been some discussion about the
> particulars of how OTS is validating GSUB/GPOS tables, Behdad has pointed
> out places where what OTS is doing is not completely correct. But those
> should be tackled as individual issues I think, if there's a need.

Agreed.

Comment 14

[Jonathan Kew [:jfkthame]]

Attachment: Allow OpenType Layout tables (GDEF/GPOS/GSUB) to pass through OTS unchecked, relying on harfbuzz to handle them safely

FTR, here's the patch that would work around this issue on our side. :ritu, do you want to consider taking this for FF44? I don't think it's worth taking on trunk if we're not going to uplift it, as that implies we're relying on Google to fix the fonts on their side to avoid the upcoming breakage; and if they're going to fix the fonts, we don't need to disable the validation. But if we're not confident the fixed fonts will be deployed in time, we could consider taking this for 44 (only) to give them an extra 6-week grace period.

Comment 15

[Dave Crossland]

The problem isn't the 8 fonts remaining to be fixed at Google, but rather the many proprietary fonts that aren't actively maintained, and self-hosted, so there is no central way to propagate a fix.

Comment 16

[Jonathan Kew [:jfkthame]]

Yes, though most of those are probably not as widely used as the Google Fonts service.

If we decide (as a long-term "fix" here) not to reject fonts with this type of error, by relaxing the OTS check or bypassing OTS altogether, then we're removing any incentive or pressure for sites/authors/vendors to fix or replace their out-of-spec fonts; we're saying we accept that fonts on the web will not be required to conform to the OpenType standard, ever.

Maybe that's what we'll have to do in the end, but it's a position I'd prefer not to take unless widespread web-compat considerations force it on us. As a general principle, we should promote the conformant use of relevant standards, where such standards exist.

Comment 17

[Dave Crossland]

I agree, and thus I am documenting where I can how to fix these errors :)  https://github.com/khaledhosny/ots/pull/88

Comment 18

[Behdad Esfahbod]

(In reply to John Daggett (:jtd) from comment #11)
> Right, nothing we can do other than disable OTS, which is not acceptable
> from a security standpoint.

Wrong.


> I think there has been some discussion about the
> particulars of how OTS is validating GSUB/GPOS tables, Behdad has pointed
> out places where what OTS is doing is not completely correct. But those
> should be tackled as individual issues I think, if there's a need.

Then since Firefox is the only major user of OTS, maybe you guys can either spend the time to fix it, or pay someone to do.  The GSUB/GPOS situation there is really grim; I don't think you understand the extent of the situation.  Read this post of mine for example:

  https://lists.w3.org/Archives/Public/public-webfonts-wg/2016Jan/0004.html

So, there are three things that can be done:

  - Write OTS code for GSUB/GPOS from scratch,

  - Fix OTS code for GSUB/GPOS,

  - Don't use OTS code for GSUB/GPOS.

Firefox can do all three of them.  Neither of those is "disable OTS".

> I agree completely that this is an issue of concern. I did notice that the
> message didn't always appear when I loaded a new Google docs document.

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Jonathan Kew (:jfkthame) from comment #14)
> Created attachment 8707823 [details] [diff] [review]
> Allow OpenType Layout tables (GDEF/GPOS/GSUB) to pass through OTS unchecked,
> relying on harfbuzz to handle them safely
> 
> FTR, here's the patch that would work around this issue on our side. :ritu,
> do you want to consider taking this for FF44? I don't think it's worth
> taking on trunk if we're not going to uplift it, as that implies we're
> relying on Google to fix the fonts on their side to avoid the upcoming
> breakage; and if they're going to fix the fonts, we don't need to disable
> the validation. But if we're not confident the fixed fonts will be deployed
> in time, we could consider taking this for 44 (only) to give them an extra
> 6-week grace period.

Jonathan, I think we should not rush a fix into Fx44 because even though this issue somehow slows down the "open new gdoc" to "new gdoc opens and ready for us" experience IMO, it is still not severe enough to take a rushed fix in Fx44 two days before we enter RC. We should still discuss this actively and take the right fix for Fx45, whether it is disabling certain fonts from our side or getting folks outside Mozilla to fix them.

Jtd mentioned that this warning does not show up on his Fx44 every single time. So I might be a minority and this isn't a release blocked as such.

Comment 20

[John Daggett (:jtd)]

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

> If we decide (as a long-term "fix" here) not to reject fonts with this type
> of error, by relaxing the OTS check or bypassing OTS altogether, then we're
> removing any incentive or pressure for sites/authors/vendors to fix or
> replace their out-of-spec fonts; we're saying we accept that fonts on the
> web will not be required to conform to the OpenType standard, ever.
> 
> Maybe that's what we'll have to do in the end, but it's a position I'd
> prefer not to take unless widespread web-compat considerations force it on
> us. As a general principle, we should promote the conformant use of relevant
> standards, where such standards exist.

While I agree with this in principle, I think the validation code in OTS is somewhat suspect, as Behdad has pointed out. I think that code really needs to be up to snuff before we reject fonts with supposed problems. I don't want to create a lot of busy work for font designers because our validation code isn't quite right. The backtracking example Behdad listed in his post looks totally braindead for example and just the sort of thing to trip up a select set of fonts.

The other aspect of this that's troubling is that font services are beginning to do some very elaborate things with fonts, building them dynamically on the fly for example. In that context, I doubt we'll ever get much feedback from devs or users until the problem has existed for a long time. Given that we're not passing GSUB/GPOS to a system API, which was one of the concerns leading to the use of OTS, I think skipping the OTS validation of these tables would be fine.

Comment 21

[John Daggett (:jtd)]

Comment on attachment 8707823 [details] [diff] [review]
Allow OpenType Layout tables (GDEF/GPOS/GSUB) to pass through OTS unchecked, relying on harfbuzz to handle them safely

I'm completely fine with this change, I think it's better to let Harfbuzz fixup and deal with problematic OTL tables rather than gatekeeping via OTS.

Comment 22

[John Daggett (:jtd)]

(In reply to Ritu Kothari (:ritu) from comment #19)

> Jtd mentioned that this warning does not show up on his Fx44 every single
> time. So I might be a minority and this isn't a release blocked as such.

I was simply commenting on why this hadn't been reported earlier. In fact I think taking a fix for this would be a good idea but if Google is going to fix the fonts I don't think we absolutely need to take it for FF44.

Comment 23

[John Daggett (:jtd)]

(In reply to Behdad Esfahbod from comment #18)
> (In reply to John Daggett (:jtd) from comment #11)
> > Right, nothing we can do other than disable OTS, which is not acceptable
> > from a security standpoint.
> 
> Wrong.

You're saying disabling OTS would be fine? I think there's a role for some form of sanity-checking on incoming font data, since Windows in particular is slow to fix security issues related to font bugs. 

> Then since Firefox is the only major user of OTS, maybe you guys can either
> spend the time to fix it, or pay someone to do.

Really? My understanding was that Chrome uses OTS but not GSUB/GPOS validation. Or Chrome has switched to some other package for font validation?

>  The GSUB/GPOS situation there is really grim; I don't think you
>  understand the extent of the situation.  Read this post of mine for
>  example:
> 
>   https://lists.w3.org/Archives/Public/public-webfonts-wg/2016Jan/0004.html

Yup, saw that. I agree with you here, I think validation code that has bogus checks like this is going to trip up lots of fonts and in the deadliest way, in content simply doesn't display as designed without us ever knowing that it was happening.

I think we should pass through OTL tables without OTS validation. On another bug we can deal with whether we want to work on the OTS validation code for OTL tables or not.

Comment 24

[Behdad Esfahbod]

(In reply to John Daggett (:jtd) from comment #23)
> (In reply to Behdad Esfahbod from comment #18)
> > (In reply to John Daggett (:jtd) from comment #11)
> > > Right, nothing we can do other than disable OTS, which is not acceptable
> > > from a security standpoint.
> > 
> > Wrong.
> 
> You're saying disabling OTS would be fine?

No, I was saying that "nothing we can do other than disable OTS" is wrong, and I provided three things you can do.


> I think there's a role for some
> form of sanity-checking on incoming font data, since Windows in particular
> is slow to fix security issues related to font bugs. 
> 
> > Then since Firefox is the only major user of OTS, maybe you guys can either
> > spend the time to fix it, or pay someone to do.
> 
> Really? My understanding was that Chrome uses OTS but not GSUB/GPOS
> validation. Or Chrome has switched to some other package for font validation?

I confused myself as well.  Yes, I meant the GSUB/GPOS.

I never suggested to disable OTS as a whole.  I think OTS is a great tool for protecting against code we don't control (CFF engine in Windows, etc).  But for code we do control, I prefer that the code itself be robust against bad input.  And in particular, the GSUB/GPOS code in OTS happens to be really bad as is.

> >  The GSUB/GPOS situation there is really grim; I don't think you
> >  understand the extent of the situation.  Read this post of mine for
> >  example:
> > 
> >   https://lists.w3.org/Archives/Public/public-webfonts-wg/2016Jan/0004.html
> 
> Yup, saw that. I agree with you here, I think validation code that has bogus
> checks like this is going to trip up lots of fonts and in the deadliest way,
> in content simply doesn't display as designed without us ever knowing that
> it was happening.
> 
> I think we should pass through OTL tables without OTS validation. On another
> bug we can deal with whether we want to work on the OTS validation code for
> OTL tables or not.

Comment 25

[John Daggett (:jtd)]

So thinking about this a bit more, I think the right approach would be to take Jonathan's patch, land it on trunk and uplift it to aurora. In addition, we should probably log a separate bug to review OTS validation of GSUB/GPOS/GDEF tables.

Comment 26

[John Daggett (:jtd)]

Assigning to Jonathan for now.

Comment 27

[Dave Crossland]

Merriweather and Merriweather Sans were both updated in the Google Fonts API today, resolving the OTS error, but it introduced a problem with 'fi' and 'fl' ligatures which will be fixed on Monday.

Several other families were also corrected, with just Miltonian Tattoo to be updated on Monday also.

Comment 28

[Dave Crossland]

Also, it would be good to ensure a note about this is included in the v44 release notes. One of the OTS related bugs in this tracker has the relnotes-44 tag, but the release notes themselves do not yet mention OTS.

Comment 29

[John Daggett (:jtd)]

Jonathan, are you okay with landing this patch?

Comment 30

[John Daggett (:jtd)]

(In reply to Dave Crossland from comment #27)
> Merriweather and Merriweather Sans were both updated in the Google Fonts API
> today, resolving the OTS error, but it introduced a problem with 'fi' and
> 'fl' ligatures which will be fixed on Monday.
> 
> Several other families were also corrected, with just Miltonian Tattoo to be
> updated on Monday also.

Thanks for the update Dave!

Comment 31

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) from comment #29)
> Jonathan, are you okay with landing this patch?

I'm a bit reluctant, given that in some cases (such as the Merriweather example) this may be just hiding the fact that the fonts have errors, thereby reducing the chance that they'll ever get fixed. If we don't care about the spec-conformance of fonts on the web -- "forget the specs, whatever works is good enough :)" -- maybe that's OK, but it's not a position I am keen to take if we can help it.

We know that the broken fonts served via Google Fonts are getting fixed right now. I'm sure there are others out there that will be affected, but we don't know how widespread the problem will be. Given that we haven't been getting a flood of reports during Fx44 Beta, perhaps they're pretty rare.

So my preference is to hold off on landing the bypass patch here, and only take it if we find that the level of "breakage" in Fx44 (which we already know we're going to ship in this state) is unacceptable. Otherwise, I lean towards keeping the stricter validation and pushing back against sites that deploy erroneous fonts.

Note, also, that even if we trust that harfbuzz's handling of invalid OTLayout tables is 100% robust and we'll never run into a potentially-dangerous bug there, the result may be fonts that appear to "work" -- in that they load in the browser and show the expected style of glyphs -- yet do not have the layout behavior intended by the designer. Harfbuzz tries to "fix up" invalid tables such that it can use them without risk of crashing, but this provides the author/designer with absolutely no feedback indicating that the font might not behave as they intended. By explicitly rejecting the font if the tables are invalid, we make it much more likely that the author will realize something is wrong.

Comment 32

[Chris Peterson [:cpeterson]]

(In reply to Jonathan Kew (:jfkthame) from comment #31)
> (In reply to John Daggett (:jtd) from comment #29)
> > Jonathan, are you okay with landing this patch?
> 
> I'm a bit reluctant, given that in some cases (such as the Merriweather
> example) this may be just hiding the fact that the fonts have errors,
> thereby reducing the chance that they'll ever get fixed. If we don't care
> about the spec-conformance of fonts on the web -- "forget the specs,
> whatever works is good enough :)" -- maybe that's OK, but it's not a
> position I am keen to take if we can help it.

What if we only enable OTS font sanitizer errors in the Firefox pre-release channels?

People running Nightly or Dev Edition (web developers!) will see and report font errors, but less technical people running Beta or Release won't think "This website is broken in Firefox, so I will switch to Chrome". We could still run the OTS font sanitizer in Beta and Release, but only report font problems in the web console. Then in a few months, we can try to ship the sanitizer errors to all channels again.

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Please don't forget that the sanitizer is an important security measure to prevent us from sending malformed fonts to OS libraries that aren't designed to deal with malicious input.  We have to use it on all channels in order to ship downloadable fonts in a secure way.  There might be questions on whether certain tests in the sanitizers are necessary or whether all OSes are known to handle a particular malformedness reasonably, but we can't consider just turning off the sanitizer.

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Though I admit that that's maybe less of an issue with harfbuzz today than it was when we originally started supporting downloadable fonts -- but if we want to consider disabling parts of the sanitizer we'd need to do a very careful security review of what code we're exposing to potentially-malicious fonts.)

Comment 35

[John Daggett (:jtd)]

(In reply to Chris Peterson [:cpeterson] from comment #32)

> What if we only enable OTS font sanitizer errors in the Firefox pre-release
> channels?

Chris, what the patch here does is simply pass through tables that only harfbuzz uses, the GSUB/GPOS/GDEF ones. I think the argument for rejecting fonts for problems in these tables is a lot weaker than for other tables that are used by black box system font handling routines that in some cases run at elevated priv levels. OTS validation is really a must for those tables.

My concern is that I'm not sure the sanitizer is really always correctly reporting errors, based on Behdad's comments. It's one thing to reject a font for clear errors but another to reject it for somewhat artificial reasons.

Comment 36

[Dave Crossland]

Just to confirm, all Google Fonts should now be updated for Firefox 44 tomorrow

Comment 37

[Jonathan Kew [:jfkthame]]

(In reply to Chris Peterson [:cpeterson] from comment #32)
> What if we only enable OTS font sanitizer errors in the Firefox pre-release
> channels?
> 
> People running Nightly or Dev Edition (web developers!) will see and report
> font errors, but less technical people running Beta or Release won't think
> "This website is broken in Firefox, so I will switch to Chrome".

Modulo the clarification that we're not talking about potentially disabling OTS altogether -- this is only about validating the OpenType Layout tables, not the rest of the glyph data, hinting, etc. -- we could consider trying something like this.

> We could
> still run the OTS font sanitizer in Beta and Release, but only report font
> problems in the web console.

No, AFAIR with the current OTS API, we don't have the option of reporting GDEF/GPOS/GSUB errors but allowing the font to be used anyway. The patch to let those tables through means OTS won't check them at all. Though maybe we could propose an API modification to support this behavior.

Comment 38

[Jonathan Kew [:jfkthame]]

(In reply to Dave Crossland from comment #36)
> Just to confirm, all Google Fonts should now be updated for Firefox 44
> tomorrow

Thanks, Dave, that's great. I can confirm I no longer get the "fonts could not be loaded" warning when opening a new Google Doc, and Merriweather Sans, for instance, now seems to work as expected.

Comment 39

[Jonathan Kew [:jfkthame]]

Given that this is now fixed on Google's side, I'm closing this as WFM.

Migrating the patch to bug 1244693, which relates to the same font-rejection issue elsewhere.