Prevent Blocked Crates from Crashing the Game

[atlimit8]

Prevent reading not yet cached Actor.Crushable() in Crate ctor using HierarchicalPathFinder.ActorIsBlocking(Actor actor).

The crash only occurs if the crate might be blocked. Otherwise, crates land just fine. In bleed (4e031a6) always crashes on the first crate when I play "Island Duel" in td.

Test Mod: td
Test Map: Island Duel

Line:

			foreach (var crushable in actor.Crushables)

Stack trace:
OpenRA.Mods.Common.dll!OpenRA.Mods.Common.Pathfinder.HierarchicalPathFinder.ActorIsBlocking(OpenRA.Actor actor) Line 660 (OpenRA.Mods.Common/Pathfinder/HierarchicalPathFinder.cs:660) OpenRA.Mods.Common.dll!OpenRA.Mods.Common.Pathfinder.HierarchicalPathFinder.RequireBlockingRefreshInCell(OpenRA.CPos cell) Line 607 (OpenRA.Mods.Common/Pathfinder/HierarchicalPathFinder.cs:607) OpenRA.Mods.Common.dll!OpenRA.Mods.Common.Traits.ActorMap.AddInfluence(OpenRA.Actor self, OpenRA.Traits.IOccupySpace ios) Line 428 (OpenRA.Mods.Common/Traits/World/ActorMap.cs:428) OpenRA.Mods.Common.dll!OpenRA.Mods.Common.Traits.Crate.SetLocation(OpenRA.Actor self, OpenRA.CPos cell) Line 224 (OpenRA.Mods.Common/Traits/Crates/Crate.cs:224) OpenRA.Mods.Common.dll!OpenRA.Mods.Common.Traits.Crate.SetPosition(OpenRA.Actor self, OpenRA.CPos cell, OpenRA.Traits.SubCell subCell) Line 203 (OpenRA.Mods.Common/Traits/Crates/Crate.cs:203) OpenRA.Mods.Common.dll!OpenRA.Mods.Common.Traits.Crate.Crate(OpenRA.ActorInitializer init, OpenRA.Mods.Common.Traits.CrateInfo info) Line 94 (OpenRA.Mods.Common/Traits/Crates/Crate.cs:94) OpenRA.Mods.Common.dll!OpenRA.Mods.Common.Traits.CrateInfo.Create(OpenRA.ActorInitializer init) Line 33 (OpenRA.Mods.Common/Traits/Crates/Crate.cs:33) OpenRA.Game.dll!OpenRA.Actor.Actor(OpenRA.World world, string name, OpenRA.Primitives.TypeDictionary initDict) Line 163 (OpenRA.Game/Actor.cs:163) OpenRA.Game.dll!OpenRA.World.CreateActor(bool addToWorld, string name, OpenRA.Primitives.TypeDictionary initDict) Line 339 (OpenRA.Game/World.cs:339) OpenRA.Game.dll!OpenRA.World.CreateActor(string name, OpenRA.Primitives.TypeDictionary initDict) Line 329 (OpenRA.Game/World.cs:329) OpenRA.Mods.Common.dll!OpenRA.Mods.Common.Traits.CrateSpawner.SpawnCrate.AnonymousMethod__0(OpenRA.World w) Line 168 (OpenRA.Mods.Common/Traits/World/CrateSpawner.cs:168) OpenRA.Game.dll!OpenRA.World.Tick() Line 464 (OpenRA.Game/World.cs:464) OpenRA.Game.dll!OpenRA.Game.InnerLogicTick(OpenRA.Network.OrderManager orderManager) Line 634 (OpenRA.Game/Game.cs:634) OpenRA.Game.dll!OpenRA.Game.LogicTick() Line 658 (OpenRA.Game/Game.cs:658) OpenRA.Game.dll!OpenRA.Game.Loop() Line 830 (OpenRA.Game/Game.cs:830) OpenRA.Game.dll!OpenRA.Game.Run() Line 883 (OpenRA.Game/Game.cs:883) OpenRA.Game.dll!OpenRA.Game.InitializeAndRun(string[] args) Line 313 (OpenRA.Game/Game.cs:313) OpenRA.dll!OpenRA.Launcher.Program.Main(string[] args) Line 26 (OpenRA.Launcher/Program.cs:26) [External Code] (Unknown Source:0)

My solution is to simply use Crate.Location to store the location in init.GetOrDefault<LocationInit>() and then call SetPosition(self, Location) in INotifyCreated.Created(Actor self).

Fetching actor traits will always be unreliable from trait constructors since the actor is under construction too.

[RoosterDragon]

It looks like other Traits could(?) run into similar problems: TDGunboat, Aircraft, Mobile. I wonder if it's be better to do Crushables = TraitsImplementing<ICrushable>(); just before the foreach (var traitInfo in Info.TraitsInConstructOrder()) loop in the Actor ctor. This would allow any traits to get crushables during actor construction. Then we can still keep Crushables = crushablesList.ToArray(); at the end so we've got a cached result to rely on after actor creation.

[LipkeGu]

i fixed it by using #21332

[atlimit8]

@RoosterDragon Even if null checks were placed in all the the trait lookup code that might be called, the traits will not be constructed if they are created after the trait with the offending constructor. This is a case of hot path code looking up traits being called in the trait constructor. Only making sure that all accessed traits are marked as required for the trait under construction will guarantee that the code will work as it would after all of the traits are constructed.

We might have to place rules or guidelines for what can be called in trait constructors since aside from requiring traits, Require<TTraitInfo> is the only way to have traits available and that does not guarantee that its methods will not attempt to access traits that have not been created yet. That is the reason I created INotifyCreated back in #6014, to perform trait initialization that requires other traits to be available.

I prepended (rebased) a commit that throws in HierarchicalPathFinder.ActorIsBlocking(Actor actor) if actor.Crushables == null with the message $"Actor {actor} is still in the process of being created. Cannot determine if it is blocking." to make such an error clear. I made it the first commit to make it easier to test.

I also added another commit that has the same check in Aircraft.IsBlockedBy(Actor self, Actor otherActor, Actor ignoreActor, bool blockedByMobile = true) and Locomotor.IsBlockedBy(Actor actor, Actor otherActor, Actor ignoreActor, CPos cell, BlockedByActor check, CellFlag cellFlag) and added the trait type name in the error messages. Those changes may cause more crashes; however, those crashes would expose bugs. While those issues could be written to a log instead, the silence would prevent reports of those bugs.

[RoosterDragon]

Good point, we need the construction complete as well.

Can we move the throw-if-null check into the Crushables getter of Actor instead? That would avoid having to scatter checks everywhere.

[PunkPun]

I agree that the checks feel out of place. I recon instead of this we should have unit tests that actually run the game, then with a feature-full testmap we could detect these more easily. The unit tests should also cover all missions, cause we are never manually reviewing them all.

I suppose a separate issue should be opened for this

[atlimit8]

I replaced the scattered checks with a single null check in ICrushable[] Crushables { get }.

[PunkPun]

changelog