Bug 18512 – getPresentationAttribute returns a shared object

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

[Jonathan Haas]

Suppose you've got the following SVG:

  <rect id="firstrect"  x="100" y="100" width="100" height="100" fill="red" />
  <rect id="secondrect" x="100" y="250" width="100" height="100" fill="red" />

The "fill='red'" in both objects will be mapped to the same attribute. Calling getPresentationAttribute('fill') on either rectangle will return this shared attribute reference. Modifying the returned presentation attribute will modify both rectangles. Even more fun, the change is permanent until WebKit is restarted. So executing this code:

    function onLoad()
    {
        var rect = document.getElementById("firstrect");
        var fill = rect.getPresentationAttribute("fill");
        fill.setRGBColor("green");
    }

...means that every SVG object that's loaded from then on with the attribute fill="red" will be drawn as green.

Verified on Windows XP with the latest service packs installed, but I've no reason to think the bug is platform-specific.

[Jonathan Haas]

Created attachment 20556 [details]
Repro case

[Jonathan Haas]

Confirmed that this happens in the Mac OS X build. Results in other browsers:

o Firefox 3.0b5 doesn't appear to support getPresentationAttribute(), or if it does, doesn't support changing the returned value. Both squares appear red.
o IE 7 doesn't support SVG, period.
o Opera 9.27 appears to work. The top square turns green, the bottom square remains red

[Eric Seidel (no email)]

Yeah, I guess we can't just wrap the style object.  Need to make modifications to the inline style decl:
spec: http://www.w3.org/TR/SVG/types.html#InterfaceSVGStylable
original bug: https://bugs.webkit.org/show_bug.cgi?id=13976

[Eric Seidel (no email)]

I don't think that "every attribute which is loaded from then on" will have red.  Rather any sibling element which happens to be sharing the RenderStyle with this element will suddenly turn red.

[Jonathan Haas]

Try it. Load the repro case in Safari and then without restarting, visit an SVG document with fill="red". LayoutTests/svg/custom/viewport-clip.svg will do.

[Dave Hyatt]

Style sharing for HTML actually looks at the mapped attributes, and if they don't match it avoids sharing.  I don't know if this is helpful or not.  It sounds similar.

[Rob Buis]

Created attachment 20694 [details]
First attempt

This small patch fixes the problem. However I wonder if there is a nicer way to do it. I guess there is nothing wrong with sharing the style declaration for this attribute though, since in the testcase the values are equal and we do not know up front that we are going to use getPresentationAttribute, which is used very rarely anyway. So I really can't see another way, though maybe the unsharing can be done with less statements or some helper method in StyledElement. I'll do the testcase later, I think the original testcase in the bug is a good base for an automated test.
Cheers,

Rob.

[Rob Buis]

Created attachment 20696 [details]
Now with testcase

Just added a testcase, code is the same.
Cheers,

Rob.

Comment on attachment 20696 [details]
Now with testcase

Is it possible that the style will not be shared at the time you call getPresentationAttribute() but a later addition to the DOM will make it shared?

[Darin Adler]

Comment on attachment 20696 [details]
Now with testcase

+    if (cssSVGAttr->style()->refCount() > 1) {

The hasOneRef() function was made for uses like this one. I think it'd be better to use that.

r=me

[Darin Adler]

Committed revision 34448.