[Webkit-unassigned] [Bug 53088] SVG: "filter" race condition may prevent SVG elements from being re-drawn

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=53088


Branimir Lambov <blambov at google.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #119015|1                           |0
        is obsolete|                            |




--- Comment #12 from Branimir Lambov <blambov at google.com>  2011-12-20 09:15:14 PST ---
(From update of attachment 119015)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.