Chris Peterson [:cpeterson] Reporter
	Description • 8 years ago

Jonathan, this Google Docs font bug sounds a lot like OTS font sanitizer bug 1239176, which was an issue with Google's Merriweather woff2 fonts.

STR:
1. In Firefox Nightly or Aurora, open a Google Doc such as:
https://docs.google.com/document/d/1OHpQOvZz76_10ebJP2AKvvXUF3H9yd6FC89F5jS4mks/
2. Wait about five seconds for the doc to finish loading.

RESULT:
Google Docs shows a warning that says "Some fonts could not be loaded. Try reloading." I don't see any obvious errors in the devtools console.

I can reproduce this warning consistently with Nightly 53 and Aurora 52 (on Windows 10 and macOS 10.12), but not in Beta 51 or Release 50 (or Chrome, of course).

However, I think this is a problem with the Nightly and Aurora pre-release builds, in general, and not a regression in 52. I tried bisecting between good version 50 (no warning) and bad version 53 (warning) and all Nightly builds were bad, even those that for versions 50 and 51 that are good in the Beta and Release channels. Do we have any font sanitizing checks running only in pre-release channels?

	Dave Crossland
	Comment 1 • 8 years ago

I am the Google Fonts program manager. I can't reproduce this with Nightly 53.0a1 (2017-01-17) (64-bit) on OS X 10.12.2 (16C67)

The doc seems to use Proxima Nova only. Those font files *should* be good, unlike some random libre font.

	Jonathan Kew [:jfkthame] Assignee
	Comment 2 • 8 years ago

(In reply to Chris Peterson [:cpeterson] from comment #0)
> However, I think this is a problem with the Nightly and Aurora pre-release
> builds, in general, and not a regression in 52. I tried bisecting between
> good version 50 (no warning) and bad version 53 (warning) and all Nightly
> builds were bad, even those that for versions 50 and 51 that are good in the
> Beta and Release channels. Do we have any font sanitizing checks running
> only in pre-release channels?

Yes -- on pre-release channels, we run the OpenType Layout tables (GDEF/GSUB/GPOS) through OTS, which will attempt to validate them and will reject the font if they're out of spec; but on beta/release, we allow those tables to bypass OTS even if they have spec violations, as in general harfbuzz can still handle them safely.

(In reply to Dave Crossland from comment #1)
> I am the Google Fonts program manager. I can't reproduce this with Nightly
> 53.0a1 (2017-01-17) (64-bit) on OS X 10.12.2 (16C67)
> 
> The doc seems to use Proxima Nova only. Those font files *should* be good,
> unlike some random libre font.

When I tried loading the doc from comment 0, it tried (and failed, in Nightly on macOS) to load "Varela Round", rejecting it because of a bad 'DFLT' entry in the GSUB script table. This is a common font bug (and there are moves afoot to perhaps modify the spec to allow it, but as of now it's an OpenType spec violation and causes OTS to complain).

	Chris Peterson [:cpeterson] Reporter
	Comment 3 • 8 years ago

(In reply to Jonathan Kew (:jfkthame) from comment #2)
> When I tried loading the doc from comment 0, it tried (and failed, in
> Nightly on macOS) to load "Varela Round", rejecting it because of a bad
> 'DFLT' entry in the GSUB script table. This is a common font bug (and there
> are moves afoot to perhaps modify the spec to allow it, but as of now it's
> an OpenType spec violation and causes OTS to complain).

Jonathan, so is this a bug in the "Verla Round" font that Google should fix and not something we should workaround in Firefox?

I now see that Firefox was logging more error info in the browser console (CTRL+SHIFT+J):

downloadable font: Layout: DFLT script doesn't satisfy the spec. DefaultLangSys is NULL (font-family: "Varela Round--Menu" style:normal weight:normal stretch:normal src index:0) source: https://fonts.gstatic.com/s/varelaround/v8/APH4jr0uSos5wiut5cpjrkWnNVpw0CtxFlw7ZW7Wg4I.woff2 (unknown)

downloadable font: Layout: Failed to parse script table 0 (font-family: "Varela Round--Menu" style:normal weight:normal stretch:normal src index:0) source: https://fonts.gstatic.com/s/varelaround/v8/APH4jr0uSos5wiut5cpjrkWnNVpw0CtxFlw7ZW7Wg4I.woff2 (unknown)

downloadable font: GSUB: Failed to parse script list table (font-family: "Varela Round--Menu" style:normal weight:normal stretch:normal src index:0) source: https://fonts.gstatic.com/s/varelaround/v8/APH4jr0uSos5wiut5cpjrkWnNVpw0CtxFlw7ZW7Wg4I.woff2 (unknown)

downloadable font: rejected by sanitizer (font-family: "Varela Round--Menu" style:normal weight:normal stretch:normal src index:0) source: https://fonts.gstatic.com/s/varelaround/v8/APH4jr0uSos5wiut5cpjrkWnNVpw0CtxFlw7ZW7Wg4I.woff2

	Jonathan Kew [:jfkthame] Assignee
	Comment 4 • 8 years ago

(In reply to Chris Peterson [:cpeterson] from comment #3)
> (In reply to Jonathan Kew (:jfkthame) from comment #2)
> > When I tried loading the doc from comment 0, it tried (and failed, in
> > Nightly on macOS) to load "Varela Round", rejecting it because of a bad
> > 'DFLT' entry in the GSUB script table. This is a common font bug (and there
> > are moves afoot to perhaps modify the spec to allow it, but as of now it's
> > an OpenType spec violation and causes OTS to complain).
> 
> Jonathan, so is this a bug in the "Verla Round" font that Google should fix
> and not something we should workaround in Firefox?

Yes.

Dave, could you please get the Varela Round resources validated and fixed? Thanks!

	Chris Peterson [:cpeterson] Reporter
	Comment 5 • 8 years ago

(In reply to Jonathan Kew (:jfkthame) from comment #2)
> Yes -- on pre-release channels, we run the OpenType Layout tables
> (GDEF/GSUB/GPOS) through OTS, which will attempt to validate them and will
> reject the font if they're out of spec;

Jonathan, is there a pref to disable the OTS validation in pre-release channels? This "Some fonts could not be loaded" warning is causing some problems for our automated performance tests of Google Docs with Nightly builds.

	Jonathan Kew [:jfkthame] Assignee
	Comment 6 • 8 years ago

No, it's a compile-time #ifdef:

https://dxr.mozilla.org/mozilla-central/rev/1e0e193b0812f68a12fbd69198552af62347af1e/gfx/thebes/gfxUserFontSet.cpp#184

I suppose we could consider a runtime pref if it's really important...

	will69
	Comment 7 • 8 years ago

This issue also pertains to "Alegreya Sans":

downloadable font: Layout: DFLT script doesn't satisfy the spec. DefaultLangSys is NULL (font-family: "Alegreya Sans" style:normal weight:900 stretch:normal src index:2) source: http://fonts.gstatic.com/l/font?kit=11EDm-lum6tskJMBbdy9abQRuKjhSm_sX2fZ1VbgI48pufQyA2LjKJYw0jQtDPSM&skey=8820b7058d1ab315&v=v3

downloadable font: Layout: Failed to parse script table 0 (font-family: "Alegreya Sans" style:normal weight:900 stretch:normal src index:2) source: http://fonts.gstatic.com/l/font?kit=11EDm-lum6tskJMBbdy9abQRuKjhSm_sX2fZ1VbgI48pufQyA2LjKJYw0jQtDPSM&skey=8820b7058d1ab315&v=v3

downloadable font: GSUB: Failed to parse script list table (font-family: "Alegreya Sans" style:normal weight:900 stretch:normal src index:2) source: http://fonts.gstatic.com/l/font?kit=11EDm-lum6tskJMBbdy9abQRuKjhSm_sX2fZ1VbgI48pufQyA2LjKJYw0jQtDPSM&skey=8820b7058d1ab315&v=v3

downloadable font: rejected by sanitizer (font-family: "Alegreya Sans" style:normal weight:900 stretch:normal src index:2) source: http://fonts.gstatic.com/l/font?kit=11EDm-lum6tskJMBbdy9abQRuKjhSm_sX2fZ1VbgI48pufQyA2LjKJYw0jQtDPSM&skey=8820b7058d1ab315&v=v3

	will69
	Comment 8 • 8 years ago

And now for the important part ;-)

This works:

@import url(http://fonts.googleapis.com/css?family=Alegreya+Sans:900);

This fails:

@import url(http://fonts.googleapis.com/css?family=Alegreya+Sans:900&text=0123456789RGWBY-*%3a%20);

	Jonathan Kew [:jfkthame] Assignee
	Comment 9 • 8 years ago

Interesting; that seems to suggest that perhaps the subsetting tool being used on Google Fonts is generating out-of-spec OpenType tables.

I looked into the OpenType tables in the font files served by each of those two URLs, and confirmed that in the second (small subset) example, the GSUB table is an empty shell, where the script records remain but all the features have been removed (presumably because none of the glyphs in the chosen subset are involved in any of the features), and with the features gone, the language system records are also gone. Unfortunately, this includes stripping out the DefaultLangSys record for the DFLT script, which the OT spec says is required.

Dave, something to look into?

	Chris Peterson [:cpeterson] Reporter
	Comment 10 • 8 years ago

Today I also see console warnings about "Amatic SC--Menu" and "Merriweather--Menu" (like 2016 Merriweather bug 1239176).

	Dave Crossland
	Comment 11 • 8 years ago

Thanks Jonathan, the Google Fonts engineering team is looking into it! :)

	Jonathan Kew [:jfkthame] Assignee
	Comment 12 • 8 years ago

To help people who want to control this behavior for testing purposes, let's make it a runtime pref rather than a build-time condition. The default behavior will still be as now -- such fonts will load on Beta/Release, so we don't annoy too many users, but Nightly/DevEd will block them and issue console error messages to encourage people to report and fix out-of-spec fonts and font-deployment workflows.

	Jonathan Kew [:jfkthame] Assignee
	Comment 14 • 8 years ago

:jrmuizel - review ping?

	Chris Peterson [:cpeterson] Reporter
	Comment 15 • 8 years ago

Dave, do you have any updates on an ETA for fixing these Google fonts? Is there a Google bug number we can track for future reference? Thanks!

	Jeff Muizelaar [:jrmuizel]
	Comment 16 • 8 years ago

Comment on attachment 8832519 [details] [diff] [review]
Expose a pref to control the validation of OpenType Layout tables, so that Nightly/Aurora users can choose to bypass validation (like we do on Beta/Release) if they really want out-of-spec fonts to be loaded

Review of attachment 8832519 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxUserFontSet.cpp
@@ +180,5 @@
> +    {
> +        // Whether to apply OTS validation to OpenType Layout tables
> +        mCheckOTLTables =
> +            Preferences::GetBool("gfx.downloadable_fonts.otl_validation",
> +                                 true);

Do we want to check this everytime? Perhaps using gfxPrefs would be a better idea? It makes things fast and type safe. Preferences::GetBool isn't the cheapest.

	Dave Crossland
	Comment 17 • 8 years ago

So, as jkew suspected, it seems that the font files that are input into the gf api are legit, and it is the font files that are output by the api serving system that are causing this error. Specifically, the subsetter which is producing these 'menu' files (which have only the glyphs needed to render the font family name in the 400 roman/regular.) So, there's nothing I can do here; a google engineer needs to address this, if you want it to be addressed by google. however, perhaps mozilla can address it? 

I hope Behdad can write more about this, I've told you everything I know :)

	Jonathan Kew [:jfkthame] Assignee
	Comment 18 • 8 years ago

Is the subsetting being done by pyftsubset, or some other tool?

Comment 9 identifies the basic flaw here (the DFLT script lacks a DefaultLangSys record, as a result of all the GSUB features having been dropped); so the fix is for the subsetting tool to either remove the table completely in the case where no real content remains, or preserve the DefaultLangSys record even though it has not features left.

	Jonathan Kew [:jfkthame] Assignee
	Comment 19 • 8 years ago

(In reply to Jeff Muizelaar [:jrmuizel] from comment #16)
> Do we want to check this everytime? Perhaps using gfxPrefs would be a better
> idea? It makes things fast and type safe. Preferences::GetBool isn't the
> cheapest.

Good idea, I'll switch it to gfxPrefs before pushing.

	Jonathan Kew [:jfkthame] Assignee
	Comment 20 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/340c5eaf341c7dfdf2957b17da4a67999df92537
Bug 1331737 - Expose a pref to control the validation of OpenType Layout tables, so that Nightly/Aurora users can choose to bypass validation (like we do on Beta/Release) if they really want out-of-spec fonts to be loaded. r=jrmuizel

	Jonathan Kew [:jfkthame] Assignee
	Comment 21 • 8 years ago

:cpeterson, once the patch above makes it into the builds you're using, you'll be able to bypass the validation by setting gfx.downloadable_fonts.otl_validation to false, so that should unblock your testing.

This doesn't fix the bad fonts, of course; that can only be done at Google's end. The default behavior on Nightly/Aurora will still be to validate and reject them.

	Chris Peterson [:cpeterson] Reporter
	Comment 22 • 8 years ago

(In reply to Jonathan Kew (:jfkthame) from comment #21)
> :cpeterson, once the patch above makes it into the builds you're using,
> you'll be able to bypass the validation by setting
> gfx.downloadable_fonts.otl_validation to false, so that should unblock your
> testing.

Thanks, Jonathan. This is a big help for Hasal team profiling Google Docs performance.

> This doesn't fix the bad fonts, of course; that can only be done at Google's
> end. The default behavior on Nightly/Aurora will still be to validate and
> reject them.

Dave, what should we do if we see font warnings about Google fonts in the future? Can we just email you? These font warnings don't indicate a Firefox bug, so filing a Bugzilla bug report probably doesn't make sense.

	Carsten Book [:Tomcat]
	Comment 23 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/340c5eaf341c

	Julien Cristau [:jcristau]
	Comment 24 • 7 years ago

52 is about to release and 53 about to become beta, so won't be affected anymore, wontfix.

	TMart
	Comment 25 • 7 years ago

Is this still active in current Nightly 55.0a1 (2017-04-15)? I have a Xenforo where Google Font Cabin doesn't get loaded in Nightly 55 with gfx.downloadable_fonts.otl_validation set to false, it does work in Aurora 54 however.

	Chris Peterson [:cpeterson] Reporter
	Comment 26 • 7 years ago

Thanks for the report, TMart. I see font errors and missing alphabet text for the "Cabin" font on the following page, as you described:

https://fonts.google.com/specimen/Cabin

But the font renders correctly if I set the gfx.downloadable_fonts.otl_validation pref to false and reload the page. Are you seeing a different problem? This is most likely a problem with Google's font data itself, not a Firefox bug, so there's not much we can do about it.

I also see many font errors on the Google Fonts home page and in Google Docs.

https://fonts.google.com/

	TMart
	Comment 27 • 7 years ago

I tested this on another installation, there it works fine with the validation pref. So it has to be an issue with my profile.

	Behdad Esfahbod
	Comment 30 • 7 years ago

FWIW this has been fixed in the OpenType 1.8.2 spec.  Ie. DFLT script does not require a non-NULL DefaultLangSys, and is allowed to have non-zero LangSys tables.

	Behdad Esfahbod
	Comment 31 • 7 years ago

http://typedrawers.com/discussion/2276/opentype-1-8-2-released

	Jonathan Kew [:jfkthame] Assignee
	Comment 32 • 7 years ago

(In reply to Behdad Esfahbod from comment #30)
> FWIW this has been fixed in the OpenType 1.8.2 spec.  Ie. DFLT script does
> not require a non-NULL DefaultLangSys,

Is this correct? According to https://www.microsoft.com/typography/otspec/chapter2.htm#scriptsLangs, the requirement

  "If there is a 'DFLT' script table, it must have a non-NULL DefaultLangSys value"

still stands.

> and is allowed to have non-zero
> LangSys tables.

Yes, this is now permitted (and OTS has been adjusted accordingly).

	Behdad Esfahbod
	Comment 33 • 7 years ago

(In reply to Jonathan Kew (:jfkthame) from comment #32)
> (In reply to Behdad Esfahbod from comment #30)
> > FWIW this has been fixed in the OpenType 1.8.2 spec.  Ie. DFLT script does
> > not require a non-NULL DefaultLangSys,
> 
> Is this correct? According to
> https://www.microsoft.com/typography/otspec/chapter2.htm#scriptsLangs, the
> requirement
> 
>   "If there is a 'DFLT' script table, it must have a non-NULL DefaultLangSys
> value"
> 
> still stands.

Yes, unfortunately.  Although I hope we all agree it makes no sense...

I went ahead and updated our subsetter to keep this around.

> > and is allowed to have non-zero
> > LangSys tables.
> 
> Yes, this is now permitted (and OTS has been adjusted accordingly).

	Behdad Esfahbod
	Comment 34 • 7 years ago

(In reply to Behdad Esfahbod from comment #33)
> (In reply to Jonathan Kew (:jfkthame) from comment #32)
> > (In reply to Behdad Esfahbod from comment #30)
> > > FWIW this has been fixed in the OpenType 1.8.2 spec.  Ie. DFLT script does
> > > not require a non-NULL DefaultLangSys,
> > 
> > Is this correct? According to
> > https://www.microsoft.com/typography/otspec/chapter2.htm#scriptsLangs, the
> > requirement
> > 
> >   "If there is a 'DFLT' script table, it must have a non-NULL DefaultLangSys
> > value"
> > 
> > still stands.
> 
> Yes, unfortunately.  Although I hope we all agree it makes no sense...
> 
> I went ahead and updated our subsetter to keep this around.

https://github.com/fonttools/fonttools/commit/6eb807b55f4fcc55a9b5fd8f3e320c89428462dd