LibWeb: Fix NodeIterator retargeting during removals

Align NodeIterator with the current DOM spec traversal and pre-removal
algorithms. The spec now covers the cases we were previously working
around to match the behaviour of other engines. The algorithm we had
implemented to protect against removal during filtering matches
the spec algorithm quite closely, with slightly different factoring.

See:
 * whatwg/dom#1479
 * whatwg/dom#1477

This fixes the case where removing an ancestor of the reference or
candidate reference, with the pointer before it and no following
node inside the root, should retarget to the previous in-tree node
instead of leaving the iterator on a detached node.

Author: shannonbooth

Libraries/LibWeb/DOM/NodeIterator.cpp

@@ -15,12 +15,12 @@ namespace Web::DOM {
 
 GC_DEFINE_ALLOCATOR(NodeIterator);
 
-NodeIterator::NodeIterator(JS::Realm& realm, Node& root)
+NodeIterator::NodeIterator(JS::Realm& realm, GC::Ref<Node> root, NodePointer reference)
     : PlatformObject(realm)
     , m_root(root)
-    , m_reference({ root })
+    , m_reference(reference)
 {
-    root.document().register_node_iterator({}, *this);
+    m_root->document().register_node_iterator({}, *this);
 }
 
 NodeIterator::~NodeIterator() = default;
@@ -44,17 +44,17 @@ void NodeIterator::visit_edges(Cell::Visitor& visitor)
     visitor.visit(m_root);
     visitor.visit(m_reference.node);
 
-    if (m_traversal_pointer.has_value())
-        visitor.visit(m_traversal_pointer->node);
+    if (m_candidate_reference.has_value())
+        visitor.visit(m_candidate_reference->node);
 }
 
 // https://dom.spec.whatwg.org/#dom-document-createnodeiterator
 GC::Ref<NodeIterator> NodeIterator::create(JS::Realm& realm, Node& root, unsigned what_to_show, GC::Ptr<NodeFilter> filter)
 {
     // 1. Let iterator be a new NodeIterator object.
-    // 2. Set iterator’s root and iterator’s reference to root.
-    // 3. Set iterator’s pointer before reference to true.
-    auto iterator = realm.create<NodeIterator>(realm, root);
+    // 2. Set iterator’s root to root.
+    // 3. Set iterator’s reference to (root, true).
+    auto iterator = realm.create<NodeIterator>(realm, root, NodePointer { root, true });
 
     // 4. Set iterator’s whatToShow to whatToShow.
     iterator->m_what_to_show = what_to_show;
@@ -76,65 +76,82 @@ void NodeIterator::detach()
 // https://dom.spec.whatwg.org/#concept-nodeiterator-traverse
 JS::ThrowCompletionOr<GC::Ptr<Node>> NodeIterator::traverse(Direction direction)
 {
-    // 1. Let node be iterator’s reference.
-    // 2. Let beforeNode be iterator’s pointer before reference.
-    m_traversal_pointer = m_reference;
+    // 1. Set iterator’s candidate reference to iterator’s reference.
+    m_candidate_reference = m_reference;
 
-    GC::Ptr<Node> candidate;
+    // 2. Let result be null.
+    GC::Ptr<Node> result;
 
     // 3. While true:
     while (true) {
-        // 4. Branch on direction:
+        // 1. If type is "next":
         if (direction == Direction::Next) {
-            // next
-            // If beforeNode is false, then set node to the first node following node in iterator’s iterator collection.
-            // If there is no such node, then return null.
-            if (!m_traversal_pointer->is_before_node) {
-                auto* next_node = m_traversal_pointer->node->next_in_pre_order(m_root.ptr());
-                if (!next_node)
-                    return nullptr;
-                m_traversal_pointer->node = *next_node;
-            } else {
-                // If beforeNode is true, then set it to false.
-                m_traversal_pointer->is_before_node = false;
+            // 1. If iterator’s candidate reference’s pointer before is false:
+            if (!m_candidate_reference->pointer_before) {
+                // 1. Let following be the first node following candidate reference’s node in iterator’s iterator collection.
+                //    If there is no such node, then break.
+                auto* following = m_candidate_reference->node->next_in_pre_order(m_root);
+                if (!following)
+                    break;
+
+                // 2. Set candidate reference to (following, false).
+                m_candidate_reference = { *following, false };
             }
-        } else {
-            // previous
-            // If beforeNode is true, then set node to the first node preceding node in iterator’s iterator collection.
-            // If there is no such node, then return null.
-            if (m_traversal_pointer->is_before_node) {
-                if (m_traversal_pointer->node.ptr() == m_root.ptr())
-                    return nullptr;
-                auto* previous_node = m_traversal_pointer->node->previous_in_pre_order();
-                if (!previous_node)
-                    return nullptr;
-                m_traversal_pointer->node = *previous_node;
-            } else {
-                // If beforeNode is false, then set it to true.
-                m_traversal_pointer->is_before_node = true;
+            // 2. Otherwise, set candidate reference to (candidate reference’s node, false).
+            else {
+                m_candidate_reference = { m_candidate_reference->node, false };
             }
         }
+        // 2. Otherwise:
+        else {
+            // 1. If iterator’s candidate reference’s pointer before is true:
+            if (m_candidate_reference->pointer_before) {
+                // 1. Let preceding be the first node preceding candidate reference’s node in iterator’s iterator collection.
+                //    If there is no such node, then break.
+                if (m_candidate_reference->node == m_root)
+                    break;
+                auto* preceding = m_candidate_reference->node->previous_in_pre_order();
+                if (!preceding)
+                    break;
+
+                // 2. Set iterator’s candidate reference to (preceding, true).
+                m_candidate_reference = { *preceding, true };
+            }
+            // 2. Otherwise, set candidate reference to (candidate reference’s node, true).
+            else {
+                m_candidate_reference = { m_candidate_reference->node, true };
+            }
+        }
+
+        // 3. Let node be candidate reference’s node.
+        GC::Ref<Node> node = m_candidate_reference->node;
 
-        // NOTE: If the NodeFilter deletes the iterator's current traversal pointer,
-        //       we will automatically retarget it. However, in that case, we're expected
-        //       to return the node passed to the filter, not the adjusted traversal pointer's
-        //       node after the filter returns!
-        candidate = m_traversal_pointer->node;
+        // 4. Let filterResult be the result of filtering node within iterator. If this throws an exception,
+        //    then set iterator’s candidate reference to null and rethrow that exception.
+        auto filter_result = filter(node);
+        if (filter_result.is_error()) {
+            m_candidate_reference.clear();
+            return filter_result.release_error();
+        }
+
+        // 5. If filterResult is FILTER_ACCEPT:
+        if (filter_result.value() == NodeFilter::Result::FILTER_ACCEPT) {
+            // 1. Set iterator’s reference to iterator’s candidate reference.
+            m_reference = *m_candidate_reference;
 
-        // 2. Let result be the result of filtering node within iterator.
-        auto result = TRY(filter(*m_traversal_pointer->node));
+            // 2. Set result to node.
+            result = node;
 
-        // 3. If result is FILTER_ACCEPT, then break.
-        if (result == NodeFilter::Result::FILTER_ACCEPT)
+            // 3. Break.
             break;
+        }
     }
 
-    // 4. Set iterator’s reference to node.
-    // 5. Set iterator’s pointer before reference to beforeNode.
-    m_reference = m_traversal_pointer.release_value();
+    // 4. Set iterator’s candidate reference to null.
+    m_candidate_reference.clear();
 
-    // 6. Return node.
-    return candidate;
+    // 5. Return result.
+    return result;
 }
 
 // https://dom.spec.whatwg.org/#concept-traversal-filter
@@ -183,76 +200,60 @@ JS::ThrowCompletionOr<NodeFilter::Result> NodeIterator::filter(Node& node)
 // https://dom.spec.whatwg.org/#dom-nodeiterator-nextnode
 JS::ThrowCompletionOr<GC::Ptr<Node>> NodeIterator::next_node()
 {
+    // The nextNode() method steps are to return the result of traversing with this and "next".
     return traverse(Direction::Next);
 }
 
 // https://dom.spec.whatwg.org/#dom-nodeiterator-previousnode
 JS::ThrowCompletionOr<GC::Ptr<Node>> NodeIterator::previous_node()
 {
+    // The previousNode() method steps are to return the result of traversing with this and "previous".
     return traverse(Direction::Previous);
 }
 
-void NodeIterator::run_pre_removing_steps_with_node_pointer(Node& to_be_removed_node, NodePointer& pointer)
+// https://dom.spec.whatwg.org/#nodeiterator-adjust
+NodeIterator::NodePointer NodeIterator::adjust_node_pointer(NodePointer node_pointer, Node& to_be_removed_node)
 {
-    // NOTE: This function tries to match the behavior of other engines, but not the DOM specification
-    //       as it's a known issue that the spec doesn't match how major browsers behave.
-    //       Spec bug: https://github.com/whatwg/dom/issues/907
-
-    if (!to_be_removed_node.is_descendant_of(root()))
-        return;
-
-    if (!to_be_removed_node.is_inclusive_ancestor_of(pointer.node))
-        return;
-
-    if (pointer.is_before_node) {
-        if (auto* node = to_be_removed_node.next_in_pre_order(root())) {
-            while (node && node->is_descendant_of(to_be_removed_node))
-                node = node->next_in_pre_order(root());
-            if (node)
-                pointer.node = *node;
-            return;
-        }
-        if (auto* node = to_be_removed_node.previous_in_pre_order()) {
-            if (to_be_removed_node.is_ancestor_of(pointer.node)) {
-                while (node && node->is_descendant_of(to_be_removed_node))
-                    node = node->previous_in_pre_order();
-            }
-            if (node) {
-                pointer = {
-                    .node = *node,
-                    .is_before_node = false,
-                };
-            }
-        }
-        return;
+    // 1. If toBeRemovedNode is not an inclusive ancestor of nodePointer’s node, or toBeRemovedNode is an inclusive
+    //    ancestor of nodeIterator’s root, then return nodePointer.
+    if (!to_be_removed_node.is_inclusive_ancestor_of(node_pointer.node) || to_be_removed_node.is_inclusive_ancestor_of(root()))
+        return node_pointer;
+
+    // 2. If nodePointer’s pointer before is true:
+    if (node_pointer.pointer_before) {
+        // 1. Let next be toBeRemovedNode’s first following node that is an inclusive descendant of nodeIterator’s root
+        //    and is not an inclusive descendant of toBeRemovedNode, if there is such a node; otherwise null.
+        auto* next = to_be_removed_node.next_in_pre_order(root());
+        while (next && next->is_descendant_of(to_be_removed_node))
+            next = next->next_in_pre_order(root());
+
+        // 2. If next is non-null, then return (next, true).
+        if (next)
+            return { *next, true };
     }
 
-    if (auto* node = to_be_removed_node.previous_in_pre_order()) {
-        if (to_be_removed_node.is_ancestor_of(pointer.node)) {
-            while (node && node->is_descendant_of(to_be_removed_node))
-                node = node->previous_in_pre_order();
-        }
-        if (node)
-            pointer.node = *node;
-        return;
-    }
-    auto* node = to_be_removed_node.next_in_pre_order(root());
-    if (to_be_removed_node.is_ancestor_of(pointer.node)) {
-        while (node && node->is_descendant_of(to_be_removed_node))
-            node = node->previous_in_pre_order();
-    }
-    if (node)
-        pointer.node = *node;
+    // 3. Let newNode be toBeRemovedNode’s parent, if toBeRemovedNode’s previous sibling is null; otherwise the
+    //    inclusive descendant of toBeRemovedNode’s previous sibling that appears last in tree order.
+    // NOTE: This is exactly toBeRemovedNode’s previous node in pre-order. It is never null here, because step 1
+    //       guarantees toBeRemovedNode is a proper descendant of the root and thus has a parent.
+    auto* new_node = to_be_removed_node.previous_in_pre_order();
+    VERIFY(new_node);
+
+    // 4. Return (newNode, false).
+    return { *new_node, false };
 }
 
 // https://dom.spec.whatwg.org/#nodeiterator-pre-removing-steps
 void NodeIterator::run_pre_removing_steps(Node& to_be_removed_node)
 {
-    // NOTE: If we're in the middle of traversal, we have to adjust the traversal pointer in response to node removal.
-    if (m_traversal_pointer.has_value())
-        run_pre_removing_steps_with_node_pointer(to_be_removed_node, *m_traversal_pointer);
-
-    run_pre_removing_steps_with_node_pointer(to_be_removed_node, m_reference);
+    // 1. Set nodeIterator’s reference to the result of adjusting a node pointer given nodeIterator’s reference,
+    //    nodeIterator, and toBeRemovedNode.
+    m_reference = adjust_node_pointer(m_reference, to_be_removed_node);
+
+    // 2. If nodeIterator’s candidate reference is non-null, then set nodeIterator’s candidate reference to the result
+    //    of adjusting a node pointer given nodeIterator’s candidate reference, nodeIterator, and toBeRemovedNode.
+    if (m_candidate_reference.has_value())
+        m_candidate_reference = adjust_node_pointer(*m_candidate_reference, to_be_removed_node);
 }
 
 }

Libraries/LibWeb/DOM/NodeIterator.h

@@ -25,7 +25,7 @@ class NodeIterator final : public Bindings::PlatformObject {
 
     GC::Ref<Node> root() { return m_root; }
     GC::Ref<Node> reference_node() { return m_reference.node; }
-    bool pointer_before_reference_node() const { return m_reference.is_before_node; }
+    bool pointer_before_reference_node() const { return m_reference.pointer_before; }
     unsigned what_to_show() const { return m_what_to_show; }
 
     GC::Ptr<NodeFilter> filter() const;
@@ -38,7 +38,14 @@ class NodeIterator final : public Bindings::PlatformObject {
     void run_pre_removing_steps(Node&);
 
 private:
-    explicit NodeIterator(JS::Realm&, Node& root);
+    // https://dom.spec.whatwg.org/#node-pointer
+    // A node pointer is a tuple consisting of a node (a node) and a pointer before (a boolean).
+    struct NodePointer {
+        GC::Ref<Node> node;
+        bool pointer_before { true };
+    };
+
+    explicit NodeIterator(JS::Realm&, GC::Ref<Node> root, NodePointer);
 
     virtual void initialize(JS::Realm&) override;
     virtual void visit_edges(Cell::Visitor&) override;
@@ -56,21 +63,13 @@ class NodeIterator final : public Bindings::PlatformObject {
     // https://dom.spec.whatwg.org/#concept-traversal-root
     GC::Ref<Node> m_root;
 
-    struct NodePointer {
-        GC::Ref<Node> node;
-
-        // https://dom.spec.whatwg.org/#nodeiterator-pointer-before-reference
-        bool is_before_node { true };
-    };
-
-    void run_pre_removing_steps_with_node_pointer(Node&, NodePointer&);
+    NodePointer adjust_node_pointer(NodePointer, Node& to_be_removed_node);
 
     // https://dom.spec.whatwg.org/#nodeiterator-reference
     NodePointer m_reference;
 
-    // While traversal is ongoing, we keep track of the current node pointer.
-    // This allows us to adjust it during traversal if calling the filter ends up removing the node from the DOM.
-    Optional<NodePointer> m_traversal_pointer;
+    // https://dom.spec.whatwg.org/#nodeiterator-candidate-reference
+    Optional<NodePointer> m_candidate_reference;
 
     // https://dom.spec.whatwg.org/#concept-traversal-whattoshow
     unsigned m_what_to_show { 0 };

Tests/LibWeb/Text/expected/wpt-import/dom/traversal/NodeIterator-removal-during-filtering.txt

@@ -2,9 +2,8 @@ Harness status: OK
 
 Found 4 tests
 
-3 Pass
-1 Fail
+4 Pass
 Pass	Removing the accepted node during its own filtering retargets the reference and returns the filtered node
 Pass	Removing an already-visited subtree during filtering does not disrupt forward traversal
 Pass	referenceNode during filtering reflects the last accepted node, not the candidate
-Fail	Removing an ancestor of the in-flight position during filtering, pointer before it, with no following node within root
+Pass	Removing an ancestor of the in-flight position during filtering, pointer before it, with no following node within root

Tests/LibWeb/Text/expected/wpt-import/dom/traversal/NodeIterator-removal.txt

@@ -2,8 +2,7 @@ Harness status: OK
 
 Found 25 tests
 
-24 Pass
-1 Fail
+25 Pass
 Pass	Test removing node paras[0]
 Pass	Test removing node paras[0].firstChild
 Pass	Test removing node paras[1].firstChild
@@ -28,4 +27,4 @@ Pass	Test removing node xmlComment
 Pass	Test removing node foreignComment
 Pass	Test removing node xmlDoctype
 Pass	Removing an inclusive ancestor of reference, pointer before reference is true, with a following node within root
-Fail	Removing an inclusive ancestor of reference, pointer before reference is true, with no following node within root
+Pass	Removing an inclusive ancestor of reference, pointer before reference is true, with no following node within root