Fix a NRE in DeliverUnit.ReleaseUnit.OnFirstRun

[abcdefg30]

Closes #19279.

[pchote]

How does that happen in practice?

[Mailaender]

It makes sense that this could be null. self would probably crash due to being dead.

[pchote]

Checking the source replay confirmed my suspicion that something more subtle was happing there.

The problem is that actors may run their activity in the time after being killed but before the end-tick disposal cleanup happening (IsDead: true, WillDispose: true, Disposed: false). The carryall (ActorID 900) is killed at the start of the tick, and its death handler disposes Carryable and sets it to null. The activity then runs and crashes due to the null actor.

We have two options here:

1. Keep the DeliverUnit null check, but change the comment to explain this behaviour - this does fix the specific crash, but won't help with any other crashes of this type that haven't been discovered yet.
2. Add a more general if (WillDispose) return in Actor.Tick - this is a more general fix, but might cause regressions if we have any code that relied on activities ticking while being disposed.

[abcdefg30]

Updated as discussed on Discord (second push; first is a rebase): We go with 1) for now and do 2) for next+1.

[pchote]

https://github.com/OpenRA/OpenRA/wiki/Changelog-(bleed)/_compare/5e51a9f71ad079ea21fc0f073fb2ad355de48d98

[reaperrr]

The problem is that actors may run their activity in the time after being killed but before the end-tick disposal cleanup happening (IsDead: true, WillDispose: true, Disposed: false). The carryall (ActorID 900) is killed at the start of the tick, and its death handler disposes Carryable and sets it to null. The activity then runs and crashes due to the null actor.

As far as I can tell, the problem is that player interfaces like INotifyWinStateChanged.OnPlayerLost don't adhere to the implicit order that the simulation code is designed around (activities -> traits -> effects), messing up the order in which the simulation code would expect things to happen. For example by triggering INotifyOwnerLost.OnOwnerLost in OwnerLostAction before any of the activities or regular trait stuff happens.

In my opinion the most robust fix would be to simply not allow killing actors before Actor.Tick() at all. OwnerLostAction and comparable traits that implement player interfaces probably should not directly affect actors until ITick.Tick.