Bug 47498 - Crash while processing ill-formed SVG with cycles.
Summary: Crash while processing ill-formed SVG with cycles.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-11 12:01 PDT by Cosmin Truta
Modified: 2010-10-17 12:46 PDT (History)
6 users (show)

See Also:


Attachments
First attempt to fix (3.70 KB, patch)
2010-10-14 19:11 PDT, Cosmin Truta
ossy: review-
Details | Formatted Diff | Diff
Fix and layout test (6.42 KB, patch)
2010-10-15 18:24 PDT, Cosmin Truta
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cosmin Truta 2010-10-11 12:01:10 PDT
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]
Comment 1 Cosmin Truta 2010-10-12 12:37:37 PDT
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
Comment 2 Cosmin Truta 2010-10-12 12:50:09 PDT
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.
Comment 3 Cosmin Truta 2010-10-14 19:11:02 PDT
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.
Comment 4 Early Warning System Bot 2010-10-14 19:19:36 PDT
Attachment 70819 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4465036
Comment 5 Cosmin Truta 2010-10-14 19:24:14 PDT
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]
    ...
Comment 6 Cosmin Truta 2010-10-14 19:28:14 PDT
(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.
Comment 7 Csaba Osztrogonác 2010-10-14 23:29:12 PDT
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
Comment 8 Nikolas Zimmermann 2010-10-15 02:15:25 PDT
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).
Comment 9 Nikolas Zimmermann 2010-10-15 02:16:08 PDT
(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 :-)
Comment 10 Nikolas Zimmermann 2010-10-15 02:25:37 PDT
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.
Comment 11 Nikolas Zimmermann 2010-10-15 02:41:03 PDT
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! :-)
Comment 12 Cosmin Truta 2010-10-15 17:57:51 PDT
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.
Comment 13 Cosmin Truta 2010-10-15 18:24:29 PDT
Created attachment 70939 [details]
Fix and layout test

One step at a time. To be continued in bug 47759.
Comment 14 Nikolas Zimmermann 2010-10-17 12:32:20 PDT
Comment on attachment 70939 [details]
Fix and layout test

Good job, r=me.
Comment 15 WebKit Commit Bot 2010-10-17 12:46:48 PDT
Comment on attachment 70939 [details]
Fix and layout test

Clearing flags on attachment: 70939

Committed r69927: <http://trac.webkit.org/changeset/69927>
Comment 16 WebKit Commit Bot 2010-10-17 12:46:55 PDT
All reviewed patches have been landed.  Closing bug.