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

Iterating over the NamedNodeMap returned by Element.prototype.attributes is unsafe and vulnerable to race conditions #79

Merged
merged 2 commits into from
Dec 21, 2024

Conversation

botandrose
Copy link

Closes #41 . More context there, but the gist of it is resolving this issue:

From https://www.oreilly.com/library/view/javascript-the-definitive/0596000480/re534.html (emphasis mine)

Although the NamedNodeMap interface allows you to iterate through its nodes by position, it does not represent an ordered collection of nodes. Any changes made to the NamedNodeMap (such as by removeNamedItem( ) or setNamedItem( )) may result in a complete reordering of the elements. Thus, you must not modify a NamedNodeMap while you are iterating through its elements.

@botandrose botandrose force-pushed the non-live-named-node-map branch from 8da35d9 to 6fbb0f2 Compare December 20, 2024 16:37
@botandrose
Copy link
Author

More here. Benchmarking reveals roughly 5% perf hit due to all the array instantiations: https://www.measurethat.net/Benchmarks/ListResults/33103 , and we still haven't verified if this is the issue or even the fix! Not great.

I did find a unmerged PR for morphdom that seems to address the exact same issue, but in a simpler way. This may be performant enough to do instead, and also describes the exact circumstances needed to reproduce. https://github.com/patrick-steele-idem/morphdom/pull/242/files

So I'm going to move forward with:

  1. Trying to create a failing test using attributeChangedCallback to remove a second attribute when a first is removed
  2. Verify the fix
  3. Benchmark this other strategy

@botandrose botandrose force-pushed the non-live-named-node-map branch from 6fbb0f2 to 899fd9a Compare December 21, 2024 20:59
…ned by

Element.prototype.attributes is unsafe and vulnerable to race conditions from
synchronous custom element attribute callbacks.

E.g. <turbo-frame>'s src and complete attributes are linked.
@botandrose botandrose force-pushed the non-live-named-node-map branch from 899fd9a to 166bdc2 Compare December 21, 2024 21:01
@botandrose
Copy link
Author

botandrose commented Dec 21, 2024

@1cg Okay, this is ready for your review.

Benchmarks here: https://www.measurethat.net/Benchmarks/ListResults/33105

It seems about the same in Chrome, slightly slower in Safari, and slightly faster (?!) in Firefox.

Semi-interesting footnote: Array.from(el.attributes) also makes the test pass.

@1cg 1cg merged commit 6c47b83 into bigskysoftware:main Dec 21, 2024
2 checks passed
@botandrose botandrose deleted the non-live-named-node-map branch December 21, 2024 22:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeError: Cannot read properties of undefined (reading 'name')
3 participants