Bug 53088 - SVG: "filter" race condition may prevent SVG elements from being re-drawn
Summary: SVG: "filter" race condition may prevent SVG elements from being re-drawn
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows Vista
: P1 Normal
Assignee: Nobody
URL:
Keywords:
: 72795 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-01-25 08:06 PST by Berend-Jan Wever
Modified: 2012-01-03 12:54 PST (History)
9 users (show)

See Also:


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 Details | Formatted Diff | Diff
Patch (19.69 KB, patch)
2011-12-12 09:00 PST, Branimir Lambov
no flags Details | Formatted Diff | Diff
Patch (20.41 KB, patch)
2011-12-13 07:09 PST, Branimir Lambov
no flags Details | Formatted Diff | Diff
Patch (20.63 KB, patch)
2011-12-20 09:13 PST, Branimir Lambov
no flags Details | Formatted Diff | Diff
Patch (20.65 KB, patch)
2011-12-22 02:35 PST, Branimir Lambov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Berend-Jan Wever 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.
Comment 1 Berend-Jan Wever 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.
Comment 2 Branimir Lambov 2011-12-09 04:57:56 PST
Created attachment 118560 [details]
Testcase using animated SVG
Comment 3 Branimir Lambov 2011-12-12 07:57:48 PST
Created attachment 118789 [details]
Patch
Comment 4 Early Warning System Bot 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
Comment 5 WebKit Review Bot 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
Comment 6 Branimir Lambov 2011-12-12 09:00:28 PST
Created attachment 118803 [details]
Patch
Comment 7 Nikolas Zimmermann 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.
Comment 8 Branimir Lambov 2011-12-13 07:09:17 PST
Created attachment 119015 [details]
Patch
Comment 9 Branimir Lambov 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.
Comment 10 Nikolas Zimmermann 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);
Comment 11 Branimir Lambov 2011-12-20 09:13:49 PST
Created attachment 120037 [details]
Patch
Comment 12 Branimir Lambov 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.
Comment 13 Nikolas Zimmermann 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.
Comment 14 WebKit Review Bot 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
Comment 15 Branimir Lambov 2011-12-22 02:35:27 PST
Created attachment 120293 [details]
Patch
Comment 16 WebKit Review Bot 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>
Comment 17 WebKit Review Bot 2011-12-22 06:45:26 PST
All reviewed patches have been landed.  Closing bug.
Comment 18 Ryosuke Niwa 2011-12-22 21:59:15 PST
WinCE build fix landed in http://trac.webkit.org/changeset/103605.
Comment 19 Tim Horton 2012-01-03 12:54:47 PST
*** Bug 72795 has been marked as a duplicate of this bug. ***