Attachment 443895 Details for Bug 232972 – Patch

[patch] Patch

bug-232972-20211110185727.patch (text/plain), 7.35 KB, created by Said Abou-Hallawa on 2021-11-10 18:57:28 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Said Abou-Hallawa

Created: 2021-11-10 18:57:28 PST

Size: 7.35 KB

patch

obsolete

>Subversion Revision: 285597
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 1e0ed974db178ebdc38bce79acf174b5ed32e0a2..2a68eadbb3abf0328c496deeb61caa435bdd76f5 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,27 @@
>+2021-11-10  Said Abou-Hallawa  <said@apple.com>
>+
>+        REGRESSION(r285481): Infinite recursion with cyclic filter reference
>+        https://bugs.webkit.org/show_bug.cgi?id=232972
>+        rdar://85264240
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Before r285481, we were creating the ImageBuffer of the referenced SVGElement
>+        for the FEImage through RenderSVGResourceFilter::postApplyResource(). Now
>+        we create this ImageBuffer through RenderSVGResourceFilter::applyResource(). 
>+        The difference is at the end of RenderSVGResourceFilter::applyResource()
>+        we add an entry to m_rendererFilterDataMap. This entry was preventing 
>+        trying to rebuild the SVGFilter for the same renderer if there is a
>+        cyclic reference.
>+
>+        The fix is to add the entry in m_rendererFilterDataMap before creating the
>+        SVGFilter. If an error happens, this entry will be removed before returning.
>+
>+        Test: svg/filters/feImage-cyclic-reference.svg
>+
>+        * rendering/svg/RenderSVGResourceFilter.cpp:
>+        (WebCore::RenderSVGResourceFilter::applyResource):
>+
> 2021-11-10  Said Abou-Hallawa  <said@apple.com>
> 
>         [GPU Process] Make SVGFilter and CSSFilter work in the same coordinates system
>diff --git a/Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp b/Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp
>index 537e655ee5d666e4f8550874f921ab23f01e0e54..b0270a13bbfb1b1959916d7cb823ef4cebfdf6ab 100644
>--- a/Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp
>+++ b/Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp
>@@ -130,17 +130,23 @@ bool RenderSVGResourceFilter::applyResource(RenderElement& renderer, const Rende
>         return false; // Already built, or we're in a cycle, or we're marked for removal. Regardless, just do nothing more now.
>     }
> 
>-    auto filterData = makeUnique<FilterData>();
>+    auto addResult = m_rendererFilterDataMap.set(&renderer, makeUnique<FilterData>());
>+    auto filterData = addResult.iterator->value.get();
>+    
>     FloatRect targetBoundingBox = renderer.objectBoundingBox();
> 
>     filterData->boundaries = SVGLengthContext::resolveRectangle<SVGFilterElement>(&filterElement(), filterElement().filterUnits(), targetBoundingBox);
>-    if (filterData->boundaries.isEmpty())
>+    if (filterData->boundaries.isEmpty()) {
>+        m_rendererFilterDataMap.remove(&renderer);
>         return false;
>+    }
> 
>     // Determine absolute transformation matrix for filter. 
>     AffineTransform absoluteTransform = SVGRenderingContext::calculateTransformationToOutermostCoordinateSystem(renderer);
>-    if (!absoluteTransform.isInvertible())
>+    if (!absoluteTransform.isInvertible()) {
>+        m_rendererFilterDataMap.remove(&renderer);
>         return false;
>+    }
> 
>     // Eliminate shear of the absolute transformation matrix, to be able to produce unsheared tile images for feTile.
>     FloatSize filterScale(absoluteTransform.xScale(), absoluteTransform.yScale());
>@@ -160,13 +166,17 @@ bool RenderSVGResourceFilter::applyResource(RenderElement& renderer, const Rende
> 
>     // Create all relevant filter primitives.
>     filterData->builder = buildPrimitives(*filterData->filter);
>-    if (!filterData->builder)
>+    if (!filterData->builder) {
>+        m_rendererFilterDataMap.remove(&renderer);
>         return false;
>+    }
> 
>     static const unsigned maxTotalOfEffectInputs = 100;
>     FilterEffect* lastEffect = filterData->builder->lastEffect();
>-    if (!lastEffect || lastEffect->totalNumberOfEffectInputs() > maxTotalOfEffectInputs)
>+    if (!lastEffect || lastEffect->totalNumberOfEffectInputs() > maxTotalOfEffectInputs) {
>+        m_rendererFilterDataMap.remove(&renderer);
>         return false;
>+    }
> 
>     LOG_WITH_STREAM(Filters, stream << "RenderSVGResourceFilter::applyResource\n" << *filterData->builder->lastEffect());
> 
>@@ -183,9 +193,8 @@ bool RenderSVGResourceFilter::applyResource(RenderElement& renderer, const Rende
>     // If the drawingRegion is empty, we have something like <g filter=".."/>.
>     // Even if the target objectBoundingBox() is empty, we still have to draw the last effect result image in postApplyResource.
>     if (filterData->drawingRegion.isEmpty()) {
>-        ASSERT(!m_rendererFilterDataMap.contains(&renderer));
>+        ASSERT(m_rendererFilterDataMap.contains(&renderer));
>         filterData->savedContext = context;
>-        m_rendererFilterDataMap.set(&renderer, WTFMove(filterData));
>         return false;
>     }
> 
>@@ -200,9 +209,8 @@ bool RenderSVGResourceFilter::applyResource(RenderElement& renderer, const Rende
> #endif
>     auto sourceGraphic = SVGRenderingContext::createImageBuffer(filterData->drawingRegion, effectiveTransform, colorSpace, renderingMode, context);
>     if (!sourceGraphic) {
>-        ASSERT(!m_rendererFilterDataMap.contains(&renderer));
>+        ASSERT(m_rendererFilterDataMap.contains(&renderer));
>         filterData->savedContext = context;
>-        m_rendererFilterDataMap.set(&renderer, WTFMove(filterData));
>         return false;
>     }
>     
>@@ -216,9 +224,7 @@ bool RenderSVGResourceFilter::applyResource(RenderElement& renderer, const Rende
> 
>     context = &sourceGraphicContext;
> 
>-    ASSERT(!m_rendererFilterDataMap.contains(&renderer));
>-    m_rendererFilterDataMap.set(&renderer, WTFMove(filterData));
>-
>+    ASSERT(m_rendererFilterDataMap.contains(&renderer));
>     return true;
> }
> 
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 5e556e42c4ad6bb535dc09681002ed08eb1768ad..a6e5c30cb626cf5339dfa4d70bc234c90b6726d1 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,14 @@
>+2021-11-10  Said Abou-Hallawa  <said@apple.com>
>+
>+        REGRESSION(r285481): Infinite recursion with cyclic filter reference
>+        https://bugs.webkit.org/show_bug.cgi?id=232972
>+        rdar://85264240
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * svg/filters/feImage-cyclic-reference-expected.txt: Added.
>+        * svg/filters/feImage-cyclic-reference.svg: Added.
>+
> 2021-11-10  Said Abou-Hallawa  <said@apple.com>
> 
>         [GPU Process] Make SVGFilter and CSSFilter work in the same coordinates system
>diff --git a/LayoutTests/svg/filters/feImage-cyclic-reference-expected.txt b/LayoutTests/svg/filters/feImage-cyclic-reference-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..654ddf7f17efab58e42fabe385257964ae2c5d41
>--- /dev/null
>+++ b/LayoutTests/svg/filters/feImage-cyclic-reference-expected.txt
>@@ -0,0 +1 @@
>+This test passes if it does not crash.
>diff --git a/LayoutTests/svg/filters/feImage-cyclic-reference.svg b/LayoutTests/svg/filters/feImage-cyclic-reference.svg
>new file mode 100644
>index 0000000000000000000000000000000000000000..03a92bfa62cca9e556fb74388ee365317d2d8eb2
>--- /dev/null
>+++ b/LayoutTests/svg/filters/feImage-cyclic-reference.svg
>@@ -0,0 +1,11 @@
>+<svg xmlns="http://www.w3.org/2000/svg">
>+    <filter id="filter">
>+        <feImage href="#rect"/>
>+    </filter>
>+    <rect x="10" y="10" width="100" height="100" filter="url(#filter)"/>
>+    <text x="10" y="120">This test passes if it does not crash.</text>
>+    <script>
>+        if (window.testRunner)
>+            testRunner.dumpAsText(true);
>+    </script>
>+</svg>

Flags:

wenson_hsieh: review+
ews-feeder: commit-queue-

Actions: View | Formatted Diff | Diff

Attachments on bug 232972: 443878 | 443895 | 444129