Bug 85089 – [Qt] ASSERT in FontCustomPlatformDataQt.cpp with invalid font in data URI

• Component: CSS
• Status: RESOLVED FIXED

[Keith Rosenblatt]

FontCustomPlatformData::fontPlatformData() asserts on m_rawFont.isValid() when visiting a page with a @font-face containing a data URI src with invalid font data.

[Keith Rosenblatt]

Created attachment 139254 [details]
Patch

[Pierre Rossi]

Comment on attachment 139254 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=139254&action=review

Oh, and it looks like this would allow removing fast/css/font-face-download-error.html from the Skipped list, which is very nice indeed. :)

> Source/WebCore/platform/graphics/qt/FontCustomPlatformDataQt.cpp:77
> +        delete data;

Maybe we could avoid allocating and de-allocating memory for a FontCustomPlatformData instance altogether in that case by creating a temporary QRawFont instance on the stack before that and doing the load / check there.
If it's valid, the implicit sharing makes copying it to the newly created FontCustomPlatformData almost free anyway.

[Simon Hausmann]

Comment on attachment 139254 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=139254&action=review

I think the patch is correct, but at the same time I think a layout test should cover this. It might be that there is already one, in which case this should be mentioned in the ChangeLog.

> Source/WebCore/ChangeLog:11
> +        No new tests.

Well, shouldn't there be one then? :) It doesn't sound difficult to create a layout test that triggers the assert in a debug build.

>> Source/WebCore/platform/graphics/qt/FontCustomPlatformDataQt.cpp:77
>> +        delete data;
> 
> Maybe we could avoid allocating and de-allocating memory for a FontCustomPlatformData instance altogether in that case by creating a temporary QRawFont instance on the stack before that and doing the load / check there.
> If it's valid, the implicit sharing makes copying it to the newly created FontCustomPlatformData almost free anyway.

I agree with Pierre. Either a temporary QRawFont object or putting FontCustomPlatformData into an OwnPtr that is leaked() on return.

[Keith Rosenblatt]

Created attachment 139847 [details]
Patch

[Keith Rosenblatt]

Comment on attachment 139254 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=139254&action=review

>> Source/WebCore/ChangeLog:11
>> +        No new tests.
> 
> Well, shouldn't there be one then? :) It doesn't sound difficult to create a layout test that triggers the assert in a debug build.

Indeed there should.  For some reason I thought that tests would not be run with a debug build.

>>> Source/WebCore/platform/graphics/qt/FontCustomPlatformDataQt.cpp:77
>>> +        delete data;
>> 
>> Maybe we could avoid allocating and de-allocating memory for a FontCustomPlatformData instance altogether in that case by creating a temporary QRawFont instance on the stack before that and doing the load / check there.
>> If it's valid, the implicit sharing makes copying it to the newly created FontCustomPlatformData almost free anyway.
> 
> I agree with Pierre. Either a temporary QRawFont object or putting FontCustomPlatformData into an OwnPtr that is leaked() on return.

Makes sense.  New patch attached.  Thank you for your comments.

[WebKit Review Bot]

Comment on attachment 139847 [details]
Patch

Clearing flags on attachment: 139847

Committed r115882: <http://trac.webkit.org/changeset/115882>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.