Bug 39536 – SVG Filter: Crash if parent and child elements use the same filter

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[jay]

open uri, wait around 5 seconds

[jay]

Created attachment 56787 [details]
crash log

[jay]

Created attachment 56788 [details]
copy of URI file at time of crash

[Dirk Schulze]

Created attachment 56818 [details]
Test case - crashes WebKit!

This is a simple test case for the crash. It seems not possible, that a parent and a child element take the same filter:

<g filter="url(#filter)">
    <rect width="100" height="100" filter="url(#filter)"/>
</g>

Not absolutely sure, what causes this crash, but I guess it's the following line:

http://trac.webkit.org/browser/trunk/WebCore/rendering/RenderSVGResourceFilter.cpp#L253

This overwrites the reference to the temporary context. The problem is maybe, that we overwrite the saved context twice by calling it twice and so loose the reference to the original context. We might think about storing the sourceGraphicBuffer and the saved context in FilterData. This should fix this issue, but I did not test it yet.

[Dirk Schulze]

Created attachment 56876 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 56876 [details]
Patch

LGTM, r=me.

[W. James MacLean]

(In reply to comment #3)
> 
> This overwrites the reference to the temporary context. The problem is maybe, that we overwrite the saved context twice by calling it twice and so loose the reference to the original context. We might think about storing the sourceGraphicBuffer and the saved context in FilterData. This should fix this issue, but I did not test it yet.

I've been looking at this bug from the chrome point of view, and agree about Dirk's suggested fix (I was getting ready to submit a similar patch).

Can I please ask that the patch also be tested on

http://upload.wikimedia.org/wikipedia/commons/b/b5/Cloud_computing.svg

as it has a reasonably complicated nesting of parents/children in the filter structure?

[Dirk Schulze]

Comment on attachment 56876 [details]
Patch

Clearing flags on attachment: 56876

Committed r60439: <http://trac.webkit.org/changeset/60439>

[Dirk Schulze]

All reviewed patches have been landed.  Closing bug.

[W. James MacLean]

There appears to be fairly identical context-saving code in RenderSVGResourceGradient{.h|.cpp}.

Can this same problem arise with gradients also?

[Dirk Schulze]

(In reply to comment #6)
> (In reply to comment #3)
> > 
> > This overwrites the reference to the temporary context. The problem is maybe, that we overwrite the saved context twice by calling it twice and so loose the reference to the original context. We might think about storing the sourceGraphicBuffer and the saved context in FilterData. This should fix this issue, but I did not test it yet.
> 
> I've been looking at this bug from the chrome point of view, and agree about Dirk's suggested fix (I was getting ready to submit a similar patch).
> 
> Can I please ask that the patch also be tested on
> 
> http://upload.wikimedia.org/wikipedia/commons/b/b5/Cloud_computing.svg
> 
> as it has a reasonably complicated nesting of parents/children in the filter structure?

I checked the page on QtWebkit WebKitGtk and Chromium, all without the patch but filters enabled. Only Chromium crashed on this page. Are you sure, that this patch can fix the issue? Do you have a backtrace?

[Dirk Schulze]

(In reply to comment #9)
> There appears to be fairly identical context-saving code in RenderSVGResourceGradient{.h|.cpp}.
> 
> Can this same problem arise with gradients also?

Can you explain your fears a bit more? Filters can be applied to a parent as well as a child. This is not possible on gradients or patterns, since only RenderPath takes care about fill and stroke properties. But RenderPath can't have childs.

[W. James MacLean]

(In reply to comment #10)
> 
> I checked the page on QtWebkit WebKitGtk and Chromium, all without the patch but filters enabled. Only Chromium crashed on this page. Are you sure, that this patch can fix the issue? Do you have a backtrace?

I tried the patch this morning, and it does appear to fix the problem for Chromium.

[W. James MacLean]

(In reply to comment #11)
> 
> Can you explain your fears a bit more? Filters can be applied to a parent as well as a child. This is not possible on gradients or patterns, since only RenderPath takes care about fill and stroke properties. But RenderPath can't have childs.

My fear was that perhaps gradients could be applied to children as well, but if that's not the case then it's not an issue. Thanks!

[Dirk Schulze]

(In reply to comment #13)
> (In reply to comment #11)
> > 
> > Can you explain your fears a bit more? Filters can be applied to a parent as well as a child. This is not possible on gradients or patterns, since only RenderPath takes care about fill and stroke properties. But RenderPath can't have childs.
> 
> My fear was that perhaps gradients could be applied to children as well, but if that's not the case then it's not an issue. Thanks!

Yes, only clipper and masker can be applied to parent and children beside filters. But I don't see a issue there. Only the boundries are used by all instances of the same resource and this is wanted and used to speed up the rendering.