Bug 208279 – Hit test with clipPath referencing parent element causes infinite recursion

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Doug Kelly]

When using a hit test that checks an element nested inside a clipPath (but the child element refers to the clipPath), this can result in an infinite recursion.  An extra check is needed to break the cycle when performing the hit test.

<rdar://problem/58381090>

[Doug Kelly]

Created attachment 391817 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 391817 [details]
Patch

Seems ok.

[WebKit Commit Bot]

Comment on attachment 391817 [details]
Patch

Clearing flags on attachment: 391817

Committed r257616: <https://trac.webkit.org/changeset/257616>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Said Abou-Hallawa]

Comment on attachment 391817 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=391817&action=review

> Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:290
> +        const RenderStyle& style = renderer->style();
> +        if (is<ReferenceClipPathOperation>(style.clipPath())) {
> +            auto& clipPath = downcast<ReferenceClipPathOperation>(*style.clipPath());
> +            AtomString id(clipPath.fragment());
> +            RenderSVGResourceClipper* clipper = getRenderSVGResourceById<RenderSVGResourceClipper>(document(), id);
> +            if (clipper == this)
> +                continue;
> +        }

I do not think this is the right solution. Detecting SVG resources cyclic referencing is more complicated than going one level up in the resources tree. For example if you delete the last two lines of your test case below:

    <clipPath id="clippath" clipPathUnits="objectBoundingBox">
    <text clip-path="url(#clippath)" to="currentColor">Text</text>

And you add these lines instead:

    <g id="group">
        <text clip-path="url(#clippath)" to="currentColor">Text</text>
    </g>
    <clipPath id="clippath" clipPathUnits="objectBoundingBox">
        <use href="#group"/>
    </clipPath>

The new test case will hit the following infinite recursive call stack:

#1	0x000000011c854e44 in WebCore::RenderBlock::nodeAtPoint(WebCore::HitTestRequest const&, WebCore::HitTestResult&, WebCore::HitTestLocation const&, WebCore::LayoutPoint const&, WebCore::HitTestAction) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/RenderBlock.cpp:1994
#2	0x000000011cc241e1 in WebCore::RenderSVGText::nodeAtFloatPoint(WebCore::HitTestRequest const&, WebCore::HitTestResult&, WebCore::FloatPoint const&, WebCore::HitTestAction) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/svg/RenderSVGText.cpp:444
#3	0x000000011cbcc501 in WebCore::RenderSVGContainer::nodeAtFloatPoint(WebCore::HitTestRequest const&, WebCore::HitTestResult&, WebCore::FloatPoint const&, WebCore::HitTestAction) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/svg/RenderSVGContainer.cpp:170
#4	0x000000011cbcc501 in WebCore::RenderSVGContainer::nodeAtFloatPoint(WebCore::HitTestRequest const&, WebCore::HitTestResult&, WebCore::FloatPoint const&, WebCore::HitTestAction) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/svg/RenderSVGContainer.cpp:170
#5	0x000000011cbe0606 in WebCore::RenderSVGResourceClipper::hitTestClipContent(WebCore::FloatRect const&, WebCore::FloatPoint const&) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:293
#6	0x000000011c8553c8 in WebCore::RenderBlock::nodeAtPoint(WebCore::HitTestRequest const&, WebCore::HitTestResult&, WebCore::HitTestLocation const&, WebCore::LayoutPoint const&, WebCore::HitTestAction) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/RenderBlock.cpp:2048
#7	0x000000011cc241e1 in WebCore::RenderSVGText::nodeAtFloatPoint(WebCore::HitTestRequest const&, WebCore::HitTestResult&, WebCore::FloatPoint const&, WebCore::HitTestAction) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/svg/RenderSVGText.cpp:444

Please see SVGResourcesCycleSolver::resolveCycles().

[Said Abou-Hallawa]

Created attachment 392173 [details]
test case (will crash)

[Ryosuke Niwa]

Let's track that in a separate bug.