Bug 51061 - REGRESSION (64275): Shape pattern-image fill turns black
Summary: REGRESSION (64275): Shape pattern-image fill turns black
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.6
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-12-14 14:47 PST by Matthew Delaney
Modified: 2011-09-13 14:53 PDT (History)
10 users (show)

See Also:


Attachments
testcase1 (6.59 KB, application/zip)
2010-12-14 14:47 PST, Matthew Delaney
no flags Details
patch (7.91 KB, patch)
2011-09-12 18:18 PDT, Tim Horton
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
patch which should build more places (6.56 KB, patch)
2011-09-13 10:36 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
patch (8.23 KB, patch)
2011-09-13 10:39 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
patch with casting functions (8.78 KB, patch)
2011-09-13 13:10 PDT, Tim Horton
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.