Bug 47498 – Crash while processing ill-formed SVG with cycles.

• Component: SVG
• Status: RESOLVED FIXED

[Cosmin Truta]

This is the Chromium issue 55521
http://code.google.com/p/chromium/issues/detail?id=55521

The SVG cycle solver crashes when processing input like the following:

<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
<filter id="crash">
<textPath fill="url(#crash)"></textPath>
</filter>
</svg>

This happens inside SVGResourcesCycleSolver::breakCycle, where the resourceType() is FilterResourceType, but the resource leading to cycle is in fact m_resources->fill(), not m_resources->filter().
The same crash occurs when replacing fill= with stroke=. In this case, the resource leading to cycle is m_resources->stroke() instead of m_resources->filter().

Here is a stack trace:
***
ASSERTION FAILED: resourceLeadingToCycle == m_resources->filter()
(third_party/WebKit/WebCore/rendering/SVGResourcesCycleSolver.cpp:197 void WebCore::SVGResourcesCycleSolver::breakCycle(WebCore::RenderSVGResourceContainer*))
        WebCore::SVGResourcesCycleSolver::breakCycle() [0x146ca67]
        WebCore::SVGResourcesCycleSolver::resolveCycles() [0x146c69c]
        WebCore::SVGResourcesCache::addResourcesFromRenderObject() [0x1468a82]
        WebCore::SVGResourcesCache::clientUpdatedFromElement() [0x146900d]
        WebCore::RenderSVGInline::updateFromElement() [0x1516536]
        WebCore::SVGStyledElement::attach() [0x14cad16]
        WebCore::XMLDocumentParser::startElementNs() [0xfb274c]
        WebCore::startElementNsHandler() [0xfb37fb]
        ...


WebKit also crashes upon encountering other attributes (like clip= and mask=), but that happens in an entirely different place, inside SVGRenderSupport::layoutChildren. Here is a stack trace:
***
SHOULD NEVER BE REACHED
(third_party/WebKit/WebCore/rendering/RenderInline.h:104 virtual void WebCore::RenderInline::layout())
        WebCore::RenderInline::layout() [0x123e51c]
        WebCore::SVGRenderSupport::layoutChildren() [0x145b4ed]
        WebCore::RenderSVGHiddenContainer::layout() [0x151725d]
        WebCore::RenderSVGResourceContainer::layout() [0x14e92fe]
        WebCore::SVGRenderSupport::layoutChildren() [0x145b4ed]
        WebCore::RenderSVGRoot::layout() [0x14f828f]
        WebCore::RenderBlock::layoutBlockChild() [0x11d30bd]
        WebCore::RenderBlock::layoutBlockChildren() [0x11d2c44]
        WebCore::RenderBlock::layoutBlock() [0x11d03ab]
        WebCore::RenderBlock::layout() [0x11cfcc2]
        WebCore::RenderView::layout() [0x12c7cf5]
        WebCore::FrameView::layout() [0x115e381]
        WebCore::Document::implicitClose() [0xf19084]
        WebCore::FrameLoader::checkCallImplicitClose() [0x10def33]
        WebCore::FrameLoader::checkCompleted() [0x10ded06]
        WebCore::FrameLoader::finishedParsing() [0x10dea75]
        WebCore::Document::finishedParsing() [0xf21520]
        WebCore::XMLDocumentParser::end() [0xfae37f]
        WebCore::XMLDocumentParser::finish() [0xfae3b8]
        WebCore::Document::finishParsing() [0xf194ee]
        WebCore::DocumentWriter::endIfNotLoadingMainResource() [0x10d943d]
        WebCore::DocumentWriter::end() [0x10d9393]
        WebCore::DocumentLoader::finishedLoading() [0x10cf397]
        WebCore::FrameLoader::finishedLoading() [0x10e5281]
        WebCore::MainResourceLoader::didFinishLoading() [0x10f62cb]
        WebCore::ResourceLoader::didFinishLoading() [0x1101b57]
        WebCore::ResourceHandleInternal::didFinishLoading() [0x19ccd8c]

[Cosmin Truta]

Below is the cycle solver's debug output, obtained from running the test case with DEBUG_CYCLE_DETECTION set to 1.

Before cycle detection:
-> this=0x7ffbb1d073f0, SVGResources(renderer=0x7ffbb1cd5f18, node=0x7ffbb1d1e1c0)
 | DOM Tree:
#document	0x7ffbc78a9400
	svg	0x7ffbb3bdb6c0
		#text	0x7ffbb3bf4ba0 "\n"
		filter	0x7ffbb1d23c00
			#text	0x7ffbb3bf4b40 "\n"
*			textPath	0x7ffbb1d1e1c0

 | List of resources:
 |-> Fill       : 0x7ffbb41333d8 (node=0x7ffbb1d23c00)

Detecting wheter any resources references any of following objects:
Local resources:
|> RenderSVGResourceFilter: object=0x7ffbb41333d8 (node=0x7ffbb1d23c00)
Parent resources:
|> RenderSVGResourceFilter: object=0x7ffbb41333d8 (node=0x7ffbb1d23c00)
ASSERTION FAILED: resourceLeadingToCycle == m_resources->filter()
(third_party/WebKit/WebCore/rendering/SVGResourcesCycleSolver.cpp:197 void WebCore::SVGResourcesCycleSolver::breakCycle(WebCore::RenderSVGResourceContainer*))
Segmentation fault

[Cosmin Truta]

Here is what happens, according to Niko's interpretation of the debug output:

It's processing the <textPath>, and it has "Fill" set to the filter element. That should be avoided, in advance.

The failing ASSERT is placed correctly.

We shouldn't store a fill resource pointing to a filter at all. Only linear/radialGradients/patterns should be allowed as fill/stroke: only clip as clipper, only mask as masker, only filter as filter.
That's just missing. It should be easy to patch into SVGResources::buildCachedResources.
Once that is fixed, the cycle detection logic won't run at all for these cases in "static inline RenderSVGResourceContainer* paintingResourceFromSVGPaint(Document* document, SVGPaint* paint, AtomicString& id, bool& hasPendingResource)"
There's the getRenderSVGResourceContainerById() call that returns the filter here. We should check if the resource type is lingrad/radgrad or pattern. If it's not the container, it should be nulled.

[Cosmin Truta]

Created attachment 70819 [details]
First attempt to fix

I'm submitting this patch to ask for review and advice only, without a test, a ChangeLog entry, or an intention to commit.

I am checking the resource type inside paintingResourceFromSVGPaint, instead of doing this inside buildCachedResources. The other alternative would have required doing the same check, two times: once for fill, and once for stroke.
The patch also contains a series of ASSERT's that I consider useful.

But this seems not to be sufficient, as the code still crashes inside RenderInline::layout. I'm probably missing a node that should be set to NULL, but I don't know where exactly should I do that. Since the filter has been invalidated, nothing should be rendered. I think there are some children at a point where shouldn't be.

It is worth mentioning that the crash after applying the patch is the same, regardless what attribute (clip=, fill=, mask=, stroke=) is being used.
I believe the patch that I'm submitting does solve the initialization issue discussed in comment #2, but there is another lingering issue that's causing grief. I also believe that the fix to do for the remaining issue will resolve the behavior of all of these attributes.

[Early Warning System Bot]

Attachment 70819 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4465036

[Cosmin Truta]

Here is the stack trace that comes from running the test case with <textPath clip=...> and <textPath mask=...> under the original WebKit, which is the same as the stack trace that comes from running the test case with <textPath fill=...> and <textPath stroke=...> under the WebKit patched with what I've just submitted for review.

SHOULD NEVER BE REACHED
(third_party/WebKit/WebCore/rendering/RenderInline.h:104 virtual void WebCore::RenderInline::layout())
[21085:21085:1752502611133:ERROR:base/process_util_posix.cc(105)] Received signal 11
    StackTrace::StackTrace() [0x5a6e38]
    base::(anonymous namespace)::StackDumpSignalHandler() [0x55a461]
    0x7f8b18510530
    WebCore::RenderInline::layout() [0x123e51c]
    WebCore::SVGRenderSupport::layoutChildren() [0x145b4ed]
    WebCore::RenderSVGHiddenContainer::layout() [0x1517565]
    WebCore::RenderSVGResourceContainer::layout() [0x14e9606]
    WebCore::SVGRenderSupport::layoutChildren() [0x145b4ed]
    WebCore::RenderSVGRoot::layout() [0x14f8597]
    WebCore::RenderBlock::layoutBlockChild() [0x11d30bd]
    WebCore::RenderBlock::layoutBlockChildren() [0x11d2c44]
    WebCore::RenderBlock::layoutBlock() [0x11d03ab]
    WebCore::RenderBlock::layout() [0x11cfcc2]
    WebCore::RenderView::layout() [0x12c7cf5]
    WebCore::FrameView::layout() [0x115e381]
    WebCore::Document::implicitClose() [0xf19084]
    WebCore::FrameLoader::checkCallImplicitClose() [0x10def33]
    WebCore::FrameLoader::checkCompleted() [0x10ded06]
    WebCore::FrameLoader::finishedParsing() [0x10dea75]
    WebCore::Document::finishedParsing() [0xf21520]
    WebCore::XMLDocumentParser::end() [0xfae37f]
    WebCore::XMLDocumentParser::finish() [0xfae3b8]
    WebCore::Document::finishParsing() [0xf194ee]
    WebCore::DocumentWriter::endIfNotLoadingMainResource() [0x10d943d]
    WebCore::DocumentWriter::end() [0x10d9393]
    WebCore::DocumentLoader::finishedLoading() [0x10cf397]
    WebCore::FrameLoader::finishedLoading() [0x10e5281]
    WebCore::MainResourceLoader::didFinishLoading() [0x10f62cb]
    WebCore::ResourceLoader::didFinishLoading() [0x1101b57]
    WebCore::ResourceHandleInternal::didFinishLoading() [0x19cd094]
    ...

[Cosmin Truta]

(In reply to comment #4)
> Attachment 70819 [details] did not build on qt:
> Build output: http://queues.webkit.org/results/4465036

The build dies with "unused variable" warnings-as-errors.
I must surround the ASSERT's with #ifndef NDEBUG guards.

[Csaba Osztrogonác]

Comment on attachment 70819 [details]
First attempt to fix

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

I'm not familiar with SVG, so I can't review the logic of the patch.
r- due to build error

> WebCore/rendering/SVGResources.cpp:453
> +    RenderSVGResourceType resourceType = clipper->resourceType();
> +    ASSERT(resourceType == ClipperResourceType);
> +

To avoid "unused variable" warning(error), you should 
use ASSERT_UNUSED, or don't use a new local variable.

But I propose avoiding unnecessary local variable,
which is dead code in relase mode:

ASSERT(clipper->resourceType() == ClipperResourceType);

> WebCore/rendering/SVGResources.cpp:476
> +    RenderSVGResourceType resourceType = filter->resourceType();
> +    ASSERT(resourceType == FilterResourceType);
> +

ditto

> WebCore/rendering/SVGResources.cpp:498
> +    RenderSVGResourceType resourceType = markerStart->resourceType();
> +    ASSERT(resourceType == MarkerResourceType);

ditto

> WebCore/rendering/SVGResources.cpp:520
> +    RenderSVGResourceType resourceType = markerMid->resourceType();
> +    ASSERT(resourceType == MarkerResourceType);

ditto

> WebCore/rendering/SVGResources.cpp:542
> +    RenderSVGResourceType resourceType = markerEnd->resourceType();
> +    ASSERT(resourceType == MarkerResourceType);

ditto

> WebCore/rendering/SVGResources.cpp:564
> +    RenderSVGResourceType resourceType = masker->resourceType();
> +    ASSERT(resourceType == MaskerResourceType);

ditto

> WebCore/rendering/SVGResources.cpp:586
> +    RenderSVGResourceType resourceType = fill->resourceType();
> +    ASSERT(resourceType == PatternResourceType || resourceType == LinearGradientResourceType || resourceType == RadialGradientResourceType);

ditto

> WebCore/rendering/SVGResources.cpp:608
> +    RenderSVGResourceType resourceType = stroke->resourceType();
> +    ASSERT(resourceType == PatternResourceType || resourceType == LinearGradientResourceType || resourceType == RadialGradientResourceType);

ditto

[Nikolas Zimmermann]

Comment on attachment 70819 [details]
First attempt to fix

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

> WebCore/rendering/SVGResources.cpp:166
> +        RenderSVGResourceType resourceType = container->resourceType();
> +        if (resourceType == PatternResourceType || resourceType == LinearGradientResourceType || resourceType == RadialGradientResourceType)
> +            return container;

This is dangerous, if the type doesn't match, it will now set hasPendingResource to true, before returning 0.
It's not a pending resource, it's just wrong resource type. So should definately add a "return 0" right after the "return container" statement, in case the types don't match.

> WebCore/rendering/SVGResources.cpp:475
> +    ASSERT(resourceType == FilterResourceType);

These asserts are really helpful, but there's no need to store them in a local variable, that would also save ossys suggestion to add ASSERT_UNUSED.
Just use ASSERT(filter->resourceType() == FilterResourceType).

[Nikolas Zimmermann]

(In reply to comment #8)
> These asserts are really helpful, but there's no need to store them in a local variable, that would also save ossys suggestion to add ASSERT_UNUSED.
> Just use ASSERT(filter->resourceType() == FilterResourceType).

Just realized this was already suggested by Ossy, sorry :-)

[Nikolas Zimmermann]

The ASSERTION that you see, with RenderInline, is easy to reproduce:

<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
<textPath/>
</svg>

It's _only_ textPath related.

Your patch should fix the assertion for:
<filter id="foo">
<rect fill="url(#foo)"/>
</filter>

This testcase already works as expected
<filter id="foo">
<rect mask="url(#foo)"/>
</filter>

Same for clip, etc.

The RenderInline assertion also happens for <svg><tspan/></svg>, and <tref>. All renderers that inherit from RenderSVGInline are affected. These are all elements that aren't allowed to appear without a <text> parent.
It's embarassing we still have bugs like this!

It needs to be fixed in the DOM, I'm just looking at it.

[Nikolas Zimmermann]

Okay, it's quite easy to fix:

Let's check the specs content model (only showing relevant text elements now):

<text> may contain: 'a', ‘altGlyph’, ‘textPath’, ‘tref’, ‘tspan’
<tspan> may contain: 'a', 'altGlyph', 'tref', 'tspan'
<tref> may contain: nothing
<textPath> may contain: 'a', 'tref', 'tspan'
<altGlyph> may contain: any element or character data

"SVGTextElement::childShouldCreateRenderer(Node* node) const" has to be added, which checks
wheter the passed in node tagName is 'a', 'altGlyph', 'textPath', 'tref', 'tspan'. These are the only children which are supposed to create renderers within a <text> subtree. The same should be added for SVGTSpanElement, checking for 'a', 'altGlyph', 'tref' and 'tspan', and SVGTextPathElement, checking for 'a', 'tref' and 'tspan'.

This way we assure only the right elements create renderers within a <text> subtree.

The second step to solve the problem is to add "bool rendererIsNeeded(RenderStyle*)" methods to SVGTSpanElement, SVGTRefElement and SVGTextPathElement, that check wheter the _parentNode()_ has the right tag name. (see SVGGElement::rendererIsNeeded as example).

SVGTSpanElement needs to check wheter its parent is 'textPath' or 'text' or 'tspan' or 'altGlyph'.
SVGTRefElement needs to check wheter its parent is 'textPath' or 'text' or 'tspan' or 'altGlyph'.
SVGTextPathElement needs to check wheter its parent is 'text'.

This will get rid of the assertion that you see. Combined with your attached test, this will solve the problem completly.

Good luck! :-)

[Cosmin Truta]

I opened the bug 47759 "Crash while processing ill-formed <textPath> ouside of <text>" to continue the work on <textPath>, and I am submitting a patch and layout test that is restricted to fixing the checks on invalid resources.

[Cosmin Truta]

Created attachment 70939 [details]
Fix and layout test

One step at a time. To be continued in bug 47759.

[Nikolas Zimmermann]

Comment on attachment 70939 [details]
Fix and layout test

Good job, r=me.

[WebKit Commit Bot]

Comment on attachment 70939 [details]
Fix and layout test

Clearing flags on attachment: 70939

Committed r69927: <http://trac.webkit.org/changeset/69927>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.