Bug 63365 - ASSERTs in RenderInline::layout()
Summary: ASSERTs in RenderInline::layout()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords: InRadar
: 82634 (view as bug list)
Depends on:
Blocks: 82634
  Show dependency treegraph
 
Reported: 2011-06-24 16:52 PDT by Tim Horton
Modified: 2012-05-23 16:16 PDT (History)
4 users (show)

See Also:


Attachments
repro (220 bytes, image/svg+xml)
2011-06-24 16:53 PDT, Tim Horton
no flags Details
Patch (13.49 KB, patch)
2012-05-21 09:46 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (6.21 KB, patch)
2012-05-21 11:27 PDT, Rob Buis
zimmermann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2011-06-24 16:52:26 PDT
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.
Comment 1 Tim Horton 2011-06-24 16:53:13 PDT
Created attachment 98563 [details]
repro
Comment 2 Nikolas Zimmermann 2011-06-24 23:35:46 PDT
(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?
Comment 3 Tim Horton 2011-06-27 10:29:09 PDT
(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).
Comment 4 Rob Buis 2012-05-21 09:46:36 PDT
Created attachment 143052 [details]
Patch
Comment 5 Stephen Chenney 2012-05-21 10:30:19 PDT
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
Comment 6 Rob Buis 2012-05-21 11:27:44 PDT
Created attachment 143064 [details]
Patch
Comment 7 Rob Buis 2012-05-21 11:29:00 PDT
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!
Comment 8 Nikolas Zimmermann 2012-05-21 23:38:55 PDT
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.
Comment 9 Rob Buis 2012-05-22 11:08:28 PDT
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.
Comment 10 Rob Buis 2012-05-22 11:10:05 PDT
Committed r118002: <http://trac.webkit.org/changeset/118002>
Comment 11 Stephen Chenney 2012-05-22 12:10:39 PDT
*** Bug 82634 has been marked as a duplicate of this bug. ***
Comment 12 Andy Estes 2012-05-23 16:16:35 PDT
What (if any) is the symptom of this bug in release builds?