Bug 31302 – Add WOFF support for @font-face

• Component: New Bugs
• Status: RESOLVED FIXED

[Sam Weinig]

Mozilla has added support for WOFF (https://bugzilla.mozilla.org/show_bug.cgi?id=507970).  More information on the format is available in the WOFF spec - http://people.mozilla.org/~jkew/woff/woff-spec-latest.html.

[John Lascurettes]

If it helps the decision making process:

As stated, Firefox 3.6 (and other Gecko 1.9.2 browsers) support WOFF. And according to this page, Microsoft is considering adding future support for WOFF: http://en.wikipedia.org/wiki/Web_typography#Web_Open_Font_Format . I could find no work on Presto/Opera outside of speculations. 

Services such as Typekit are also supporting WOFF and offer it as a solution for supporting custom fonts on Mobile WebKit browsers over SVG: 
http://getsatisfaction.com/typekit/topics/typekit_support_for_the_iphone 
and http://blog.typekit.com/2009/07/16/why-we-support-the-new-webfont-proposal/

[Stephen Weiss]

Just tacking on here that if Webkit would only support this feature, embedded, license-friendly fonts could actually be feasible on the web for most users (since IE and most font makers already support EOT, WOFF closes the circle for FF and hopefully also Safari/Chrome)... It would be an incredible boon at the right time.  I know we could really use it... sooner rather than later...  so please give it serious consideration!   Thanks!

[Adam Langley]

Chiming in for Chromium here.

Here's our tracking bug: http://code.google.com/p/chromium/issues/detail?id=25543

It looks like we're going to try and do this for milestone 6. We already have a transcoder (http://code.google.com/p/ots/) to protect the font libraries on our various platforms from arbitrary input. I plan on adding WOFF support to that so that the font appears to be a TTF to WebKit.

Thus our plans are mostly independent of WebKit support for WOFF. (I feel a little bad that I can't lift all ships with this work but, from a design point of view, OTS is the correct place for Chromium to add support.)

Other WebKit ports are welcome to use OTS of course.

[Maciej Stachowiak]

(In reply to comment #3)
> Chiming in for Chromium here.
> 
> Here's our tracking bug:
> http://code.google.com/p/chromium/issues/detail?id=25543
> 
> It looks like we're going to try and do this for milestone 6. We already have a
> transcoder (http://code.google.com/p/ots/) to protect the font libraries on our
> various platforms from arbitrary input. I plan on adding WOFF support to that
> so that the font appears to be a TTF to WebKit.
> 
> Thus our plans are mostly independent of WebKit support for WOFF. (I feel a
> little bad that I can't lift all ships with this work but, from a design point
> of view, OTS is the correct place for Chromium to add support.)
> 
> Other WebKit ports are welcome to use OTS of course.

It would be much better to find a way to do this that works for all WebKit ports, since we probably want WOFF --> TTF/OTF translation to work consistently.

[Adam Langley]

> It would be much better to find a way to do this that works for all WebKit
> ports, since we probably want WOFF --> TTF/OTF translation to work
> consistently.

I agree in principle. However, Chromium isn't willing to expose the system TrueType renderer to the outside world on any platform. (OS X's may be great, but Windows and Linux have a history of issues.)

Thus, in order to transcode the WOFF, we need to uncompress it in the transcoder. From there, it doesn't make sense for us to recompress in order to pass the data to a WebKit generic WOFF handler. We are just going to carry the data as TTF from that point onwards.

As https://bugs.webkit.org/show_bug.cgi?id=38217 shows, the WebKit change for Chromium is basically a single line.

Created attachment 62915 [details]
Add woffToSfnt() and use it on Mac and Windows

The test remains disabled on Windows because zlib isn’t included in the WebKit auxiliary libraries for Windows.

[WebKit Review Bot]

Attachment 62915 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/graphics/chromium/FontCustomPlatformData.cpp:210:  Line contains only semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
WebCore/platform/graphics/mac/FontCustomPlatformData.cpp:94:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/platform/graphics/WOFFToSfnt.cpp:1:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 244 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Seidel (no email)]

Attachment 62915 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3574635

[Oliver Hunt]

Comment on attachment 62915 [details]
Add woffToSfnt() and use it on Mac and Windows



WebCore/platform/graphics/WOFFToSfnt.cpp:65
 +      if (buffer->size() - offset < sizeof(value))
SharedBuffer::size() and offset are unsigned, if offset is greater than buffer->size() you overflow and get a very large value

WebCore/platform/graphics/WOFFToSfnt.cpp:77
 +      if (buffer->size() - offset < sizeof(value))
ditto

WebCore/platform/graphics/WOFFToSfnt.cpp:89
 +      if (vector.size() > vector.capacity() - sizeof(value))
Why do you need this check? why not just just use vectors automagic growing?

WebCore/platform/graphics/WOFFToSfnt.cpp:103
 +          return false;
Same question again

WebCore/platform/graphics/WOFFToSfnt.cpp:150
 +      if (woff->size() - offset < sizeof(uint16_t) + sizeof(uint16_t) + sizeof(uint32_t) + sizeof(uint32_t) + sizeof(uint32_t) + sizeof(uint32_t) + sizeof(uint32_t))
Overflow again -- unsigned maths is dangerous if you checking for overflow :-/

(In reply to comment #9)
> (From update of attachment 62915 [details])
> 
> 
> WebCore/platform/graphics/WOFFToSfnt.cpp:65
>  +      if (buffer->size() - offset < sizeof(value))
> SharedBuffer::size() and offset are unsigned, if offset is greater than buffer->size() you overflow and get a very large value

See assertion in the line above. offset can never exceed the buffer size.

> WebCore/platform/graphics/WOFFToSfnt.cpp:77
>  +      if (buffer->size() - offset < sizeof(value))
> ditto

Ditto.

> 
> WebCore/platform/graphics/WOFFToSfnt.cpp:89
>  +      if (vector.size() > vector.capacity() - sizeof(value))
> Why do you need this check? why not just just use vectors automagic growing?

The draft specification requires that the output not exceed totalSfntSize.

> WebCore/platform/graphics/WOFFToSfnt.cpp:103
>  +          return false;
> Same question again

Ditto.

> WebCore/platform/graphics/WOFFToSfnt.cpp:150
>  +      if (woff->size() - offset < sizeof(uint16_t) + sizeof(uint16_t) + sizeof(uint32_t) + sizeof(uint32_t) + sizeof(uint32_t) + sizeof(uint32_t) + sizeof(uint32_t))
> Overflow again -- unsigned maths is dangerous if you checking for overflow :-/

offset can never exceed the buffer size.

Created attachment 62916 [details]
Add woffToSfnt() and use it on Mac and Windows

Trying to fix build error on Leopard.

[Darin Adler]

Comment on attachment 62916 [details]
Add woffToSfnt() and use it on Mac and Windows

> +    while (searchRange < numTables) {
> +        entrySelector++;
> +        searchRange <<= 1;
> +    }

If numTables is a the maximum value, this will be an infinite loop.

> +    searchRange <<= 4;

If numTables is a large value, this could overflow.

> +    uint16_t rangeShift = (numTables << 4) - searchRange;

If numTables is a large value, this could overflow.

> +    if ((totalSfntSize - sfnt.size()) / (4 * sizeof(uint32_t)) < numTables)
> +        return false;

What guarantees that totalSfntSize is >= sfnt.size()? Since numTables is restricted to 16 bits, you could do this math with multiplication instead of divison.

> +        uint32_t tableCompLength;
> +        if (!readUInt32(woff, offset, tableCompLength))
> +            return false;

How about "compressed" instead of "comp"?

> +        uint32_t tableOrigLength;
> +        if (!readUInt32(woff, offset, tableOrigLength) || tableCompLength > tableOrigLength)
> +            return false;

How about "original" instead of "orig"?

> +// Returns false if woff is not a valid WOFF font. Otherwise returns true and writes
> +// the sfnt payload into sfnt.
> +bool woffToSfnt(SharedBuffer* woff, Vector<uint8_t>& sfnt);

How about a verb? Maybe convertWOFFToSfnt?

> +#if OS(LINUX)
> +class String;
> +#endif

I think you can make the forward declaration unconditional.

> +static void freeSfntData(void*, const void*data, size_t)
> +{
> +    fastFree(const_cast<void*>(data));
> +}

Missing space after the "*" before data.

> +    Vector<uint8_t> sfnt;
> +    RefPtr<SharedBuffer> sfntBuffer;
> +    if (woffToSfnt(buffer, sfnt)) {
> +        sfntBuffer = SharedBuffer::adoptVector(*reinterpret_cast<Vector<char>*>(&sfnt));
> +        buffer = sfntBuffer.get();
> +    }

The reinterpret_cast to a Vector<char>* seems really ugly and dangerous. Is there any way around this? Perhaps we could just write into a Vector<char>. When reading, the fact that we don't know if it's signed or unsigned could be an issue. But when writing, it should not.

review- because you should defend against crazy values for numTables and totalSfntSize.

(In reply to comment #12)

Thanks for the review!

> > +    if ((totalSfntSize - sfnt.size()) / (4 * sizeof(uint32_t)) < numTables)
> > +        return false;

> What guarantees that totalSfntSize is >= sfnt.size()?

sfnt’s capacity is set to totalSfntSize, and earlier calls to write*() don’t change that; and a Vector’s size never exceeds its capacity.

I am going to deal with large numTables values in all of the above and use multiplication instead of division for that last comparison.

> How about "compressed" instead of "comp"?
> How about "original" instead of "orig"?

I have named the variables after the fields in the draft specification <http://www.w3.org/TR/2010/WD-WOFF-20100727/#TableDirectory>.

> > +bool woffToSfnt(SharedBuffer* woff, Vector<uint8_t>& sfnt);
> 
> How about a verb? Maybe convertWOFFToSfnt?

Sounds good. I will change that. Should I rename the files accordingly?

> > +#if OS(LINUX)
> > +class String;
> > +#endif
> 
> I think you can make the forward declaration unconditional.

OK.

> 
> > +static void freeSfntData(void*, const void*data, size_t)
> > +{
> > +    fastFree(const_cast<void*>(data));
> > +}
> 
> Missing space after the "*" before data.

Will fix.

> > +    Vector<uint8_t> sfnt;
> > +    RefPtr<SharedBuffer> sfntBuffer;
> > +    if (woffToSfnt(buffer, sfnt)) {
> > +        sfntBuffer = SharedBuffer::adoptVector(*reinterpret_cast<Vector<char>*>(&sfnt));
> > +        buffer = sfntBuffer.get();
> > +    }
> 
> The reinterpret_cast to a Vector<char>* seems really ugly and dangerous. Is there any way around this? Perhaps we could just write into a Vector<char>. When reading, the fact that we don't know if it's signed or unsigned could be an issue. But when writing, it should not.

I am going to see if I can make sfnt a Vector<char> :-(

Created attachment 62920 [details]
Add ConvertWOFFToSfnt() and use it on Mac and Windows

In addition to addressing Darin’s comments, corrected the calculation of searchRange and entrySelector.

[WebKit Review Bot]

Attachment 62920 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/graphics/chromium/FontCustomPlatformData.cpp:210:  Line contains only semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
WebCore/platform/graphics/mac/FontCustomPlatformData.cpp:94:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 2 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Seidel (no email)]

Attachment 62920 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3567616

Created attachment 62922 [details]
Add ConvertWOFFToSfnt() and use it on Mac and Windows

Trying to fix build error on Leopard.

[Darin Adler]

(In reply to comment #13)
> > > +bool woffToSfnt(SharedBuffer* woff, Vector<uint8_t>& sfnt);
> > 
> > How about a verb? Maybe convertWOFFToSfnt?
> 
> Sounds good. I will change that. Should I rename the files accordingly?

I think that file names should be noun phrases, and function names should be verb phrases. So I would not name a file after a function.

[Darin Adler]

(In reply to comment #18)
> I think that file names should be noun phrases, and function names should be verb phrases. So I would not name a file after a function.

I see that you have done this, and I think it’s OK, but the above does represent my normal feelings on this.

(In reply to comment #18)
> (In reply to comment #13)
> > > > +bool woffToSfnt(SharedBuffer* woff, Vector<uint8_t>& sfnt);
> > > 
> > > How about a verb? Maybe convertWOFFToSfnt?
> > 
> > Sounds good. I will change that. Should I rename the files accordingly?
> 
> I think that file names should be noun phrases, and function names should be verb phrases. So I would not name a file after a function.

Well, but “accordingly” i didn’t necessarily mean “identically” (it was just my default choice). I will think of a different name.

[Darin Adler]

Comment on attachment 62922 [details]
Add ConvertWOFFToSfnt() and use it on Mac and Windows

> +    uint32_t totalSfntSize;
> +    if (!readUInt32(woff, offset, totalSfntSize))
> +        return false;
> +
> +    if (!sfnt.tryReserveCapacity(totalSfntSize))
> +        return false;

Reading a number from a file and then immediately trying to allocate a buffer of that size seems too trusting, even if we do use the version of the memory allocation function that fails if memory is exhausted. Are there any reality checks we can do on this size that can eliminate pathological cases?

>  #if !defined(BUILDING_ON_TIGER) && !defined(BUILDING_ON_LEOPARD)
> -    RetainPtr<CFDataRef> bufferData(AdoptCF, buffer->createCFData());
> -    RetainPtr<CGDataProviderRef> dataProvider(AdoptCF, CGDataProviderCreateWithCFData(bufferData.get()));
> +    RetainPtr<CGDataProviderRef> dataProvider;
> +#if !HAVE(WOFF)
> +    Vector<char> sfnt;
> +    if (convertWOFFToSfnt(buffer, sfnt)) {
> +        size_t sfntSize = sfnt.size();
> +        dataProvider.adoptCF(CGDataProviderCreateWithData(0, sfnt.releaseBuffer(), sfntSize, freeSfntData));
> +    } else
> +#endif // !HAVE(WOFF)
> +    {
> +        RetainPtr<CFDataRef> bufferData(AdoptCF, buffer->createCFData());
> +        dataProvider.adoptCF(CGDataProviderCreateWithCFData(bufferData.get()));
> +    }

I think it would be better encapsulation to put this code into a function. For one thing, the WOFF part could be an early return rather than an else with braces outside the #if, which would be less subtle. Sure, the lack of PassRetainPtr gives you a way to blame this all on me, but I still think a function would be great, since it's just SharedBuffer in and CFDataRef out.

This code treats a non-WOFF font and a malformed WOFF font, including cases of failure due to memory exhaustion, the same. Is that really the intended algorithm? If the WOFF fails for any reason, then try to treat it as another file format? That's OK, I guess, if it's really what you intend.

> +bool convertWOFFToSfnt(SharedBuffer* woff, Vector<char>& sfnt);

Maybe I should have suggested adding a try prefix on this function name. Not sure that would be an improvement, though. It would be nice some day to figure out a good naming scheme for "return a boolean to indicate success" functions.

> +    Vector<uint8_t> sfnt;
> +    if (convertWOFFToSfnt(buffer, sfnt)) {

I don't think this will compile any more, because you changed the vector type at my suggestion.

r=me, but consider improvements too

Created attachment 63035 [details]
Add ConvertWOFFToSfnt() and use it on Mac and Windows

Addressed some of the comments. The sfnt vector now grows incrementally as it is being written into, however for each font table, memory allocation is still attempted for whatever uncompressed size the WOFF table directory claims that the table has, as uncompress() offers no way to verify the claim without uncompressing into a buffer.

[WebKit Review Bot]

Attachment 63035 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/graphics/chromium/FontCustomPlatformData.cpp:210:  Line contains only semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Total errors found: 1 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 63035 [details]
Add ConvertWOFFToSfnt() and use it on Mac and Windows

> +            if (!sfnt.tryReserveCapacity(sfnt.size() + tableOrigLength))
> +                return false;

What guarantees this won't overflow? On a 32-bit system if tableOrigLength was 0xFFFFFFFF it seems like we could end up not reserving any capacity and decoding past the end of the buffer.

(In reply to comment #24)
> (From update of attachment 63035 [details])
> > +            if (!sfnt.tryReserveCapacity(sfnt.size() + tableOrigLength))
> > +                return false;
> 
> What guarantees this won't overflow? On a 32-bit system if tableOrigLength was 0xFFFFFFFF it seems like we could end up not reserving any capacity and decoding past the end of the buffer.

This earlier check:

        if (tableOrigLength > totalSfntSize || sfnt.size() > totalSfntSize - tableOrigLength)
            return false;

guarantees that sfnt.size() + tableOrigLength doesn’t overflow and is no more than totalSfntSize.

Fixed in <http://trac.webkit.org/projects/webkit/changeset/64434>.