Setting up a bug for use with experimental versions supporting new web font formats. Currently there are several formats being discussed on the www-font mailing list, primarily .webfont, EOT-Lite and ZOT. All are basically wrapper formats around OpenType/TrueType font data.

This is the patch I used to build an experimental version with .zot support. It's not complete - for one thing, I didn't implement a separate font-format hint for .zot - but seemed to work OK.

Initial patch to load EOT-Lite formatted fonts. I omitted the version check, since this is still under discussion and various tools use a myriad set of versions. Support for a version hint is not included. Testcase example: http://people.mozilla.org/~jdaggett/eotlite.html Next step is to take Jonathan's patch and add in zot support.

Adds in support for zot fonts and fixes bug in original EOT-Lite patch. Memory handling of zot fonts is hacky here, maintain a list of decode buffer pointers and clear them out when gfxUserFontSet is taken down (ick). Test page with ZOT fonts: http://www.jfkew.plus.com/zot/zot-test.html

Try server build here: http://bit.ly/eotlitezot Test pages for direct linking, EOT-Lite and ZOT formats: http://people.mozilla.org/~jdaggett/webfonts Enjoy!

https://bugzilla.mozilla.org/attachment.cgi?oldid=392422&newid=393284&action=interdiff&format=raw > GFX_USERFONT_ZOT = 4, > - GFX_USERFONT_WEBFONT = 5 > + GFX_USERFONT_WEBOTF = 5 Why did you replace .webfont rather than ZOT?

Err.... no particular reason; WebOTF is a hybrid of the two, actually. AFAIK the webfont constant was just a placeholder, it hadn't been implemented anyway.

Then GFX_USERFONT_ZOT is no longer required, I think.

Oops, previous version of the patch used an obsolete header format for WebOTF. :( Also eliminated the GFX_USERFONT_ZOT constant.

This version of the patch uses code based on the latest rev of the WOFF (formerly WebOTF) spec. There is a test page using Gentium in WOFF format, as well the spec itself and code for font-conversion tools, all available from http://www.jfkew.plus.com/woff/.

Sorry for the bugspam - previous patch wasn't properly refreshed. :(

This version of the patch reworks the management of the downloaded data, based on some discussion with Roc. The basic idea here is that once the font data is downloaded, ownership of the data is passed from the nsStreamLoader to the gfxUserFontSet, for use when creating a new gfxFontEntry. And if the font entry requires the data to persist (which is the case for Freetype fonts, I believe), it takes over responsibility for the buffer. To do this, the downloaded data is stored as an nsTArray<PRUint8>, so that we can use SwapElements to transfer ownership of the actual data buffer between objects as needed.

Comment on attachment 397546 [details] [diff] [review] updated WOFF patch with new buffer management for downloaded font data I like the fix you and roc came up with, that's much better than existing code. I'm going to ask jduell to review the netwerk portion of the patch, it makes sense to me. But I'm minusing the review here due to buffer overrun issues in the woff.c code (see below). > +// for use with EOT-Lite header, native data in little-endian form > +#ifdef IS_LITTLE_ENDIAN > +#define NS_LE_SWAP16(x) (x) > +#define NS_LE_SWAP32(x) (x) > +#else > +#define NS_LE_SWAP16(x) ((((x) & 0xff) << 8) | (((x) >> 8) & 0xff)) > +#define NS_LE_SWAP32(x) ((NS_LE_SWAP16((x) & 0xffff) << 16) | (NS_LE_SWAP16((x) >> 16))) > +#endif Don't need this code along with the AutoSwapLE classes below this, it's only for EOT-Lite usage. > + default: > + // should we have a warning here? > + break; A NS_WARNING here would be nice. > +const char * woffEncode(const char * sfntData, uint32_t sfntLen, > + uint16_t majorVersion, uint16_t minorVersion, > + uint32_t * woffLen, uint32_t * status); I don't see any reason to use char * here rather than uint8_t *. > + origLen = READ32BE(woffDir[i].origLen); > + compLen = READ32BE(woffDir[i].compLen); > + if (offset + origLen > totalLen) { > + break; > + } > + memcpy(sfntData + offset, > + woffData + READ32BE(woffDir[i].offset), origLen); This code allows reads/writes past the end of buffers. If origLen == 0xFFFFFFFF and compLen == origLen, a crash will almost certainly occur in memcpy, since the sum 'offset + origLen' would overflow into offset - 1 and satisfy the overflow check. I think something like this would fix it: if (offset + origLen > totalLen && offset + origLen > offset) + offset += origLen; + while ((offset & 3) != 0) { + sfntData[offset++] = 0; + } This also allows potentially writing off the end of the output buffer. + head = (sfntHeadTable *)(sfntData + headOffset); + head->checkSumAdjustment = 0; + csumPtr = (const uint32_t *)sfntData; + while (csumPtr < (const uint32_t *)(sfntData + totalLen)) { + csum += READ32BE(*csumPtr); + csumPtr++; + } + csum = 0xb1b0afbaU - csum; + head->checkSumAdjustment = READ32BE(csum); This seems like a bit of a spec issue to me, implementations shouldn't be required to recompute checksums, this seems like it's better handled by tools. When recalculation is needed, it would be better I think to calculate this from the checksums in the table headers rather than making yet another pass over the data file. Current implementations ignore the checksum. It would be nice to disable all the encoding code in woff.c. Maybe a simple conditional DISABLE_ENCODING or something like that, with the default being to compile everything and the conditional defined in the makefile. The sfnt structures in woff-private.h and woff.c all appear to be short aligned but I think for safety you should have a #pragma pack(1) there so that the code doesn't implicitly assume a given alignment within TrueType structures. I haven't yet taken a close look at the encoding routines, I think those may need to change based on discussions related to checksums.

This should fix the potential buffer overflows in decoding, and handles the checksum recalculation more efficiently.

Sorry for the bugspam, but I have added some more error-checking to the woff.c functions, and the Makefile option to skip compiling the encoding functions (forgot to finish that last time). Regarding the font checksum, I'd prefer not to require specific behavior in the encoder (with regard to table ordering, etc), but instead have added a note to the spec pointing out that a decoder needs to recalculate the checksum and update it in the 'head' table. I've modified the decoding function in woff.c to do this without scanning the entire font; only the new sfnt table directory needs to be summed, along with the per-table checksums (which are stable). This makes it a very cheap operation, and it's much simpler this way than trying to write and explain a spec that involves calculating a "predicted" checksum for the decoded font during the encoding process.

This patch adds an extractData() method on nsIStreamLoader to fetch the data into a caller-provided array. The issue here is that for downloaded fonts (@font-face), it makes most sense for the gfx classes to manage the lifetime of the data once it has been downloaded; depending on the font format and the platform, it may be quickly discarded or it may need to be held until we are finished with the font. The new extractData() method allows the caller to obtain the downloaded data in an nsTArray, and deletes it from the loader; by using SwapElements(), we can do this without having to copy the (potentially large) buffer of font data. The loader can then be safely destroyed, and the gfx classes have sole responsibility for the font data.

This depends on the preceding patch to nsIStreamLoader. Tryserver builds available at https://build.mozilla.org/tryserver-builds/jkew@mozilla.com-try-452d9ee9a051/

The proposed nsIStreamLoader API worries me, because it's so easy to misuse. Once you extract, you suddenly have this data pointer you were given that points to .... where? The data you extracted? Something else? I'm not quite sure what a better approach is, though. Would it make sense to just manually realloc in the streamloader instead of using a TArray and then have a success code the consumer can return for "I took ownership of this data"? That doesn't help the consumer get it into TArray form, though....

(In reply to comment #18) > I'm not quite sure what a better approach is, though. Would it make sense to > just manually realloc in the streamloader instead of using a TArray and then > have a success code the consumer can return for "I took ownership of this > data"? That's good enough for us. Should we do that?

I think I would prefer that, yeah.

Revised the network patch to allow the observer to take over the data pointer by returning a custom success code. (Corresponding WOFF patch will follow.)

This seems to work ok here on Mac, Windows, and Linux. Not thoroughly tested (including invalid fonts, etc) yet.

Comment on attachment 400443 [details] [diff] [review] revised nsIStreamLoader patch >+++ b/netwerk/base/public/nsIStreamLoader.idl >+ * If the observer wants to take over responsibility for the >+ * data buffer (result), it returns NS_SUCCESS_ADOPTED_DATA >+ * in place of NS_OK as its success code. The loader will then >+ * "forget" about the data, and not free() it in its own >+ * destructor; observer must call free() when the data is >+ * no longer required. Those should be NS_Free, not free(). And the various allocation/deallocation in this patch should use NS_Malloc, NS_Free, NS_Realloc >+++ b/netwerk/base/src/nsStreamLoader.cpp >@@ -119,17 +145,34 @@ nsStreamLoader::WriteSegmentFun(nsIInput >+ if (count > 0xffffffffU - self->mLength) { >+ return NS_ERROR_ILLEGAL_VALUE; // is there a better error to use here? Did you want PR_UNIT32_MAX instead of the hex constant? Could just use NS_ERROR_OUT_OF_MEMORY here too, I guess, but ILLEGAL_VALUE is maybe better. >+++ b/netwerk/base/src/nsStreamLoader.h >+ PRUint32 mAllocated; >+ PRUint32 mLength; Please document the difference. r=bzbarsky with those nits picked.

John, FYI: I've found a potential segfault in the woff decoder (a bogus offset in the table directory can cause it to read from an invalid address). Fixed this locally, but I'll wait for additional comments from you before posting yet another version of the patch, as I'm sure you'll find some things too.

These are some reftests for @font-face loading, based on a small font from openfontlibrary.org. We test that both .ttf and .woff formats load, then test versions with bad checksums (which should still load) and some invalid files that should NOT load. (There are of course many more kinds of invalid font error that could be tested; this is just a beginning.)

Unfortunately, I think the current patch now suffers because of the nsStreamLoader changes. With the current patch, it's unclear exactly when and where memory behind a const PRUInt8* pointer is getting deleted. Within gfxUserFontSet::OnLoadComplete, the data pointed to by aFontData is deleted *either* in PrepareOpenTypeData or in MakePlatformFont and implementations of MakePlatformFont are now obligated to free the pointer passed in. This means that there's a dangling pointer to this memory in the remainder of OnLoadComplete. This just seems like a bug waiting to happen when someone tries to change the code later without clearly understanding the peculiar pointer dance that's going on here. The original nsTArray approach seems much better, we're looking to signal in the code that an explicit pointer ownership change has occurred. Simply passing back an error code signals the change but does nothing to provide a solid mechansim for this. Maybe the OnLoadComplete should use a local nsAutoPtr to hold the pointer, then pass a reference into MakePlatformFont? The Linux version could simply transfer it to it's own nsAutoPtr (a copy explicitly changes ownership). Versions for other platforms wouldn't need to worry about it, the nsAutoPtr mechanism would do the clean up automatically. > + // mFontData holds the data used to instantiate the FT_Face; > + // this has to persist until we are finished with the face, > + // then be released with NS_Free(). > + const PRUint8* mFontData; As above, use nsAutoPtr<const PRUint8> instead? > PRBool > gfxUserFontSet::OnLoadComplete(gfxFontEntry *aFontToLoad, > nsISupports *aLoader, > const PRUint8 *aFontData, PRUint32 aLength, > nsresult aDownloadStatus) The stream loader has been trimmed out below this layer and should be trimmed out here also. > +/* These macros to read values as big-endian only work on "real" variables, > + not general expressions, because of the use of &(x), but they are > + designed to work on both BE and LE machines without the need for a > + configure check. For production code, we might want to replace this > + with something more efficient. */ > +/* read a 32-bit BigEndian value */ > +#define READ32BE(x) ( ( (uint32_t) ((uint8_t*)&(x))[0] << 24 ) + \ > + ( (uint32_t) ((uint8_t*)&(x))[1] << 16 ) + \ > + ( (uint32_t) ((uint8_t*)&(x))[2] << 8 ) + \ > + (uint32_t) ((uint8_t*)&(x))[3] ) > +/* read a 16-bit BigEndian value */ > +#define READ16BE(x) ( ( (uint16_t) ((uint8_t*)&(x))[0] << 8 ) + \ > + (uint16_t) ((uint8_t*)&(x))[1] ) I think it would be better to conditionalize these on MOZILLA_CLIENT and use the PR_SWAP macros instead when building for Mozilla. > +typedef struct { > + uint32_t tag; > + uint32_t offset; > + uint16_t oldIndex; > + uint16_t newIndex; > +} tagAndOffset; I think this can be reduced to just the offset and oldIndex fields, the tag can always be referenced from the "old" data and newIndex is simply the index in the array. The woffDecode routine calls woffGetDecodedSize which calls sanityCheck. After that woffDecode calls woffDecodeToBuffer which again calls sanityCheck. Better to have static internal functions for woffGetDecodedSize and woffDecodeToBuffer which assume sanityCheck has already been called, then call sanityCheck in woffDecode so that sanityCheck is only called once within woffDecode. Right now sanityCheck only looks at header values, might be handy if it explicitly checked all the woff directory entries to test (1) the sum of the origLen values does not overflow and is equal to totalSfntSize, (2) validate the offset/lengths point to valid ranges within the original data and (3) compLen <= origLen is true for all values (the spec says compLen > origLen is invalid but the code treats this as compLen == origLeng (i.e. uncompressed)). Within woffDecodeToBuffer: > + if (compLen < origLen) { > + uint32_t sourceOffset = READ32BE(woffDir[tableIndex].offset); > + uLongf destLen = origLen; > + if (uncompress((Bytef *)(sfntData + offset), &destLen, > + (const Bytef *)(woffData + sourceOffset), > + compLen) != Z_OK || destLen != origLen) { > + FAIL(eWOFF_compression_failure); > + } > + } else { > + uint32_t sourceOffset = READ32BE(woffDir[tableIndex].offset); > + memcpy(sfntData + offset, woffData + sourceOffset, origLen); > + } As noted above, compLen > origLen should be an error. Also, you're not validating the woff offset so a read past the end of the buffer seems possible. > + while (offset < totalLen && (offset & 3) != 0) { > + sfntData[offset++] = 0; > + } This will pad all but the last table, I think we should probably be careful about padding even the last table. > + while (csumPtr < (const uint32_t *)(sfntData + 12 + > + numTables * sizeof(sfntDirEntry))) { Change the '12' here to sizeof(sfntHeader). > + csum = 0xb1b0afbaU - csum; This should be a named constant similar to HEAD_CHECKSUM_CALC_CONST in gfxFontUtils.cpp. > + sfntData = (uint8_t *) malloc(bufLen); Calls to free, malloc and realloc should be #define'd to PR_FREE, etc. when MOZILLA_CLIENT is defined (plus prmem.h included). I only looked at the decode routines today since those are the ones that affect Mozilla production code. The encode routines don't affect the build (unless the format changes) so they can be changed without review. I'm going to play around with those tomorrow.

In woffGetMetadata: > + uint32_t offset, compLen; > + uLong origLen; > + uint8_t * data = NULL; Change uLong to uint32_t? + if (offset + compLen > woffLen) { + FAIL(eWOFF_invalid); + } This doesn't handle the case where (offset + compLen) overflows. Same comment for woffGetPrivateData. +#ifndef WOFF_DISABLE_ENCODING Maybe this should be #ifndef MOZILLA_CLIENT instead. And we shouldn't be compiling woffGetMetadata, woffGetPrivateData, woffGetFontVersion since they're not used. I also spotted a few unused variables, not sure why the compiler isn't flagging those more clearly: woffEncode - totalLength woffGetDecodedSize - numTables

(In reply to comment #29) > Maybe the OnLoadComplete should use a local > nsAutoPtr to hold the pointer, then pass a reference into MakePlatformFont? > The Linux version could simply transfer it to it's own nsAutoPtr (a copy > explicitly changes ownership). Versions for other platforms wouldn't need to > worry about it, the nsAutoPtr mechanism would do the clean up automatically. AIUI, we can't do that: the memory was allocated via NS_Alloc, not operator new. nsAutoPtr would try to use operator delete to free it, which is wrong.

Right. If we can guarantee that no one will ever use this interface across modules (which we can't even for Firefox in a non-libxul build), then we could use nsAutoPtr. As it is, we're stuck with doing it all through the XPCOM allocator...

(In reply to comment #29) > Unfortunately, I think the current patch now suffers because of the > nsStreamLoader changes. With the current patch, it's unclear exactly when and > where memory behind a const PRUInt8* pointer is getting deleted. Within > gfxUserFontSet::OnLoadComplete, the data pointed to by aFontData is deleted > *either* in PrepareOpenTypeData or in MakePlatformFont and implementations of > MakePlatformFont are now obligated to free the pointer passed in. This means > that there's a dangling pointer to this memory in the remainder of > OnLoadComplete. This just seems like a bug waiting to happen when someone > tries to change the code later without clearly understanding the peculiar > pointer dance that's going on here. > > The original nsTArray approach seems much better, we're looking to signal in > the code that an explicit pointer ownership change has occurred. Simply > passing back an error code signals the change but does nothing to provide a > solid mechansim for this. Maybe the OnLoadComplete should use a local > nsAutoPtr to hold the pointer, then pass a reference into MakePlatformFont? > The Linux version could simply transfer it to it's own nsAutoPtr (a copy > explicitly changes ownership). Versions for other platforms wouldn't need to > worry about it, the nsAutoPtr mechanism would do the clean up automatically. It's not clear to me that an nsAutoPtr-like model would be all that much clearer really. (See http://developer.mikek.dk/#post45 for some comments!) Once we start passing nsAutoPtr around between routines, the ownership and copying behavior has plenty of scope for errors. For example, passing a reference to nsAutoPtr into MakePlatformFont *might* result in the caller's pointer becoming null, depending whether the MakePlatformFont implementation (or something it calls) chooses to copy the pointer. That's not at all obvious at the call site. Anyhow, we can't use nsAutoPtr for an NS_Alloc'ed block. So what I have done instead is to add more comments, trying to document more explicitly where ownership of the font data is passing from one object to another. I've also set aFontData to NULL in OnLoadComplete after calling MakePlatformFont, so it's clear that this pointer is no longer usable. See if you think this is a reasonable approach. This update should also address the various comments relating to the WOFF-decoding functions. And while this is in flux, I've moved the call to ValidateSFNTHeaders into gfxUserFontSet, so it's done in one place before calling MakePlatformFont. The previous version of the patch broke one of the @font-face reftests (fallback after a bad src url was broken); this now passes them all reftests locally. Pushing to tryserver for confirmation.

This adds a format hint ("woff") for the new format. Does this also need to be mentioned in the CSS fonts spec or somewhere like that?

Update the reftests to include a simple check that the "woff" hint is accepted.

Comment on attachment 400838 [details] [diff] [review] fix silly error on Windows in the previous patch Looks good. Not really happy with MakePlatformFont having to delete the font data, it still feels odd but I guess I can live with this. One small nit: > +#ifdef WOFF_MOZILLA_CLIENT > +# include <prnetdb.h> > +# define READ32BE(x) PR_ntohl(x) > +# define READ16BE(x) PR_ntohs(x) > +#else Make these NS_SWAP32 and NS_SWAP16 macros rather than function call versions.

Comment on attachment 400839 [details] [diff] [review] add a "woff" format hint to @font-face I'll add the "woff" hint when I do another round of CSS3 Fonts spec edits.

Comment on attachment 400840 [details] [diff] [review] update reftests to include "woff" format hint Looks good. No need for the nofont version, just make the bad font cases != ref version.

http://hg.mozilla.org/mozilla-central/rev/1e069f8e6582 (nsIStreamLoader) http://hg.mozilla.org/mozilla-central/rev/96fadd96245e (implement WOFF) http://hg.mozilla.org/mozilla-central/rev/a32001c51b16 (format hint) http://hg.mozilla.org/mozilla-central/rev/d9171815b5b1 (reftests)

Uses a local copy of the byte-swap macros in woff-private.h because NS_SWAP16/32 are defined in a C++ header that we cannot include here.

Requesting 1.9.2 approval for the WOFF patches (three code patches, plus tests) as we would like to be able to announce that the new font format will be supported in FF3.6.

Additional comments re the request for 1.9.2 approval: This is the new web font format that has been thrashed out between ourselves and others; it has the endorsement of dozens of font designers and vendors, and offers the potential to become a truly interoperable web font solution. Other browser vendors have generally expressed a willingness to implement this if they see it being adopted; we are in a position to kick-start this process if we can announce that it will be supported in our upcoming FF 3.6 release. If we miss FF 3.6, so that WOFF support will not be in our shipping product until 3.7, we may lose the current momentum and font-vendor support, and end up with a much more fragmented web-font world as alternative EOT-based solutions (that we consider inferior) are pushed elsewhere. The code has landed on trunk without problems, and should be safe to land on 1.9.2; I have checked locally that it applies, builds, and tests successfully on the branch. I believe it to be a low-risk feature: although it is a new file format to support, the main decoding is done by zlib (stable, well-tested code), and the result is standard OpenType data that is then handled by our existing font and text code. There is no impact on font and text usage for existing pages that do not use the new format. The actual interpretation of the WOFF file structure is of course new code, so John and I have tried to ensure that this is safe from overflows, etc. On the other hand, once a WOFF file has been decoded to OpenType form, we can actually be more confident that the data we're handing to the (sometimes fragile) platform font APIs is structurally valid than with fonts that are downloaded directly in OpenType format.

(In reply to comment #44) > The code has landed on trunk without problems, and should be safe to land on > 1.9.2; I have checked locally that it applies, builds, and tests successfully > on the branch. I posted a 1.9.2 try server build with attachment 401197 [details] [diff] [review] in order to make sure that this doesn't break tests/talos on this branch. I have tagged the build with "woff"; please watch the try tree for results in the next few hours.

The patch doesn't compile, see: <http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1253644361.1253646354.5293.gz>.

As noted in comment #43, there are three code patches, plus the tests. Your tryserver build only included patch #2 of 3, so this failure is entirely expected. I am currently re-testing a complete rollup patch on an updated 1.9.2 tree, and will post it here after a successful local build and test.

1.9.2-based tryserver builds at https://build.mozilla.org/tryserver-builds/jkew@mozilla.com-try-d98c75a2ab05/

Current WOFF spec: http://people.mozilla.com/~jkew/woff/woff-2009-09-16.html

Mention added to the docs on @font-face here: https://developer.mozilla.org/en/CSS/@font-face

(In reply to comment #51) > Mention added to the docs on @font-face here: > > https://developer.mozilla.org/en/CSS/@font-face I see that document now says Gecko 1.9.2 (Firefox 3.6) added support for WOFF but this may be premature, as we don't yet know if it will be approved for 1.9.2. The code is on trunk, which means it should go into 1.9.3 (presumably for FF 3.7), but still waiting for a response to the a192 request.

Whoops, I misread a comment earlier, thought it said this was checked in on 1.9.2, but it's just that there's a patch ready. I'll revise the text.

Comment on attachment 402191 [details] [diff] [review] rolled-up patch with all code changes + tests, for 1.9.2 a192=shaver

Refreshed patch and pushed to branch: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3c6ec910a672

Fine. Now should there be another bug about EOT Lite support?

(In reply to comment #56) > Fine. Now should there be another bug about EOT Lite support? I agree. Please file it.

Filed: bug 520357