| Summary: | Crash while processing ill-formed SVG with cycles. | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Cosmin Truta <ctruta> | ||||||
| Component: | SVG | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | commit-queue, krit, mdelaney7, ossy, webkit-ews, zimmermann | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Attachments: |
|
||||||||
|
Description
Cosmin Truta
2010-10-11 12:01:10 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 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. 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. Attachment 70819 [details] did not build on qt: Build output: http://queues.webkit.org/results/4465036 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]
...
(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 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 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). (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 :-) 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. 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! :-) 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. Created attachment 70939 [details] Fix and layout test One step at a time. To be continued in bug 47759. Comment on attachment 70939 [details]
Fix and layout test
Good job, r=me.
Comment on attachment 70939 [details] Fix and layout test Clearing flags on attachment: 70939 Committed r69927: <http://trac.webkit.org/changeset/69927> All reviewed patches have been landed. Closing bug. |