Bug 47759 – Crash while processing ill-formed <textPath> ouside of <text>

• Component: SVG
• Status: RESOLVED FIXED

[Cosmin Truta]

Although <textPath> is required to exist inside <text>, the SVG processor shouldn't crash when encountering ill-formed test cases like the following:

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

This is a continuation of the work on bug 47498. The text below is Nikolas Zimmermann's analysis.

"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! :-)"

[Nikolas Zimmermann]

We've discussed this with Dirk & Rob, and I think what I described is more a workaround than a real fix.
The question is: shall we create "invalid" DOMs at all, aka. those that violate the SVG "content model"?
If we only ever create valid DOMs, we wouldn't need to add rendererIsNeeded methods, that compare tag names.

In the past we had checkDTD/checkAddChild methods on the HTML* Elements that did that job, they're gone now replaced by the HTML5 TreeBuilder (IIRC).

I'm CC'ing Adam & Eric for their insight, on how to solve this problem in a clean fashion.

[Cosmin Truta]

Created attachment 74958 [details]
Fix and layout test

There is one thing that isn't exactly done as stated in the content model spec:
SVGTextPathElement::childShouldCreateRenderer allows <textPath> inside <textPath>. (SVGTextPathElement::rendererIsNeeded is fine, however.)
This is to accommodate <textPath><a><textpath>Text</textPath></a></textPath>, which is tested in svg/custom/text-linking.svg

[Nikolas Zimmermann]

(In reply to comment #2)
> Created an attachment (id=74958) [details]
> Fix and layout test
> 
> There is one thing that isn't exactly done as stated in the content model spec:
> SVGTextPathElement::childShouldCreateRenderer allows <textPath> inside <textPath>. (SVGTextPathElement::rendererIsNeeded is fine, however.)
> This is to accommodate <textPath><a><textpath>Text</textPath></a></textPath>, which is tested in svg/custom/text-linking.svg

I guess the testcase is wrong, can you compare to Opera?

[Nikolas Zimmermann]

Comment on attachment 74958 [details]
Fix and layout test

Looks good to me in general, can you verify that text-linking.svg is wrong, first and completely adhere to the defined content model?

[Cosmin Truta]

Created attachment 75045 [details]
Opera screenshot

(In reply to comment #3)
> > This is to accommodate <textPath><a><textpath>Text</textPath></a></textPath>, which is tested in svg/custom/text-linking.svg
> 
> I guess the testcase is wrong, can you compare to Opera?

I was puzzled about this myself, I thought I was reading the SVG spec incorrectly.

Opera (both 10 release and 11 beta) shows "textPath in link in textPath", but Inkscape doesn't. Inkscape doesn't show any of the two textPaths.

An Opera screenshot is attached. A bunch of things seem to broken in Opera.

Inkscape shows the following (just straight lines, no curved text paths):

** begin Inkscape output
tspan in link
tref in link Referenced character data

AltGlyph content
This should not be rendered
tspan in link in tspan
** end Inkscape output

[Nikolas Zimmermann]

(In reply to comment #5)
> Created an attachment (id=75045) [details]
> Opera screenshot
> 
> (In reply to comment #3)
> > > This is to accommodate <textPath><a><textpath>Text</textPath></a></textPath>, which is tested in svg/custom/text-linking.svg
> > 
> > I guess the testcase is wrong, can you compare to Opera?
> 
> I was puzzled about this myself, I thought I was reading the SVG spec incorrectly.
> 
> Opera (both 10 release and 11 beta) shows "textPath in link in textPath", but Inkscape doesn't. Inkscape doesn't show any of the two textPaths.

Hm, okay it's fine with me for now. Just go ahead and land the patch, we can still tweak the content model, if needed.

[Nikolas Zimmermann]

Comment on attachment 74958 [details]
Fix and layout test

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

Can you modify the test, then I'll r+ it.

> LayoutTests/svg/custom/invalid-text-content.svg:5
> +        layoutTestController.dumpAsText();

Hm, how about removing dumpAsText() here and showing the actual render tree? Would be easier to track the renderer creation with it...

[Cosmin Truta]

(In reply to comment #7)
> (From update of attachment 74958 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74958&action=review
> 
> Can you modify the test, then I'll r+ it.
> 
> > LayoutTests/svg/custom/invalid-text-content.svg:5
> > +        layoutTestController.dumpAsText();
> 
> Hm, how about removing dumpAsText() here and showing the actual render tree? Would be easier to track the renderer creation with it...

Just chatted with Rob Buis, the author of that test case, and he agreed that line is in error. I will fix the test case, by adding <textPath><a><text><textPath/></text></a></textPath>, as Rob suggested.

I will also show the render tree, I agree it makes the renderer creation more tractable.

[Cosmin Truta]

Created attachment 75109 [details]
Fix and layout test

Actually, <text> inside <a> inside <textPath> shouldn't work either, because the spec says "The ‘a’ element may contain any element that its parent may contain, except itself", and <textPath> cannot contain <text>.

In my interpretation of the content model, <textPath> cannot be inside another <textPath> in any way. That line from text-linking.svg must be removed. However, since I don't have the means to regenerate the expected image output of text-linking.svg, I will open a separate bug 50211. This will be committed after that one gets committed.

[Cosmin Truta]

Created attachment 75110 [details]
Fix and layout test

Oops... the previous patch accidentally included a modification to text-linking.svg. That should be done in bug 50211.

[Cosmin Truta]

Comment on attachment 75110 [details]
Fix and layout test

Setting cq?, given that the patches for bugs 50211, 50310 have finally landed.

[Eric Seidel (no email)]

Comment on attachment 75110 [details]
Fix and layout test

Why does this test need pixel results?  And even so, it could use the Ahem font to avoid needing custom platform results.

[Nikolas Zimmermann]

(In reply to comment #12)
> (From update of attachment 75110 [details])
> Why does this test need pixel results?  And even so, it could use the Ahem font to avoid needing custom platform results.

I explicitely asked Cosmin to avoid dumpAsText() here, as we want to see the dump of the render tree, to know exactly which renderers are created. The side effect, is that this now needs pixel test results. I don't see how Ahem can help here, if Cosmin wants to display a real text on screen?

We could also move the "pass" conditition in a comment, as recently suggested on webkit-dev.

[Eric Seidel (no email)]

Move the pass to a comment, make it a green square and then the results can be shared between all platforms. :)

[Cosmin Truta]

Created attachment 76239 [details]
Fix and layout test

Followed Eric's suggestion.

[Eric Seidel (no email)]

Comment on attachment 76239 [details]
Fix and layout test

Assuming your test case still would crash on an unfixed webkit, then LGTM!

[Cosmin Truta]

(In reply to comment #16)
> (From update of attachment 76239 [details])
> Assuming your test case still would crash on an unfixed webkit, then LGTM!

Yes, it still crashes without this patch.
And even without the crash, if I carefully remove the sections that make it crash, I can see (the below-baseline/descent part of) the text "This should not be rendered", in the image capture, and in the dumped render tree.

[Cosmin Truta]

Created attachment 76244 [details]
Fix and layout test

Resubmitting with the layout test slightly changed:
"This should not be rendered." --> "This should not appear in rendering."

With the new test, if the text is inadvertently (incorrectly) rendered, the text descent (the lower part of 'p' in "appear" and the lower part of 'g' in "rendering") shows up as dirt in the rendered image, thus making the failure more obvious, even when it's not crashing.

Could you please redo the r+/cq+?

[WebKit Commit Bot]

Comment on attachment 76244 [details]
Fix and layout test

Clearing flags on attachment: 76244

Committed r73820: <http://trac.webkit.org/changeset/73820>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.