From 795daf9fa26fc712b8d1692c6423eaadfb92a08d Mon Sep 17 00:00:00 2001 From: "dino@apple.com" Date: Fri, 18 Sep 2015 01:09:57 +0000 Subject: [PATCH] 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. git-svn-id: https://svn.webkit.org/repository/webkit/trunk@189954 268f45cc-cd09-0410-ab3c-d52691b4dbfc --- LayoutTests/ChangeLog | 13 +++++ .../svg/custom/pattern-3-step-cycle-expected.txt | 3 + LayoutTests/svg/custom/pattern-3-step-cycle.html | 23 ++++++++ Source/WebCore/ChangeLog | 30 ++++++++++ Source/WebCore/platform/Logging.h | 1 + .../rendering/svg/SVGResourcesCycleSolver.cpp | 65 +++++++++++++++++----- 6 files changed, 122 insertions(+), 13 deletions(-) create mode 100644 LayoutTests/svg/custom/pattern-3-step-cycle-expected.txt create mode 100644 LayoutTests/svg/custom/pattern-3-step-cycle.html diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog index 7322565..df3e7a8 100644 --- a/LayoutTests/ChangeLog +++ b/LayoutTests/ChangeLog @@ -1,5 +1,18 @@ 2015-09-17 Dean Jackson + 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 + 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 index 0000000..ea4527b --- /dev/null +++ b/LayoutTests/svg/custom/pattern-3-step-cycle-expected.txt @@ -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 index 0000000..b5bf320 --- /dev/null +++ b/LayoutTests/svg/custom/pattern-3-step-cycle.html @@ -0,0 +1,23 @@ + + +

PASS if no crash.

+ + + + + + + + + + + + diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog index edb5941..122a60a 100644 --- a/Source/WebCore/ChangeLog +++ b/Source/WebCore/ChangeLog @@ -1,5 +1,35 @@ 2015-09-17 Dean Jackson + 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 + Cyclic resources were not detected if the reference had deep containers https://bugs.webkit.org/show_bug.cgi?id=149182 diff --git a/Source/WebCore/platform/Logging.h b/Source/WebCore/platform/Logging.h index 33d1705..d59b894 100644 --- a/Source/WebCore/platform/Logging.h +++ b/Source/WebCore/platform/Logging.h @@ -69,6 +69,7 @@ namespace WebCore { M(RemoteInspector) \ M(ResourceLoading) \ M(SQLDatabase) \ + M(SVG) \ M(Services) \ M(SpellingAndGrammar) \ M(StorageAPI) \ diff --git a/Source/WebCore/rendering/svg/SVGResourcesCycleSolver.cpp b/Source/WebCore/rendering/svg/SVGResourcesCycleSolver.cpp index 13b88d2..be76c1f 100644 --- a/Source/WebCore/rendering/svg/SVGResourcesCycleSolver.cpp +++ b/Source/WebCore/rendering/svg/SVGResourcesCycleSolver.cpp @@ -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" @@ -33,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) @@ -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. // ... // @@ -54,34 +63,56 @@ bool SVGResourcesCycleSolver::resourceContainsCycles(RenderElement& renderer) co HashSet 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. // ... // ... for (auto& child : childrenOfType(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 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(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 -- 1.8.3.1