941019 - Update OTS to r106 or Later

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Paul Hardy]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0 (Beta/Release)
Build ID: 20131112160018

Steps to reproduce:

GNU Unifont ("unifont.ttf") is a free bitmap font that provides support for the entire Unicode Basic Multilingual Plane.  Its "post" table contains a name for each glyph, and there are over 50,000 such glyphs.  Microsoft's 2001 TrueType version 2.0 spec mentions that "post" table index values above 32,767 are "reserved for future use".  As a result, the Chromium OpenType Sanitiser rejected Unifont and another large Unicode font, Wen Quan Yi ("wqy-zenhei.ttf"), in previous versions.  Wen Quan Yi is the most popular free Chinese font on the Web, offering CJK ideographs and much more at multiple point sizes.

OTS r106, permits "post"index values greater than 32,767.  Therefore upgrading to the latest OTS will allow Unifont and Wen Quan Yi to pass as valid fonts.

I started a discussion on this here concerning the WOFF 1.0 validator, but this is a related issue:

https://savannah.gnu.org/support/?108441

Information on r106 of OTS is available here:

https://codereview.chromium.org/68173026/

Information on the later r107 (which fixes an android_aosp build failure) is available here:

https://codereview.chromium.org/76073002/

with a path to the code here:

https://src.chromium.org/viewvc/chrome?revision=236175&view=revision


Thanks!


Paul Hardy

Comment 1

[Jonathan Kew [:jfkthame]]

We should look into taking an OTS update to pick up the change from upstream r106, at least.

A complication is that the upstream OTS repository has also added code for "WOFF2" support, but we don't want to be pulling that code into Gecko just yet; that's for a separate bug once the WOFF2 format/spec begins to stabilize. So we'll need to patch OTS so as to skip the WOFF2 support (and the Brotli dependency that pulls in).

Comment 2

[Frédéric Wang (:fredw)]

OTS r109 fixes bug 959578.
https://codereview.chromium.org/139353002/

OTS r110 adds support for the MATH table, which is necessary for bug 407059.
https://codereview.chromium.org/139563002/

Comment 3

[Frédéric Wang (:fredw)]

(In reply to Jonathan Kew (:jfkthame) from comment #1)
> A complication is that the upstream OTS repository has also added code for
> "WOFF2" support, but we don't want to be pulling that code into Gecko just
> yet; that's for a separate bug once the WOFF2 format/spec begins to
> stabilize. So we'll need to patch OTS so as to skip the WOFF2 support (and
> the Brotli dependency that pulls in).

Do you want me to try that? I'm willing to upgrade to include support for the MATH table ; or perhaps I should just attach a patch to bug 407059 in order to whitelist the MATH table?

Comment 4

[Jonathan Kew [:jfkthame]]

I'd prefer to take an OTS update rather than patching locally, wherever possible. So if you can do this, that'd be great.

We should look into the possibility of switching to https://github.com/khaledhosny/ots/ as our upstream source for OTS, as it seems likely to be more actively supported than the Google Code project. (We'll need to keep an eye on the changes being done there to make sure they're all things we're happy to take. But initially, at least, the fork includes some patches we've been maintaining locally, so switching upstream will make things simpler for us.)

Comment 5

[Frédéric Wang (:fredw)]

OK, I'll see what I can do.

Comment 6

[Frédéric Wang (:fredw)]

OK, I tried to update to Khaled's version and I have submitted a first version of the patches to try server: https://tbpl.mozilla.org/?tree=Try&rev=1a04503be30d

My understanding is that:

- We still need the four local patches mentioned in README.mozilla
- We need one local patch for the SVG glyph table
- We need one local patch to disable WOFF2 and its dependency (although it seems it is turned off by default)
- Khaled has integrated the patches for bug 670901 into its GitHub repository so we don't need them. However, we have a MOZ_OTS_REPORT_ERRORS build option and I suspect OTS won't build when it is turned off. Do we want to keep this MOZ_OTS_REPORT_ERRORS option? If so, I'll probably another patch.

Did I miss anything?

Comment 7

[Jonathan Kew [:jfkthame]]

As I understand it, we can replace the ots-graphite.patch with a table action callback (see upstream commit ea4decac27c31d69c5ca88714428b2f0f58975e7 which introduced this, and 32d78aa581460adb1d5490a255883d88f60ba330 for an example of its use). This will be much cleaner.

We should be able to handle the SVG table in the same way.

Last I looked, woff2 wasn't enabled by default, but the code was still built, which means all the brotli code would also be pulled in. It'd be nice to #ifdef that out for now, until woff2 is sufficiently stable for us to expose it, to avoid carrying the extra unused code. Can we do this by just #ifdef'ing a couple of function calls from the main ots code?

It'd be fine to drop the MOZ_OTS_REPORT_ERRORS build option if the error-reporting callback is now integrated upstream. I haven't looked at what Khaled did there yet, but feel free to get rid of our #define.

Comment 8

[Frédéric Wang (:fredw)]

(In reply to Jonathan Kew (:jfkthame) from comment #7)
> As I understand it, we can replace the ots-graphite.patch with a table
> action callback (see upstream commit
> ea4decac27c31d69c5ca88714428b2f0f58975e7 which introduced this, and
> 32d78aa581460adb1d5490a255883d88f60ba330 for an example of its use). This
> will be much cleaner.
> 
> We should be able to handle the SVG table in the same way.

OK, I'll look into that.

> 
> Last I looked, woff2 wasn't enabled by default, but the code was still
> built, which means all the brotli code would also be pulled in. It'd be nice
> to #ifdef that out for now, until woff2 is sufficiently stable for us to
> expose it, to avoid carrying the extra unused code. Can we do this by just
> #ifdef'ing a couple of function calls from the main ots code?
> 

Yes, I think this will just be ifdef'ing the code removed in https://hg.mozilla.org/try/rev/3b2fccc9ba7e

> It'd be fine to drop the MOZ_OTS_REPORT_ERRORS build option if the
> error-reporting callback is now integrated upstream. I haven't looked at
> what Khaled did there yet, but feel free to get rid of our #define.

I think the changes were essentially the same (including typo corrections), except for the MOZ_OTS_REPORT_ERRORS.

Comment 9

[Frédéric Wang (:fredw)]

I have updated the patches and submit them to Try server:
https://tbpl.mozilla.org/?tree=Try&rev=b0cc67c0dcff

Comment 10

[Khaled Hosny]

If there are any local Mozilla patches that I missed, I’d be happy to incorporate them.

Comment 11

[Frédéric Wang (:fredw)]

The previous reftests failed with the graphite and SVG tables. There was an error with a missing gfxUserFontSet prefix that might explain the build failures on windows (I just tried again here: https://tbpl.mozilla.org/?tree=Try&rev=14e131a137e9). However, that does not help for the reftest failures. My understanding was that we no longer need the graphite.cc and svg.cc files, but only to have the TableAction callback to skip sanitizing, right? The svg.cc seemed to perform a couple of verifications, so maybe we want to keep that?

(In reply to Khaled Hosny from comment #10)
> If there are any local Mozilla patches that I missed, I’d be happy to
> incorporate them.

I think it is safe to take

https://hg.mozilla.org/try/rev/dc26e3340574
https://hg.mozilla.org/try/rev/62a1e38c7511
https://hg.mozilla.org/try/rev/01e7e8f1e4c9

as they are just some fixes for compilation errors on some platforms.

Comment 12

[Jonathan Kew [:jfkthame]]

(In reply to Frédéric Wang (:fredw) from comment #11)
> The previous reftests failed with the graphite and SVG tables. There was an
> error with a missing gfxUserFontSet prefix that might explain the build
> failures on windows (I just tried again here:
> https://tbpl.mozilla.org/?tree=Try&rev=14e131a137e9). However, that does not
> help for the reftest failures. My understanding was that we no longer need
> the graphite.cc and svg.cc files, but only to have the TableAction callback
> to skip sanitizing, right?

That was my understanding, too, but it looks like something didn't work right. If you can debug it, great; otherwise I'll try to take a look.

> The svg.cc seemed to perform a couple of
> verifications, so maybe we want to keep that?

IIRC, our code that accesses the table is designed to be robust, so I think these are redundant. We should double-check that in gfxSVGGlyphs.cpp.

> (In reply to Khaled Hosny from comment #10)
> > If there are any local Mozilla patches that I missed, I’d be happy to
> > incorporate them.
> 
> I think it is safe to take
> 
> https://hg.mozilla.org/try/rev/dc26e3340574

Is this actually needed? Looking at the code, I can't seem to make sense of it.

> https://hg.mozilla.org/try/rev/62a1e38c7511

I believe this is an artifact of the mozilla build environment, and the weird stuff we do with STL headers. Still, it's clearly harmless to take upstream.

> https://hg.mozilla.org/try/rev/01e7e8f1e4c9

This one actually fixes a possible runtime crash on certain platforms (at least some SPARC systems, IIRC), so I think it's important to take.

Comment 13

[Frédéric Wang (:fredw)]

(In reply to Jonathan Kew (:jfkthame) from comment #12)
> That was my understanding, too, but it looks like something didn't work
> right. If you can debug it, great; otherwise I'll try to take a look.

Yes, I wanted to debug that, but preferred to ask before as perhaps I was doing the wrong thing.

> > (In reply to Khaled Hosny from comment #10)
> > > If there are any local Mozilla patches that I missed, I’d be happy to
> > > incorporate them.
> > 
> > I think it is safe to take
> > 
> > https://hg.mozilla.org/try/rev/dc26e3340574
> 
> Is this actually needed? Looking at the code, I can't seem to make sense of
> it.

I think my compiler complained about this local chksum integer shadowing the chksum() function and stopped. That probably depends on the compiler version and also on other stuff like fail-on-warning.

Comment 14

[Jonathan Kew [:jfkthame]]

(In reply to Frédéric Wang (:fredw) from comment #13)
> > > https://hg.mozilla.org/try/rev/dc26e3340574
> > 
> > Is this actually needed? Looking at the code, I can't seem to make sense of
> > it.
> 
> I think my compiler complained about this local chksum integer shadowing the
> chksum() function and stopped. That probably depends on the compiler version
> and also on other stuff like fail-on-warning.

Ah, OK - that makes sense, thanks.

(Maybe just use a "tmp" variable, like in the block immediately following, rather than the chksum__ suffix? After all, this value isn't really the checksum, it's just one component being added in to it.)

Comment 15

[Khaled Hosny]

(In reply to Jonathan Kew (:jfkthame) from comment #12)
> (In reply to Frédéric Wang (:fredw) from comment #11)
> > (In reply to Khaled Hosny from comment #10)
> > > If there are any local Mozilla patches that I missed, I’d be happy to
> > > incorporate them.
> > 
> > I think it is safe to take
> > 
> > https://hg.mozilla.org/try/rev/dc26e3340574
> 
> Is this actually needed? Looking at the code, I can't seem to make sense of
> it.

Same here. Took the other two.

Comment 16

[Frédéric Wang (:fredw)]

(In reply to Khaled Hosny from comment #15)
> (In reply to Jonathan Kew (:jfkthame) from comment #12)
> > (In reply to Frédéric Wang (:fredw) from comment #11)
> > > (In reply to Khaled Hosny from comment #10)
> > > > If there are any local Mozilla patches that I missed, I’d be happy to
> > > > incorporate them.
> > > 
> > > I think it is safe to take
> > > 
> > > https://hg.mozilla.org/try/rev/dc26e3340574
> > 
> > Is this actually needed? Looking at the code, I can't seem to make sense of
> > it.
> 
> Same here. Took the other two.

See comment 13 and comment 14. Also, I don't know if that's a good idea since this is likely to be temporary, but perhaps you can add a WOFF2 build option (https://hg.mozilla.org/try/rev/a252d0c9cfa4)

Comment 17

[Frédéric Wang (:fredw)]

> Same here. Took the other two.

I can't see them on your GitHub repository...

Comment 18

[Khaled Hosny]

(In reply to Frédéric Wang (:fredw) from comment #17)
> > Same here. Took the other two.
> 
> I can't see them on your GitHub repository...

I pushed them and few other -Wshadow fixes while at it.

Comment 19

[Frédéric Wang (:fredw)]

(In reply to Khaled Hosny from comment #18)
> I pushed them and few other -Wshadow fixes while at it.

Thanks, I've refreshed my patches: https://tbpl.mozilla.org/?tree=Try&rev=ee7e0980099e

I still need to understand why https://hg.mozilla.org/try/rev/a7a2cce72a21 is not working...

Comment 20

[Jonathan Kew [:jfkthame]]

Endianness problem with the table tag, I think.

Khaled, see my comment just added to https://github.com/khaledhosny/ots/commit/ea4decac27c31d69c5ca88714428b2f0f58975e7. I think the tag should be swapped to native-endian before being exposed in an API like this. Is that OK with you?

Comment 21

[Khaled Hosny]

(In reply to Frédéric Wang (:fredw) from comment #19)
> I still need to understand why https://hg.mozilla.org/try/rev/a7a2cce72a21
> is not working...

My guess is that the comparison between the tag OTS passes and the value TRUETYPE_TAG() makes fail. I have seen something like that, but I thought it was a fault on my side. OTS has an interesting Tag() command that I’m not sure how it works https://github.com/khaledhosny/ots/blob/master/src/ots.cc#L57-L61.

Comment 22

[Frédéric Wang (:fredw)]

(In reply to Khaled Hosny from comment #21)
> (In reply to Frédéric Wang (:fredw) from comment #19)
> My guess is that the comparison between the tag OTS passes and the value
> TRUETYPE_TAG() makes fail. I have seen something like that, but I thought it
> was a fault on my side. OTS has an interesting Tag() command that I’m not
> sure how it works
> https://github.com/khaledhosny/ots/blob/master/src/ots.cc#L57-L61.

Thanks yes we've been discussing that on IRC with Jonathan. The Tag() just copies the memory occupied by the string while the TRUETYPE_TAG builds an 32bit integers from the four single chars using bit operations. Unfortunately, this means that won't be the same for little/big endians. Jonathan suggested that you apply ntohl() to htonl() as appropriate where tags are being passed through a public API, so that we will come back to what Tag() does. So I think it was a fault on my side for not reading carefully but that should be fixed on your side :-)

Comment 23

[Khaled Hosny]

Should be fixed now (my last comment was written a bit earlier but I’m experiencing some connectivity issues so it arrived much later).

Comment 24

[Frédéric Wang (:fredw)]

Thanks. Reftests work locally. I've submitted the new version to try:
https://tbpl.mozilla.org/?tree=Try&rev=048b3fcee4c4

Comment 25

[Jonathan Kew [:jfkthame]]

(In reply to Khaled Hosny from comment #23)
> Should be fixed now

Thanks!

Might be worth adding a comment in opentype-sanitizer.h where the TableActionFunc type is declared, to explicitly note that the tag is a native integer (so that the tag for the 'cmap' table is 0x636d6170, etc, regardless of the platform's endianness).

Comment 26

[Frédéric Wang (:fredw)]

Attachment: Part 1 - Delete Chromium's ots directory

Comment 27

[Frédéric Wang (:fredw)]

Attachment: Part 2 - Add khaledhosny's ots directory

Comment 28

[Frédéric Wang (:fredw)]

Attachment: Part 3 - Apply patch to make Process function externally visible for Windows DLL

Comment 29

[Frédéric Wang (:fredw)]

Attachment: Part 4 - Add #ifdef's to disable WOFF2 support

Comment 30

[Frédéric Wang (:fredw)]

Attachment: Part 5 - Add README.mozilla, moz.build files and local patches

Comment 31

[Frédéric Wang (:fredw)]

Attachment: Part 6 - Remove MOZ_OTS_REPORT_ERRORS, whitelist Graphite/SVG tables and use SetMessageCallback

Comment 32

[Jonathan Kew [:jfkthame]]

Sorry for the hassle, but would you mind replacing parts 1 and 2 with a single patch that simply overwrites our existing ots directory with Khaled's? (And removes the graphite and svg files that aren't present upstream.)

Given that the bulk of the code will be unchanged, that should give us more useful history/blame for the ots sources in our tree than a total delete-then-add sequence.

(And leave the README.mozilla and moz.build files in place at this stage; then part 5 can update them as needed instead of starting afresh. In particular for moz.build, this will make it easier to see what the build changes really are.)

Thanks!

Comment 33

[Frédéric Wang (:fredw)]

Attachment: Part 1 - Switch upstream source to khaledhosny/ots

Comment 34

[Frédéric Wang (:fredw)]

Attachment: Part 2 - Apply patch to make Process function externally visible for Windows DLL.

Comment 35

[Frédéric Wang (:fredw)]

Attachment: Part 3 - Add #ifdef's to disable WOFF2 support

Comment 36

[Frédéric Wang (:fredw)]

Attachment: Part 4 - Update README.mozilla, moz.build files and local patches

Comment 37

[Frédéric Wang (:fredw)]

Attachment: Part 5 - Remove MOZ_OTS_REPORT_ERRORS, whitelist Graphite/SVG tables and use SetMessageCallback

Comment 38

[Frédéric Wang (:fredw)]

Attachment: Part 1 - Switch upstream source to khaledhosny/ots

Sorry, I forgot to remove the svg/graphite files in the previous patch.

Comment 39

[Khaled Hosny]

(In reply to Frédéric Wang (:fredw) from comment #16)
> Also, I don't know if that's a good idea
> since this is likely to be temporary, but perhaps you can add a WOFF2 build
> option (https://hg.mozilla.org/try/rev/a252d0c9cfa4)

I’ve added OTS_DISABLE_WOFF2, defining it will disable WOFF2 support.

Comment 40

[Jonathan Kew [:jfkthame]]

Comment on attachment 8368456 [details] [diff] [review]
Part 1 - Switch upstream source to khaledhosny/ots

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

LGTM - thanks for taking care of this!

(If you want to update again to Khaled's latest commit, and adjust the following patches accordingly (I think it's just drop part 3, and set OTS_DISABLE_WOFF2 to True in moz.build), that'd be OK with me; but it's also fine to just land these as they are, and we'll pick that up next time we do an update.)

Comment 41

[Frédéric Wang (:fredw)]

OK, let's handle the WOFF2 stuff in the next update.

Comment 42

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc86d09a7d6b
https://hg.mozilla.org/integration/mozilla-inbound/rev/17cd1d2bcbd5
https://hg.mozilla.org/integration/mozilla-inbound/rev/b015ee22f280
https://hg.mozilla.org/integration/mozilla-inbound/rev/587d9ae58de6
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd5db3c0a02a

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cc86d09a7d6b
https://hg.mozilla.org/mozilla-central/rev/17cd1d2bcbd5
https://hg.mozilla.org/mozilla-central/rev/b015ee22f280
https://hg.mozilla.org/mozilla-central/rev/587d9ae58de6
https://hg.mozilla.org/mozilla-central/rev/cd5db3c0a02a