From 3b63582068f24ac2bc4be27b437f0ddf8c0d7d87 Mon Sep 17 00:00:00 2001
From: Andreas Kling <andreas@ladybird.org>
Date: Wed, 30 Jul 2025 16:57:03 +0200
Subject: [PATCH] LibWeb: More thoroughly detach layout/paint trees from each
 other

Before committing a new layout (and thus building a new paint tree)
we now go through both the old paint tree and the layout tree and detach
them from each other.

This is a little extra work, but it ensures that there are no lingering
references across the trees, which we were apparently accumulating in
some cases on Discord, causing GC leaks.
---
 Libraries/LibWeb/Layout/LayoutState.cpp | 15 ++++++++++++---
 Libraries/LibWeb/Painting/Paintable.cpp |  6 ++++++
 Libraries/LibWeb/Painting/Paintable.h   |  2 ++
 3 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/Libraries/LibWeb/Layout/LayoutState.cpp b/Libraries/LibWeb/Layout/LayoutState.cpp
index 5787f616711..8b035b885df 100644
--- a/Libraries/LibWeb/Layout/LayoutState.cpp
+++ b/Libraries/LibWeb/Layout/LayoutState.cpp
@@ -214,14 +214,23 @@ static void build_paint_tree(Node& node, Painting::Paintable* parent_paintable =
 
 void LayoutState::commit(Box& root)
 {
-    // NOTE: In case this is a relayout of an existing tree, we start by detaching the old paint tree
-    //       from the layout tree. This is done to ensure that we don't end up with any old-tree pointers
-    //       when text paintables shift around in the tree.
+    // Go through the old paintable tree and detach everything from the layout tree.
+    // The layout tree should only point to the new paintable tree which we're about to build.
+    if (auto* old_paintable_root = root.first_paintable()) {
+        old_paintable_root->for_each_in_inclusive_subtree([&](Painting::Paintable& paintable) {
+            paintable.detach_from_layout_node();
+            return TraversalDecision::Continue;
+        });
+    }
+
+    // For completeness, also go through the layout tree and detach all paintables.
     root.for_each_in_inclusive_subtree([&](Layout::Node& node) {
         node.clear_paintables();
         return TraversalDecision::Continue;
     });
 
+    // After this point, we should have a clean slate to build the new paint tree.
+
     HashTable<Layout::InlineNode*> inline_nodes;
 
     root.document().for_each_shadow_including_inclusive_descendant([&](DOM::Node& node) {
diff --git a/Libraries/LibWeb/Painting/Paintable.cpp b/Libraries/LibWeb/Painting/Paintable.cpp
index 0485ae28c28..d7e8b620c54 100644
--- a/Libraries/LibWeb/Painting/Paintable.cpp
+++ b/Libraries/LibWeb/Painting/Paintable.cpp
@@ -36,6 +36,12 @@ Paintable::~Paintable()
 {
 }
 
+void Paintable::detach_from_layout_node()
+{
+    if (m_list_node.is_in_list())
+        m_list_node.remove();
+}
+
 void Paintable::visit_edges(Cell::Visitor& visitor)
 {
     Base::visit_edges(visitor);
diff --git a/Libraries/LibWeb/Painting/Paintable.h b/Libraries/LibWeb/Painting/Paintable.h
index 8eae9db57f6..810e58d9ba5 100644
--- a/Libraries/LibWeb/Painting/Paintable.h
+++ b/Libraries/LibWeb/Painting/Paintable.h
@@ -56,6 +56,8 @@ class Paintable
 public:
     virtual ~Paintable();
 
+    void detach_from_layout_node();
+
     [[nodiscard]] bool is_visible() const { return m_visible; }
     [[nodiscard]] bool is_positioned() const { return m_positioned; }
     [[nodiscard]] bool is_fixed_position() const { return m_fixed_position; }