RESOLVED FIXED 53088
SVG: "filter" race condition may prevent SVG elements from being re-drawn 
https://bugs.webkit.org/show_bug.cgi?id=53088
Summary SVG: "filter" race condition may prevent SVG elements from being re-drawn
Berend-Jan Wever
Reported 2011-01-25 08:06:25 PST
Created attachment 80066 [details] Repro Chromium bug: http://code.google.com/p/chromium/issues/detail?id=70769 Applying a filter to an SVG element may cause Chrome to stop drawing updates to that SVG element. There appears to be some kind of race condition involved because updates made almost immediately after loading the SVG will be applied, but after a certain (unknown) period, not more changes are applied to the object. Repro: <?xml version="1.0" standalone="yes"?> <svg onload="go()" xmlns="http://www.w3.org/2000/svg" id="svg"> <defs> <filter id="Gaussian_Blur"> <feGaussianBlur in="SourceGraphic" stdDeviation="5"/> </filter> </defs> <foreignObject x="0" y="0" width="100%" height="50"> <body xmlns="http://www.w3.org/1999/xhtml"> <button onclick="document.getElementById('svg').forceRedraw()">forceRedraw()</button> <button onclick="orange()">orange</button> </body> </foreignObject> <g style="filter:url(#Gaussian_Blur)"> <ellipse cx="50" cy="50" rx="10" ry="10" style="fill:red; padding: 10px;" id="blurred"> </ellipse> </g> <ellipse cx="100" cy="50" rx="10" ry="10" style="fill:red; padding: 10px;" id="normal"> </ellipse> <script> function replaceSpanInForeignObject(oForeignObject, sColor) { oForeignObject.style.fill = sColor; } function go() { replaceSpanInForeignObject(document.getElementById('normal'), 'blue'); replaceSpanInForeignObject(document.getElementById('blurred'), 'blue'); setTimeout(function () { replaceSpanInForeignObject(document.getElementById('normal'), 'green'); replaceSpanInForeignObject(document.getElementById('blurred'), 'green'); }, 1); } function orange() { replaceSpanInForeignObject(document.getElementById('normal'), 'orange'); replaceSpanInForeignObject(document.getElementById('blurred'), 'orange'); } </script> </svg> The repro shows two red circles, which are immediately changed to green on loading the page and then changed again to blue using a very small timeout. Loading the page will result in either two green circles, or a blue blurred and a green normal circle, depending on timing. Safari does not seem to be fast enough for me to show this, but it happens about 50% of the time for me in Chrome. Pressing the "orange" button should change both circles to orange, but this only has effect on the non-blurred circle. Pressing the "forceRedraw()" button, calls that method of the SVG element, but this seems to have no effect.
Attachments
Repro (1.48 KB, image/svg+xml)
2011-01-25 08:06 PST, Berend-Jan Wever 		no flags
Details
Testcase using animated SVG (980 bytes, image/svg+xml)
2011-12-09 04:57 PST, Branimir Lambov 		no flags
Details
Patch (20.12 KB, patch)
2011-12-12 07:57 PST, Branimir Lambov 		no flags
DetailsFormatted DiffDiff
Patch (19.69 KB, patch)
2011-12-12 09:00 PST, Branimir Lambov 		no flags
DetailsFormatted DiffDiff
Patch (20.41 KB, patch)
2011-12-13 07:09 PST, Branimir Lambov 		no flags
DetailsFormatted DiffDiff
Patch (20.63 KB, patch)
2011-12-20 09:13 PST, Branimir Lambov 		no flags
DetailsFormatted DiffDiff
Patch (20.65 KB, patch)
2011-12-22 02:35 PST, Branimir Lambov 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Berend-Jan Wever
Comment 1 2011-01-25 08:35:43 PST
Work around: remove the filter style, force a redraw, re-apply the filter style: function redraw() { var oContainer = document.getElementById('blur_container'); var sFilter = oContainer.style.filter; oContainer.style.filter = ''; oContainer.offsetTop; // force redraw oContainer.setAttribute('style', 'filter:url(#Gaussian_Blur)'); } This requires you to add id="blur_container" to the "g" element and call this function every time you modify one of its children.
Branimir Lambov
Comment 2 2011-12-09 04:57:56 PST
Created attachment 118560 [details] Testcase using animated SVG
Branimir Lambov
Comment 3 2011-12-12 07:57:48 PST
Created attachment 118789 [details] Patch
Early Warning System Bot
Comment 4 2011-12-12 08:06:16 PST
Comment on attachment 118789 [details] Patch Attachment 118789 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10836164
WebKit Review Bot
Comment 5 2011-12-12 08:23:16 PST
Comment on attachment 118789 [details] Patch Attachment 118789 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10841135
Branimir Lambov
Comment 6 2011-12-12 09:00:28 PST
Created attachment 118803 [details] Patch
Nikolas Zimmermann
Comment 7 2011-12-12 09:17:07 PST
Comment on attachment 118789 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118789&action=review Nice patch, but with some risks. r- for the test problem and my questions regarding the approach. > Source/WebCore/rendering/svg/RenderSVGResource.cpp:153 > +#if ENABLE(FILTERS) This won't build on release builds. You need to use UNUSED_PARAM(object) in the #else branch. > Source/WebCore/rendering/svg/RenderSVGResource.cpp:162 > + if (object->style()->svgStyle()->hasFilter()) { You could early return here, if there's no filter. Its the preferred way in WebKit. > Source/WebCore/rendering/svg/RenderSVGResource.cpp:164 > + if (resources && resources->filter()) Ditto. > Source/WebCore/rendering/svg/RenderSVGResource.cpp:177 > + // Invalidate filters as they will hold cached copies of their children. > + markFiltersForInvalidation(object); Hm, we have various other places with special cased filter code - can you check all of them, and tell us whether your "sledgehammer" approach (which is probably correct) potentially makes them unnecessary. It's been a while since I checked the code the last time, but I'll bet grepp'ing for resources->filter() or sth. similar will show you several places with invalidation special cases for filters. I just want be sure that we don't add new code, as general as yours, that supersedes existing incorrect special cases. > LayoutTests/svg/filters/filter-refresh.svg:125 > + // eventSender.leapForward(100) does not seem to trigger the animation, so we wait here. > + setTimeout(change, 60); This is always a bit dangerous, potentially leading to races. I'd rather start the animations manually from script, by using begin="indefinite" everywhere, and calling foo.beginElement() for all animations, at once. As you're using <set> animations, you can then set a 0ms timer, if that fires, the animation should have ran. But this should only be done if you _need_ SMIL animations to trigger the bug. We don't support begin/endEvent handling yet, so you cannot reliable know when the SMIL animation finished. I guess you don't want SMIL animations at all so I'd like to see a variant of this test without any <set> elements at all, but rather with scripted animations from JS. That would be safer.
Branimir Lambov
Comment 8 2011-12-13 07:09:17 PST
Created attachment 119015 [details] Patch
Branimir Lambov
Comment 9 2011-12-13 07:16:07 PST
Comment on attachment 119015 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119015&action=review Thank you for your review. I applied your suggestions and uploaded a new version of the patch that tests the problem using a script. I found only two interesting references to filter(), which I've commented on below. > Source/WebCore/rendering/svg/SVGResourcesCache.cpp:-127 > - resources->removeClientFromCache(object); This is being called from layout(), which in turn will never be called if the filter is not already invalidated. > Source/WebCore/rendering/svg/SVGResourcesCache.cpp:-155 > - resources->removeClientFromCache(renderer); This is not sufficient as any filter in the ascendants' chain will block updates to the element. Changed to invalidate all filters in the chain.
Nikolas Zimmermann
Comment 10 2011-12-14 01:53:55 PST
Comment on attachment 119015 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119015&action=review Great work! I have some concerns, especially regarding performance, as you walk the parent hierachy twice in certain cases. See my comments: > Source/WebCore/rendering/svg/RenderSVGResource.cpp:162 > + RenderStyle* style = object->style(); > + ASSERT(style); > + > + const SVGRenderStyle* svgStyle = style->svgStyle(); > + ASSERT(svgStyle); I think we can safely assume, that these are available, SVG always assumed they aren't, but none of the other rendering/ code does (functions like containingBlock() etc, always access m_style w/o null checks). Just remove those lines completly, except the ASSERT(object); I'm not sure that checking hasFilter() actually still makes sense, this check dates back to a time where we didn't have a SVGResourcesCache, and had to lookup the filter right from the style. Today, we just query the SVGResourcesCache, so you can safely remove the hasFilter() check, and avoid the asserts and local variables. > Source/WebCore/rendering/svg/RenderSVGResource.cpp:168 > + if (!(resources && resources->filter())) if (!resources || !resources->filter()) reads better. > Source/WebCore/rendering/svg/RenderSVGResource.cpp:200 > + markFiltersForInvalidation(object); > + markParentsForInvalidation(object); This now walks the parent chine twice! We have to avoid that, you need to refactor the code further. eg. with a general markForFoobarMethod(), that takes an enum MarkMode { MarkFiltersForInvalidation = 1 << 0, MarkParentsForInvaldiation = 1 << 1}, so you can combine flags like MarkFilterForInvalidation | MarkParentsForInvalidation, or just use them individually. This way you can write one optimized method, that walks the parent hierachy once, and does the right job for each of the combinations. >> Source/WebCore/rendering/svg/SVGResourcesCache.cpp:-127 >> - resources->removeClientFromCache(object); > > This is being called from layout(), which in turn will never be called if the filter is not already invalidated. This information needs to go in the ChangeLog! >> Source/WebCore/rendering/svg/SVGResourcesCache.cpp:-155 >> - resources->removeClientFromCache(renderer); > > This is not sufficient as any filter in the ascendants' chain will block updates to the element. Changed to invalidate all filters in the chain. Same here. > LayoutTests/svg/filters/filter-refresh.svg:115 > + setTimeout(wait, 0); > + > + function wait() { > + // Single timeout doesn't seem to wait for the initial painting, wait again. > + setTimeout(change, 0); > + } You have to listen for the window.onload = function() { .. } event. There you can call setTimeout(change, 0); This <script> otherwise executes immediately, as its seen by the parser. I'd highly propose to split up this testcase. Large testcases like this makes it hard to track errors and debug them later, once they fail. I know this will be lots of work, but it'll really help us to get full coverage for all filter invalidation combinations, with nice self-contained testcases, for all kinds of cases. nested containers, use, etc. as in your large all-in-one testcase now :-) > LayoutTests/svg/filters/filter-refresh.svg:125 > + setTimeout("layoutTestController.notifyDone()", 0); You'd usually use anonymous functions here: setTimeout(function() { layoutTestController.notifyDone(); }, 0);
Branimir Lambov
Comment 11 2011-12-20 09:13:49 PST
Created attachment 120037 [details] Patch
Branimir Lambov
Comment 12 2011-12-20 09:15:14 PST
Comment on attachment 119015 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119015&action=review >> Source/WebCore/rendering/svg/RenderSVGResource.cpp:162 >> + ASSERT(svgStyle); > > I think we can safely assume, that these are available, SVG always assumed they aren't, but none of the other rendering/ code does (functions like containingBlock() etc, always access m_style w/o null checks). > Just remove those lines completly, except the ASSERT(object); > > I'm not sure that checking hasFilter() actually still makes sense, this check dates back to a time where we didn't have a SVGResourcesCache, and had to lookup the filter right from the style. > Today, we just query the SVGResourcesCache, so you can safely remove the hasFilter() check, and avoid the asserts and local variables. Done. >> Source/WebCore/rendering/svg/RenderSVGResource.cpp:168 >> + if (!(resources && resources->filter())) > > if (!resources || !resources->filter()) reads better. Done. >> Source/WebCore/rendering/svg/RenderSVGResource.cpp:200 >> + markParentsForInvalidation(object); > > This now walks the parent chine twice! We have to avoid that, you need to refactor the code further. eg. with a general markForFoobarMethod(), that takes an enum MarkMode { MarkFiltersForInvalidation = 1 << 0, MarkParentsForInvaldiation = 1 << 1}, so you can combine flags like MarkFilterForInvalidation | MarkParentsForInvalidation, or just use them individually. This way you can write one optimized method, that walks the parent hierachy once, and does the right job for each of the combinations. Actually, we have to process the filters whether we are called from the element or not, so changed this to always apply them. >>> Source/WebCore/rendering/svg/SVGResourcesCache.cpp:-127 >>> - resources->removeClientFromCache(object); >> >> This is being called from layout(), which in turn will never be called if the filter is not already invalidated. > > This information needs to go in the ChangeLog! Done. >>> Source/WebCore/rendering/svg/SVGResourcesCache.cpp:-155 >>> - resources->removeClientFromCache(renderer); >> >> This is not sufficient as any filter in the ascendants' chain will block updates to the element. Changed to invalidate all filters in the chain. > > Same here. Done. >> LayoutTests/svg/filters/filter-refresh.svg:115 >> + } > > You have to listen for the window.onload = function() { .. } event. There you can call setTimeout(change, 0); > This <script> otherwise executes immediately, as its seen by the parser. > > I'd highly propose to split up this testcase. Large testcases like this makes it hard to track errors and debug them later, once they fail. > I know this will be lots of work, but it'll really help us to get full coverage for all filter invalidation combinations, with nice self-contained testcases, for all kinds of cases. nested containers, use, etc. as in your large all-in-one testcase now :-) Thanks for the suggestion, changed the test to use onload. Honestly speaking I don't see much benefit in splitting the test up. The problem being tested is specific enough, and if the test fails it is easy to run it in a browser and see the lines that draw the red square(s). On the other hand, splitting would mean slower test runs and more gardening overhead for anyone that has to deal with a test failure. >> LayoutTests/svg/filters/filter-refresh.svg:125 >> + setTimeout("layoutTestController.notifyDone()", 0); > > You'd usually use anonymous functions here: setTimeout(function() { layoutTestController.notifyDone(); }, 0); Done.
Nikolas Zimmermann
Comment 13 2011-12-21 12:31:56 PST
Comment on attachment 120037 [details] Patch Okay, I thought about this again it looks good, just go ahead with it. r=me.
WebKit Review Bot
Comment 14 2011-12-21 22:46:39 PST
Comment on attachment 120037 [details] Patch Rejecting attachment 120037 [details] from commit-queue. New failing tests: svg/filters/filter-refresh.svg Full output: http://queues.webkit.org/results/10986332
Branimir Lambov
Comment 15 2011-12-22 02:35:27 PST
Created attachment 120293 [details] Patch
WebKit Review Bot
Comment 16 2011-12-22 06:45:19 PST
Comment on attachment 120293 [details] Patch Clearing flags on attachment: 120293 Committed r103539: <http://trac.webkit.org/changeset/103539>
WebKit Review Bot
Comment 17 2011-12-22 06:45:26 PST
All reviewed patches have been landed. Closing bug.
Ryosuke Niwa
Comment 18 2011-12-22 21:59:15 PST
WinCE build fix landed in http://trac.webkit.org/changeset/103605.
Tim Horton
Comment 19 2012-01-03 12:54:47 PST
*** Bug 72795 has been marked as a duplicate of this bug. ***
Note You need to log in before you can comment on or make changes to this bug.