From 28fdc7af05b410ba9f2c7edf7aa95db0e0e666c7 Mon Sep 17 00:00:00 2001
From: Andreas Kling <kling@serenityos.org>
Date: Tue, 1 Aug 2023 08:23:13 +0200
Subject: [PATCH] LibWeb: Detach stale layout nodes from DOM during layout tree
 build

When toggling `display: none` on an element, it can go from having a
layout subtree to not having one. In the `none` case, we were previously
leaving stale layout nodes hanging off DOM nodes in the subtree.

These layout nodes could be queried for outdated information and
probably other things that we shouldn't allow.

Fix this by having TreeBuilder prune any old layout nodes hanging off
nodes in a subtree after its subtree root doesn't produce a layout node.
---
 .../setting-display-none-should-nuke-subtree.txt  |  2 ++
 .../setting-display-none-should-nuke-subtree.html | 15 +++++++++++++++
 Userland/Libraries/LibWeb/DOM/Node.cpp            |  2 +-
 Userland/Libraries/LibWeb/DOM/Node.h              |  2 +-
 Userland/Libraries/LibWeb/Layout/TreeBuilder.cpp  | 15 ++++++++++++++-
 5 files changed, 33 insertions(+), 3 deletions(-)
 create mode 100644 Tests/LibWeb/Text/expected/setting-display-none-should-nuke-subtree.txt
 create mode 100644 Tests/LibWeb/Text/input/setting-display-none-should-nuke-subtree.html

diff --git a/Tests/LibWeb/Text/expected/setting-display-none-should-nuke-subtree.txt b/Tests/LibWeb/Text/expected/setting-display-none-should-nuke-subtree.txt
new file mode 100644
index 0000000000..da1feed8cf
--- /dev/null
+++ b/Tests/LibWeb/Text/expected/setting-display-none-should-nuke-subtree.txt
@@ -0,0 +1,2 @@
+100
+0
diff --git a/Tests/LibWeb/Text/input/setting-display-none-should-nuke-subtree.html b/Tests/LibWeb/Text/input/setting-display-none-should-nuke-subtree.html
new file mode 100644
index 0000000000..9e9f093397
--- /dev/null
+++ b/Tests/LibWeb/Text/input/setting-display-none-should-nuke-subtree.html
@@ -0,0 +1,15 @@
+<script src="include.js"></script>
+<script>
+    test(() => {
+        let a = document.createElement("div");
+        let b = document.createElement("div");
+        a.appendChild(b);
+        document.body.appendChild(a);
+
+        b.style = 'width: 100px;';
+
+        println(b.offsetWidth);
+        a.style.display = 'none';
+        println(b.offsetWidth);
+    });
+</script>
diff --git a/Userland/Libraries/LibWeb/DOM/Node.cpp b/Userland/Libraries/LibWeb/DOM/Node.cpp
index c67b5051a8..7406f10e1d 100644
--- a/Userland/Libraries/LibWeb/DOM/Node.cpp
+++ b/Userland/Libraries/LibWeb/DOM/Node.cpp
@@ -882,7 +882,7 @@ void Node::set_layout_node(Badge<Layout::Node>, JS::NonnullGCPtr<Layout::Node> l
     m_layout_node = layout_node;
 }
 
-void Node::detach_layout_node(Badge<DOM::Document>)
+void Node::detach_layout_node(Badge<Layout::TreeBuilder>)
 {
     m_layout_node = nullptr;
 }
diff --git a/Userland/Libraries/LibWeb/DOM/Node.h b/Userland/Libraries/LibWeb/DOM/Node.h
index b3e319ce04..ca817cb9d0 100644
--- a/Userland/Libraries/LibWeb/DOM/Node.h
+++ b/Userland/Libraries/LibWeb/DOM/Node.h
@@ -185,7 +185,7 @@ public:
     Painting::Paintable const* paintable() const;
 
     void set_layout_node(Badge<Layout::Node>, JS::NonnullGCPtr<Layout::Node>);
-    void detach_layout_node(Badge<DOM::Document>);
+    void detach_layout_node(Badge<Layout::TreeBuilder>);
 
     virtual bool is_child_allowed(Node const&) const { return true; }
 
diff --git a/Userland/Libraries/LibWeb/Layout/TreeBuilder.cpp b/Userland/Libraries/LibWeb/Layout/TreeBuilder.cpp
index aacef3e999..765733f786 100644
--- a/Userland/Libraries/LibWeb/Layout/TreeBuilder.cpp
+++ b/Userland/Libraries/LibWeb/Layout/TreeBuilder.cpp
@@ -210,8 +210,22 @@ ErrorOr<void> TreeBuilder::create_pseudo_element_if_needed(DOM::Element& element
 
 ErrorOr<void> TreeBuilder::create_layout_tree(DOM::Node& dom_node, TreeBuilder::Context& context)
 {
+    JS::GCPtr<Layout::Node> layout_node;
     Optional<TemporaryChange<bool>> has_svg_root_change;
 
+    ScopeGuard remove_stale_layout_node_guard = [&] {
+        // If we didn't create a layout node for this DOM node,
+        // go through the DOM tree and remove any old layout nodes since they are now all stale.
+        if (!layout_node) {
+            dom_node.for_each_in_inclusive_subtree([&](auto& node) {
+                node.detach_layout_node({});
+                if (is<DOM::Element>(node))
+                    static_cast<DOM::Element&>(node).clear_pseudo_element_nodes({});
+                return IterationDecision::Continue;
+            });
+        }
+    };
+
     if (dom_node.is_svg_container()) {
         has_svg_root_change.emplace(context.has_svg_root, true);
     } else if (dom_node.requires_svg_container() && !context.has_svg_root) {
@@ -220,7 +234,6 @@ ErrorOr<void> TreeBuilder::create_layout_tree(DOM::Node& dom_node, TreeBuilder::
 
     auto& document = dom_node.document();
     auto& style_computer = document.style_computer();
-    JS::GCPtr<Layout::Node> layout_node;
     RefPtr<CSS::StyleProperties> style;
     CSS::Display display;