<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[190050] releases/WebKitGTK/webkit-2.10</title>
</head>
<body>
<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; }
#msg dl a { font-weight: bold}
#msg dl a:link { color:#fc3; }
#msg dl a:active { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/190050">190050</a></dd>
<dt>Author</dt> <dd>carlosgc@webkit.org</dd>
<dt>Date</dt> <dd>2015-09-21 01:15:32 -0700 (Mon, 21 Sep 2015)</dd>
</dl>
<h3>Log Message</h3>
<pre>Merge <a href="http://trac.webkit.org/projects/webkit/changeset/189954">r189954</a> - Multi-hop reference cycles not detected.
https://bugs.webkit.org/show_bug.cgi?id=149181
Reviewed by John Honeycutt.
Source/WebCore:
SVG's cycle detection was not picking up a
case where an element was drawing a pattern, that
referenced another pattern, that referenced another
pattern, that referenced the original pattern.
The issue was that we were forgetting to check the
children of the renderer itself, rather than just
the children of the referenced renderers.
Found by running a test from Blink.
I also took the opportunity to clean up the debugging
code that logs cycle detection.
Test: svg/custom/pattern-3-step-cycle.html
* platform/Logging.h: Add a new SVG channel. I can't believe we
didn't already have one!
* rendering/svg/SVGResourcesCycleSolver.cpp:
(WebCore::SVGResourcesCycleSolver::resourceContainsCycles): Check the referenced
resources for cycles.
(WebCore::SVGResourcesCycleSolver::resolveCycles): Logging update.
LayoutTests:
Test comes from:
https://chromium.googlesource.com/chromium/blink/+/master/LayoutTests/svg/custom/pattern-3-step-cycle.html
* svg/custom/pattern-3-step-cycle-expected.txt: Added.
* svg/custom/pattern-3-step-cycle.html: Added.</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#releasesWebKitGTKwebkit210LayoutTestsChangeLog">releases/WebKitGTK/webkit-2.10/LayoutTests/ChangeLog</a></li>
<li><a href="#releasesWebKitGTKwebkit210SourceWebCoreChangeLog">releases/WebKitGTK/webkit-2.10/Source/WebCore/ChangeLog</a></li>
<li><a href="#releasesWebKitGTKwebkit210SourceWebCoreplatformLoggingh">releases/WebKitGTK/webkit-2.10/Source/WebCore/platform/Logging.h</a></li>
<li><a href="#releasesWebKitGTKwebkit210SourceWebCorerenderingsvgSVGResourcesCycleSolvercpp">releases/WebKitGTK/webkit-2.10/Source/WebCore/rendering/svg/SVGResourcesCycleSolver.cpp</a></li>
</ul>
<h3>Added Paths</h3>
<ul>
<li><a href="#releasesWebKitGTKwebkit210LayoutTestssvgcustompattern3stepcycleexpectedtxt">releases/WebKitGTK/webkit-2.10/LayoutTests/svg/custom/pattern-3-step-cycle-expected.txt</a></li>
<li><a href="#releasesWebKitGTKwebkit210LayoutTestssvgcustompattern3stepcyclehtml">releases/WebKitGTK/webkit-2.10/LayoutTests/svg/custom/pattern-3-step-cycle.html</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="releasesWebKitGTKwebkit210LayoutTestsChangeLog"></a>
<div class="modfile"><h4>Modified: releases/WebKitGTK/webkit-2.10/LayoutTests/ChangeLog (190049 => 190050)</h4>
<pre class="diff"><span>
<span class="info">--- releases/WebKitGTK/webkit-2.10/LayoutTests/ChangeLog 2015-09-21 08:12:28 UTC (rev 190049)
+++ releases/WebKitGTK/webkit-2.10/LayoutTests/ChangeLog 2015-09-21 08:15:32 UTC (rev 190050)
</span><span class="lines">@@ -1,5 +1,18 @@
</span><span class="cx"> 2015-09-17 Dean Jackson <dino@apple.com>
</span><span class="cx">
</span><ins>+ Multi-hop reference cycles not detected.
+ https://bugs.webkit.org/show_bug.cgi?id=149181
+
+ Reviewed by John Honeycutt.
+
+ Test comes from:
+ https://chromium.googlesource.com/chromium/blink/+/master/LayoutTests/svg/custom/pattern-3-step-cycle.html
+
+ * svg/custom/pattern-3-step-cycle-expected.txt: Added.
+ * svg/custom/pattern-3-step-cycle.html: Added.
+
+2015-09-17 Dean Jackson <dino@apple.com>
+
</ins><span class="cx"> Cyclic resources were not detected if the reference had deep containers
</span><span class="cx"> https://bugs.webkit.org/show_bug.cgi?id=149182
</span><span class="cx">
</span></span></pre></div>
<a id="releasesWebKitGTKwebkit210LayoutTestssvgcustompattern3stepcycleexpectedtxt"></a>
<div class="addfile"><h4>Added: releases/WebKitGTK/webkit-2.10/LayoutTests/svg/custom/pattern-3-step-cycle-expected.txt (0 => 190050)</h4>
<pre class="diff"><span>
<span class="info">--- releases/WebKitGTK/webkit-2.10/LayoutTests/svg/custom/pattern-3-step-cycle-expected.txt (rev 0)
+++ releases/WebKitGTK/webkit-2.10/LayoutTests/svg/custom/pattern-3-step-cycle-expected.txt 2015-09-21 08:15:32 UTC (rev 190050)
</span><span class="lines">@@ -0,0 +1,3 @@
</span><ins>+PASS if no crash.
+
+
</ins></span></pre></div>
<a id="releasesWebKitGTKwebkit210LayoutTestssvgcustompattern3stepcyclehtml"></a>
<div class="addfile"><h4>Added: releases/WebKitGTK/webkit-2.10/LayoutTests/svg/custom/pattern-3-step-cycle.html (0 => 190050)</h4>
<pre class="diff"><span>
<span class="info">--- releases/WebKitGTK/webkit-2.10/LayoutTests/svg/custom/pattern-3-step-cycle.html (rev 0)
+++ releases/WebKitGTK/webkit-2.10/LayoutTests/svg/custom/pattern-3-step-cycle.html 2015-09-21 08:15:32 UTC (rev 190050)
</span><span class="lines">@@ -0,0 +1,23 @@
</span><ins>+<!DOCTYPE html>
+<script>
+if (window.testRunner) {
+ testRunner.dumpAsText();
+ testRunner.waitUntilDone();
+ window.onload = function() {
+ testRunner.notifyDone();
+ };
+}
+</script>
+<p>PASS if no crash.</p>
+<svg width="100" height="100">
+ <rect width="100" height="100" fill="url(#p1)"/>
+ <pattern id="p3" width="1" height="1">
+ <rect fill="url(#p1)" width="100" height="100"/>
+ </pattern>
+ <pattern id="p2" width="1" height="1">
+ <rect fill="url(#p3)" width="100" height="100"/>
+ </pattern>
+ <pattern id="p1" width="1" height="1">
+ <rect fill="url(#p2)" width="100" height="100"/>
+ </pattern>
+</svg>
</ins></span></pre></div>
<a id="releasesWebKitGTKwebkit210SourceWebCoreChangeLog"></a>
<div class="modfile"><h4>Modified: releases/WebKitGTK/webkit-2.10/Source/WebCore/ChangeLog (190049 => 190050)</h4>
<pre class="diff"><span>
<span class="info">--- releases/WebKitGTK/webkit-2.10/Source/WebCore/ChangeLog 2015-09-21 08:12:28 UTC (rev 190049)
+++ releases/WebKitGTK/webkit-2.10/Source/WebCore/ChangeLog 2015-09-21 08:15:32 UTC (rev 190050)
</span><span class="lines">@@ -1,5 +1,35 @@
</span><span class="cx"> 2015-09-17 Dean Jackson <dino@apple.com>
</span><span class="cx">
</span><ins>+ Multi-hop reference cycles not detected.
+ https://bugs.webkit.org/show_bug.cgi?id=149181
+
+ Reviewed by John Honeycutt.
+
+ SVG's cycle detection was not picking up a
+ case where an element was drawing a pattern, that
+ referenced another pattern, that referenced another
+ pattern, that referenced the original pattern.
+
+ The issue was that we were forgetting to check the
+ children of the renderer itself, rather than just
+ the children of the referenced renderers.
+
+ Found by running a test from Blink.
+
+ I also took the opportunity to clean up the debugging
+ code that logs cycle detection.
+
+ Test: svg/custom/pattern-3-step-cycle.html
+
+ * platform/Logging.h: Add a new SVG channel. I can't believe we
+ didn't already have one!
+ * rendering/svg/SVGResourcesCycleSolver.cpp:
+ (WebCore::SVGResourcesCycleSolver::resourceContainsCycles): Check the referenced
+ resources for cycles.
+ (WebCore::SVGResourcesCycleSolver::resolveCycles): Logging update.
+
+2015-09-17 Dean Jackson <dino@apple.com>
+
</ins><span class="cx"> Cyclic resources were not detected if the reference had deep containers
</span><span class="cx"> https://bugs.webkit.org/show_bug.cgi?id=149182
</span><span class="cx">
</span></span></pre></div>
<a id="releasesWebKitGTKwebkit210SourceWebCoreplatformLoggingh"></a>
<div class="modfile"><h4>Modified: releases/WebKitGTK/webkit-2.10/Source/WebCore/platform/Logging.h (190049 => 190050)</h4>
<pre class="diff"><span>
<span class="info">--- releases/WebKitGTK/webkit-2.10/Source/WebCore/platform/Logging.h 2015-09-21 08:12:28 UTC (rev 190049)
+++ releases/WebKitGTK/webkit-2.10/Source/WebCore/platform/Logging.h 2015-09-21 08:15:32 UTC (rev 190050)
</span><span class="lines">@@ -68,6 +68,7 @@
</span><span class="cx"> M(RemoteInspector) \
</span><span class="cx"> M(ResourceLoading) \
</span><span class="cx"> M(SQLDatabase) \
</span><ins>+ M(SVG) \
</ins><span class="cx"> M(Services) \
</span><span class="cx"> M(SpellingAndGrammar) \
</span><span class="cx"> M(StorageAPI) \
</span></span></pre></div>
<a id="releasesWebKitGTKwebkit210SourceWebCorerenderingsvgSVGResourcesCycleSolvercpp"></a>
<div class="modfile"><h4>Modified: releases/WebKitGTK/webkit-2.10/Source/WebCore/rendering/svg/SVGResourcesCycleSolver.cpp (190049 => 190050)</h4>
<pre class="diff"><span>
<span class="info">--- releases/WebKitGTK/webkit-2.10/Source/WebCore/rendering/svg/SVGResourcesCycleSolver.cpp 2015-09-21 08:12:28 UTC (rev 190049)
+++ releases/WebKitGTK/webkit-2.10/Source/WebCore/rendering/svg/SVGResourcesCycleSolver.cpp 2015-09-21 08:15:32 UTC (rev 190050)
</span><span class="lines">@@ -20,9 +20,7 @@
</span><span class="cx"> #include "config.h"
</span><span class="cx"> #include "SVGResourcesCycleSolver.h"
</span><span class="cx">
</span><del>-// Set to a value > 0, to debug the resource cache.
-#define DEBUG_CYCLE_DETECTION 0
-
</del><ins>+#include "Logging.h"
</ins><span class="cx"> #include "RenderAncestorIterator.h"
</span><span class="cx"> #include "RenderSVGResourceClipper.h"
</span><span class="cx"> #include "RenderSVGResourceFilter.h"
</span><span class="lines">@@ -33,6 +31,15 @@
</span><span class="cx"> #include "SVGResources.h"
</span><span class="cx"> #include "SVGResourcesCache.h"
</span><span class="cx">
</span><ins>+// Set to truthy value to debug the resource cache.
+#define DEBUG_CYCLE_DETECTION 0
+
+#if DEBUG_CYCLE_DETECTION
+#define LOG_DEBUG_CYCLE(...) LOG(SVG, __VA_ARGS__)
+#else
+#define LOG_DEBUG_CYCLE(...) ((void)0)
+#endif
+
</ins><span class="cx"> namespace WebCore {
</span><span class="cx">
</span><span class="cx"> SVGResourcesCycleSolver::SVGResourcesCycleSolver(RenderElement& renderer, SVGResources& resources)
</span><span class="lines">@@ -47,6 +54,8 @@
</span><span class="cx">
</span><span class="cx"> bool SVGResourcesCycleSolver::resourceContainsCycles(RenderElement& renderer) const
</span><span class="cx"> {
</span><ins>+ LOG_DEBUG_CYCLE("\n(%p) Check for cycles\n", &renderer);
+
</ins><span class="cx"> // First operate on the resources of the given renderer.
</span><span class="cx"> // <marker id="a"> <path marker-start="url(#b)"/> ...
</span><span class="cx"> // <marker id="b" marker-start="url(#a)"/>
</span><span class="lines">@@ -54,34 +63,56 @@
</span><span class="cx"> HashSet<RenderSVGResourceContainer*> resourceSet;
</span><span class="cx"> resources->buildSetOfResources(resourceSet);
</span><span class="cx">
</span><ins>+ LOG_DEBUG_CYCLE("(%p) Examine our cached resources\n", &renderer);
+
</ins><span class="cx"> // Walk all resources and check whether they reference any resource contained in the resources set.
</span><span class="cx"> for (auto* resource : resourceSet) {
</span><ins>+ LOG_DEBUG_CYCLE("(%p) Check %p\n", &renderer, resource);
</ins><span class="cx"> if (m_allResources.contains(resource))
</span><span class="cx"> return true;
</span><ins>+
+ // Now check if the resources themselves contain cycles.
+ if (resourceContainsCycles(*resource))
+ return true;
</ins><span class="cx"> }
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+ LOG_DEBUG_CYCLE("(%p) Now the children renderers\n", &renderer);
+
</ins><span class="cx"> // Then operate on the child resources of the given renderer.
</span><span class="cx"> // <marker id="a"> <path marker-start="url(#b)"/> ...
</span><span class="cx"> // <marker id="b"> <path marker-start="url(#a)"/> ...
</span><span class="cx"> for (auto& child : childrenOfType<RenderElement>(renderer)) {
</span><ins>+
+ LOG_DEBUG_CYCLE("(%p) Checking child %p\n", &renderer, &child);
+
</ins><span class="cx"> if (auto* childResources = SVGResourcesCache::cachedResourcesForRenderer(child)) {
</span><ins>+
+ LOG_DEBUG_CYCLE("(%p) Child %p had cached resources. Check them.\n", &renderer, &child);
+
</ins><span class="cx"> // A child of the given 'resource' contains resources.
</span><span class="cx"> HashSet<RenderSVGResourceContainer*> childResourceSet;
</span><span class="cx"> childResources->buildSetOfResources(childResourceSet);
</span><span class="cx">
</span><span class="cx"> // Walk all child resources and check whether they reference any resource contained in the resources set.
</span><span class="cx"> for (auto* resource : childResourceSet) {
</span><ins>+ LOG_DEBUG_CYCLE("(%p) Child %p had resource %p\n", &renderer, &child, resource);
</ins><span class="cx"> if (m_allResources.contains(resource))
</span><span class="cx"> return true;
</span><span class="cx"> }
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+ LOG_DEBUG_CYCLE("(%p) Recurse into child %p\n", &renderer, &child);
+
</ins><span class="cx"> // Walk children recursively, stop immediately if we found a cycle
</span><span class="cx"> if (resourceContainsCycles(child))
</span><span class="cx"> return true;
</span><ins>+
+ LOG_DEBUG_CYCLE("\n(%p) Child %p was ok\n", &renderer, &child);
</ins><span class="cx"> }
</span><span class="cx">
</span><ins>+ LOG_DEBUG_CYCLE("\n(%p) No cycles found\n", &renderer);
+
</ins><span class="cx"> return false;
</span><span class="cx"> }
</span><span class="cx">
</span><span class="lines">@@ -89,8 +120,8 @@
</span><span class="cx"> {
</span><span class="cx"> ASSERT(m_allResources.isEmpty());
</span><span class="cx">
</span><del>-#if DEBUG_CYCLE_DETECTION > 0
- fprintf(stderr, "\nBefore cycle detection:\n");
</del><ins>+#if DEBUG_CYCLE_DETECTION
+ LOG_DEBUG_CYCLE("\nBefore cycle detection:\n");
</ins><span class="cx"> m_resources.dump(&m_renderer);
</span><span class="cx"> #endif
</span><span class="cx">
</span><span class="lines">@@ -104,16 +135,16 @@
</span><span class="cx"> for (auto& resource : ancestorsOfType<RenderSVGResourceContainer>(m_renderer))
</span><span class="cx"> ancestorResources.add(&resource);
</span><span class="cx">
</span><del>-#if DEBUG_CYCLE_DETECTION > 0
- fprintf(stderr, "\nDetecting whether any resources references any of following objects:\n");
</del><ins>+#if DEBUG_CYCLE_DETECTION
+ LOG_DEBUG_CYCLE("\nDetecting whether any resources references any of following objects:\n");
</ins><span class="cx"> {
</span><del>- fprintf(stderr, "Local resources:\n");
</del><ins>+ LOG_DEBUG_CYCLE("Local resources:\n");
</ins><span class="cx"> for (RenderObject* resource : localResources)
</span><del>- fprintf(stderr, "|> %s : %p (node %p)\n", resource->renderName(), resource, resource->node());
</del><ins>+ LOG_DEBUG_CYCLE("|> %s : %p (node %p)\n", resource->renderName(), resource, resource->node());
</ins><span class="cx">
</span><span class="cx"> fprintf(stderr, "Parent resources:\n");
</span><span class="cx"> for (RenderObject* resource : ancestorResources)
</span><del>- fprintf(stderr, "|> %s : %p (node %p)\n", resource->renderName(), resource, resource->node());
</del><ins>+ LOG_DEBUG_CYCLE("|> %s : %p (node %p)\n", resource->renderName(), resource, resource->node());
</ins><span class="cx"> }
</span><span class="cx"> #endif
</span><span class="cx">
</span><span class="lines">@@ -128,15 +159,23 @@
</span><span class="cx">
</span><span class="cx"> ASSERT(!m_allResources.isEmpty());
</span><span class="cx">
</span><ins>+#if DEBUG_CYCLE_DETECTION
+ LOG_DEBUG_CYCLE("\nAll resources:\n");
+ for (auto* resource : m_allResources)
+ LOG_DEBUG_CYCLE("- %p\n", resource);
+#endif
+
</ins><span class="cx"> // The job of this function is to determine wheter any of the 'resources' associated with the given 'renderer'
</span><span class="cx"> // references us (or whether any of its kids references us) -> that's a cycle, we need to find and break it.
</span><span class="cx"> for (auto* resource : localResources) {
</span><del>- if (ancestorResources.contains(resource) || resourceContainsCycles(*resource))
</del><ins>+ if (ancestorResources.contains(resource) || resourceContainsCycles(*resource)) {
+ LOG_DEBUG_CYCLE("\n**** Detected a cycle (see the last test in the output above) ****\n");
</ins><span class="cx"> breakCycle(*resource);
</span><ins>+ }
</ins><span class="cx"> }
</span><span class="cx">
</span><del>-#if DEBUG_CYCLE_DETECTION > 0
- fprintf(stderr, "\nAfter cycle detection:\n");
</del><ins>+#if DEBUG_CYCLE_DETECTION
+ LOG_DEBUG_CYCLE("\nAfter cycle detection:\n");
</ins><span class="cx"> m_resources.dump(&m_renderer);
</span><span class="cx"> #endif
</span><span class="cx">
</span></span></pre>
</div>
</div>
</body>
</html>