Bug 18512

Summary: getPresentationAttribute returns a shared object
Product: WebKit Reporter: Jonathan Haas <myrdred>
Component: SVGAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Major CC: mitz
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows XP   
Attachments:
Description Flags
Repro case
none
First attempt
none
Now with testcase darin: review+

Description Jonathan Haas 2008-04-15 08:33:35 PDT
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.
Comment 1 Jonathan Haas 2008-04-15 08:34:18 PDT
Created attachment 20556 [details]
Repro case
Comment 2 Jonathan Haas 2008-04-15 14:03:30 PDT
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
Comment 3 Eric Seidel (no email) 2008-04-16 11:22:24 PDT
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
Comment 4 Eric Seidel (no email) 2008-04-16 11:24:01 PDT
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.
Comment 5 Jonathan Haas 2008-04-16 11:32:38 PDT
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.
Comment 6 Dave Hyatt 2008-04-19 02:25:41 PDT
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.

Comment 7 Rob Buis 2008-04-20 00:54:19 PDT
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.
Comment 8 Rob Buis 2008-04-20 07:04:02 PDT
Created attachment 20696 [details]
Now with testcase

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

Rob.
Comment 9 mitz 2008-04-28 18:00:40 PDT
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?
Comment 10 Darin Adler 2008-05-24 23:22:03 PDT
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
Comment 11 Darin Adler 2008-06-08 12:23:16 PDT
Committed revision 34448.