Bug 51061 – REGRESSION (64275): Shape pattern-image fill turns black

• Component: SVG
• Status: RESOLVED FIXED

[Matthew Delaney]

Created attachment 76576 [details]
testcase1

Open test case and click. It goes black but shouldn't.

Working on making an even more reduced test case right now, though this one isn't too bad to use.

[Matthew Delaney]

<rdar://problem/8504705>

[Adele Peterson]

We're pretty sure this was caused by http://trac.webkit.org/changeset/64275.

[Nikolas Zimmermann]

Hm, I didn't investigate yet, I only noticed that when zooming in/out the pattern reappers.
Seems like a simple invalidation problem, i'll look at it soon, if no one else does :-)

[Tim Horton]

Nikolas: have you had a chance to look at this one again? The only interesting thing I noticed was that if the id of the image changes, it works fine, so I agree - some kind of invalidation or caching thing. Also, if you null out the fill attribute, recreate the image, and reset the fill after a setTimeout with 0 delay (via Matt), that fixes it, but if you fail to null out the fill, no amount of time will fix it, so I don't think it's a timing thing. (Seems like setFill isn't getting called in the case where the value of the fill property hasn't changed, even if the thing it's referencing has, and some important stuff happens downwind of setFill)

If you don't get a chance, I'll get back to playing with it at some point.

[Nikolas Zimmermann]

(In reply to comment #4)
> Nikolas: have you had a chance to look at this one again? The only interesting thing I noticed was that if the id of the image changes, it works fine, so I agree - some kind of invalidation or caching thing. Also, if you null out the fill attribute, recreate the image, and reset the fill after a setTimeout with 0 delay (via Matt), that fixes it, but if you fail to null out the fill, no amount of time will fix it, so I don't think it's a timing thing. (Seems like setFill isn't getting called in the case where the value of the fill property hasn't changed, even if the thing it's referencing has, and some important stuff happens downwind of setFill)
> 
> If you don't get a chance, I'll get back to playing with it at some point.


Hi Tim, I'd highly appreciate if you could give it a try - still blocked by CSS / SVG Font patches.

[Tim Horton]

Created attachment 107122 [details]
patch

[Early Warning System Bot]

Comment on attachment 107122 [details]
patch

Attachment 107122 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9646270

[Gyuyoung Kim]

Comment on attachment 107122 [details]
patch

Attachment 107122 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9649283

[WebKit Review Bot]

Comment on attachment 107122 [details]
patch

Attachment 107122 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9641753

[WebKit Review Bot]

Comment on attachment 107122 [details]
patch

Attachment 107122 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9647290

[Collabora GTK+ EWS bot]

Comment on attachment 107122 [details]
patch

Attachment 107122 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9655051

[Tim Horton]

Created attachment 107192 [details]
patch which should build more places

[Tim Horton]

Created attachment 107193 [details]
patch

[Simon Fraser (smfr)]

Comment on attachment 107192 [details]
patch which should build more places

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

> Source/WebCore/rendering/svg/SVGResourcesCache.cpp:170
> +        SVGStyledElement* clientElement = static_cast<SVGStyledElement*>(it->first->node());

How do you know that the first node is a SVGStyledElement? Is that always true?

[Tim Horton]

(In reply to comment #14)
> (From update of attachment 107192 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107192&action=review
> 
> > Source/WebCore/rendering/svg/SVGResourcesCache.cpp:170
> > +        SVGStyledElement* clientElement = static_cast<SVGStyledElement*>(it->first->node());
> 
> How do you know that the first node is a SVGStyledElement? Is that always true?

The only way something can get into the cache which we're iterating through here is through the clientUpdateFromElement ---> addResourcesForRenderObject path. There are a few calls to clientUpdateFromElement:

* in RenderSVGBlock, which adds itself (so that's safe)
* in RenderSVGInline, which adds itself (so that's safe)
* in RenderSVGModelObject, which adds itself (so that's safe)
* in RenderSVGResourceContainer, which is adding from a HashSet of SVGStyledElements (so that's safe)
* in RenderSVGRoot, which adds itself (so that's safe)
* in SVGResourcesCache, via clientStyleChanged, which is called:
     * from RenderSVGBlock, which adds itself
     * from RenderSVGInline, which adds itself
     * from RenderSVGModelObject, which adds itself
     * from RenderSVGRoot, which adds itself
     * from SVGInlineTextBox, which adds its parent

So, I'm *pretty* sure it has to be an SVGStyledElement. Should I use a different cast/check the type/add an ASSERT()?

[Simon Fraser (smfr)]

A pattern we use elsewhere is to create a toFoo() function which first asserts that the element is the expected type, and then does the cast.

[Tim Horton]

Created attachment 107218 [details]
patch with casting functions

[WebKit Review Bot]

Comment on attachment 107218 [details]
patch with casting functions

Clearing flags on attachment: 107218

Committed r95047: <http://trac.webkit.org/changeset/95047>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.