Multi-hop reference cycles not detected.
authordino@apple.com <dino@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 18 Sep 2015 01:09:57 +0000 (01:09 +0000)
committerdino@apple.com <dino@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 18 Sep 2015 01:09:57 +0000 (01:09 +0000)
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.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@189954 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/svg/custom/pattern-3-step-cycle-expected.txt [new file with mode: 0644]
LayoutTests/svg/custom/pattern-3-step-cycle.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/platform/Logging.h
Source/WebCore/rendering/svg/SVGResourcesCycleSolver.cpp

index 7322565..df3e7a8 100644 (file)
@@ -1,5 +1,18 @@
 2015-09-17  Dean Jackson  <dino@apple.com>
 
+        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>
+
         Cyclic resources were not detected if the reference had deep containers
         https://bugs.webkit.org/show_bug.cgi?id=149182
 
diff --git a/LayoutTests/svg/custom/pattern-3-step-cycle-expected.txt b/LayoutTests/svg/custom/pattern-3-step-cycle-expected.txt
new file mode 100644 (file)
index 0000000..ea4527b
--- /dev/null
@@ -0,0 +1,3 @@
+PASS if no crash.
+
+
diff --git a/LayoutTests/svg/custom/pattern-3-step-cycle.html b/LayoutTests/svg/custom/pattern-3-step-cycle.html
new file mode 100644 (file)
index 0000000..b5bf320
--- /dev/null
@@ -0,0 +1,23 @@
+<!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>
index edb5941..122a60a 100644 (file)
@@ -1,5 +1,35 @@
 2015-09-17  Dean Jackson  <dino@apple.com>
 
+        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>
+
         Cyclic resources were not detected if the reference had deep containers
         https://bugs.webkit.org/show_bug.cgi?id=149182
 
index 33d1705..d59b894 100644 (file)
@@ -69,6 +69,7 @@ namespace WebCore {
     M(RemoteInspector) \
     M(ResourceLoading) \
     M(SQLDatabase) \
+    M(SVG) \
     M(Services) \
     M(SpellingAndGrammar) \
     M(StorageAPI) \
index 13b88d2..be76c1f 100644 (file)
@@ -20,9 +20,7 @@
 #include "config.h"
 #include "SVGResourcesCycleSolver.h"
 
-// Set to a value > 0, to debug the resource cache.
-#define DEBUG_CYCLE_DETECTION 0
-
+#include "Logging.h"
 #include "RenderAncestorIterator.h"
 #include "RenderSVGResourceClipper.h"
 #include "RenderSVGResourceFilter.h"
 #include "SVGResources.h"
 #include "SVGResourcesCache.h"
 
+// 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
+
 namespace WebCore {
 
 SVGResourcesCycleSolver::SVGResourcesCycleSolver(RenderElement& renderer, SVGResources& resources)
@@ -47,6 +54,8 @@ SVGResourcesCycleSolver::~SVGResourcesCycleSolver()
 
 bool SVGResourcesCycleSolver::resourceContainsCycles(RenderElement& renderer) const
 {
+    LOG_DEBUG_CYCLE("\n(%p) Check for cycles\n", &renderer);
+
     // First operate on the resources of the given renderer.
     // <marker id="a"> <path marker-start="url(#b)"/> ...
     // <marker id="b" marker-start="url(#a)"/>
@@ -54,34 +63,56 @@ bool SVGResourcesCycleSolver::resourceContainsCycles(RenderElement& renderer) co
         HashSet<RenderSVGResourceContainer*> resourceSet;
         resources->buildSetOfResources(resourceSet);
 
+        LOG_DEBUG_CYCLE("(%p) Examine our cached resources\n", &renderer);
+
         // Walk all resources and check whether they reference any resource contained in the resources set.
         for (auto* resource : resourceSet) {
+            LOG_DEBUG_CYCLE("(%p) Check %p\n", &renderer, resource);
             if (m_allResources.contains(resource))
                 return true;
+
+            // Now check if the resources themselves contain cycles.
+            if (resourceContainsCycles(*resource))
+                return true;
         }
     }
 
+    LOG_DEBUG_CYCLE("(%p) Now the children renderers\n", &renderer);
+
     // Then operate on the child resources of the given renderer.
     // <marker id="a"> <path marker-start="url(#b)"/> ...
     // <marker id="b"> <path marker-start="url(#a)"/> ...
     for (auto& child : childrenOfType<RenderElement>(renderer)) {
+
+        LOG_DEBUG_CYCLE("(%p) Checking child %p\n", &renderer, &child);
+
         if (auto* childResources = SVGResourcesCache::cachedResourcesForRenderer(child)) {
+
+            LOG_DEBUG_CYCLE("(%p) Child %p had cached resources. Check them.\n", &renderer, &child);
+
             // A child of the given 'resource' contains resources.
             HashSet<RenderSVGResourceContainer*> childResourceSet;
             childResources->buildSetOfResources(childResourceSet);
 
             // Walk all child resources and check whether they reference any resource contained in the resources set.
             for (auto* resource : childResourceSet) {
+                LOG_DEBUG_CYCLE("(%p) Child %p had resource %p\n", &renderer, &child, resource);
                 if (m_allResources.contains(resource))
                     return true;
             }
         }
 
+        LOG_DEBUG_CYCLE("(%p) Recurse into child %p\n", &renderer, &child);
+
         // Walk children recursively, stop immediately if we found a cycle
         if (resourceContainsCycles(child))
             return true;
+
+        LOG_DEBUG_CYCLE("\n(%p) Child %p was ok\n", &renderer, &child);
     }
 
+    LOG_DEBUG_CYCLE("\n(%p) No cycles found\n", &renderer);
+
     return false;
 }
 
@@ -89,8 +120,8 @@ void SVGResourcesCycleSolver::resolveCycles()
 {
     ASSERT(m_allResources.isEmpty());
 
-#if DEBUG_CYCLE_DETECTION > 0
-    fprintf(stderr, "\nBefore cycle detection:\n");
+#if DEBUG_CYCLE_DETECTION
+    LOG_DEBUG_CYCLE("\nBefore cycle detection:\n");
     m_resources.dump(&m_renderer);
 #endif
 
@@ -104,16 +135,16 @@ void SVGResourcesCycleSolver::resolveCycles()
     for (auto& resource : ancestorsOfType<RenderSVGResourceContainer>(m_renderer))
         ancestorResources.add(&resource);
 
-#if DEBUG_CYCLE_DETECTION > 0
-    fprintf(stderr, "\nDetecting whether any resources references any of following objects:\n");
+#if DEBUG_CYCLE_DETECTION
+    LOG_DEBUG_CYCLE("\nDetecting whether any resources references any of following objects:\n");
     {
-        fprintf(stderr, "Local resources:\n");
+        LOG_DEBUG_CYCLE("Local resources:\n");
         for (RenderObject* resource : localResources)
-            fprintf(stderr, "|> %s : %p (node %p)\n", resource->renderName(), resource, resource->node());
+            LOG_DEBUG_CYCLE("|> %s : %p (node %p)\n", resource->renderName(), resource, resource->node());
 
         fprintf(stderr, "Parent resources:\n");
         for (RenderObject* resource : ancestorResources)
-            fprintf(stderr, "|> %s : %p (node %p)\n", resource->renderName(), resource, resource->node());
+            LOG_DEBUG_CYCLE("|> %s : %p (node %p)\n", resource->renderName(), resource, resource->node());
     }
 #endif
 
@@ -128,15 +159,23 @@ void SVGResourcesCycleSolver::resolveCycles()
 
     ASSERT(!m_allResources.isEmpty());
 
+#if DEBUG_CYCLE_DETECTION
+    LOG_DEBUG_CYCLE("\nAll resources:\n");
+    for (auto* resource : m_allResources)
+        LOG_DEBUG_CYCLE("- %p\n", resource);
+#endif
+
     // The job of this function is to determine wheter any of the 'resources' associated with the given 'renderer'
     // references us (or whether any of its kids references us) -> that's a cycle, we need to find and break it.
     for (auto* resource : localResources) {
-        if (ancestorResources.contains(resource) || resourceContainsCycles(*resource))
+        if (ancestorResources.contains(resource) || resourceContainsCycles(*resource)) {
+            LOG_DEBUG_CYCLE("\n**** Detected a cycle (see the last test in the output above) ****\n");
             breakCycle(*resource);
+        }
     }
 
-#if DEBUG_CYCLE_DETECTION > 0
-    fprintf(stderr, "\nAfter cycle detection:\n");
+#if DEBUG_CYCLE_DETECTION
+    LOG_DEBUG_CYCLE("\nAfter cycle detection:\n");
     m_resources.dump(&m_renderer);
 #endif