dom icon indicating copy to clipboard operation
dom copied to clipboard

TreeWalker.nextNode loops forever if last node of a detached node tree is not accepted by the filter

Open Lubrsi opened this issue 3 years ago • 1 comments

For example, given:

var el = document.createElement("div");
var walker = document.createTreeWalker(document, 0, null);
walker.currentNode = el;
var next = walker.nextNode()

Following these steps: https://dom.spec.whatwg.org/#dom-treewalker-nextnode

1. Let node be this's current.

This is el above.

2. Let result be FILTER_ACCEPT.
3. While true:
   1. While result is not FILTER_REJECT and node has a child:

result is FILTER_ACCEPT but el has no children, so we skip this.

    2. Let sibling be null.
    3. Let temporary be node.
    4. While temporary is non-null:
       1. If temporary is this’s root, then return null.

temporary is el and this's root is document, so we skip this.

       2. Set sibling to temporary’s next sibling.
       3. If sibling is non-null, then set node to sibling and break.

el has no siblings, so we skip this.

        4. Set temporary to temporary’s parent.

el has no parent, so we set temporary to null and thus exit the loop.

    5. Set result to the result of filtering node within this.
    6. If result is FILTER_ACCEPT, then set this’s current to node and return node.

We passed 0 for whatToShow in createTreeWalker, meaning result is FILTER_SKIP, so we skip this and return to the top of the While true:

Note that on all future iterations, none of the conditions have changed as the only variable that's changed is result, which has changed from FILTER_ACCEPT to FILTER_SKIP. This only affects step 3.1 which still fails to meet the condition.

It seems that If temporary is this’s root, then return null. was meant to protect against this, but it was not considered that the root may not be reachable, for example when currentNode is in a detached node tree where root is the document.

WebKit, Gecko and Blink do not loop forever in this case and simply return null.

This was seen in https://archive.org/includes/build/js/ia-topnav.min.js?v=a0bdccf6, which does something more like:

var walker = document.createTreeWalker(document, NodeFilter.SHOW_ELEMENT | NodeFilter.SHOW_COMMENT, null, false);

var el = document.createElement("template");
el.innerHTML = "text<div></div>text";

walker.currentNode = el.content;

var node;

for (; (node = walker.nextNode()) !== null;) {
    // Do something with node.
}

The call to nextNode in question is located at line 2, column 24518 of the given JavaScript.

Lubrsi avatar Aug 06 '22 23:08 Lubrsi

Something similar was fixed in https://github.com/whatwg/dom/pull/747; might be worth checking that out and following links to see if there was something obvious that was missed.

domenic avatar Aug 07 '22 02:08 domenic