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

Alex Christensen
Reported 2022-05-18 12:26:48 PDT
It can be hit.
Attachments
Patch (2.32 KB, patch)
2022-05-18 12:28 PDT, Alex Christensen 		no flags
DetailsFormatted DiffDiff
Patch (2.49 KB, patch)
2022-05-20 09:24 PDT, Alex Christensen 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Alex Christensen
Comment 1 2022-05-18 12:28:16 PDT
Created attachment 459552 [details] Patch
Tim Nguyen (:ntim)
Comment 2 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
Alex Christensen
Comment 3 2022-05-19 09:45:09 PDT
Do you have any better suggestions?
Antti Koivisto
Comment 4 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.
Alex Christensen
Comment 5 2022-05-20 09:24:52 PDT
Created attachment 459619 [details] Patch
EWS
Comment 6 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].
Radar WebKit Bug Importer
Comment 7 2022-05-20 19:26:31 PDT
<rdar://problem/93691077>
Note You need to log in before you can comment on or make changes to this bug.