Skip to content
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.

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

NodeList.item() returns removed nodes #499

Closed
qtow opened this issue Jun 15, 2023 · 2 comments · Fixed by #514
Closed

NodeList.item() returns removed nodes #499

qtow opened this issue Jun 15, 2023 · 2 comments · Fixed by #514
Assignees
Labels
bug Something isn't working help-wanted External contributions welcome
Milestone

Comments

@qtow
Copy link

qtow commented Jun 15, 2023

Example:

const doc = new DOMParser().parseFromString("<root><child/></root>", "text/xml")
const nodes = doc.getElementsByTagName("child");

// correct: { length: 1, item_0: "child" }
const first = {
    length: nodes.length,
    item_0: nodes.item(0)?.tagName ?? null,
};

doc.firstChild.removeChild(doc.firstChild.firstChild);

// should be: { length: 0, item_0: null}
// incorrect: { length: 0, item_0: "child" }
const second = {
    length: nodes.length,
    item_0: nodes.item(0)?.tagName ?? null,
};

console.log(first);
console.log(second);

Cause:
LiveNodeList is updated at dom.js:312 by copying all owned properties from an array. If the new array is shorter, indexes >= length are not removed.

Possible Solution:
Before or after copy(ls, list), explicitly delete the extra properties:

for (var i = ls.length; i in list; i += 1) {
    delete list[i];
}

Runtime & Version:
xmldom: 0.8.8
nodejs: 20.3.0

Additional context:
This causes problems with patterns like this:

for (let node; (node = nodes.item(0)) !== null; node.parentNode.removeChild(node)) {
    // ...
}
@qtow qtow added bug Something isn't working needs investigation Information is missing and needs to be researched labels Jun 15, 2023
@karfau
Copy link
Member

karfau commented Jun 15, 2023

Thanks for the great bug report.
I would even suggest we should drop the property of part of the removeChild call using the delete keyword.
Your analysis was of course correct that the issue is in _updateLiveList.

@karfau karfau added help-wanted External contributions welcome and removed needs investigation Information is missing and needs to be researched labels Jun 15, 2023
@karfau karfau added this to the 0.9.0 milestone Jun 15, 2023
@karfau karfau self-assigned this Jun 17, 2023
karfau added a commit that referenced this issue Jul 16, 2023
karfau added a commit that referenced this issue Jul 19, 2023
- `NodeList.item` now checks the `length` attribute before accessing the
item at a provided index and returns `null` it the index is out of range
- when updating `LiveNodeList` (as part of accessing the `item` method)
and the new `length` is shorter then the current one, any references on
indexes following the index starting from `length` are `delete`d, and
`null` is returned instead of `undefined`
- added some exports to the `lib/dom.js` module to enable testing

fixes #499
karfau added a commit that referenced this issue Jul 19, 2023
- `NodeList.item` now checks the `length` attribute before accessing the
item at a provided index and returns `null` it the index is out of range
- when updating `LiveNodeList` (as part of accessing the `item` method)
and the new `length` is shorter then the current one, any references on
indexes following the index starting from `length` are `delete`d, and
`null` is returned instead of `undefined`
- added some exports to the `lib/dom.js` module to enable testing

fixes #499
karfau added a commit that referenced this issue Jul 19, 2023
- `NodeList.item` now checks the `length` attribute before accessing the
item at a provided index and returns `null` it the index is out of range
- when updating `LiveNodeList` (as part of accessing the `item` method)
and the new `length` is shorter then the current one, any references on
indexes following the index starting from `length` are `delete`d, and
`null` is returned instead of `undefined`
- added some exports to the `lib/dom.js` module to enable testing

fixes #499
@karfau
Copy link
Member

karfau commented Jul 19, 2023

The fix landed in 0.7 (lts), 0.8 (latest) and 0.9 (next)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help-wanted External contributions welcome
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants