bevy-ui: Fix crash when despawning a Node that had its ChildOf removed.

[CyberspaceDreamn]

Objective

Prevent an occasional crash in taffy / bevy_ui when despawning Nodes that had their ChildOf component removed.

Fixes #25303. ¹

Solution

When a Node has its ChildOf removed, also remove its TaffyTree::parent. I believe this prevents bevy from calling TaffyTree::remove twice with the same NodeId.

Testing

• bevy_ui unit tests ²
• a mostly reliable demo of this crash (link) ²

Footnotes

1. I cannot replicate the crash with this patch applied, but solid proof is difficult with heisenbugs. ↩

2. Tested only on Linux, but as this is a logic bug platform should not matter. ↩ ↩²

[github-actions]

Welcome, new contributor!

Please make sure you've read our contributing guide, as well as our policy regarding AI usage, and we look forward to reviewing your pull request shortly ✨

[ickshonpe]

This removes the new root from the previous parent's children, before the implicit viewport node is added. So then in update_children_recursively set_children for the previous parent doesn't see the root and doesn't overwrite the root's implicit viewport node parent. I think this works, but there mght be a shorter fix.

[ickshonpe]

The underlying problem is that the new root is given an implicit taffy parent representing the viewport in get_or_insert_taffy_viewport, but its not removed from its old parent's children at the same time. Then when the old parent's children are updated it sees the new root was removed and clears the new root's parent, which it expects to be the old parent, but it isn't, it's the new implicit viewport parent.

So it should be sufficient to just remove the new root node from its previous parent's (if it had a previous parent) taffy children in get_or_insert_taffy_viewport. If that makes sense.

[CyberspaceDreamn]

You are mostly correct, but it is the root being present in multiple children, not the missing TaffyTree::parent that causes this crash.

from taffy/src/taffy_tree.rs:631-635

if let Some(children) = self.children.get(key) {
    for child in children.iter().copied() {
        self.parents[child.into()] = None; // crashes here with `invalid SlotMap key used`
    }
}

If self.parents[child.into()] still existed here, assigning to it would not be accessing an invalid slotmap key. Despawning the Node will also crash even if it has been several seconds after removing its ChildOf, so update order within one frame is not the cause of this crash.

I believe the root cause is an unresolved bug in TaffyTree::add_child DioxusLabs/taffy#1100. bevy's update order seems to partially mitigate that issue & explains the inconsistency of the crash.

Either way, explicitly removing its previous parent in get_or_insert_taffy_viewport, as you suggested, does prevent bevy from crashing until/unless a patch lands upstream.

[ickshonpe]

This seems to work reliably as a regression test:

    #[test]
    fn node_with_sibling_should_have_implicit_viewport_node_parent_after_unparenting() {
        #[derive(Component)]
        struct Marker;
        let mut app = setup_ui_test_app();
        let world = app.world_mut();
        world.spawn(Node::default());
        let child_a = world.spawn(Node::default()).id();
        let child_b = world.spawn(Node::default()).id();
        world
            .spawn((Node::default(), Marker))
            .add_children(&[child_a, child_b]);
        app.update();
        app.world_mut().entity_mut(child_a).remove::<ChildOf>();
        app.update();
        let ui_surface = app.world().resource::<UiSurface>();
        let node = ui_surface.entity_to_taffy[&child_a];
        assert_eq!(ui_surface.taffy.parent(node.id), node.viewport_id);
        assert!(ui_surface
            .taffy
            .children(node.viewport_id.unwrap())
            .is_ok_and(|children| children.contains(&node.id)));
    }

It makes assumptions about archetypes and query ordering etc that might not hold in later bevy versions though.

[gagnus]

it looks right to me but I'm not experienced enough with taffy to say for sure.