Closed Bug 941019 Opened 11 years ago Closed 10 years ago

Update OTS to r106 or Later

Categories

(Core :: Graphics, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: unifoundry, Assigned: fredw)

References

Details

Attachments

(5 files, 7 obsolete files)

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
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
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
(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)
OK, I'll see what I can do.
Assignee: nobody → fred.wang
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.
(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.
I have updated the patches and submit them to Try server:
https://tbpl.mozilla.org/?tree=Try&rev=b0cc67c0dcff
If there are any local Mozilla patches that I missed, I’d be happy to incorporate them.
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.
(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.)
(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.
(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)
> Same here. Took the other two.

I can't see them on your GitHub repository...
(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.
(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)
(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.
(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)
Should be fixed now (my last comment was written a bit earlier but I’m experiencing some connectivity issues so it arrived much later).
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).
Attachment #8368184 - Flags: review?(jfkthame)
Attachment #8368186 - Flags: review?(jfkthame)
Attachment #8368191 - 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!
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)
Attachment #8368449 - Flags: review?(jfkthame)
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)
(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+
OK, let's handle the WOFF2 stuff in the next update.
Keywords: checkin-needed
Depends on: 968026
Depends on: 968139
Blocks: 1020927
No longer depends on: 968139
You need to log in before you can comment on or make changes to this bug.