[webkit-reviews] review granted: [Bug 224426] Make FontFace parsing worker-safe : [Attachment 426012] Patch

Wed Apr 14 11:42:26 PDT 2021

Darin Adler <darin at apple.com> has granted Chris Lord <clord at igalia.com>'s
request for review:
Bug 224426: Make FontFace parsing worker-safe
https://bugs.webkit.org/show_bug.cgi?id=224426

Attachment 426012: Patch

https://bugs.webkit.org/attachment.cgi?id=426012&action=review

--- Comment #4 from Darin Adler <darin at apple.com> ---
Comment on attachment 426012
  --> https://bugs.webkit.org/attachment.cgi?id=426012
Patch

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

I think just "pool" or "valuePool" is a fine name for arguments or variables of
type CSSValuePool.

> Source/WebCore/ChangeLog:11
> +	   new CSSPropertyParserWorkerSafe and make sure they're safe to use in
a

Not 100% thrilled with the name "WorkerSafe". I feel about as good about it as
the term "ThreadSafe"; both seem insufficiently precise concepts to me.

To make this all win/win, and not *just* about fonts in onscreen canvas, it
would be great to make sure these separate property parsers are easy to use in
other cases too. Firing up the full CSS parser to parse once specific property
might be an unwanted pattern elsewhere too, unnecessarily inefficient for some
uses.

> Source/WebCore/css/FontFace.cpp:84
> -	       auto value = FontFace::parseString(string, CSSPropertySrc);
> -	       if (!is<CSSValueList>(value))
> -		   return Exception { SyntaxError };
> -	       CSSFontFace::appendSources(result->backing(),
downcast<CSSValueList>(*value), &document, false);
> -	       return { };
> +	       if (auto value =
CSSPropertyParserWorkerSafe::parseFontFaceSrc(string,
CSSParserContext(document))) {
> +		   CSSFontFace::appendSources(result->backing(), *value,
&document, false);
> +		   return { };
> +	       }
> +	       return Exception { SyntaxError };

Better: Guarding a local variable with a if.
Worse: Making the failure case the "main code" and the success case the guarded
block. There’s a clarity reason we prefer early return; too bad it’s
incompatible with guarding locals with if.

The call to CSSPropertyParserWorkerSafe::parseFontFaceSrc is so much more
verbose than the FontFace::parseString call with a really long class name. And
also, the need to explicitly write out CSSParserContext. Is there some way we
can keep this idiom a little simpler?

> Source/WebCore/css/FontFace.cpp:178
> +    if (familyNameToUse.contains('\'') && is<Document>(context) &&
downcast<Document>(context).quirks().shouldStripQuotationMarkInFontFaceSetFamil
y())

Seems like code running in workers will need quirks too, eventually, for the
same reason we need them for documents. Probably should inherit them from the
document it’s created from. This code change here is a stopgap measure.

> Source/WebCore/css/FontFace.cpp:196
> -    if (auto value = parseString(style, CSSPropertyFontStyle)) {
> +    if (auto value = CSSPropertyParserWorkerSafe::parseFontFaceStyle(style,
HTMLStandardMode, context.cssValuePool())) {

Frustrating that this code gets so verbose. Any way to cut down on the
arguments?

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:100
> +    explicit CalcParser(CSSParserTokenRange& range, CalculationCategory
destinationCategory, ValueRange valueRange = ValueRangeAll, CSSValuePool&
cssValuePool = CSSValuePool::singleton())