Bug 240593

Summary: Remove invalid assertion in parseKeywordValue
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: CSSAssignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, koivisto, macpherson, menard, ntim, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

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>