Closed Bug 833283 Opened 9 years ago Closed 9 years ago
remove the option to bypass OTS for downloaded fonts
We landed the OTS font sanitizer for gecko-1.9.2 (and then backported to 1.9.1). It's been switched on by default ever since; although there's a pref to bypass it, we don't consider that a supported or secure option. At this point, I think we can consider it stable/tested/trusted enough that there's no longer any need for a preference to disable it; we can make it a non-negotiable part of downloaded font support. Eliminating that pref also allows us to remove the old (and *much* less thorough) font validation we used to use, and the separate WOFF support code, as OTS handles WOFF decoding internally.
Minor correction - we mustn't discard the original WOFF data until we've copied the metadata block, if any.
Attachment #704829 - Flags: review?(jdaggett)
Comment on attachment 704829 [details] [diff] [review] remove the option to bypass OTS for downloaded fonts Looks good. I think we could still probably reduce the overall number of times the font data gets copied around but that's easy enough to deal with later.
Attachment #704829 - Flags: review?(jdaggett) → review+
Target Milestone: --- → mozilla21
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
(In reply to Ryan VanderMeulen [:RyanVM][UTC-4] from comment #6) > https://hg.mozilla.org/mozilla-central/rev/32a44be4703c Ryan, not sure why you're connecting that changeset to this bug. Whoops?
(In reply to John Daggett (:jtd) from comment #7) > (In reply to Ryan VanderMeulen [:RyanVM][UTC-4] from comment #6) > > https://hg.mozilla.org/mozilla-central/rev/32a44be4703c > > Ryan, not sure why you're connecting that changeset to this bug. Whoops? It was in bug 833382, but had a typo in the bug number in the changeset pointing to this bug instead.
(And note that Ryan is presumably just using https://mcmerge.mozilla.org/ to put the data from merges into Bugzilla; though m-cMerge allows manual review, the person doing the merge can't catch everything, and it's really just the fault of the data in the changesets he's merging.)
Not having the option is, rather inconvenient. Would you reconsider this change?
Hmm. Care to be more explicit about the inconvenience? I don't think we'd want to directly revert the change here (which allowed us to drop a whole bunch of code that we don't want to keep maintaining), but we might be able to solve it in a simpler way via the new OTS TableAction callback.
(In reply to Jonathan Kew (:jfkthame) from comment #11) > Hmm. Care to be more explicit about the inconvenience? > > I don't think we'd want to directly revert the change here (which allowed us > to drop a whole bunch of code that we don't want to keep maintaining), but > we might be able to solve it in a simpler way via the new OTS TableAction > callback. I've been working on handcrafting a font for certain HTML uses. . I've hit many OTS issues, which I've reported and fixed. However, without a preference for circumventing OTS, I have no way of testing my font short of recompiling Firefox. I understand that this is also true with any other part of Firefox. Still would be very convenient to either be able to disable OTS, or make Firefox use my custom OTS. I understand that neither option may be very interesting from a maintenance point of view for you. Still wanted to raise the need.  https://groups.google.com/forum/#!topic/fonttools/JR1yvwyHj-I
Well, we could trivially support a pref that makes the TableAction callback return PASSTHRU for all tables. However, I just tried this, and it doesn't work, because OTS no longer notices what tables are present at all, and rejects all fonts as having neither TT nor PS outlines. (And would presumably reject for several other reasons as well.) If you/Khaled can tweak OTS such that using PASSTHRU for all standard tables works as expected (i.e. the top-level OTS code still knows what tables were present, so it can accept the font as a whole) then I think we could give you a solution. (It'd be best to file a new bug for the Gecko side of this, though, rather than continuing here in an old resolved:fixed bug.)
Sounds good. Will do. Thanks.
You need to log in before you can comment on or make changes to this bug.