Bug 55119

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)

*** Bug 48120 has been marked as a duplicate of this bug. ***

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.

Attachment 83782 [details] did not build on win: Build output: http://queues.webkit.org/results/8019140

(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 :-)

Created attachment 83785 [details] Patch v2 Try to fix win build.

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.

(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.

Attachment 83785 [details] did not build on win: Build output: http://queues.webkit.org/results/8044130

(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?

Attachment 83785 [details] did not build on win: Build output: http://queues.webkit.org/results/8043158

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/ ?

APIs typo is fixed. Landed in r79675.