1 files changed, 79 insertions, 53 deletions

diff --git a/Source/WebCore/rendering/svg/SVGResourcesCycleSolver.cpp b/Source/WebCore/rendering/svg/SVGResourcesCycleSolver.cpp
index c6776f667..be76c1f9d 100644
--- a/Source/WebCore/rendering/svg/SVGResourcesCycleSolver.cpp
+++ b/Source/WebCore/rendering/svg/SVGResourcesCycleSolver.cpp
@@ -20,12 +20,8 @@
 #include "config.h"
 #include "SVGResourcesCycleSolver.h"
 
-// Set to a value > 0, to debug the resource cache.
-#define DEBUG_CYCLE_DETECTION 0
-
-#if ENABLE(SVG)
-#include "RenderElement.h"
-#include "RenderIterator.h"
+#include "Logging.h"
+#include "RenderAncestorIterator.h"
 #include "RenderSVGResourceClipper.h"
 #include "RenderSVGResourceFilter.h"
 #include "RenderSVGResourceMarker.h"
@@ -35,6 +31,15 @@
 #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)
@@ -49,43 +54,65 @@ 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)"/>
-    if (SVGResources* resources = SVGResourcesCache::cachedResourcesForRenderObject(renderer)) {
+    if (auto* resources = SVGResourcesCache::cachedResourcesForRenderer(renderer)) {
         HashSet<RenderSVGResourceContainer*> resourceSet;
         resources->buildSetOfResources(resourceSet);
 
-        // Walk all resources and check wheter they reference any resource contained in the resources set.
-        for (auto resource : 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)) {
-        SVGResources* childResources = SVGResourcesCache::cachedResourcesForRenderObject(child);
-        if (!childResources)
-            continue;
-        
-        // A child of the given 'resource' contains resources. 
-        HashSet<RenderSVGResourceContainer*> childResourceSet;
-        childResources->buildSetOfResources(childResourceSet);
-
-        // Walk all child resources and check wheter they reference any resource contained in the resources set.
-        for (auto& resource : childResourceSet) {
-            if (m_allResources.contains(resource))
-                return true;
+
+        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;
 }
 
@@ -93,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,49 +131,52 @@ void SVGResourcesCycleSolver::resolveCycles()
     ASSERT(!localResources.isEmpty());
 
     // Add all parent resource containers to the HashSet.
-    HashSet<RenderSVGResourceContainer*> parentResources;
-    auto parent = m_renderer.parent();
-    while (parent) {
-        if (parent->isSVGResourceContainer())
-            parentResources.add(parent->toRenderSVGResourceContainer());
-        parent = parent->parent();
-    }
+    HashSet<RenderSVGResourceContainer*> ancestorResources;
+    for (auto& resource : ancestorsOfType<RenderSVGResourceContainer>(m_renderer))
+        ancestorResources.add(&resource);
 
-#if DEBUG_CYCLE_DETECTION > 0
-    fprintf(stderr, "\nDetecting wheter 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");
-        for (auto it = localResources.begin(), end = localResources.end(); it != end; ++it)
-            fprintf(stderr, "|> %s: object=%p (node=%p)\n", (*it)->renderName(), *it, (*it)->node());
+        LOG_DEBUG_CYCLE("Local resources:\n");
+        for (RenderObject* resource : localResources)
+            LOG_DEBUG_CYCLE("|> %s : %p (node %p)\n", resource->renderName(), resource, resource->node());
 
         fprintf(stderr, "Parent resources:\n");
-        for (auto it = parentResources.begin(), end = parentResources.end(); it != end; ++it)
-            fprintf(stderr, "|> %s: object=%p (node=%p)\n", (*it)->renderName(), *it, (*it)->node());
+        for (RenderObject* resource : ancestorResources)
+            LOG_DEBUG_CYCLE("|> %s : %p (node %p)\n", resource->renderName(), resource, resource->node());
     }
 #endif
 
     // Build combined set of local and parent resources.
     m_allResources = localResources;
-    for (auto it = parentResources.begin(), end = parentResources.end(); it != end; ++it)
-        m_allResources.add(*it);
+    for (auto* resource : ancestorResources)
+        m_allResources.add(resource);
 
     // If we're a resource, add ourselves to the HashSet.
-    if (m_renderer.isSVGResourceContainer())
-        m_allResources.add(m_renderer.toRenderSVGResourceContainer());
+    if (is<RenderSVGResourceContainer>(m_renderer))
+        m_allResources.add(&downcast<RenderSVGResourceContainer>(m_renderer));
 
     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 wheter any of its kids references us) -> that's a cycle, we need to find and break it.
-    for (auto it = localResources.begin(), end = localResources.end(); it != end; ++it) {
-        RenderSVGResourceContainer& resource = **it;
-        if (parentResources.contains(&resource) || resourceContainsCycles(resource))
-            breakCycle(resource);
+    // 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)) {
+            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");
-    m_resources.dump(m_renderer);
+#if DEBUG_CYCLE_DETECTION
+    LOG_DEBUG_CYCLE("\nAfter cycle detection:\n");
+    m_resources.dump(&m_renderer);
 #endif
 
     m_allResources.clear();
@@ -183,10 +213,8 @@ void SVGResourcesCycleSolver::breakCycle(RenderSVGResourceContainer& resourceLea
             m_resources.resetStroke();
         break;
     case FilterResourceType:
-#if ENABLE(FILTERS)
         ASSERT(&resourceLeadingToCycle == m_resources.filter());
         m_resources.resetFilter();
-#endif
         break;
     case ClipperResourceType:
         ASSERT(&resourceLeadingToCycle == m_resources.clipper());
@@ -199,5 +227,3 @@ void SVGResourcesCycleSolver::breakCycle(RenderSVGResourceContainer& resourceLea
 }
 
 }
-
-#endif