Bug 227368 – CSS parser "consume declaration" algorithm does not handle whitespace correctly

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

[Darin Adler]

CSS parser "consume declaration" algorithm does not handle whitespace correctly

[Darin Adler]

Created attachment 432195 [details]
Patch

[Darin Adler]

Created attachment 432306 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Darin Adler]

Anders reviewed the original version of this, but I had to change a lot more so I didn’t mark it as already reviewed by him.

[Darin Adler]

Created attachment 432316 [details]
Patch

[Cameron McCormack (:heycam)]

Comment on attachment 432306 [details]
Patch

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

Patch looks good to my non-WebKit-reviewer eyes.

Is there an existing test that smashing together two tokens with an empty valued variable reference in between does not get treated as a single token, and gets serialized correctly? If not, it would be good to have one.

> Source/WebCore/ChangeLog:24
> +        (WebCore::CSSCustomPropertyValue::equals const): Removed unneeded pointer
> +        comparison optimization. Added support for Empty value.

Can you explain why this is unneeded? (Is a similar check done elsewhere, or is it not important to optimize, or we'll never end up in here comparing the same objects?)

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2474
> +    }, [&](auto&) -> Ref<CSSValue> {

It amazes me that something like this is possible in C++!

> Source/WebCore/css/CSSCustomPropertyValue.cpp:62
> +        },  [&](const Ref<CSSVariableReferenceValue>& value) {

Nit: drop one of the two spaces after the comma.

> Source/WebCore/css/CSSCustomPropertyValue.cpp:98
>      }, [&](const Length&) {
> -        CSSTokenizer tokenizer(cssText());
> -
> +        CSSTokenizer tokenizer(customCSSText());
>          auto tokenizerRange = tokenizer.tokenRange();
>          while (!tokenizerRange.atEnd())
>              result.append(tokenizerRange.consume());
>      }, [&](const Ref<StyleImage>&) {
> -        CSSTokenizer tokenizer(cssText());
> -
> +        CSSTokenizer tokenizer(customCSSText());
>          auto tokenizerRange = tokenizer.tokenRange();
>          while (!tokenizerRange.atEnd())
>              result.append(tokenizerRange.consume());
>      });

Since any typed custom property will be serialized then re-tokenized here, can we use a `[&](auto&) { ... }` to handle both the Length and StyleImage cases?

[Sam Weinig]

Comment on attachment 432306 [details]
Patch

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

> Source/WebCore/css/CSSCustomPropertyValue.h:42
> +    struct Empty { };
> +    using VariantValue = Variant<Empty, Ref<CSSVariableReferenceValue>, CSSValueID, Ref<CSSVariableData>, Length, Ref<StyleImage>>;

C++ gives us std::monostate for this usecase https://en.cppreference.com/w/cpp/utility/variant/monostate if you want to use it instead of a custom Empty type. We of course call ours WTF::Monostate and it is in wtf/Variant.h. I don't think one is better than the other, sometimes having a good name is a better choice, just wanted to throw it out there.

> Source/WebCore/css/parser/CSSParserImpl.cpp:820
>          while (last->type() == WhitespaceToken)
>              --last;

Is the reason this is safe, because we know for sure that range is not all whitespace tokens due to the range.consumeWhitespace(); above? Seems subtle enough for a little comment.

> Source/WebCore/css/parser/CSSParserImpl.cpp:831
> +        if (last->type() == IdentToken && equalIgnoringASCIICase(last->value(), "important")) {
> +            --last;
> +            while (last->type() == WhitespaceToken)
> +                --last;
> +            if (last->type() == DelimiterToken && last->delimiter() == '!') {
> +                important = true;
> +                --last;
> +                declarationValueEnd = last + 1;
> +            }

This is also a little tricky to follow, but I think this works because the degenerate case for this function would be:

Input Range:  ":important"
Input Range As tokens: (Ident - ":", Ident - "important", EOF) 

So the check for "(last->type() == DelimiterToken && last->delimiter() == '!')", last would be on the colon, which is fine.

> Source/WebCore/css/parser/CSSParserImpl.cpp:841
>          AtomString variableName = token.value().toAtomString();

auto ;)

[Darin Adler]

Created attachment 432417 [details]
Patch

[Darin Adler]

Most recent patch is an attempt to pass all the tests in EWS, but haven’t yet incorporated Cameron’s and Sam’s comments.

[Darin Adler]

Comment on attachment 432306 [details]
Patch

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

>> Source/WebCore/ChangeLog:24
>> +        comparison optimization. Added support for Empty value.
> 
> Can you explain why this is unneeded? (Is a similar check done elsewhere, or is it not important to optimize, or we'll never end up in here comparing the same objects?)

It’s not needed because comparisons with the same object aren’t common enough to need optimization. Every == function could have this optimization, but almost none need it. I think "never" is too strong, but pretty sure it’s not at all frequent.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2474
>> +    }, [&](auto&) -> Ref<CSSValue> {
> 
> It amazes me that something like this is possible in C++!

Modern C++ is a surprisingly different language from the one that most of us think we are working in.

>> Source/WebCore/css/CSSCustomPropertyValue.cpp:62
>> +        },  [&](const Ref<CSSVariableReferenceValue>& value) {
> 
> Nit: drop one of the two spaces after the comma.

Fixed.

>> Source/WebCore/css/CSSCustomPropertyValue.cpp:98
>>      });
> 
> Since any typed custom property will be serialized then re-tokenized here, can we use a `[&](auto&) { ... }` to handle both the Length and StyleImage cases?

Yes, done.

>> Source/WebCore/css/CSSCustomPropertyValue.h:42
>> +    using VariantValue = Variant<Empty, Ref<CSSVariableReferenceValue>, CSSValueID, Ref<CSSVariableData>, Length, Ref<StyleImage>>;
> 
> C++ gives us std::monostate for this usecase https://en.cppreference.com/w/cpp/utility/variant/monostate if you want to use it instead of a custom Empty type. We of course call ours WTF::Monostate and it is in wtf/Variant.h. I don't think one is better than the other, sometimes having a good name is a better choice, just wanted to throw it out there.

Will use Monostate. Considered it and chose a custom Empty structure because it specifically means "serializes as the empty string" but after reading your comment I think Monostate is better.

>> Source/WebCore/css/parser/CSSParserImpl.cpp:820
>>              --last;
> 
> Is the reason this is safe, because we know for sure that range is not all whitespace tokens due to the range.consumeWhitespace(); above? Seems subtle enough for a little comment.

Rewrote to add a range check instead of relying on the more subtle safety guarantee.

>> Source/WebCore/css/parser/CSSParserImpl.cpp:831
>> +            }
> 
> This is also a little tricky to follow, but I think this works because the degenerate case for this function would be:
> 
> Input Range:  ":important"
> Input Range As tokens: (Ident - ":", Ident - "important", EOF) 
> 
> So the check for "(last->type() == DelimiterToken && last->delimiter() == '!')", last would be on the colon, which is fine.

Rewrote to add a range check instead of relying on the more subtle safety guarantee.

>> Source/WebCore/css/parser/CSSParserImpl.cpp:841
>>          AtomString variableName = token.value().toAtomString();
> 
> auto ;)

Done.

[Darin Adler]

Created attachment 432422 [details]
Patch

[Darin Adler]

Committed r279358 (239225@main): <https://commits.webkit.org/239225@main>

[Radar WebKit Bug Importer]

<rdar://problem/79892563>