Bug 63365 – ASSERTs in RenderInline::layout()

• Component: SVG
• Status: RESOLVED FIXED

[Tim Horton]

Steps to repro:

1. Open the attached SVG in a debug build of a WebKit browser.

Expected result: Nothing is drawn.
Actual result: Crash due to an assertion.

<rdar://problem/7403871>

Diagnosis:

There are a variety of text elements which are laid out by their (expected) parents. If their parent is something else instead, they fall back on RenderInline, which isn't too happy about this.

What's the right fix? Technically this is invalid.

[Tim Horton]

Created attachment 98563 [details]
repro

[Nikolas Zimmermann]

(In reply to comment #0)
> Steps to repro:
> 
> 1. Open the attached SVG in a debug build of a WebKit browser.
> 
> Expected result: Nothing is drawn.
> Actual result: Crash due to an assertion.
> 
> <rdar://problem/7403871>
> 
> Diagnosis:
> 
> There are a variety of text elements which are laid out by their (expected) parents. If their parent is something else instead, they fall back on RenderInline, which isn't too happy about this.
> 
> What's the right fix? Technically this is invalid.

Good catch. We had a similar problem before:
<text> <circle> .... Arbitary children as <text> elements caused crashes. We've fixed that by only allowing a specific subset of rendererers to be children of <text>.
bool SVGTextElement::childShouldCreateRenderer(Node* child) const
{
    if (child->isTextNode()
        || child->hasTagName(SVGNames::aTag)
#if ENABLE(SVG_FONTS)
        || child->hasTagName(SVGNames::altGlyphTag)
#endif
        || child->hasTagName(SVGNames::textPathTag)
        || child->hasTagName(SVGNames::trefTag)
        || child->hasTagName(SVGNames::tspanTag))
        return true;

    return false;
}

This function handles the aformentioned "only-allow-specific-children-for-<text>".
We need a similar method, to avoid creating renderers for <textPath>/<tref>/... when it's parent is not <text>: that's done by reimplementing rendererIsNeeded():

Looking closer through the SVGTSpan/TRef/TextPath/.. elements we already did that:

bool SVGTSpanElement::rendererIsNeeded(const NodeRenderingContext& context)
{
    if (parentNode()
        && (parentNode()->hasTagName(SVGNames::aTag)
#if ENABLE(SVG_FONTS)
            || parentNode()->hasTagName(SVGNames::altGlyphTag)
#endif
            || parentNode()->hasTagName(SVGNames::textTag)
            || parentNode()->hasTagName(SVGNames::textPathTag)
            || parentNode()->hasTagName(SVGNames::tspanTag)))
        return StyledElement::rendererIsNeeded(context);

    return false;
}

See SVGTextPathElement:
bool SVGTextPathElement::rendererIsNeeded(const NodeRenderingContext& context)
{
    if (parentNode()
        && (parentNode()->hasTagName(SVGNames::aTag)
            || parentNode()->hasTagName(SVGNames::textTag)))
        return StyledElement::rendererIsNeeded(context);

    return false;
}

We explicitely allow <a> to be a parent of <textPath>. Is this not correct? Is this supposed to work? What does Opera say?

[Tim Horton]

(In reply to comment #2)
> We explicitely allow <a> to be a parent of <textPath>. Is this not correct? Is this supposed to work? What does Opera say?

That's interesting! I don't *think* that's correct, it would appear that Opera does not render <textPath> when it's a child of <a>.  This also isn't the only case of hitting this/similar bug (<tspan> inside <g> is another case I've seen, which opera also doesn't render).

[Rob Buis]

Created attachment 143052 [details]
Patch

[Stephen Chenney]

Comment on attachment 143052 [details]
Patch

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

Yeah! I'm all for patches that enforce the content model. We've seen security problems with this in the recent past.

Does this cover all of the places where we may need to prevent text content child elements from appearing? Given that they are only allowed as children of other text content child elements, or of a text element, would it not be clearer to add a SVG-wide childShouldCreateRenderer that prevents text content child elements, and then override it for the allowed cases?

> Source/WebCore/svg/SVGDefsElement.h:46
> +    virtual bool childShouldCreateRenderer(const NodeRenderingContext&) const;

Add OVERRIDE

> Source/WebCore/svg/SVGGElement.h:54
> +    virtual bool childShouldCreateRenderer(const NodeRenderingContext&) const;

Add OVERRIDE

> Source/WebCore/svg/SVGSVGElement.h:147
> +    virtual bool childShouldCreateRenderer(const NodeRenderingContext&) const;

Add OVERRIDE

> Source/WebCore/svg/SVGSymbolElement.h:51
> +    virtual bool childShouldCreateRenderer(const NodeRenderingContext&) const;

Add OVERRIDE

> Source/WebCore/svg/SVGUseElement.h:73
> +    virtual bool childShouldCreateRenderer(const NodeRenderingContext&) const;

Add OVERRIDE

[Rob Buis]

Created attachment 143064 [details]
Patch

[Rob Buis]

Hi Stephen,

(In reply to comment #5)
> (From update of attachment 143052 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143052&action=review
> 
> Yeah! I'm all for patches that enforce the content model. We've seen security problems with this in the recent past.
> 
> Does this cover all of the places where we may need to prevent text content child elements from appearing? Given that they are only allowed as children of other text content child elements, or of a text element, would it not be clearer to add a SVG-wide childShouldCreateRenderer that prevents text content child elements, and then override it for the allowed cases?

Yes, I think you are right, I tried to implement this in my latest patch. It also makes it a much simpler patch, thanks!

[Nikolas Zimmermann]

Comment on attachment 143064 [details]
Patch

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

r=me.

> Source/WebCore/svg/SVGElement.cpp:499
> +        if (childContext.node()->hasTagName(SVGNames::textPathTag)

We might want to think about adding a static HashSet<QName> of allowed tags here, and check against that.
childShouldCreateRenderer() is used quite often. Not sure if its worth it though.

[Rob Buis]

Hi Niko,

(In reply to comment #8)
> (From update of attachment 143064 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143064&action=review
> 
> r=me.
> 
> > Source/WebCore/svg/SVGElement.cpp:499
> > +        if (childContext.node()->hasTagName(SVGNames::textPathTag)
> 
> We might want to think about adding a static HashSet<QName> of allowed tags here, and check against that.
> childShouldCreateRenderer() is used quite often. Not sure if its worth it though.

I think it is a good idea, I added that before landing.

[Rob Buis]

Committed r118002: <http://trac.webkit.org/changeset/118002>

[Stephen Chenney]

*** Bug 82634 has been marked as a duplicate of this bug. ***

[Andy Estes]

What (if any) is the symptom of this bug in release builds?