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]
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.
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.
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.
Bug 26219.
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?).
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.
Bug 18512 was supposed to fix this?
When it fails it's not hitting the !cssSVGAttr->style()->hasOneRef() case.
A huge thank you to simon for the investigation!
Sigh. This started occurring much more frequently on the bots as of yesterday. I'll try to find time today to look more.
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()); }
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.
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.
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().
Created attachment 41979 [details] Speculative fix
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.
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.
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.
Created attachment 41982 [details] Cunning test case. I am cunning. Here is a 100% reproducible case. I'll make sure my fix fixes it.
Created attachment 41988 [details] Patch v1
Comment on attachment 41988 [details] Patch v1 Oh that simple testcase exposes the bug? It makes me cry :( r=me, looks sane.
Simon said this looked good to him too, so I"ll land this and email hyatt so that he sees this go by.
Comment on attachment 41988 [details] Patch v1 Clearing flags on attachment: 41988 Committed r50185: <http://trac.webkit.org/changeset/50185>
All reviewed patches have been landed. Closing bug.