Bug 204575 – Enable full CSS color parsing within a worker for OffscreenCanvas

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Chris Lord]

After bug 182686, CSSParser::parseColorWorkerSafe was introduced, which uses CSSParserFastPaths:parseColor with a given CSSValuePool that is safe to use within the worker and thus allows CSS colour parsing within a worker.

parseColorWorkerSafe skips using the full CSS parser, however, as for that to be safe within a worker requires a more significant change. This bug tracks making full colour parsing work within workers.

My current WIP patch for this splits the functions in CSSParserFastPaths into worker-safe variants and the standard, non-worker-safe variants. This is done based on my inspection and some small changes - for the most (entire?) part, what makes colour-parsing not safe within a worker is using CSSValuePool::singleton() (the result of which should only be used on the main thread), so adding a CSSValuePool& parameter makes them safe to use within workers, assuming the given CSSValuePool was created on the same thread. Whether my analysis is accurate is of course within question.

[Chris Lord]

Created attachment 384295 [details]
Patch

[Chris Lord]

diff has made a meal of this patch, but reading the header of CSSPropertyParserHelpers gives a better idea of what's actually happened, which I'll break down here too:

- By inspection, trace through consumeColor and see what dependent functions are required and where, if at all, they employ processes that aren't safe to do in a worker.

- Add a namespace to CSSPropertyParserHelpers 'WorkerSafe' to indicate what functions are verified safe to use within a worker.

- Move consumeCommaIncludingWhiteSpace, consumeSlashIncludingWhiteSpace and consumeFunction to WorkerSafe namespace - they don't do anything that isn't safe to do within a worker that I can see.

- Add CSSValuePool parameters to consumeColor, consumeIdent, consumeAngle, consumePercent, consumeNumber, consumeNumberRaw and move them to the WorkerSafe namespace.

- Use consumeColor directly in CSSParser::parseColorWorkerSafe and use the worker-provided CSSValuePool that we already have in this function.

- Make the other minor changes required elsewhere to take into account the extra CSSValuePool parameter/different namespace of CSSPropertyParserHelpers functions.


Something I've not done - At this point, parseColor is actually just a slower version of parseColorWorkerSafe (the full-parser path in this function actually ends up calling CSSParserFastPaths::parseColor twice for colours that don't take the fast path, once directly and the second time by using CSSParser::parseSingleValue, which calls CSSParserFastPaths::maybeParseValue, which calls CSSParserFastPaths::parseColor again).

We may want to just rename parseColorWorkerSafe to parseColor, pending review/approval of the code in this patch.

[Chris Lord]

I'm re-titling this bug as further work on it has lead me to believe that this can be done without any major (or even minor) alterations to existing code, with bug 216370 giving us extra assurance that cross-thread pollution isn't happening.

The main cause of issue was the colour being parsed into a CSSPrimitiveValue, then back to a Color, but I believe this step can be bypassed, and along with skipping system colours, no static objects need to be involved.

[Chris Lord]

Ok, this quite isn't as simple as I thought - the issue I have now is that consumeNumber/consumeAngle for number parsing will use CSSValuePool::singleton - I think these functions could be converted to use consumerNumberRaw and a consumeAngleRaw could be added though, so I'm exploring that avenue.

[Chris Lord]

Created attachment 408807 [details]
Patch

[Chris Lord]

Created attachment 408809 [details]
Patch

[Chris Lord]

Created attachment 408810 [details]
Patch

[Darin Adler]

Comment on attachment 408810 [details]
Patch

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

Approach looks fine, but there is too much code duplication and we could improve the style of the new code, especially with how it uses return value.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:131
> +    bool consumeDoubleRaw(double& result)

The naming here is inconsistent with that the naming above. The functions above name what they consume, not the type they convert to. Maybe consumeValueRaw?

Also, I suggest using Optional<double> instead of an out argument (even though consumeNumberRaw doesn’t do that). The consumeNumberRaw function was written a long time ago and is using an archaic style.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:309
> +bool consumePercentRaw(CSSParserTokenRange& range, double& result)

Optional<double> return value.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:330
>  RefPtr<CSSPrimitiveValue> consumePercent(CSSParserTokenRange& range, ValueRange valueRange)

Can this call the new consumePercentRaw?

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:383
> +CSSUnitType consumeAngleRaw(CSSParserTokenRange& range, CSSParserMode cssParserMode, UnitlessQuirk unitless, double& result)

Return a structure that contains both the result and its units, do not use an out argument for one and a return value for the other.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:418
>  RefPtr<CSSPrimitiveValue> consumeAngle(CSSParserTokenRange& range, CSSParserMode cssParserMode, UnitlessQuirk unitless)

I’m concerned that we are repeating too much code; I’d like this function to simply call the other.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:600
> +    bool isPercentage = false;
> +    double colorParameter;

We should consider putting these in a structure so we can make helper functions and don’t need so many if statements and extra arguments.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:692
> +    double angleInDegrees;
> +    switch (unitType) {
> +    case CSSUnitType::CSS_DEG:
> +        angleInDegrees = value;
> +        break;
> +    case CSSUnitType::CSS_RAD:
> +        angleInDegrees = rad2deg(value);
> +        break;
> +    case CSSUnitType::CSS_GRAD:
> +        angleInDegrees = grad2deg(value);
> +        break;
> +    case CSSUnitType::CSS_TURN:
> +        angleInDegrees = turn2deg(value);
> +        break;
> +    default:
> +        ASSERT_NOT_REACHED();
> +        return Color();
> +    }

Need to find a way to share this code with computeDegrees function instead of having it twice.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:836
> +bool consumeColorWorkerSafe(CSSParserTokenRange& range, CSSParserMode cssParserMode, Color& result)

Should return Color and use "invalid" as the failure value as we would use nullptr or nullopt elsewhere.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:840
> +        // FIXME: System colours are not worker-safe

I think you mean something more like this:

    // FIXME: Need a worker-safe way to compute the system colors. For now, we detect the system color, but then intentionally fail parsing.

[Chris Lord]

Created attachment 408911 [details]
Patch

[Chris Lord]

I think I've addressed most, if not all of this in this updated patch; details in-line.

(In reply to Darin Adler from comment #8)
> Comment on attachment 408810 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=408810&action=review
> 
> Approach looks fine, but there is too much code duplication and we could
> improve the style of the new code, especially with how it uses return value.
> 
> > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:131
> > +    bool consumeDoubleRaw(double& result)
> 
> The naming here is inconsistent with that the naming above. The functions
> above name what they consume, not the type they convert to. Maybe
> consumeValueRaw?
> 
> Also, I suggest using Optional<double> instead of an out argument (even
> though consumeNumberRaw doesn’t do that). The consumeNumberRaw function was
> written a long time ago and is using an archaic style.

I removed consumeDoubleRaw and added consumePercentRaw and consumeAngleRaw - I wasn't returning the type of angle, which was exposed once the code between consumeAngle and consumeAngleRaw was shared. All the new functions return Optional<type> now and I've added an Angle type that's a struct of the CSSUnitType and the double value. CalcParser.consumeAngleRaw also uses this type to preserve the angle type.

> > 
> > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:330
> >  RefPtr<CSSPrimitiveValue> consumePercent(CSSParserTokenRange& range, ValueRange valueRange)
> 
> Can this call the new consumePercentRaw?

Yes, partially - both this and consumeAngle can share their Raw counterparts, but not for CSS calc values (which will lose their 'calc' designator if we just take the raw, calculated value). I've made this change to cut down on duplication.

> > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:383
> > +CSSUnitType consumeAngleRaw(CSSParserTokenRange& range, CSSParserMode cssParserMode, UnitlessQuirk unitless, double& result)
> 
> Return a structure that contains both the result and its units, do not use
> an out argument for one and a return value for the other.

Done with the aformentioned 'Angle' type.

> > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:600
> > +    bool isPercentage = false;
> > +    double colorParameter;
> 
> We should consider putting these in a structure so we can make helper
> functions and don’t need so many if statements and extra arguments.

I didn't make this change - I'm not against it, but given it'd only be required in one place and the pre-existing code did this, it felt somewhat overkill to include in this patch. I'm happy to make the change if you think differently though.

> > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:692
> > +    double angleInDegrees;
> > +    switch (unitType) {
> > +    case CSSUnitType::CSS_DEG:
> > +        angleInDegrees = value;
> > +        break;
> > +    case CSSUnitType::CSS_RAD:
> > +        angleInDegrees = rad2deg(value);
> > +        break;
> > +    case CSSUnitType::CSS_GRAD:
> > +        angleInDegrees = grad2deg(value);
> > +        break;
> > +    case CSSUnitType::CSS_TURN:
> > +        angleInDegrees = turn2deg(value);
> > +        break;
> > +    default:
> > +        ASSERT_NOT_REACHED();
> > +        return Color();
> > +    }
> 
> Need to find a way to share this code with computeDegrees function instead
> of having it twice.

I've added a static computeDegrees to CSSPrimitiveValue, which is shared by both this and the computeDegrees method on CSSPrimitiveValue. I'm not sure if this is the best place for it or not, but it seemed somewhat in keeping with the other static methods on CSSPrimitiveValue.

> > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:836
> > +bool consumeColorWorkerSafe(CSSParserTokenRange& range, CSSParserMode cssParserMode, Color& result)
> 
> Should return Color and use "invalid" as the failure value as we would use
> nullptr or nullopt elsewhere.

Done.

> > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:840
> > +        // FIXME: System colours are not worker-safe
> 
> I think you mean something more like this:
> 
>     // FIXME: Need a worker-safe way to compute the system colors. For now,
> we detect the system color, but then intentionally fail parsing.

Changed to this exact comment.

[Chris Lord]

Created attachment 408917 [details]
Patch

[Darin Adler]

Comment on attachment 408917 [details]
Patch

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

> Source/WebCore/css/CSSPrimitiveValue.cpp:529
> +    return CSSPrimitiveValue::computeDegrees(primitiveType(), doubleValue());

No need for the CSSPrimitiveValue prefix here.

[Chris Lord]

Created attachment 408930 [details]
Patch

[EWS]

Committed r267154: <https://trac.webkit.org/changeset/267154>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408930 [details].

[Radar WebKit Bug Importer]

<rdar://problem/68999600>