- Revision
- 131488
- Author
- [email protected]
- Date
- 2012-10-16 12:48:18 -0700 (Tue, 16 Oct 2012)
Log Message
An feImage that tries to render itself should be stopped
https://bugs.webkit.org/show_bug.cgi?id=94652
Reviewed by Eric Seidel.
Source/WebCore:
An SVG feImage filter element will accept, as the src to render, an
SVG document that makes use of the feImage itself. This causes the
feImage to try to draw itself while already in the process of drawing
itself. Various problems arise from this. The invariant we wish to
maintain is that no element in the src tree of an feImage element
refers to that feImage.
This patch adds a flag to all FilterData objects that tracks whether or
not the filter is currently applying itself, and avoids applying the
filter recursively.
While it may seem better to catch this problem when the src is set, or
when the filter is built, that turns out to be challenging and
inefficient. Say we choose to test when the src atttribute is set. To
do so would require looking through all of the DOM nodes that will be
rendered for the src, finding all resources used, and checking if any
of them make use fo the feImage element that we are setting the source
for. The infrastructure is not in place to do that, and it would
involve walking a potentially very large portion of the DOM in order
to detect a very rare situation. Note that it is not enough just to
walk the DOM directly under the src; we also need to recursively follow any
resource links to see if they use the feImage (e.g. patterns or
masks or use or ...).
If we instead try to use the renderer node to find self referencing,
we need to recursively walk a potentially very large render tree,
tracing all resources in search of the feImage. This would need to be
done every time the filter is built, which is again a significant
overhead for a situation that is very unlikely to occur. And we do not
have methods that make it easy to find feImage filter effect nodes; they are
hidden behind filter resource nodes.
Hence the runtime check to catch the problem. The check must be in
FilterData and RenderSVGResourceFilter code because we must prevent
the destruction of the feImage when we encounter it recursively.
This patch also renames FilterData::builded to FilterData::isBuilt.
Test: svg/filters/feImage-self-referencing.html
* rendering/svg/RenderSVGResourceFilter.cpp:
(WebCore::ApplyingFilterEffectGuard): Guard to ensure that, in the future, we always
clear the isApplying flag even if the postApplyResource method returns early.
(WebCore::RenderSVGResourceFilter::applyResource): Do not apply a resource that is already applying and
rename builded to isBuilt.
(WebCore::RenderSVGResourceFilter::postApplyResource): Mark a resource as applying and clear after
it is done. Abort if a resource is already applying when the method begins. Rename builded to isBuilt.
(WebCore::RenderSVGResourceFilter::primitiveAttributeChanged): Rename builded to isBuilt.
* rendering/svg/RenderSVGResourceFilter.h:
(WebCore::FilterData::FilterData):
(FilterData): Add isApplying and rename builded to isBuilt.
LayoutTests:
Ref-test to verify that an feImage that tries to draw itself will not
crash. This test must be render in order to verify the result. Do not
convert to dumpAstext.
* svg/filters/feImage-self-referencing-expected.html: Added.
* svg/filters/feImage-self-referencing.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (131487 => 131488)
--- trunk/LayoutTests/ChangeLog 2012-10-16 19:45:46 UTC (rev 131487)
+++ trunk/LayoutTests/ChangeLog 2012-10-16 19:48:18 UTC (rev 131488)
@@ -1,3 +1,17 @@
+2012-10-16 Stephen Chenney <[email protected]>
+
+ An feImage that tries to render itself should be stopped
+ https://bugs.webkit.org/show_bug.cgi?id=94652
+
+ Reviewed by Eric Seidel.
+
+ Ref-test to verify that an feImage that tries to draw itself will not
+ crash. This test must be render in order to verify the result. Do not
+ convert to dumpAstext.
+
+ * svg/filters/feImage-self-referencing-expected.html: Added.
+ * svg/filters/feImage-self-referencing.html: Added.
+
2012-10-16 Nico Weber <[email protected]>
[chromium/mac] Make spelling indicator HighDPI
Added: trunk/LayoutTests/svg/filters/feImage-self-referencing-expected.html (0 => 131488)
--- trunk/LayoutTests/svg/filters/feImage-self-referencing-expected.html (rev 0)
+++ trunk/LayoutTests/svg/filters/feImage-self-referencing-expected.html 2012-10-16 19:48:18 UTC (rev 131488)
@@ -0,0 +1,6 @@
+<html>
+ <body>
+ <p>PASS if no crash</p>
+ </body>
+</html>
+
Added: trunk/LayoutTests/svg/filters/feImage-self-referencing.html (0 => 131488)
--- trunk/LayoutTests/svg/filters/feImage-self-referencing.html (rev 0)
+++ trunk/LayoutTests/svg/filters/feImage-self-referencing.html 2012-10-16 19:48:18 UTC (rev 131488)
@@ -0,0 +1,23 @@
+<html>
+ <head>
+ <script>
+ _onload_ = function() {
+ document.getElementById("feImage").setAttributeNS('http://www.w3.org/1999/xlink', 'xlink:href', '#svgRoot')
+ }
+ </script>
+ </head>
+ <body>
+ <p>PASS if no crash</p>
+ <svg width="10" height="10" id="svgRoot" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
+ <g>
+ <defs>
+ <filter id="imageFilter">
+ <feImage id="feImage" />
+ <feOffset dx="50" dy="50" />
+ </filter>
+ </defs>
+ <rect filter="url(#imageFilter)" x="0" y="0" width="50" height="50" fill="green" />
+ </g>
+ </svg>
+ </body>
+</html>
Modified: trunk/Source/WebCore/ChangeLog (131487 => 131488)
--- trunk/Source/WebCore/ChangeLog 2012-10-16 19:45:46 UTC (rev 131487)
+++ trunk/Source/WebCore/ChangeLog 2012-10-16 19:48:18 UTC (rev 131488)
@@ -1,3 +1,61 @@
+2012-10-16 Stephen Chenney <[email protected]>
+ An feImage that tries to render itself should be stopped
+ https://bugs.webkit.org/show_bug.cgi?id=94652
+
+ Reviewed by Eric Seidel.
+
+ An SVG feImage filter element will accept, as the src to render, an
+ SVG document that makes use of the feImage itself. This causes the
+ feImage to try to draw itself while already in the process of drawing
+ itself. Various problems arise from this. The invariant we wish to
+ maintain is that no element in the src tree of an feImage element
+ refers to that feImage.
+
+ This patch adds a flag to all FilterData objects that tracks whether or
+ not the filter is currently applying itself, and avoids applying the
+ filter recursively.
+
+ While it may seem better to catch this problem when the src is set, or
+ when the filter is built, that turns out to be challenging and
+ inefficient. Say we choose to test when the src atttribute is set. To
+ do so would require looking through all of the DOM nodes that will be
+ rendered for the src, finding all resources used, and checking if any
+ of them make use fo the feImage element that we are setting the source
+ for. The infrastructure is not in place to do that, and it would
+ involve walking a potentially very large portion of the DOM in order
+ to detect a very rare situation. Note that it is not enough just to
+ walk the DOM directly under the src; we also need to recursively follow any
+ resource links to see if they use the feImage (e.g. patterns or
+ masks or use or ...).
+
+ If we instead try to use the renderer node to find self referencing,
+ we need to recursively walk a potentially very large render tree,
+ tracing all resources in search of the feImage. This would need to be
+ done every time the filter is built, which is again a significant
+ overhead for a situation that is very unlikely to occur. And we do not
+ have methods that make it easy to find feImage filter effect nodes; they are
+ hidden behind filter resource nodes.
+
+ Hence the runtime check to catch the problem. The check must be in
+ FilterData and RenderSVGResourceFilter code because we must prevent
+ the destruction of the feImage when we encounter it recursively.
+
+ This patch also renames FilterData::builded to FilterData::isBuilt.
+
+ Test: svg/filters/feImage-self-referencing.html
+
+ * rendering/svg/RenderSVGResourceFilter.cpp:
+ (WebCore::ApplyingFilterEffectGuard): Guard to ensure that, in the future, we always
+ clear the isApplying flag even if the postApplyResource method returns early.
+ (WebCore::RenderSVGResourceFilter::applyResource): Do not apply a resource that is already applying and
+ rename builded to isBuilt.
+ (WebCore::RenderSVGResourceFilter::postApplyResource): Mark a resource as applying and clear after
+ it is done. Abort if a resource is already applying when the method begins. Rename builded to isBuilt.
+ (WebCore::RenderSVGResourceFilter::primitiveAttributeChanged): Rename builded to isBuilt.
+ * rendering/svg/RenderSVGResourceFilter.h:
+ (WebCore::FilterData::FilterData):
+ (FilterData): Add isApplying and rename builded to isBuilt.
+
2012-10-16 Nate Chapin <[email protected]>
sendResourceLoadCallbacks() is poorly named
Modified: trunk/Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp (131487 => 131488)
--- trunk/Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp 2012-10-16 19:45:46 UTC (rev 131487)
+++ trunk/Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp 2012-10-16 19:48:18 UTC (rev 131488)
@@ -57,6 +57,25 @@
namespace WebCore {
+class ApplyingFilterEffectGuard {
+public:
+ ApplyingFilterEffectGuard(FilterData* data)
+ : m_filterData(data)
+ {
+ // The guard must be constructed when the filter is not applying.
+ ASSERT(!m_filterData->isApplying);
+ m_filterData->isApplying = true;
+ }
+
+ ~ApplyingFilterEffectGuard()
+ {
+ ASSERT(m_filterData->isApplying);
+ m_filterData->isApplying = false;
+ }
+
+ FilterData* m_filterData;
+};
+
RenderSVGResourceType RenderSVGResourceFilter::s_resourceType = FilterResourceType;
RenderSVGResourceFilter::RenderSVGResourceFilter(SVGFilterElement* node)
@@ -153,7 +172,7 @@
// draw the stored filter output, not the unfiltered object as well.
if (m_filter.contains(object)) {
FilterData* filterData = m_filter.get(object);
- if (filterData->builded)
+ if (filterData->isBuilt || filterData->isApplying)
return false;
delete m_filter.take(object); // Oops, have to rebuild, go through normal code path
@@ -278,7 +297,12 @@
return;
}
- if (!filterData->builded) {
+ // We have a cycle if we are already applying the data.
+ // This can occur due to FeImage referencing a source that makes use of the FEImage itself.
+ if (filterData->isApplying)
+ return;
+
+ if (!filterData->isBuilt) {
if (!filterData->savedContext) {
removeClientFromCache(object);
return;
@@ -288,16 +312,18 @@
filterData->savedContext = 0;
}
+ ApplyingFilterEffectGuard isApplyingGuard(filterData);
+
FilterEffect* lastEffect = filterData->builder->lastEffect();
if (lastEffect && !filterData->boundaries.isEmpty() && !lastEffect->filterPrimitiveSubregion().isEmpty()) {
// This is the real filtering of the object. It just needs to be called on the
// initial filtering process. We just take the stored filter result on a
// second drawing.
- if (!filterData->builded)
+ if (!filterData->isBuilt)
filterData->filter->setSourceImage(filterData->sourceGraphicBuffer.release());
- // Always true if filterData is just built (filterData->builded is false).
+ // Always true if filterData is just built (filterData->isBuilt is false).
if (!lastEffect->hasResult()) {
lastEffect->apply();
lastEffect->correctFilterResultIfNeeded();
@@ -307,7 +333,7 @@
resultImage->transformColorSpace(lastEffect->colorSpace(), ColorSpaceDeviceRGB);
#endif
}
- filterData->builded = true;
+ filterData->isBuilt = true;
ImageBuffer* resultImage = lastEffect->asImageBuffer();
if (resultImage) {
@@ -339,7 +365,7 @@
for (; it != end; ++it) {
FilterData* filterData = it->value;
- if (!filterData->builded)
+ if (!filterData->isBuilt)
continue;
SVGFilterBuilder* builder = filterData->builder.get();
Modified: trunk/Source/WebCore/rendering/svg/RenderSVGResourceFilter.h (131487 => 131488)
--- trunk/Source/WebCore/rendering/svg/RenderSVGResourceFilter.h 2012-10-16 19:45:46 UTC (rev 131487)
+++ trunk/Source/WebCore/rendering/svg/RenderSVGResourceFilter.h 2012-10-16 19:48:18 UTC (rev 131488)
@@ -44,7 +44,8 @@
public:
FilterData()
: savedContext(0)
- , builded(false)
+ , isBuilt(false)
+ , isApplying(false)
, markedForRemoval(false)
{
}
@@ -56,7 +57,8 @@
AffineTransform shearFreeAbsoluteTransform;
FloatRect boundaries;
FloatSize scale;
- bool builded : 1;
+ bool isBuilt : 1;
+ bool isApplying : 1;
bool markedForRemoval : 1;
};
