REGRESSION(r191731): SVGPatternElement can only reference another SVGPatternElement...
authorsaid@apple.com <said@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 21 Sep 2017 00:35:41 +0000 (00:35 +0000)
committersaid@apple.com <said@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 21 Sep 2017 00:35:41 +0000 (00:35 +0000)
https://bugs.webkit.org/show_bug.cgi?id=176221

Reviewed by Tim Horton.

Source/WebCore:

According to the specs:

https://www.w3.org/TR/SVG11/filters.html#FilterElementHrefAttribute
https://www.w3.org/TR/SVG11/pservers.html#LinearGradientElementHrefAttribute
https://www.w3.org/TR/SVG11/pservers.html#RadialGradientElementHrefAttribute
https://www.w3.org/TR/SVG11/pservers.html#PatternElementHrefAttribute

The xlink:href attribute of the SVG filter, gradient and pattern elements
must reference another element within the current SVG of the same type.

In r191731, the code of SVGPatternElement::collectPatternAttributes() was
removed and replaced by RenderSVGResourcePattern::collectPatternAttributes()
to avoid cyclic reference in the pattern element. The problem is the old
code used to check whether the referenced element is<SVGPatternElement>
before casting it. This code was not copied to the new function. So we
now allow the SVGPatternElement to reference any SVG resource element.

To fix this issue, we need to prevent SVGResources from chaining an incorrect
type of element to the SVG filter, gradient and pattern elements.

We also need to use the SVGResources for getting the referenced element
when collecting the attributes for the gradient elements. SVGResources solves
the cyclic referencing issue so there is no need to repeat the same code
in many places. Also, from now on the SVGResources will have valid linked
resource only. So casting the referenced element should always be valid.

Tests: svg/custom/pattern-invalid-content-inheritance.svg

* rendering/svg/RenderSVGResourcePattern.cpp:
(WebCore::RenderSVGResourcePattern::collectPatternAttributes const): Asserts
the linkedResource is of type RenderSVGResourcePattern.
* rendering/svg/SVGResources.cpp:
(WebCore::SVGResources::SVGResources):
(WebCore::isChainableResource): Ensure that an SVG resource can reference
only an SVG resource with the valid type.
(WebCore::SVGResources::buildCachedResources):
* rendering/svg/SVGResources.h:

LayoutTests:

* svg/custom/pattern-invalid-content-inheritance-expected.svg: Added.
* svg/custom/pattern-invalid-content-inheritance.svg: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@222304 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/svg/custom/pattern-invalid-content-inheritance-expected.svg [new file with mode: 0644]
LayoutTests/svg/custom/pattern-invalid-content-inheritance.svg [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/rendering/svg/RenderSVGResourcePattern.cpp
Source/WebCore/rendering/svg/SVGResources.cpp
Source/WebCore/rendering/svg/SVGResources.h

index a48f12a..94f38dd 100644 (file)
@@ -1,3 +1,13 @@
+2017-09-20  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
+        REGRESSION(r191731): SVGPatternElement can only reference another SVGPatternElement in the same SVG document
+        https://bugs.webkit.org/show_bug.cgi?id=176221
+
+        Reviewed by Tim Horton.
+
+        * svg/custom/pattern-invalid-content-inheritance-expected.svg: Added.
+        * svg/custom/pattern-invalid-content-inheritance.svg: Added.
+
 2017-09-20  Per Arne Vollan  <pvollan@apple.com>
 
         Mark transitions/transition-display-property.html as flaky on Windows.
diff --git a/LayoutTests/svg/custom/pattern-invalid-content-inheritance-expected.svg b/LayoutTests/svg/custom/pattern-invalid-content-inheritance-expected.svg
new file mode 100644 (file)
index 0000000..aa22ba0
--- /dev/null
@@ -0,0 +1,3 @@
+<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
+    <rect fill="green" x="10" y="10" width="100" height="100"/>
+</svg>
diff --git a/LayoutTests/svg/custom/pattern-invalid-content-inheritance.svg b/LayoutTests/svg/custom/pattern-invalid-content-inheritance.svg
new file mode 100644 (file)
index 0000000..63a7a1b
--- /dev/null
@@ -0,0 +1,9 @@
+<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
+    <pattern id="pattern" height="100" width="100" patternUnits="userSpaceOnUse" xlink:href="#filter">
+        <rect width="100" height="100" fill="green"/>
+    </pattern>
+    <filter id="filter" filterUnits="userSpaceOnUse" xlink:href="#pattern">
+      <feFlood x="120" y="10" width="100" height="100" flood-color="green"/>
+    </filter>
+    <rect fill="url(#pattern)" x="10" y="10" width="100" height="100"/>
+</svg>
index 8a6cf56..c6c04ef 100644 (file)
@@ -1,3 +1,48 @@
+2017-09-20  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
+        REGRESSION(r191731): SVGPatternElement can only reference another SVGPatternElement in the same SVG document
+        https://bugs.webkit.org/show_bug.cgi?id=176221
+
+        Reviewed by Tim Horton.
+
+        According to the specs:
+
+        https://www.w3.org/TR/SVG11/filters.html#FilterElementHrefAttribute
+        https://www.w3.org/TR/SVG11/pservers.html#LinearGradientElementHrefAttribute
+        https://www.w3.org/TR/SVG11/pservers.html#RadialGradientElementHrefAttribute
+        https://www.w3.org/TR/SVG11/pservers.html#PatternElementHrefAttribute
+
+        The xlink:href attribute of the SVG filter, gradient and pattern elements
+        must reference another element within the current SVG of the same type.
+
+        In r191731, the code of SVGPatternElement::collectPatternAttributes() was
+        removed and replaced by RenderSVGResourcePattern::collectPatternAttributes()
+        to avoid cyclic reference in the pattern element. The problem is the old
+        code used to check whether the referenced element is<SVGPatternElement>
+        before casting it. This code was not copied to the new function. So we
+        now allow the SVGPatternElement to reference any SVG resource element.
+
+        To fix this issue, we need to prevent SVGResources from chaining an incorrect
+        type of element to the SVG filter, gradient and pattern elements.
+
+        We also need to use the SVGResources for getting the referenced element
+        when collecting the attributes for the gradient elements. SVGResources solves
+        the cyclic referencing issue so there is no need to repeat the same code
+        in many places. Also, from now on the SVGResources will have valid linked
+        resource only. So casting the referenced element should always be valid.
+
+        Tests: svg/custom/pattern-invalid-content-inheritance.svg
+
+        * rendering/svg/RenderSVGResourcePattern.cpp:
+        (WebCore::RenderSVGResourcePattern::collectPatternAttributes const): Asserts
+        the linkedResource is of type RenderSVGResourcePattern.
+        * rendering/svg/SVGResources.cpp:
+        (WebCore::SVGResources::SVGResources):
+        (WebCore::isChainableResource): Ensure that an SVG resource can reference
+        only an SVG resource with the valid type.
+        (WebCore::SVGResources::buildCachedResources):
+        * rendering/svg/SVGResources.h:
+
 2017-09-20  Daniel Bates  <dabates@apple.com>
 
         Spelling and grammar dots should not overlap
index 980ee3e..b70189f 100644 (file)
@@ -64,6 +64,7 @@ void RenderSVGResourcePattern::collectPatternAttributes(PatternAttributes& attri
         pattern.collectPatternAttributes(attributes);
 
         auto* resources = SVGResourcesCache::cachedResourcesForRenderer(*current);
+        ASSERT_IMPLIES(resources && resources->linkedResource(), is<RenderSVGResourcePattern>(resources->linkedResource()));
         current = resources ? downcast<RenderSVGResourcePattern>(resources->linkedResource()) : nullptr;
     }
 }
index ea9b452..f0237e3 100644 (file)
@@ -39,7 +39,6 @@
 namespace WebCore {
 
 SVGResources::SVGResources()
-    : m_linkedResource(0)
 {
 }
 
@@ -154,6 +153,21 @@ static inline String targetReferenceFromResource(SVGElement& element)
     return SVGURIReference::fragmentIdentifierFromIRIString(target, element.document());
 }
 
+static inline bool isChainableResource(const SVGElement& element, const SVGElement& linkedResource)
+{
+    if (is<SVGPatternElement>(element))
+        return is<SVGPatternElement>(linkedResource);
+
+    if (is<SVGGradientElement>(element))
+        return is<SVGGradientElement>(linkedResource);
+    
+    if (is<SVGFilterElement>(element))
+        return is<SVGFilterElement>(linkedResource);
+
+    ASSERT_NOT_REACHED();
+    return false;
+}
+
 static inline RenderSVGResourceContainer* paintingResourceFromSVGPaint(Document& document, const SVGPaintType& paintType, const String& paintUri, AtomicString& id, bool& hasPendingResource)
 {
     if (paintType != SVG_PAINTTYPE_URI && paintType != SVG_PAINTTYPE_URI_RGBCOLOR && paintType != SVG_PAINTTYPE_URI_CURRENTCOLOR)
@@ -274,10 +288,13 @@ bool SVGResources::buildCachedResources(const RenderElement& renderer, const Ren
 
     if (chainableResourceTags().contains(tagName)) {
         AtomicString id(targetReferenceFromResource(element));
-        if (setLinkedResource(getRenderSVGResourceContainerById(document, id)))
-            foundResources = true;
-        else
+        auto* linkedResource = getRenderSVGResourceContainerById(document, id);
+        if (!linkedResource)
             registerPendingResource(extensions, id, element);
+        else if (isChainableResource(element, linkedResource->element())) {
+            setLinkedResource(linkedResource);
+            foundResources = true;
+        }
     }
 
     return foundResources;
index e92b761..365db1b 100644 (file)
@@ -155,7 +155,7 @@ private:
     std::unique_ptr<ClipperFilterMaskerData> m_clipperFilterMaskerData;
     std::unique_ptr<MarkerData> m_markerData;
     std::unique_ptr<FillStrokeData> m_fillStrokeData;
-    RenderSVGResourceContainer* m_linkedResource;
+    RenderSVGResourceContainer* m_linkedResource { nullptr };
 };
 
 } // namespace WebCore