SVG filter triggers unstable layout.
authorzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 11 Mar 2020 20:22:25 +0000 (20:22 +0000)
committerzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 11 Mar 2020 20:22:25 +0000 (20:22 +0000)
https://bugs.webkit.org/show_bug.cgi?id=207444
rdar://problem/59297004

Reviewed by Simon Fraser.

SVG filter code marks DOM nodes dirty and schedules style recalc outside of the SVG root
while in layout. This could lead to unstable layout and cause battery drain.
(See webkit.org/b/208903)

* rendering/RenderLayer.cpp: Remove filterNeedsRepaint(). It's a dangerously misleading name and should
not be part of RenderLayer.
(WebCore::RenderLayer::calculateClipRects const):
* rendering/RenderLayer.h:
* rendering/RenderLayerFilters.cpp:
(WebCore::RenderLayerFilters::notifyFinished):
* rendering/svg/RenderSVGResourceContainer.cpp:
(WebCore::RenderSVGResourceContainer::markAllClientsForInvalidation):
(WebCore::RenderSVGResourceContainer::markAllClientLayersForInvalidation):

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

Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderLayer.cpp
Source/WebCore/rendering/RenderLayer.h
Source/WebCore/rendering/RenderLayerFilters.cpp
Source/WebCore/rendering/svg/RenderSVGResourceContainer.cpp

index 49a758b..4175ad3 100644 (file)
@@ -1,3 +1,25 @@
+2020-03-11  Zalan Bujtas  <zalan@apple.com>
+
+        SVG filter triggers unstable layout.
+        https://bugs.webkit.org/show_bug.cgi?id=207444
+        rdar://problem/59297004
+
+        Reviewed by Simon Fraser.
+
+        SVG filter code marks DOM nodes dirty and schedules style recalc outside of the SVG root
+        while in layout. This could lead to unstable layout and cause battery drain.
+        (See webkit.org/b/208903)
+
+        * rendering/RenderLayer.cpp: Remove filterNeedsRepaint(). It's a dangerously misleading name and should
+        not be part of RenderLayer.
+        (WebCore::RenderLayer::calculateClipRects const):
+        * rendering/RenderLayer.h:
+        * rendering/RenderLayerFilters.cpp:
+        (WebCore::RenderLayerFilters::notifyFinished):
+        * rendering/svg/RenderSVGResourceContainer.cpp:
+        (WebCore::RenderSVGResourceContainer::markAllClientsForInvalidation):
+        (WebCore::RenderSVGResourceContainer::markAllClientLayersForInvalidation):
+
 2020-03-11  Antoine Quint  <graouts@webkit.org>
 
         [Mac wk2 Release] imported/w3c/web-platform-tests/web-animations/timing-model/animations/updating-the-finished-state.html flaky fail
index 2d72c43..688289f 100644 (file)
@@ -6965,16 +6965,6 @@ void RenderLayer::updateFilterPaintingStrategy()
     m_filters->buildFilter(renderer(), page().deviceScaleFactor(), renderer().settings().acceleratedFiltersEnabled() ? RenderingMode::Accelerated : RenderingMode::Unaccelerated);
 }
 
-void RenderLayer::filterNeedsRepaint()
-{
-    // We use the enclosing element so that we recalculate style for the ancestor of an anonymous object.
-    if (Element* element = enclosingElement()) {
-        // FIXME: This really shouldn't have to invalidate layer composition, but tests like css3/filters/effect-reference-delete.html fail if that doesn't happen.
-        element->invalidateStyleAndLayerComposition();
-    }
-    renderer().repaint();
-}
-
 IntOutsets RenderLayer::filterOutsets() const
 {
     if (m_filters)
index 5ff8d37..1a5d0f0 100644 (file)
@@ -803,7 +803,6 @@ public:
     bool has3DTransform() const { return m_transform && !m_transform->isAffine(); }
     bool hasTransformedAncestor() const { return m_hasTransformedAncestor; }
 
-    void filterNeedsRepaint();
     bool hasFilter() const { return renderer().hasFilter(); }
     bool hasFilterOutsets() const { return !filterOutsets().isZero(); }
     IntOutsets filterOutsets() const;
index b680400..c081557 100644 (file)
@@ -67,7 +67,11 @@ bool RenderLayerFilters::hasFilterThatShouldBeRestrictedBySecurityOrigin() const
 
 void RenderLayerFilters::notifyFinished(CachedResource&)
 {
-    m_layer.filterNeedsRepaint();
+    // FIXME: This really shouldn't have to invalidate layer composition,
+    // but tests like css3/filters/effect-reference-delete.html fail if that doesn't happen.
+    if (auto* enclosingElement = m_layer.enclosingElement())
+        enclosingElement->invalidateStyleAndLayerComposition();
+    m_layer.renderer().repaint();
 }
 
 void RenderLayerFilters::updateReferenceFilterClients(const FilterOperations& operations)
index cddc375..39fa73f 100644 (file)
@@ -26,6 +26,7 @@
 #include "SVGRenderingContext.h"
 #include "SVGResourcesCache.h"
 #include <wtf/IsoMallocInlines.h>
+#include <wtf/SetForScope.h>
 #include <wtf/StackStats.h>
 
 namespace WebCore {
@@ -91,10 +92,13 @@ void RenderSVGResourceContainer::idChanged()
 
 void RenderSVGResourceContainer::markAllClientsForInvalidation(InvalidationMode mode)
 {
+    // FIXME: Style invalidation should either be a pre-layout task or this function
+    // should never get called while in layout. See webkit.org/b/208903.
     if ((m_clients.isEmpty() && m_clientLayers.isEmpty()) || m_isInvalidating)
         return;
 
-    m_isInvalidating = true;
+    SetForScope<bool> isInvalidating(m_isInvalidating, true);
+
     bool needsLayout = mode == LayoutAndBoundariesInvalidation;
     bool markForInvalidation = mode != ParentOnlyInvalidation;
     auto* root = SVGRenderSupport::findTreeRootObject(*this);
@@ -116,18 +120,29 @@ void RenderSVGResourceContainer::markAllClientsForInvalidation(InvalidationMode
     }
 
     markAllClientLayersForInvalidation();
-
-    m_isInvalidating = false;
 }
 
 void RenderSVGResourceContainer::markAllClientLayersForInvalidation()
 {
     if (m_clientLayers.isEmpty())
         return;
-    if ((*m_clientLayers.begin())->renderer().renderTreeBeingDestroyed())
+
+    auto& document = (*m_clientLayers.begin())->renderer().document();
+    if (!document.view() || document.renderTreeBeingDestroyed())
         return;
-    for (auto* clientLayer : m_clientLayers)
-        clientLayer->filterNeedsRepaint();
+
+    auto inLayout = document.view()->layoutContext().isInLayout();
+    for (auto* clientLayer : m_clientLayers) {
+        // FIXME: We should not get here while in layout. See webkit.org/b/208903.
+        // Repaint should also be triggered through some other means.
+        if (inLayout) {
+            clientLayer->renderer().repaint();
+            continue;
+        }
+        if (auto* enclosingElement = clientLayer->enclosingElement())
+            enclosingElement->invalidateStyleAndLayerComposition();
+        clientLayer->renderer().repaint();
+    }
 }
 
 void RenderSVGResourceContainer::markClientForInvalidation(RenderObject& client, InvalidationMode mode)