Bug 203127 – ctx.font = "" asserts in CSS parser

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Dean Jackson]

ctx.font = "" asserts in CSS parser

[Radar WebKit Bug Importer]

<rdar://problem/56391016>

[Dean Jackson]

Created attachment 381252 [details]
Patch

[Devin Rousso]

Comment on attachment 381252 [details]
Patch

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

r=me

> Source/WebCore/ChangeLog:13
> +        will assert. This was the only case I could find where we sidestepped
> +        most of the parsing infrastructure and injected a raw string.

For the record (which I think is what you were referring to), there are a number of other places where we call `CSSParser::parseValue`, and most of them already do a check for an empty string before calling it, but not all of them.

The only other one I could is `CSSFontFaceSet::matchingFacesExcludingPreinstalledFonts` (which looks to be reachable from JavaScript via `FontFaceSet.prototype.check`), so perhaps we should add a similar check there as well.

> LayoutTests/http/wpt/2dcontext/text-styles/2d.text.font.parse.invalid.html:21
> +ctx.font = '20px serif';

I realize this is a WPT, but should we have a test to check that setting a valid value after an invalid one does in fact change the `font`, just to make sure that an invalid value doesn't "permanently" mess everything up?
```
    ctx.font = '20px serif';
    ctx.font = 'invalid';
    ctx.font = '10px sans-serif';
    _assertSame(ctx.font, '10px sans-serif', "ctx.font", "'10px sans-serif'");
```

[Dean Jackson]

Comment on attachment 381252 [details]
Patch

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

>> LayoutTests/http/wpt/2dcontext/text-styles/2d.text.font.parse.invalid.html:21
>> +ctx.font = '20px serif';
> 
> I realize this is a WPT, but should we have a test to check that setting a valid value after an invalid one does in fact change the `font`, just to make sure that an invalid value doesn't "permanently" mess everything up?
> ```
>     ctx.font = '20px serif';
>     ctx.font = 'invalid';
>     ctx.font = '10px sans-serif';
>     _assertSame(ctx.font, '10px sans-serif', "ctx.font", "'10px sans-serif'");
> ```

We probably should. Feel free to make a WPT request :)

[Dean Jackson]

Will do FontFaceSet separately.

[Dean Jackson]

Committed r251270: <https://trac.webkit.org/changeset/251270>