Cherry-pick 272448.627@safari-7618-branch (51293a5). rdar://115346002

[CSS] Fix bug when using a coalesced CSSValuePair with Typed OM rdar://115346002 Reviewed by Chris Dumez. We use the CSSParser and the serialization of CSSValue to validate the input of Typed OM set(). Unfortunately, sometimes a CSSValuePair serializes to a single value while it actually contains two values: this confuses the StyleBuilder. If the pair has the same values twice "10px 10px", it serializes to only "10px", thus pass our string-based check (inside setProperty), but then crash when the actual value is a pair of length instead of a simple length. The more frequent case when the two values are distincts, such as "10px 30px", is already prevented by the string-based check. A proper fix would be to have validation method which doesn't work on the serialized string value but on the actual typed CSSValue. For the moment, we avoid crashing and warn the user with an error. * LayoutTests/fast/css/css-typed-om-typeerror-coalescing-pair-expected.txt: Added. * LayoutTests/fast/css/css-typed-om-typeerror-coalescing-pair.html: Added. * Source/WebCore/css/CSSValuePair.cpp: (WebCore::CSSValuePair::canBeCoalesced const): * Source/WebCore/css/CSSValuePair.h: * Source/WebCore/css/typedom/StylePropertyMap.cpp: (WebCore::StylePropertyMap::set): * Source/WebCore/style/StyleBuilderConverter.h: (WebCore::Style::BuilderConverter::convertLengthSizing): Canonical link: https://commits.webkit.org/272448.627@safari-7618-branch Canonical link: https://commits.webkit.org/274313.217@webkitglib/2.44
WebKit · May 13, 2024 · 5574e48 · 5574e48
1 parent b5701eb
commit 5574e48
Show file tree

Hide file tree

Showing 6 changed files with 61 additions and 3 deletions.
diff --git a/LayoutTests/fast/css/css-typed-om-typeerror-coalescing-pair-expected.txt b/LayoutTests/fast/css/css-typed-om-typeerror-coalescing-pair-expected.txt
@@ -0,0 +1,17 @@
+This test checks that we throw error when using Typed OM set() with a coalesced pair of values.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS () => {
+      var element = document.getElementById("element");
+      element.attributeStyleMap.set('width', CSSStyleValue.parse('border-bottom-left-radius', '30px'))
+    } threw exception NotSupportedError: Invalid values.
+PASS () => {
+      var element2 = document.getElementById("element2");
+      element2.attributeStyleMap.set('width', CSSStyleValue.parse('border-bottom-left-radius', '30px 10px'))
+    } threw exception TypeError: Invalid values.
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/css/css-typed-om-typeerror-coalescing-pair.html b/LayoutTests/fast/css/css-typed-om-typeerror-coalescing-pair.html
@@ -0,0 +1,25 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src="../../resources/js-test.js"></script>
+
+<div id="element">
+</div>
+<div id="element2">
+</div>
+
+<script>
+description('This test checks that we throw error when using Typed OM set() with a coalesced pair of values.')
+shouldThrow(
+    () => { 
+      var element = document.getElementById("element");
+      element.attributeStyleMap.set('width', CSSStyleValue.parse('border-bottom-left-radius', '30px'))
+    }, "'NotSupportedError: Invalid values'");
+shouldThrow(
+    () => { 
+      var element2 = document.getElementById("element2");
+      element2.attributeStyleMap.set('width', CSSStyleValue.parse('border-bottom-left-radius', '30px 10px'))
+    }, "'TypeError: Invalid values'");
+</script>
+</body>
+</html>
diff --git a/Source/WebCore/css/CSSValuePair.cpp b/Source/WebCore/css/CSSValuePair.cpp
@@ -54,6 +54,11 @@ Ref<CSSValuePair> CSSValuePair::createNoncoalescing(Ref<CSSValue> first, Ref<CSS
     return adoptRef(*new CSSValuePair(SpaceSeparator, WTFMove(first), WTFMove(second), IdenticalValueSerialization::DoNotCoalesce));
 }
 
+bool CSSValuePair::canBeCoalesced() const
+{
+    return m_coalesceIdenticalValues && m_first->equals(m_second);
+}
+
 String CSSValuePair::customCSSText() const
 {
     String first = m_first->cssText();

diff --git a/Source/WebCore/css/CSSValuePair.h b/Source/WebCore/css/CSSValuePair.h
@@ -42,6 +42,7 @@ class CSSValuePair : public CSSValue {
 
     String customCSSText() const;
     bool equals(const CSSValuePair&) const;
+    bool canBeCoalesced() const;
 
 private:
     friend bool CSSValue::addHash(Hasher&) const;

diff --git a/Source/WebCore/css/typedom/StylePropertyMap.cpp b/Source/WebCore/css/typedom/StylePropertyMap.cpp
@@ -31,6 +31,7 @@
 #include "CSSPropertyParser.h"
 #include "CSSStyleValueFactory.h"
 #include "CSSUnparsedValue.h"
+#include "CSSValuePair.h"
 #include "CSSVariableReferenceValue.h"
 #include "Document.h"
 #include "StylePropertyShorthand.h"
@@ -122,6 +123,13 @@ ExceptionOr<void> StylePropertyMap::set(Document& document, const AtomString& pr
             return Exception { ExceptionCode::TypeError, "Invalid value: This property doesn't allow <number> input"_s };
     }
 
+    // FIXME: CSSValuePair has specific behavior related to coalescing its 2 values when they are equal.
+    // Throw an error when using them with Typed OM to avoid subtle bugs when the serialization isn't representative of the value.
+    if (auto pair = dynamicDowncast<CSSValuePair>(value)) {
+        if (pair->canBeCoalesced())
+            return Exception { ExceptionCode::NotSupportedError, "Invalid values"_s };
+    }
+
     if (!setProperty(propertyID, value.releaseNonNull()))
         return Exception { ExceptionCode::TypeError, "Invalid values"_s };
 

diff --git a/Source/WebCore/style/StyleBuilderConverter.h b/Source/WebCore/style/StyleBuilderConverter.h
@@ -81,7 +81,9 @@
 namespace WebCore {
 namespace Style {
 
-// Note that we assume the CSS parser only allows valid CSSValue types.
+// FIXME: Some of those functions assume the CSS parser only allows valid CSSValue types.
+// This might not be true if we pass the CSSValue from js via CSS Typed OM.
+
 class BuilderConverter {
 public:
     static Length convertLength(const BuilderState&, const CSSValue&);
@@ -279,7 +281,7 @@ inline Length BuilderConverter::convertLengthOrAuto(const BuilderState& builderS
 
 inline Length BuilderConverter::convertLengthSizing(const BuilderState& builderState, const CSSValue& value)
 {
-    auto& primitiveValue = downcast<CSSPrimitiveValue>(value);
+    auto& primitiveValue = checkedDowncast<CSSPrimitiveValue>(value);
     switch (primitiveValue.valueID()) {
     case CSSValueInvalid:
         return convertLength(builderState, value);
@@ -304,7 +306,7 @@ inline Length BuilderConverter::convertLengthSizing(const BuilderState& builderS
         return Length(LengthType::Content);
     default:
         ASSERT_NOT_REACHED();
-        return Length();
+        return { };
     }
 }