fix panic: invalid SlotMap key used

[eidloi]

Objective

Tight, in-frame generation, re-parenting, despawning, etc., UI operations could sometime lead taffy to panic (invalid SlotMap key used) when an entity with an invalid state later despawned.

Fixes #12403

Solution

Move the remove_entities call after children updates.

Testing

sickle_ui had a case that always caused the panic. Tested before this change, after this change, and before the change again to make sure the error is there without the fix. The fix worked. Test steps and used commit described in issue #12403.

I have also ran every bevy UI example, though none of them deal with entity re-parenting or removal. No regression detected on them.

Tested on Windows only.

[github-actions]

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

[eidloi]

Wish I had the time to create a proper test suite for this.

[brandon-reinhart]

Looks good to me.

[mockersf]

not absolutely confident this doesn't have side effects, but I can't think of any and it makes sense to me

[eidloi]

@nicoburns perhaps has a strong opinion?

[nicoburns]

I do not have strong opinions on this. My disposition is similar to:

not absolutely confident this doesn't have side effects, but I can't think of any and it makes sense to me

[StrikeForceZero]

No longer relavent

~~so this is likely the offending area triggering the crash https://github.com/bevyengine/bevy/blob/158ccc6d6a45eb048cc3f40388f9cff0fc633375/crates/bevy_ui/src/layout/ui_surface.rs#L128-L131~~

Because we are unwrapping the internalmap/taffy result with the parent entity who would have already been removed by the line this PR is moving.

I did a little sanity check to double check:

Index: crates/bevy_ui/src/layout/mod.rs
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/crates/bevy_ui/src/layout/mod.rs b/crates/bevy_ui/src/layout/mod.rs
--- a/crates/bevy_ui/src/layout/mod.rs	(revision Staged)
+++ b/crates/bevy_ui/src/layout/mod.rs	(date 1719178048091)
@@ -276,7 +276,8 @@
     }
 
     // clean up removed nodes
-    ui_surface.remove_entities(removed_components.removed_nodes.read());
+    let removed_nodes = removed_components.removed_nodes.read().collect::<HashSet<_>>();
+    ui_surface.remove_entities(removed_nodes.clone().into_iter());
 
     // clean up removed cameras
     ui_surface.remove_camera_entities(removed_components.removed_cameras.read());
@@ -298,6 +299,9 @@
     }
     for (entity, children) in &children_query {
         if children.is_changed() {
+            if removed_nodes.contains(&entity) {
+                continue;
+            }
             ui_surface.update_children(entity, &children);
         }
     }

and this does in fact prevent the crash as well, but obviously (or at least imo) moving the line down below makes more sense.

~~The part that confuses me is the tests I made for this specific condition fail when run on main but pass in my PR, yet don't prevent the crash in your reproduction example..

So, I'm inclined to believe there is some kind of race condition or alternative behavior I'm not aware of triggering this, and

I think moving the line is the way to go for 14.0.

If we want to have a test suite that covers some of this, I can extract my regression tests mentioned above into their own PR after this gets merged.

---

Edit: I need to walk away from the computer for a bit, but I think I've cleared my confusion. I will confirm later tonight when I have more time.

[StrikeForceZero]

I can't create a regression test that mimics the slotkey error. It's likely related to moving a UI node to a new parent and despawning the old one.

Regardless, this is a good fix. Intuitively, you'd need to perform operations on all the children and ensure they are in sync on the taffy's side before removing the UI node.

IMO it's highly unlikely this will introduce any negative side effects. Especially considering how the layout system is already omitting the handling of other hierarchy changes that I've been meaning to open tickets for 🙃

[eidloi]

it does bother me to no end that I don't have the bare basic broken test case myself, but the slotmap ends up in the erroneous state way earlier than when the despawn trips it.