Repaint should only be called on attached renderers

alanbaradlay · alanbaradlay · commit 6f39fe014f9a · 2024-04-07T20:25:41.000-07:00
https://bugs.webkit.org/show_bug.cgi?id=272292 Reviewed by Antti Koivisto. 1. In some cases (e.g. pseudo content) a subtree root gets detached first followed by the descendant renderers 2. Repaint functions start with traversing all the way to the RenderView just to check if dispatching repaint is safe -which in most cases comes back true (normal at-layout repaint, invalidation, standard tree teardown) This patch fixes a correctness and a performance issue: correctness: In case of #1, when calling willBeRemovedFromTree() (right before deleting the descendant renderer), the renderer is already detached from the render tree. It is still attached to the root of the subtree (which is already detached), but has no access to the rest of the render tree (i.e. willBeRemovedFromTree is technically just willBeRemovedFromParent) performance: ensuring correctness eliminates the need for checking if the renderer is still attached. (Sadly ::imageChanged gets called indirectly through initializeStyle before the initial attach) * Source/WebCore/rendering/RenderObject.cpp: (WebCore::RenderObject::repaint const): Use isDescendantOf instead of isRooted() as I am planning on completely remove isRooted(). (WebCore::RenderObject::repaintRectangle const): (WebCore::RenderObject::repaintSlowRepaintObject const): * Source/WebCore/rendering/updating/RenderTreeBuilder.cpp: (WebCore::RenderTreeBuilder::destroy): (WebCore::RenderTreeBuilder::detachFromRenderElement): * Source/WebCore/rendering/updating/RenderTreeBuilder.h: Canonical link: https://commits.webkit.org/277186@main
diff --git a/LayoutTests/fast/repaint/full-repaint-on-content-change-expected.txt b/LayoutTests/fast/repaint/full-repaint-on-content-change-expected.txt
@@ -1,5 +1,6 @@
 some text
 (repaint rects
+  (rect 144 0 64 116)
   (rect 144 0 64 116)
   (rect 0 0 64 116)
   (rect 144 0 64 116)
diff --git a/Source/WebCore/rendering/RenderBox.cpp b/Source/WebCore/rendering/RenderBox.cpp
@@ -2030,12 +2030,10 @@ static StyleImage* findLayerUsedImage(WrappedImagePtr image, const FillLayer& la
 
 void RenderBox::imageChanged(WrappedImagePtr image, const IntRect*)
 {
-    if (!parent())
-        return;
-
     if ((style().borderImage().image() && style().borderImage().image()->data() == image) ||
         (style().maskBorder().image() && style().maskBorder().image()->data() == image)) {
-        repaint();
+        if (parent())
+            repaint();
         return;
     }
 
@@ -2048,6 +2046,9 @@ void RenderBox::imageChanged(WrappedImagePtr image, const IntRect*)
     bool didFullRepaint = false;
 
     auto repaintForBackgroundAndMask = [&](auto& style) {
+        if (!parent())
+            return;
+
         if (!didFullRepaint)
             didFullRepaint = repaintLayerRectsForImage(image, style.backgroundLayers(), true);
         if (!didFullRepaint)
diff --git a/Source/WebCore/rendering/RenderImage.cpp b/Source/WebCore/rendering/RenderImage.cpp
@@ -398,14 +398,15 @@ void RenderImage::repaintOrMarkForLayout(ImageSizeChangeType imageSizeChange, co
         updateInnerContentRect();
     }
 
-    LayoutRect repaintRect = contentBoxRect();
-    if (rect) {
-        // The image changed rect is in source image coordinates (pre-zooming),
-        // so map from the bounds of the image to the contentsBox.
-        repaintRect.intersect(enclosingIntRect(mapRect(*rect, FloatRect(FloatPoint(), imageResource().imageSize(1.0f)), repaintRect)));
+    if (parent()) {
+        auto repaintRect = contentBoxRect();
+        if (rect) {
+            // The image changed rect is in source image coordinates (pre-zooming),
+            // so map from the bounds of the image to the contentsBox.
+            repaintRect.intersect(enclosingIntRect(mapRect(*rect, FloatRect(FloatPoint(), imageResource().imageSize(1.0f)), repaintRect)));
+        }
+        repaintRectangle(repaintRect);
     }
-        
-    repaintRectangle(repaintRect);
 
     // Tell any potential compositing layers that the image needs updating.
     contentChanged(ImageChanged);
diff --git a/Source/WebCore/rendering/RenderListMarker.cpp b/Source/WebCore/rendering/RenderListMarker.cpp
@@ -272,11 +272,13 @@ void RenderListMarker::layout()
 
 void RenderListMarker::imageChanged(WrappedImagePtr o, const IntRect* rect)
 {
-    if (m_image && o == m_image->data()) {
-        if (width() != m_image->imageSize(this, style().usedZoom()).width() || height() != m_image->imageSize(this, style().usedZoom()).height() || m_image->errorOccurred())
-            setNeedsLayoutAndPrefWidthsRecalc();
-        else
-            repaint();
+    if (parent()) {
+        if (m_image && o == m_image->data()) {
+            if (width() != m_image->imageSize(this, style().usedZoom()).width() || height() != m_image->imageSize(this, style().usedZoom()).height() || m_image->errorOccurred())
+                setNeedsLayoutAndPrefWidthsRecalc();
+            else
+                repaint();
+        }
     }
     RenderBox::imageChanged(o, rect);
 }
diff --git a/Source/WebCore/rendering/RenderObject.cpp b/Source/WebCore/rendering/RenderObject.cpp
@@ -1039,21 +1039,24 @@ void RenderObject::issueRepaint(std::optional<LayoutRect> partialRepaintRect, Cl
 
 void RenderObject::repaint(ForceRepaint forceRepaint) const
 {
-    // Don't repaint if we're unrooted (note that view() still returns the view when unrooted)
-    if (!isRooted() || view().printing())
+    ASSERT(isDescendantOf(&view()) || is<RenderScrollbarPart>(this) || is<RenderReplica>(this));
+
+    if (view().printing())
         return;
     issueRepaint({ }, ClipRepaintToLayer::No, forceRepaint);
 }
 
 void RenderObject::repaintRectangle(const LayoutRect& repaintRect, bool shouldClipToLayer) const
 {
+    ASSERT(isDescendantOf(&view()) || is<RenderScrollbarPart>(this));
     return repaintRectangle(repaintRect, shouldClipToLayer ? ClipRepaintToLayer::Yes : ClipRepaintToLayer::No, ForceRepaint::No);
 }
 
 void RenderObject::repaintRectangle(const LayoutRect& repaintRect, ClipRepaintToLayer shouldClipToLayer, ForceRepaint forceRepaint, std::optional<LayoutBoxExtent> additionalRepaintOutsets) const
 {
-    // Don't repaint if we're unrooted (note that view() still returns the view when unrooted)
-    if (!isRooted() || view().printing())
+    ASSERT(isDescendantOf(&view()) || is<RenderScrollbarPart>(this) || is<RenderReplica>(this));
+
+    if (view().printing())
         return;
     // FIXME: layoutDelta needs to be applied in parts before/after transforms and
     // repaint containers. https://bugs.webkit.org/show_bug.cgi?id=23308
@@ -1064,9 +1067,7 @@ void RenderObject::repaintRectangle(const LayoutRect& repaintRect, ClipRepaintTo
 
 void RenderObject::repaintSlowRepaintObject() const
 {
-    // Don't repaint if we're unrooted (note that view() still returns the view when unrooted)
-    if (!isRooted())
-        return;
+    ASSERT(isDescendantOf(&view()) || is<RenderScrollbarPart>(this) || is<RenderReplica>(this));
 
     CheckedRef view = this->view();
     if (view->printing())
diff --git a/Source/WebCore/rendering/RenderTableCol.cpp b/Source/WebCore/rendering/RenderTableCol.cpp
@@ -155,6 +155,8 @@ auto RenderTableCol::rectsForRepaintingAfterLayout(const RenderLayerModelObject*
 void RenderTableCol::imageChanged(WrappedImagePtr, const IntRect*)
 {
     // FIXME: Repaint only the rect the image paints in.
+    if (!parent())
+        return;
     repaint();
 }
 
diff --git a/Source/WebCore/rendering/RenderTableRow.cpp b/Source/WebCore/rendering/RenderTableRow.cpp
@@ -242,6 +242,8 @@ void RenderTableRow::paint(PaintInfo& paintInfo, const LayoutPoint& paintOffset)
 void RenderTableRow::imageChanged(WrappedImagePtr, const IntRect*)
 {
     // FIXME: Examine cells and repaint only the rect the image paints in.
+    if (!parent())
+        return;
     repaint();
 }
 
diff --git a/Source/WebCore/rendering/RenderTableSection.cpp b/Source/WebCore/rendering/RenderTableSection.cpp
@@ -1336,6 +1336,8 @@ void RenderTableSection::paintObject(PaintInfo& paintInfo, const LayoutPoint& pa
 void RenderTableSection::imageChanged(WrappedImagePtr, const IntRect*)
 {
     // FIXME: Examine cells and repaint only the rect the image paints in.
+    if (!parent())
+        return;
     repaint();
 }
 
diff --git a/Source/WebCore/rendering/svg/RenderSVGImage.cpp b/Source/WebCore/rendering/svg/RenderSVGImage.cpp
@@ -345,7 +345,7 @@ void RenderSVGImage::notifyFinished(CachedResource& newImage, const NetworkLoadM
 
 void RenderSVGImage::imageChanged(WrappedImagePtr newImage, const IntRect* rect)
 {
-    if (renderTreeBeingDestroyed())
+    if (renderTreeBeingDestroyed() || !parent())
         return;
 
     repaintClientsOfReferencedSVGResources();
diff --git a/Source/WebCore/rendering/svg/legacy/LegacyRenderSVGImage.cpp b/Source/WebCore/rendering/svg/legacy/LegacyRenderSVGImage.cpp
@@ -254,6 +254,8 @@ bool LegacyRenderSVGImage::nodeAtFloatPoint(const HitTestRequest& request, HitTe
 
 void LegacyRenderSVGImage::imageChanged(WrappedImagePtr, const IntRect*)
 {
+    if (!parent())
+        return;
     // The image resource defaults to nullImage until the resource arrives.
     // This empty image may be cached by SVG resources which must be invalidated.
     if (auto* resources = SVGResourcesCache::cachedResourcesForRenderer(*this))
diff --git a/Source/WebCore/rendering/updating/RenderTreeBuilder.cpp b/Source/WebCore/rendering/updating/RenderTreeBuilder.cpp
@@ -160,6 +160,19 @@ void RenderTreeBuilder::destroy(RenderObject& renderer, CanCollapseAnonymousBloc
 {
     RELEASE_ASSERT(RenderTreeMutationDisallowedScope::isMutationAllowed());
     ASSERT(renderer.parent());
+
+    auto notifyDescendantRenderersBeforeSubtreeTearDownIfApplicable = [&] {
+        if (renderer.renderTreeBeingDestroyed())
+            return;
+        auto* rendererToDelete = dynamicDowncast<RenderElement>(renderer);
+        if (!rendererToDelete || !rendererToDelete->firstChild())
+            return;
+
+        for (auto& descendant : descendantsOfType<RenderObject>(*rendererToDelete))
+            descendant.willBeRemovedFromTree();
+    };
+    notifyDescendantRenderersBeforeSubtreeTearDownIfApplicable();
+
     auto toDestroy = detach(*renderer.parent(), renderer, WillBeDestroyed::Yes, canCollapseAnonymousBlock);
 
     if (auto* textFragment = dynamicDowncast<RenderTextFragment>(renderer))
@@ -168,19 +181,21 @@ void RenderTreeBuilder::destroy(RenderObject& renderer, CanCollapseAnonymousBloc
     if (auto* renderBox = dynamicDowncast<RenderBoxModelObject>(renderer))
         continuationBuilder().cleanupOnDestroy(*renderBox);
 
-    // We need to detach the subtree first so that the descendants don't have
-    // access to previous/next sublings at detach().
-    // FIXME: webkit.org/b/182909.
-    auto* childToDestroy = dynamicDowncast<RenderElement>(toDestroy.get());
-    if (!childToDestroy)
-        return;
+    auto tearDownSubTreeIfApplicable = [&] {
+        auto* rendererToDelete = dynamicDowncast<RenderElement>(toDestroy.get());
+        if (!rendererToDelete)
+            return;
 
-    while (childToDestroy->firstChild()) {
-        auto& firstChild = *childToDestroy->firstChild();
-        if (auto* node = firstChild.node())
-            node->setRenderer(nullptr);
-        destroy(firstChild);
-    }
+        auto isSubtreeTeardown = SetForScope { m_isSubtreeTeardown, IsSubtreeTeardown::Yes };
+        while (rendererToDelete->firstChild()) {
+            auto& firstChild = *rendererToDelete->firstChild();
+            if (auto* node = firstChild.node())
+                node->setRenderer(nullptr);
+            destroy(firstChild);
+        }
+    };
+    // FIXME: webkit.org/b/182909.
+    tearDownSubTreeIfApplicable();
 }
 
 void RenderTreeBuilder::attach(RenderElement& parent, RenderPtr<RenderObject> child, RenderObject* beforeChild)
@@ -953,8 +968,9 @@ RenderPtr<RenderObject> RenderTreeBuilder::detachFromRenderElement(RenderElement
     RELEASE_ASSERT_WITH_MESSAGE(!parent.view().frameView().layoutContext().layoutState(), "Layout must not mutate render tree");
     ASSERT(parent.canHaveChildren() || parent.canHaveGeneratedChildren());
     ASSERT(child.parent() == &parent);
+    ASSERT(m_isSubtreeTeardown == IsSubtreeTeardown::No || willBeDestroyed == WillBeDestroyed::Yes);
 
-    if (parent.renderTreeBeingDestroyed())
+    if (parent.renderTreeBeingDestroyed() || m_isSubtreeTeardown == IsSubtreeTeardown::Yes)
         return parent.detachRendererInternal(child);
 
     if (child.everHadLayout())
diff --git a/Source/WebCore/rendering/updating/RenderTreeBuilder.h b/Source/WebCore/rendering/updating/RenderTreeBuilder.h
@@ -51,6 +51,7 @@ class RenderTreeBuilder {
     enum class CanCollapseAnonymousBlock : bool { No, Yes };
     RenderPtr<RenderObject> detach(RenderElement&, RenderObject&, WillBeDestroyed, CanCollapseAnonymousBlock = CanCollapseAnonymousBlock::Yes) WARN_UNUSED_RETURN;
 
+    enum class IsSubtreeTeardown : bool { No, Yes };
     void destroy(RenderObject& renderer, CanCollapseAnonymousBlock = CanCollapseAnonymousBlock::Yes);
 
     // NormalizeAfterInsertion::Yes ensures that the destination subtree is consistent after the insertion (anonymous wrappers etc).
@@ -148,6 +149,7 @@ class RenderTreeBuilder {
     std::unique_ptr<Continuation> m_continuationBuilder;
     bool m_hasBrokenContinuation { false };
     IsInternalMove m_internalMovesType { IsInternalMove::No };
+    IsSubtreeTeardown m_isSubtreeTeardown { IsSubtreeTeardown::No };
 };
 
 }

Original file line number	Diff line number	Diff line change
`@@ -272,11 +272,13 @@ void RenderListMarker::layout()`
`272`	`272`
`273`	`273`	`void RenderListMarker::imageChanged(WrappedImagePtr o, const IntRect* rect)`
`274`	`274`	`{`
`275`		`- if (m_image && o == m_image->data()) {`
`276`		`- if (width() != m_image->imageSize(this, style().usedZoom()).width() \|\| height() != m_image->imageSize(this, style().usedZoom()).height() \|\| m_image->errorOccurred())`
`277`		`- setNeedsLayoutAndPrefWidthsRecalc();`
`278`		`- else`
`279`		`- repaint();`
	`275`	`+ if (parent()) {`
	`276`	`+ if (m_image && o == m_image->data()) {`
	`277`	`+ if (width() != m_image->imageSize(this, style().usedZoom()).width() \|\| height() != m_image->imageSize(this, style().usedZoom()).height() \|\| m_image->errorOccurred())`
	`278`	`+ setNeedsLayoutAndPrefWidthsRecalc();`
	`279`	`+ else`
	`280`	`+ repaint();`
	`281`	`+ }`
`280`	`282`	`}`
`281`	`283`	`RenderBox::imageChanged(o, rect);`
`282`	`284`	`}`
Original file line number	Diff line number	Diff line change
`@@ -1039,21 +1039,24 @@ void RenderObject::issueRepaint(std::optional<LayoutRect> partialRepaintRect, Cl`
`1039`	`1039`
`1040`	`1040`	`void RenderObject::repaint(ForceRepaint forceRepaint) const`
`1041`	`1041`	`{`
`1042`		`- // Don't repaint if we're unrooted (note that view() still returns the view when unrooted)`
`1043`		`- if (!isRooted() \|\| view().printing())`
	`1042`	`+ ASSERT(isDescendantOf(&view()) \|\| is<RenderScrollbarPart>(this) \|\| is<RenderReplica>(this));`
	`1043`	`+`
	`1044`	`+ if (view().printing())`
`1044`	`1045`	`return;`
`1045`	`1046`	`issueRepaint({ }, ClipRepaintToLayer::No, forceRepaint);`
`1046`	`1047`	`}`
`1047`	`1048`
`1048`	`1049`	`void RenderObject::repaintRectangle(const LayoutRect& repaintRect, bool shouldClipToLayer) const`
`1049`	`1050`	`{`
	`1051`	`+ ASSERT(isDescendantOf(&view()) \|\| is<RenderScrollbarPart>(this));`
`1050`	`1052`	`return repaintRectangle(repaintRect, shouldClipToLayer ? ClipRepaintToLayer::Yes : ClipRepaintToLayer::No, ForceRepaint::No);`
`1051`	`1053`	`}`
`1052`	`1054`
`1053`	`1055`	`void RenderObject::repaintRectangle(const LayoutRect& repaintRect, ClipRepaintToLayer shouldClipToLayer, ForceRepaint forceRepaint, std::optional<LayoutBoxExtent> additionalRepaintOutsets) const`
`1054`	`1056`	`{`
`1055`		`- // Don't repaint if we're unrooted (note that view() still returns the view when unrooted)`
`1056`		`- if (!isRooted() \|\| view().printing())`
	`1057`	`+ ASSERT(isDescendantOf(&view()) \|\| is<RenderScrollbarPart>(this) \|\| is<RenderReplica>(this));`
	`1058`	`+`
	`1059`	`+ if (view().printing())`
`1057`	`1060`	`return;`
`1058`	`1061`	`// FIXME: layoutDelta needs to be applied in parts before/after transforms and`
`1059`	`1062`	`// repaint containers. https://bugs.webkit.org/show_bug.cgi?id=23308`
`@@ -1064,9 +1067,7 @@ void RenderObject::repaintRectangle(const LayoutRect& repaintRect, ClipRepaintTo`
`1064`	`1067`
`1065`	`1068`	`void RenderObject::repaintSlowRepaintObject() const`
`1066`	`1069`	`{`
`1067`		`- // Don't repaint if we're unrooted (note that view() still returns the view when unrooted)`
`1068`		`- if (!isRooted())`
`1069`		`- return;`
	`1070`	`+ ASSERT(isDescendantOf(&view()) \|\| is<RenderScrollbarPart>(this) \|\| is<RenderReplica>(this));`
`1070`	`1071`
`1071`	`1072`	`CheckedRef view = this->view();`
`1072`	`1073`	`if (view->printing())`
Original file line number	Diff line number	Diff line change
`@@ -155,6 +155,8 @@ auto RenderTableCol::rectsForRepaintingAfterLayout(const RenderLayerModelObject*`
`155`	`155`	`void RenderTableCol::imageChanged(WrappedImagePtr, const IntRect*)`
`156`	`156`	`{`
`157`	`157`	`// FIXME: Repaint only the rect the image paints in.`
	`158`	`+ if (!parent())`
	`159`	`+ return;`
`158`	`160`	`repaint();`
`159`	`161`	`}`
`160`	`162`
Original file line number	Diff line number	Diff line change
`@@ -242,6 +242,8 @@ void RenderTableRow::paint(PaintInfo& paintInfo, const LayoutPoint& paintOffset)`
`242`	`242`	`void RenderTableRow::imageChanged(WrappedImagePtr, const IntRect*)`
`243`	`243`	`{`
`244`	`244`	`// FIXME: Examine cells and repaint only the rect the image paints in.`
	`245`	`+ if (!parent())`
	`246`	`+ return;`
`245`	`247`	`repaint();`
`246`	`248`	`}`
`247`	`249`
Original file line number	Diff line number	Diff line change
`@@ -1336,6 +1336,8 @@ void RenderTableSection::paintObject(PaintInfo& paintInfo, const LayoutPoint& pa`
`1336`	`1336`	`void RenderTableSection::imageChanged(WrappedImagePtr, const IntRect*)`
`1337`	`1337`	`{`
`1338`	`1338`	`// FIXME: Examine cells and repaint only the rect the image paints in.`
	`1339`	`+ if (!parent())`
	`1340`	`+ return;`
`1339`	`1341`	`repaint();`
`1340`	`1342`	`}`
`1341`	`1343`
Original file line number	Diff line number	Diff line change
`@@ -345,7 +345,7 @@ void RenderSVGImage::notifyFinished(CachedResource& newImage, const NetworkLoadM`
`345`	`345`
`346`	`346`	`void RenderSVGImage::imageChanged(WrappedImagePtr newImage, const IntRect* rect)`
`347`	`347`	`{`
`348`		`- if (renderTreeBeingDestroyed())`
	`348`	`+ if (renderTreeBeingDestroyed() \|\| !parent())`
`349`	`349`	`return;`
`350`	`350`
`351`	`351`	`repaintClientsOfReferencedSVGResources();`
Original file line number	Diff line number	Diff line change
`@@ -254,6 +254,8 @@ bool LegacyRenderSVGImage::nodeAtFloatPoint(const HitTestRequest& request, HitTe`
`254`	`254`
`255`	`255`	`void LegacyRenderSVGImage::imageChanged(WrappedImagePtr, const IntRect*)`
`256`	`256`	`{`
	`257`	`+ if (!parent())`
	`258`	`+ return;`
`257`	`259`	`// The image resource defaults to nullImage until the resource arrives.`
`258`	`260`	`// This empty image may be cached by SVG resources which must be invalidated.`
`259`	`261`	`if (auto* resources = SVGResourcesCache::cachedResourcesForRenderer(*this))`