Bug 51061

Summary: REGRESSION (64275): Shape pattern-image fill turns black
Product: WebKit Reporter: Matthew Delaney <mdelaney7>
Component: SVGAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: adele, dglazkov, gns, gustavo.noronha, krit, simon.fraser, thorton, webkit.review.bot, xan.lopez, zimmermann
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac (Intel)   
OS: OS X 10.6   
Attachments:
Description Flags
testcase1
none
patch
webkit-ews: commit-queue-
patch which should build more places
none
patch
none
patch with casting functions none

Description Matthew Delaney 2010-12-14 14:47:56 PST
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.
Comment 1 Matthew Delaney 2011-01-07 15:29:04 PST
<rdar://problem/8504705>
Comment 2 Adele Peterson 2011-04-22 19:57:40 PDT
We're pretty sure this was caused by http://trac.webkit.org/changeset/64275.
Comment 3 Nikolas Zimmermann 2011-04-22 23:26:25 PDT
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 :-)
Comment 4 Tim Horton 2011-06-20 17:45:46 PDT
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.
Comment 5 Nikolas Zimmermann 2011-06-20 23:21:38 PDT
(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.
Comment 6 Tim Horton 2011-09-12 18:18:25 PDT
Created attachment 107122 [details]
patch
Comment 7 Early Warning System Bot 2011-09-12 18:44:11 PDT
Comment on attachment 107122 [details]
patch

Attachment 107122 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9646270
Comment 8 Gyuyoung Kim 2011-09-12 18:56:50 PDT
Comment on attachment 107122 [details]
patch

Attachment 107122 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9649283
Comment 9 WebKit Review Bot 2011-09-12 18:58:16 PDT
Comment on attachment 107122 [details]
patch

Attachment 107122 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9641753
Comment 10 WebKit Review Bot 2011-09-12 19:41:06 PDT
Comment on attachment 107122 [details]
patch

Attachment 107122 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9647290
Comment 11 Collabora GTK+ EWS bot 2011-09-13 01:49:55 PDT
Comment on attachment 107122 [details]
patch

Attachment 107122 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9655051
Comment 12 Tim Horton 2011-09-13 10:36:17 PDT
Created attachment 107192 [details]
patch which should build more places
Comment 13 Tim Horton 2011-09-13 10:39:13 PDT
Created attachment 107193 [details]
patch
Comment 14 Simon Fraser (smfr) 2011-09-13 10:54:51 PDT
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?
Comment 15 Tim Horton 2011-09-13 11:16:17 PDT
(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()?
Comment 16 Simon Fraser (smfr) 2011-09-13 12:05:11 PDT
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.
Comment 17 Tim Horton 2011-09-13 13:10:22 PDT
Created attachment 107218 [details]
patch with casting functions
Comment 18 WebKit Review Bot 2011-09-13 14:53:11 PDT
Comment on attachment 107218 [details]
patch with casting functions

Clearing flags on attachment: 107218

Committed r95047: <http://trac.webkit.org/changeset/95047>
Comment 19 WebKit Review Bot 2011-09-13 14:53:16 PDT
All reviewed patches have been landed.  Closing bug.