Bug 39536 - SVG Filter: Crash if parent and child elements use the same filter
Summary: SVG Filter: Crash if parent and child elements use the same filter
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac (PowerPC) OS X 10.5
: P2 Critical
Assignee: Dirk Schulze
URL: http://www.pixelfans.de/test/svg/brow...
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-22 13:00 PDT by jay
Modified: 2010-05-31 11:01 PDT (History)
3 users (show)

See Also:


Attachments
crash log (33.52 KB, application/octet-stream)
2010-05-22 13:01 PDT, jay
no flags Details
copy of URI file at time of crash (1.89 KB, image/svg+xml)
2010-05-22 13:02 PDT, jay
no flags Details
Test case - crashes WebKit! (266 bytes, image/svg+xml)
2010-05-23 04:38 PDT, Dirk Schulze
no flags Details
Patch (21.90 KB, patch)
2010-05-24 05:43 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jay 2010-05-22 13:00:36 PDT
open uri, wait around 5 seconds
Comment 1 jay 2010-05-22 13:01:50 PDT
Created attachment 56787 [details]
crash log
Comment 2 jay 2010-05-22 13:02:49 PDT
Created attachment 56788 [details]
copy of URI file at time of crash
Comment 3 Dirk Schulze 2010-05-23 04:38:05 PDT
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.
Comment 4 Dirk Schulze 2010-05-24 05:43:19 PDT
Created attachment 56876 [details]
Patch
Comment 5 Nikolas Zimmermann 2010-05-26 02:19:02 PDT
Comment on attachment 56876 [details]
Patch

LGTM, r=me.
Comment 6 W. James MacLean 2010-05-26 11:34:41 PDT
(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?
Comment 7 Dirk Schulze 2010-05-31 08:25:39 PDT
Comment on attachment 56876 [details]
Patch

Clearing flags on attachment: 56876

Committed r60439: <http://trac.webkit.org/changeset/60439>
Comment 8 Dirk Schulze 2010-05-31 08:25:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 W. James MacLean 2010-05-31 09:03:42 PDT
There appears to be fairly identical context-saving code in RenderSVGResourceGradient{.h|.cpp}.

Can this same problem arise with gradients also?
Comment 10 Dirk Schulze 2010-05-31 10:26:34 PDT
(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?
Comment 11 Dirk Schulze 2010-05-31 10:28:51 PDT
(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.
Comment 12 W. James MacLean 2010-05-31 10:48:40 PDT
(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.
Comment 13 W. James MacLean 2010-05-31 10:49:51 PDT
(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!
Comment 14 Dirk Schulze 2010-05-31 11:01:25 PDT
(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.