Bug 55119 - Implement SVGColor/SVGPaint API
Summary: Implement SVGColor/SVGPaint API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
: 48120 (view as bug list)
Depends on:
Blocks: 54800
  Show dependency treegraph
 
Reported: 2011-02-24 00:31 PST by Nikolas Zimmermann
Modified: 2011-02-25 05:11 PST (History)
4 users (show)

See Also:


Attachments
Patch (85.29 KB, patch)
2011-02-25 00:45 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v2 (85.46 KB, patch)
2011-02-25 01:54 PST, Nikolas Zimmermann
krit: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2011-02-24 00:31:25 PST
The SVGColor/SVGPaint API is not implemented, fix that.
Comment 1 Nikolas Zimmermann 2011-02-25 00:45:35 PST
Created attachment 83782 [details]
Patch

This patch completly implements the SVGPaint/SVGColor API, except that any mutation of this object, is not live, it doesn't take effect on screen, nor is it visible in the computed style.
CSSValue has no concept of being mutable, a follow-up patch will add CSSMutableValue, and fix the problem for real, though I'm splitting up the patches, in order to keep them in moderate size.
(Note: this patch is large because of the new tests, mostly)
Comment 2 Nikolas Zimmermann 2011-02-25 00:49:16 PST
*** Bug 48120 has been marked as a duplicate of this bug. ***
Comment 3 Alexey Proskuryakov 2011-02-25 01:09:13 PST
Can SVGPaint and SVGColor objects ever be shared between documents? Making CSS objects mutable often creates XSS security bugs.

I don't have a specific concern, but I vaguely remember relying on the fact that SVGPaint and SVGColor are immutable when looking over CSSOM objects in search for XSS and dangling parent pointer bugs.
Comment 4 Build Bot 2011-02-25 01:22:15 PST
Attachment 83782 [details] did not build on win:
Build output: http://queues.webkit.org/results/8019140
Comment 5 Nikolas Zimmermann 2011-02-25 01:49:39 PST
(In reply to comment #3)
> Can SVGPaint and SVGColor objects ever be shared between documents? Making CSS objects mutable often creates XSS security bugs.
Yes you are right, that would be security sensitive. Short answer: they can't. See below, how you'll ever get a unique/mutable value, that's no longer shared or cached.

> 
> I don't have a specific concern, but I vaguely remember relying on the fact that SVGPaint and SVGColor are immutable when looking over CSSOM objects in search for XSS and dangling parent pointer bugs.
Hey Alexex, I wanted to CC you anyway, as I recall you worked on this before.
The problem is that rectElement.style.fill and getComputedStyle(rectElement) return exactly the same SVGPaint object, that's also living as RefPtr<SVGPaint> in our SVGRenderStyle, which is dangerous.
A follow-up patch (the old version is still on bug 54800, I'll upload a new patch, once this is in) will fix this. We will no longer store SVGPaint in SVGRenderStyle, but a fill/stroke paint type/uri/color and create a new SVGPaint object in computedStyle, that is immutable, and return en existing SVGPaint object in style.fill, that is mutable.

How does that work?
I added a CSSMutableValue class, which inherits from CSSValue and additionally stores a Node pointer, to whose Node this value is linked (rectElement.style.fill, its SVGPaint object is linked to rectElement). SVGPaint/SVGColor inherit from CSSMutableValue.

The problem is that CSSValues are shared between multiple inline style declarations.eg. <rect fill="red"/> <rect fill="red"/>. Because of the cache in StyledElement, both will share the same SVGPaint object.

If I want to script the first SVGPaint object, I'll retrieve it through getPropertyCSSValue. I modified getPropertyCSSValue to assure that the MappedAttribute for fill is no longer shared anymore. Then I link the CSSMutableValue to the Node whose CSSStyleDeclaration::getPropertyCSSValue call invoked function.

This way we can map between CSSMutableValues and their _single_ node they belong to. (It's a similar approach to how CSSMutableStyleDeclarations are handled). This has no impact on regular style sharing, only when scripting elements style through getPropertyCSSValue.

I'll have a patch up soon, so we have an even better place to discuss :-)
Comment 6 Nikolas Zimmermann 2011-02-25 01:54:06 PST
Created attachment 83785 [details]
Patch v2

Try to fix win build.
Comment 7 Dirk Schulze 2011-02-25 02:02:31 PST
Comment on attachment 83785 [details]
Patch v2

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

waiting for the bots before I review the patch. (If no one beats me).

> Source/WebCore/svg/SVGPaint.cpp:188
> +        break;
> +    case SVG_PAINTTYPE_URI_NONE:
> +    case SVG_PAINTTYPE_URI_CURRENTCOLOR:
> +    case SVG_PAINTTYPE_URI_RGBCOLOR:
> +    case SVG_PAINTTYPE_URI_RGBCOLOR_ICCCOLOR:
> +    case SVG_PAINTTYPE_URI:
> +        return referenceId == SVGURIReference::getTarget(m_uri);
> +    }
>  
> -    return referenceId == SVGURIReference::getTarget(m_uri);
> +    return false;

Hehe, here you did not take ASSERT_NOT_REACHED looks inconsistent.
Comment 8 Nikolas Zimmermann 2011-02-25 02:28:04 PST
(In reply to comment #7)
> (From update of attachment 83785 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83785&action=review
> 
> waiting for the bots before I review the patch. (If no one beats me).
> 
> > Source/WebCore/svg/SVGPaint.cpp:188
> > +        break;
> > +    case SVG_PAINTTYPE_URI_NONE:
> > +    case SVG_PAINTTYPE_URI_CURRENTCOLOR:
> > +    case SVG_PAINTTYPE_URI_RGBCOLOR:
> > +    case SVG_PAINTTYPE_URI_RGBCOLOR_ICCCOLOR:
> > +    case SVG_PAINTTYPE_URI:
> > +        return referenceId == SVGURIReference::getTarget(m_uri);
> > +    }
> >  
> > -    return referenceId == SVGURIReference::getTarget(m_uri);
> > +    return false;
> 
> Hehe, here you did not take ASSERT_NOT_REACHED looks inconsistent.
Fixed.
Comment 9 Build Bot 2011-02-25 03:04:50 PST
Attachment 83785 [details] did not build on win:
Build output: http://queues.webkit.org/results/8044130
Comment 10 Nikolas Zimmermann 2011-02-25 03:21:25 PST
(In reply to comment #9)
> Attachment 83785 [details] did not build on win:
> Build output: http://queues.webkit.org/results/8044130

Ok win doesn't like my
static inline void valueChanged() stub implementation in both SVGPaint/SVGColor. I'll just replaced all valueChanged() calls, which are no-ops w/o the follow-up patch to say, "// FIXME: A follow-up patch will call valueChanged() here."

Shall I upload a new patch?
Comment 11 Build Bot 2011-02-25 03:26:57 PST
Attachment 83785 [details] did not build on win:
Build output: http://queues.webkit.org/results/8043158
Comment 12 Dirk Schulze 2011-02-25 04:57:09 PST
Comment on attachment 83785 [details]
Patch v2

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

LGTM. r=me

> Source/WebCore/ChangeLog:11
> +        Rewrite SVGColor/SVGPaint to actually implement their desired setPaint/setColor/setURI APIS.

s/APIS/APIs/ ?
Comment 13 Nikolas Zimmermann 2011-02-25 05:11:14 PST
APIs typo is fixed. Landed in r79675.