Bug 227368 - CSS parser "consume declaration" algorithm does not handle whitespace correctly
Summary: CSS parser "consume declaration" algorithm does not handle whitespace correctly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-06-24 11:20 PDT by Darin Adler
Modified: 2021-06-28 18:35 PDT (History)
14 users (show)

See Also:


Attachments
Patch (5.13 KB, patch)
2021-06-24 11:24 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (55.09 KB, patch)
2021-06-25 16:14 PDT, Darin Adler
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (56.59 KB, patch)
2021-06-25 17:29 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (68.63 KB, patch)
2021-06-28 13:44 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (68.79 KB, patch)
2021-06-28 14:34 PDT, Darin Adler
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2021-06-24 11:20:20 PDT
CSS parser "consume declaration" algorithm does not handle whitespace correctly
Comment 1 Darin Adler 2021-06-24 11:24:40 PDT
Created attachment 432195 [details]
Patch
Comment 2 Darin Adler 2021-06-25 16:14:22 PDT
Created attachment 432306 [details]
Patch
Comment 3 EWS Watchlist 2021-06-25 16:15:21 PDT
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
Comment 4 Darin Adler 2021-06-25 16:15:33 PDT
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.
Comment 5 Darin Adler 2021-06-25 17:29:53 PDT
Created attachment 432316 [details]
Patch
Comment 6 Cameron McCormack (:heycam) 2021-06-25 17:30:23 PDT
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?
Comment 7 Sam Weinig 2021-06-25 17:31:44 PDT
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 ;)
Comment 8 Darin Adler 2021-06-28 13:44:03 PDT
Created attachment 432417 [details]
Patch
Comment 9 Darin Adler 2021-06-28 13:51:53 PDT
Most recent patch is an attempt to pass all the tests in EWS, but haven’t yet incorporated Cameron’s and Sam’s comments.
Comment 10 Darin Adler 2021-06-28 14:17:59 PDT
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.
Comment 11 Darin Adler 2021-06-28 14:34:43 PDT
Created attachment 432422 [details]
Patch
Comment 12 Darin Adler 2021-06-28 18:34:29 PDT
Committed r279358 (239225@main): <https://commits.webkit.org/239225@main>
Comment 13 Radar WebKit Bug Importer 2021-06-28 18:35:17 PDT
<rdar://problem/79892563>