Bug 29620 – SVGStyledElement::getPresentationAttribute() can return a shared CSSValue (some SVG tests randomly fail on the bot, and in release builds)

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Simon Fraser (smfr)]

A subset of SVG tests fail randomly on the build bots:

http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r48598%20(5194)/results.html

The ones that fail are usually:

svg/custom/group-opacity.svg
svg/custom/image-with-aspect-ratio-stretch.svg
svg/custom/inner-percent.svg
svg/custom/invalid-css.svg
svg/custom/invalid-fill-hex.svg
svg/custom/invalid-fill.svg

In every case, the failure is a red->green color difference:
[color=#FF0000] -> [color=#008000]

[Eric Seidel (no email)]

I could have sworn we had a bug about this already, but searching just now I don't see one.  Mark would know.

I think we should just skip them.  IIRC Mark did some investigation on the bot and was unable to find a cause.  I've never seen them fail locally, but failing on the bot is nearly as bad as tools which depend on the tree state will get false-failures.

[Simon Fraser (smfr)]

This seems like a genuine bug; I don't think we should just skip them. I can reproduce this locally with a few iterations of the tests and a release build.

[Mark Rowe (bdash)]

I'm certain that we had a bug about this too but couldn't find it when looking.  The failures are in no way specific to the bots so they're definitely worth investigating before being skipped.

[Mark Rowe (bdash)]

Bug 26219.

[Simon Fraser (smfr)]

The difference in the test result stems from a behavioral fork in StyledElement::attributeChanged() when parsing the 'fill' attribute. When the test succeeds, the call to getMappedAttributeDecl() returns null. When it fails, getMappedAttributeDecl() returns something (from an earlier test?).

[Simon Fraser (smfr)]

The mysterious green is coming from fill-SVGPaint-interface.svg, which does this:

        var fill = rect.getPresentationAttribute('fill');
        fill.setRGBColor("green");

This is touching a CSSMappedAttributeDeclaration which is in the mapped attribute cache. That's bad.

[Simon Fraser (smfr)]

Bug 18512 was supposed to fix this?

[Simon Fraser (smfr)]

When it fails it's not hitting the !cssSVGAttr->style()->hasOneRef() case.

[Eric Seidel (no email)]

A huge thank you to simon for the investigation!

[Eric Seidel (no email)]

Sigh.  This started occurring much more frequently on the bots as of yesterday.  I'll try to find time today to look more.

[Eric Seidel (no email)]

http://trac.webkit.org/browser/trunk/WebCore/svg/SVGStyledElement.cpp#L243

is the code in question.  Note there is already a FIXME there:
    // FIXME: Is it possible that the style will not be shared at the time this
    // is called, but a later addition to the DOM will make it shared?
    if (!cssSVGAttr->style()->hasOneRef()) {
        cssSVGAttr->setDecl(0);
        int propId = SVGStyledElement::cssPropertyIdForSVGAttributeName(cssSVGAttr->name());
        addCSSProperty(cssSVGAttr, propId, cssSVGAttr->value());
    }

[Eric Seidel (no email)]

http://www.w3.org/TR/SVG/types.html#InterfaceSVGStylable
is the spec definition of getPresentationAttribute:

getPresentationAttribute
Returns the base (i.e., static) value of a given presentation attribute as an object of type CSSValue. The returned object is live; changes to the objects represent immediate changes to the objects to which the CSSValue is attached.

I'm not sure exactly what to do here, but I'll make an attempt and then ask Hyatt.

[Eric Seidel (no email)]

I still don't understand what's getting shared here.  MappedAttribute?  CSSValue?  MappedAttributeDecl?  What is surviving between tests and being re-used here?

Some mapping of green -> a color value I think.  I'm just not sure which class is storing that.

[Simon Fraser (smfr)]

The shared object is a CSSMappedAttributeDeclaration, which is pointed to by the MappedAttribute. The CSSMappedAttributeDeclaration lives in the cache (mappedAttributeDecls) managed by StyledElement.

CSSMappedAttributeDeclaration  is a CSSStyleDeclaration, which returns a reference to a live CSSValue via getPropertyCSSValue().

[Eric Seidel (no email)]

Created attachment 41979 [details]
Speculative fix

[Eric Seidel (no email)]

Do I need to call:
        setNeedsStyleRecalc();
or
        if (namedAttrMap)
            mappedAttributes()->declRemoved();
?

Grepping through the source code for setDecl(0) yields conflicting examples.  I'm not really sure when declRemoved() and declAdded() should be called.  I think those functions probably need clearer names.

[Eric Seidel (no email)]

Do you have recommended steps for me to reproduce this locally?  Ideally we would come up with a test which always failed due to this.  I'm not sure I yet fully understand what's going on enough to create one.

[Simon Fraser (smfr)]

I was able to reproduce this reliably by paring down the tests in svg/custom, but keeping fill-SVGPaint-interface.svg and the tests listed in comment 1.

I'm not sure you can test this in a single test, unless you do something cunning with JS to stuff the cache, then mutate the value, then re-use that cached attribute.

[Eric Seidel (no email)]

Created attachment 41982 [details]
Cunning test case.

I am cunning.  Here is a 100% reproducible case.  I'll make sure my fix fixes it.

[Eric Seidel (no email)]

Created attachment 41988 [details]
Patch v1

[Nikolas Zimmermann]

Comment on attachment 41988 [details]
Patch v1

Oh that simple testcase exposes the bug? It makes me cry :(
r=me, looks sane.

[Eric Seidel (no email)]

Simon said this looked good to him too, so I"ll land this and email hyatt so that he sees this go by.

[WebKit Commit Bot]

Comment on attachment 41988 [details]
Patch v1

Clearing flags on attachment: 41988

Committed r50185: <http://trac.webkit.org/changeset/50185>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.