Skip to content

LibWeb: Fix NodeIterator retargeting during removals #10404

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
shannonbooth merged 2 commits into from
Jul 3, 2026
Merged

LibWeb: Fix NodeIterator retargeting during removals #10404

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
80ad1c5
Tests/LibWeb: Import some NodeIterator WPT tests
shannonbooth Jul 2, 2026
fa91815
LibWeb: Fix NodeIterator retargeting during removals
shannonbooth Jul 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
211 changes: 106 additions & 105 deletions Libraries/LibWeb/DOM/NodeIterator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -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);
}

}
25 changes: 12 additions & 13 deletions Libraries/LibWeb/DOM/NodeIterator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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 };
Expand Down
9 changes: 9 additions & 0 deletions ...s/LibWeb/Text/expected/wpt-import/dom/traversal/NodeIterator-removal-during-filtering.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
Harness status: OK

Found 4 tests

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
Pass Removing an ancestor of the in-flight position during filtering, pointer before it, with no following node within root
30 changes: 30 additions & 0 deletions Tests/LibWeb/Text/expected/wpt-import/dom/traversal/NodeIterator-removal.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
Harness status: OK

Found 25 tests

25 Pass
Pass Test removing node paras[0]
Pass Test removing node paras[0].firstChild
Pass Test removing node paras[1].firstChild
Pass Test removing node paras[5].firstChild
Pass Test removing node foreignPara1
Pass Test removing node foreignPara1.firstChild
Pass Test removing node detachedPara1
Pass Test removing node detachedPara1.firstChild
Pass Test removing node foreignPara2
Pass Test removing node xmlElement
Pass Test removing node foreignTextNode
Pass Test removing node processingInstruction
Pass Test removing node comment
Pass Test removing node doctype
Pass Test removing node foreignDoctype
Pass Test removing node paras[1]
Pass Test removing node detachedPara2
Pass Test removing node detachedPara2.firstChild
Pass Test removing node testDiv
Pass Test removing node xmlTextNode
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
Pass Removing an inclusive ancestor of reference, pointer before reference is true, with no following node within root
Loading
Loading