bevy_ecs: flush entities after running observers and hooks in despawn

[jrobsonchase]

Objective

Fixes #14467

Observers and component lifecycle hooks are allowed to perform operations that subsequently require Entities to be flushed, such as reserving a new entity. If this occurs during an on_remove hook or an OnRemove event trigger during an EntityWorldMut::despawn, a panic will occur.

Solution

Call world.flush_entities() after running on_remove hooks/observers during despawn

Testing

Added a new test that fails before the fix and succeeds afterward.

[alice-i-cecile]

Let me know when this is ready for review :)

[jrobsonchase]

@alice-i-cecile Ready!

[jrobsonchase]

Ooh, a miri failure. That's a new one for me!

[alice-i-cecile]

Some clarity nits about the test, but I agree with the fix and the test design is good.

[iiYese]

The flushes need explaining but other than that lgtm.

[chescock]

There's also a call to world.flush_entities(); at the top of this method. Do we need both, or did that one just need to get moved down here?

It looks like prior to #10756 the flush() call was right before the free() call, and the hooks and observers code got added in between. The calls to deferred_world.trigger_on_foo in bundle.rs don't have flush() calls. That makes me think that we don't need to flush before triggering things, and we only need the single call here, right before the free().

It doesn't look like anything bad can happen if we flush twice, though, so it should be fine to leave both even if they're not both needed.

[jrobsonchase]

Hmm, so if I remove the first world.flush_entities(), the tests all pass and everything appears to work fine. As long as observers and hooks are unable to call any methods on Entities that verify_flushed(), it's safe to move. Since all of those require mutable access to Entities and thus exclusive World access as far as I'm aware, we should be OK to remove the first one.

[alice-i-cecile]

Merging; thanks for the careful work and clear explanations :)

[jrobsonchase]

Whoops, left an extra line in my test that broke CI. Should be good to go now @alice-i-cecile