Bug 240593 - Remove invalid assertion in parseKeywordValue
Summary: Remove invalid assertion in parseKeywordValue
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-05-18 12:26 PDT by Alex Christensen
Modified: 2022-05-20 19:26 PDT (History)
9 users (show)

See Also:


Attachments
Patch (2.32 KB, patch)
2022-05-18 12:28 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (2.49 KB, patch)
2022-05-20 09:24 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2022-05-18 12:26:48 PDT
It can be hit.
Comment 1 Alex Christensen 2022-05-18 12:28:16 PDT
Created attachment 459552 [details]
Patch
Comment 2 Tim Nguyen (:ntim) 2022-05-19 05:33:32 PDT
Comment on attachment 459552 [details]
Patch

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

> LayoutTests/fast/css/parse-non-descriptor.html:8
> +<script>
> +    if (window.testRunner) { testRunner.dumpAsText() }
> +    onload = function() {
> +        body.style.setProperty('src', 'url(#abc)')
> +    }
> +</script>
> +<body id='body'>
> +This test passes if it does not assert.

Seems pretty wrong that we try to parse the value at all in this case, src is a descriptor for @font-face, not a CSS property (I find it unfortunate that we mix both in CSSProperties.json fwiw).

https://webkit-search.igalia.com/webkit/rev/0393f2f7c7a1e97a7a4c63441b50703cc11d493f/Source/WebCore/css/CSSProperties.json#4340-4349
Comment 3 Alex Christensen 2022-05-19 09:45:09 PDT
Do you have any better suggestions?
Comment 4 Antti Koivisto 2022-05-19 23:55:13 PDT
Comment on attachment 459552 [details]
Patch

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

> Source/WebCore/css/parser/CSSParserFastPaths.cpp:-1092
>      bool parsingDescriptor = context.enclosingRuleType && *context.enclosingRuleType != StyleRuleType::Style;
> -    ASSERT(!CSSProperty::isDescriptorOnly(propertyId) || parsingDescriptor);

Maybe the assert can be loosened instead? Does 

ASSERT(!CSSProperty::isDescriptorOnly(propertyId) || parsingDescriptor || !context.enclosingRuleType);

pass? It would be good to also add a FIXME that this is suspicious.
Comment 5 Alex Christensen 2022-05-20 09:24:52 PDT
Created attachment 459619 [details]
Patch
Comment 6 EWS 2022-05-20 19:25:16 PDT
Committed r294606 (250831@main): <https://commits.webkit.org/250831@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 459619 [details].
Comment 7 Radar WebKit Bug Importer 2022-05-20 19:26:31 PDT
<rdar://problem/93691077>