Update OTS to r106 or Later

RESOLVED FIXED in mozilla29

Status

()

RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: unifoundry, Assigned: fredw)

Tracking

Trunk
mozilla29
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 7 obsolete attachments)

3.54 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
4.05 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
17.02 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
4.33 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
337.74 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
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
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).
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 2

5 years ago
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/
Blocks: 407059, 959578
Depends on: 747816
(Assignee)

Comment 3

5 years ago
(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?
Flags: needinfo?(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.)
Flags: needinfo?(jfkthame)
(Assignee)

Comment 5

5 years ago
OK, I'll see what I can do.
Assignee: nobody → fred.wang
(Assignee)

Comment 6

5 years ago
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?
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.
(Assignee)

Comment 8

5 years ago
(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.
(Assignee)

Comment 9

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

Comment 10

5 years ago
If there are any local Mozilla patches that I missed, I’d be happy to incorporate them.
(Assignee)

Comment 11

5 years ago
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.
(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.
(Assignee)

Comment 13

5 years ago
(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.
(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

5 years ago
(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.
(Assignee)

Comment 16

5 years ago
(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)
(Assignee)

Comment 17

5 years ago
> Same here. Took the other two.

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

Comment 18

5 years ago
(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.
(Assignee)

Comment 19

5 years ago
(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...
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?
Flags: needinfo?(khaledhosny)

Comment 21

5 years ago
(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.
(Assignee)

Comment 22

5 years ago
(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 :-)
Flags: needinfo?(khaledhosny)

Comment 23

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

Comment 24

5 years ago
Thanks. Reftests work locally. I've submitted the new version to try:
https://tbpl.mozilla.org/?tree=Try&rev=048b3fcee4c4
(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).
(Assignee)

Comment 26

5 years ago
Created attachment 8368184 [details] [diff] [review]
Part 1 - Delete Chromium's ots directory
Attachment #8368184 - Flags: review?(jfkthame)
(Assignee)

Comment 27

5 years ago
Created attachment 8368186 [details] [diff] [review]
Part 2 - Add khaledhosny's ots directory
Attachment #8368186 - Flags: review?(jfkthame)
(Assignee)

Comment 28

5 years ago
Created attachment 8368189 [details] [diff] [review]
Part 3 - Apply patch to make Process function externally visible for Windows DLL
Attachment #8368189 - Flags: review?(jfkthame)
(Assignee)

Comment 29

5 years ago
Created attachment 8368191 [details] [diff] [review]
Part 4 - Add #ifdef's to disable WOFF2 support
Attachment #8368191 - Flags: review?(jfkthame)
(Assignee)

Comment 30

5 years ago
Created attachment 8368193 [details] [diff] [review]
Part 5 - Add README.mozilla, moz.build files and local patches
Attachment #8368193 - Flags: review?(jfkthame)
(Assignee)

Comment 31

5 years ago
Created attachment 8368194 [details] [diff] [review]
Part 6 - Remove MOZ_OTS_REPORT_ERRORS, whitelist Graphite/SVG tables and use SetMessageCallback
Attachment #8368194 - Flags: review?(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!
(Assignee)

Comment 33

5 years ago
Created attachment 8368445 [details] [diff] [review]
Part 1 - Switch upstream source to khaledhosny/ots
Attachment #8368184 - Attachment is obsolete: true
Attachment #8368186 - Attachment is obsolete: true
Attachment #8368189 - Attachment is obsolete: true
Attachment #8368191 - Attachment is obsolete: true
Attachment #8368193 - Attachment is obsolete: true
Attachment #8368194 - Attachment is obsolete: true
Attachment #8368184 - Flags: review?(jfkthame)
Attachment #8368186 - Flags: review?(jfkthame)
Attachment #8368189 - Flags: review?(jfkthame)
Attachment #8368191 - Flags: review?(jfkthame)
Attachment #8368193 - Flags: review?(jfkthame)
Attachment #8368194 - Flags: review?(jfkthame)
Attachment #8368445 - Flags: review?(jfkthame)
(Assignee)

Comment 34

5 years ago
Created attachment 8368447 [details] [diff] [review]
Part 2 - Apply patch to make Process function externally visible for Windows DLL.
Attachment #8368447 - Flags: review?(jfkthame)
(Assignee)

Comment 35

5 years ago
Created attachment 8368448 [details] [diff] [review]
Part 3 - Add #ifdef's to disable WOFF2 support
Attachment #8368448 - Flags: review?(jfkthame)
(Assignee)

Comment 36

5 years ago
Created attachment 8368449 [details] [diff] [review]
Part 4 - Update README.mozilla, moz.build files and local patches
(Assignee)

Updated

5 years ago
Attachment #8368449 - Flags: review?(jfkthame)
(Assignee)

Comment 37

5 years ago
Created attachment 8368450 [details] [diff] [review]
Part 5 - Remove MOZ_OTS_REPORT_ERRORS, whitelist Graphite/SVG tables and use SetMessageCallback
Attachment #8368450 - Flags: review?(jfkthame)
(Assignee)

Comment 38

5 years ago
Created attachment 8368456 [details] [diff] [review]
Part 1 - Switch upstream source to khaledhosny/ots

Sorry, I forgot to remove the svg/graphite files in the previous patch.
Attachment #8368445 - Attachment is obsolete: true
Attachment #8368445 - Flags: review?(jfkthame)
Attachment #8368456 - Flags: review?(jfkthame)

Comment 39

5 years ago
(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 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.)
Attachment #8368456 - Flags: review?(jfkthame) → review+
Attachment #8368447 - Flags: review?(jfkthame) → review+
Attachment #8368448 - Flags: review?(jfkthame) → review+
Attachment #8368449 - Flags: review?(jfkthame) → review+
Attachment #8368450 - Flags: review?(jfkthame) → review+
(Assignee)

Comment 41

5 years ago
OK, let's handle the WOFF2 stuff in the next update.
Keywords: checkin-needed
Depends on: 968026
Depends on: 968139
(Assignee)

Updated

4 years ago
Blocks: 1020927
(Assignee)

Updated

4 years ago
No longer depends on: 968139
You need to log in before you can comment on or make changes to this bug.