Don't mutate children on DomNode #33818
Conversation
Don't mutate the children of a DOM node directly.
lib/web_ui/lib/src/engine/dom.dart
Outdated
| DomElement? get parent => js_util.getProperty(this, 'parentElement'); | ||
| String? get text => js_util.getProperty(this, 'textContent'); | ||
| external DomNode? get parentNode; | ||
| external DomNode? get firstChild; |
There was a problem hiding this comment.
I think firstChild / lastChild already exist (lines 145 / 147).
There was a problem hiding this comment.
Yeah I hadn't merged main yet when I wrote this.
joshualitt
left a comment
joshualitt
left a comment
There was a problem hiding this comment.
This looks good, modulo some minor feedback.
| ) { | ||
| int indexInFlutterView = -1; | ||
| if (headClipView.parent != null) { | ||
| indexInFlutterView = skiaSceneHost!.children.indexOf(headClipView); |
There was a problem hiding this comment.
This is actually safe for now, but nice to get rid of if we decide we want to move off using List for children.
| _svgPathDefs?.children.single.children.forEach(removeElement); | ||
| final DomElement? parent = _svgPathDefs?.children.single; | ||
| if (parent != null) { | ||
| for (DomNode? child = parent.lastChild; child != null; child = parent.lastChild) { |
There was a problem hiding this comment.
We talked offline but:
while (parent.firstChild != null) {
parent.removeChild(parent.firstChild)
}
is a bit more succinct. I'm okay with either.
There was a problem hiding this comment.
I decided to stick with this because it only calls lastChild once per iteration, and I don't have to do a nullability assertion (It would have to be parent.removeChild(parent.firstChild!))
* flutter/flutter#104972 Don't mutate the children of a DOM node directly. * nextSibling should be nullable. * Remove redundant firstChild/lastChild fields. * Update to parentNode instead of parentElement.
2 participants
DomNode differs from the dart:html node in that dart:html does some magic under the hood to reflect changes made to the container to the underlying HTML node. DomNode does not do this, and its children are not mutable. Therefore use the normal JavaScript APIs to modify the tree.
Added a unit test that exercises this code path, which is when the chain of clip views changes length. This was the code path that was causing the crash described in the issue. The example in the issue reproduced this easily before these changes, and does not occur after.
Note that as part of updating the web engine to the new Dom API, @joshualitt is possibly planning on changing the type of the
childrenfield to be an immutable type (which would have caught this issue at compile time).Fixes flutter/flutter#104972