LibWeb: More thoroughly detach layout/paint trees from each other
Some checks are pending
CI / macOS, arm64, Sanitizer, Clang (push) Waiting to run
CI / Linux, x86_64, Fuzzers, Clang (push) Waiting to run
CI / Linux, x86_64, Sanitizer, GNU (push) Waiting to run
CI / Linux, x86_64, Sanitizer, Clang (push) Waiting to run
Package the js repl as a binary artifact / Linux, arm64 (push) Waiting to run
Package the js repl as a binary artifact / macOS, arm64 (push) Waiting to run
Package the js repl as a binary artifact / Linux, x86_64 (push) Waiting to run
Run test262 and test-wasm / run_and_update_results (push) Waiting to run
Lint Code / lint (push) Waiting to run
Label PRs with merge conflicts / auto-labeler (push) Waiting to run
Push notes / build (push) Waiting to run

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.
This commit is contained in:
Andreas Kling 2025-07-30 16:57:03 +02:00 committed by Alexander Kalenik
commit 3b63582068
Notes: github-actions[bot] 2025-07-30 20:59:47 +00:00
3 changed files with 20 additions and 3 deletions

View file

@ -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) {

View file

@ -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);

View file

@ -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; }