Bug 45561

Summary: NULL deref when SVG elements have table styles
Product: WebKit Reporter: Justin Schuh <jschuh>
Component: SVGAssignee: Rob Buis <rwlbuis>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ademar, ap, dbates, eric, jschuh, morrita, ossy, rwlbuis, webkit.review.bot, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
URL: data:text/html,<svg><text display="table-caption">
Bug Depends on: 60250    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Backtrace
none
Patch
none
Patch
none
Patch zimmermann: review+

Description Justin Schuh 2010-09-10 13:19:50 PDT
Here's a simple repro URL: 

data:text/html,<svg><text display="table-caption">

This issue applies to SVG elements that use the normal layout model (e.g. text and foreignObject). The problem is that RenderObject::addChild() sees the table attributes and creates an anonymous RenderTable parent for the child SVG object. So, when SVGRenderSupport::layoutChildren runs through its loop it hits a NULL deref returned from child->node() on the anonymous node here:

            if (SVGElement* element = child->node()->isSVGElement() ? static_cast<SVGElement*>(child->node()) : 0) {

I don't understand the layout well enough to know what should be happening here, but we probably want to prevent the crash.
Comment 1 Nikolas Zimmermann 2010-09-10 22:52:13 PDT
Hi Justin,

The SVG spec states, http://www.w3.org/TR/SVG/painting.html, section 11.5 "Controlling visibility"
"A value of display: none indicates that the given element and its children shall not be rendered directly (i.e., those elements are not present in the rendering tree). Any value other than none or inherit indicates that the given element shall be rendered by the SVG user agent."

That means we should treat all display values the same, as it can either mean it's rendered or not.
It should not be possible to alter "display: block" for <text> & <foreignObject> in our implementation.

Cheers,
Niko
Comment 2 Rob Buis 2011-05-03 14:32:36 PDT
Created attachment 92133 [details]
Patch
Comment 3 Daniel Bates 2011-05-03 16:00:16 PDT
Comment on attachment 92133 [details]
Patch

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

This doesn't seem like the correct fix. Consider the following test case:

data:text/html,<div style="display:table-caption"><svg style="display:inherit"><text display="inherit">This test PASSED if we don't crash.</text></svg><div>

We also need some test cases that use the CSS value "inherit".

> LayoutTests/svg/custom/display-table-caption-expected.txt:2
> +PASS
> +PASS

I suggest we add some more English to this such that a person can understand what these PASSes mean; that is, neither sub test caused a crash.

> LayoutTests/svg/custom/display-table-caption.svg:6
> +  <text display="table-caption" y="10">PASS</text>

Can we put some text/comment in this test case to explain that this test PASSED if it didn't crash? We need some English sentences in this test to explain its purpose and how to interpret the results.

> LayoutTests/svg/custom/display-table-caption.svg:7
> +  <foreignObject display="table-caption" y="10"><xhtml:div>PASS</xhtml:div></foreignObject>

I'm not very familiar with SVG and this looks like another test case for the display property. If so, I suggest separating this into its own layout test file since it seems more straightforward to diagnose a regression/crash from a minimal test case as opposed to a test case that has many sub tests.
Comment 4 Daniel Bates 2011-05-03 16:06:14 PDT
Created attachment 92154 [details]
Backtrace

Backtrace generated with Safari Version 5.0.5 (6533.21.1, r85595) using Justin Schuh's URL: 'data:text/html,<svg><text display="table-caption">'.
Comment 5 Dirk Schulze 2011-05-04 00:04:48 PDT
Comment on attachment 92133 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        In SVG for the CSS display property only accept the values block, inline, inherit and none.

Thats wrong, we definitely support all values for 'display', like Niko wrote in the quote, other values than 'none' and 'inherit' indicate that the element should get displayed. And we shouldn't do anything different than displaying the element.
Comment 6 Rob Buis 2011-05-04 06:45:45 PDT
Hi Dirk,

(In reply to comment #5)
> (From update of attachment 92133 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92133&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        In SVG for the CSS display property only accept the values block, inline, inherit and none.
> 
> Thats wrong, we definitely support all values for 'display', like Niko wrote in the quote, other values than 'none' and 'inherit' indicate that the element should get displayed. And we shouldn't do anything different than displaying the element.

You are right, I should have pointed out that it is about computed value that gets restricted. Will reword in my next patch.
Cheers,

Rob.
Comment 7 Rob Buis 2011-05-04 09:12:28 PDT
Created attachment 92254 [details]
Patch
Comment 8 Darin Adler 2011-05-04 09:39:37 PDT
Comment on attachment 92254 [details]
Patch

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

> Source/WebCore/css/CSSStyleSelector.cpp:3608
> +            EDisplay did = LIST_ITEM;

How about naming this local variable “display” instead of “did”?

I think the use of LIST_ITEM here as “a value where we definitely won’t do setDisplay” is oblique and confusing.

> Source/WebCore/css/CSSStyleSelector.cpp:3617
> +            if (isInitial)
> +                did = RenderStyle::initialDisplay();
> +            else if (isInherit)
> +                did = m_parentStyle->display();
> +            else if (primitiveValue)
> +                did = *primitiveValue;
> +            if ((did >= INLINE && did <= BLOCK) || did == NONE)
> +                m_style->setDisplay(did);
> +            return;

It would be much better if we could refactor this so it could share code with the normal case. I see the difficulty in doing that since the normal case code has return statements in it for inherit and initial.

One way to do it would be to create a stack-based object that would store the old display type and then when it goes out of scope, restores the display type if the new type is not legal for SVG. The code would look like this:

    case CSSPropertyDisplay: {
        SVGDisplayPropertyGuard guard(m_style);
        HANDLE_INHERIT_AND_INITIAL_AND_PRIMITIVE(display, Display)
        ...
    }

And:

    class SVGDisplayPropertyGuard {
        WTF_MAKE_NONCOPYABLE(SVGDisplayPropertyGuard);
    public:
        SVGDisplayPropertyGuard(Element*, RenderStyle*);
        ~SVGDisplayPropertyGuard();
    private:
    #if ENABLE(SVG)
        RenderStyle* m_style;
        EDisplay m_originalDisplayPropertyValue;
    #endif
    }

    #if !ENABLE(SVG)

    inline SVGDisplayPropertyGuard::SVGDisplayPropertyGuard(Element*, RenderStyle*)
    {
    }

    inline SVGDisplayPropertyGuard::~SVGDisplayPropertyGuard()
    {
    }

    #else

    static inline bool isAcceptableForSVGElement(EDisplay displayPropertyValue)
    {
        // FIXME: Is there a better way to write this that will make sure we keep this up to date as we add display property values?
        return displayPropertyValue >= INLINE && displayPropertyValue <= BLOCK || displayPropertyValue == NONE;
    }

    inline SVGDisplayPropertyGuard::SVGDisplayPropertyGuard(Element* element, RenderStyle* style)
    {
        if (!(element && element->isSVGElement())) {
            m_style = 0;
            return;
        }
        m_style = style;
        m_originalDisplayPropertyValue = style->display();
        ASSERT(isAcceptableForSVGElement(m_originalDisplayPropertyValue));
    }

    inline SVGDisplayPropertyGuard::~SVGDisplayPropertyGuard()
    {
        if (!m_style)
            return;
        if (isAcceptableForSVGElement(m_style->display()))
            return;
        m_style->setDisplay(m_originalDisplayPropertyValue);
    }

    #endif

What do you think of my version?
Comment 9 Rob Buis 2011-05-04 10:24:22 PDT
Hi Darin,

> What do you think of my version?

I like it because there are less changes in the original code section. I'll try that variant, see whether the tests still work and if so will put up the new patch.
Cheers,

Rob.
Comment 10 Rob Buis 2011-05-04 10:51:36 PDT
Created attachment 92281 [details]
Patch
Comment 11 Darin Adler 2011-05-04 10:57:30 PDT
Comment on attachment 92281 [details]
Patch

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

> Source/WebCore/css/CSSStyleSelector.cpp:3535
> +        WTF_MAKE_NONCOPYABLE(SVGDisplayPropertyGuard);

Wrong indentation there.
Comment 12 WebKit Review Bot 2011-05-04 15:21:56 PDT
http://trac.webkit.org/changeset/85788 might have broken Leopard Intel Debug (Tests)
The following tests are not passing:
svg/text/text-block-child-crash.xhtml
Comment 13 Hajime Morrita 2011-05-04 17:13:24 PDT
> svg/text/text-block-child-crash.xhtml
Chromium is also having a crash on the same file.

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&group=%40ToT%20-%20chromium.org&tests=svg%2Ftext%2Ftext-block-child-crash.xhtml

message:
ASSERTION FAILED: !m_layoutAttributes.isEmpty()
third_party/WebKit/Source/WebCore/rendering/svg/SVGTextLayoutEngine.cpp(54) : WebCore::SVGTextLayoutEngine::SVGTextLayoutEngine(WTF::Vector<WebCore::SVGTextLayoutAttributes, 0u>&)
Comment 14 Csaba Osztrogonác 2011-05-04 23:14:11 PDT
It asserted on all platform, so it was rolled out by http://trac.webkit.org/changeset/85834
Comment 15 Rob Buis 2011-05-06 07:46:59 PDT
Created attachment 92582 [details]
Patch
Comment 16 Rob Buis 2011-05-06 07:49:42 PDT
Fixed the problem with the crashing text-block-child-crash.xhtml test now. The problem was that the display restricting was applied to pseudo-element styles like :before, where it should not be restricted.
Cheers,

Rob.
Comment 17 Nikolas Zimmermann 2011-05-18 07:15:18 PDT
Comment on attachment 92582 [details]
Patch

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

Looks good, r=me.

> Source/WebCore/css/CSSStyleSelector.cpp:3542
> +        WTF_MAKE_NONCOPYABLE(SVGDisplayPropertyGuard);

Too much indented, 4 spaces are enough.

> Source/WebCore/css/CSSStyleSelector.cpp:3561
> +#if !ENABLE(SVG)
> +inline SVGDisplayPropertyGuard::SVGDisplayPropertyGuard(Element*, RenderStyle*)
> +{
> +}
> +
> +inline SVGDisplayPropertyGuard::~SVGDisplayPropertyGuard()
> +{
> +}
> +#else

I'd write it the other way round. #if ENABLE(SVG) ... #else .. #endif. Feel free to leave it as-is though.

> Source/WebCore/css/CSSStyleSelector.cpp:3584
> +    if (!m_style)
> +        return;
> +    if (isAcceptableForSVGElement(m_style->display()))
> +        return;

You could turn this into one line.
Comment 18 Rob Buis 2011-05-18 09:09:07 PDT
Committed r86771: <http://trac.webkit.org/changeset/86771>
Comment 19 Ademar Reis 2011-05-26 15:45:36 PDT
Revision r86771 cherry-picked into qtwebkit-2.2 with commit 51381f8 <http://gitorious.org/webkit/qtwebkit/commit/51381f8>