Skip to content

[GEN][ZH] Fix crash in StateMachine when a chained StateMachine update deletes the executor StateMachine#890

Merged
xezon merged 2 commits into
TheSuperHackers:mainfrom
xezon:xezon/fix-statemachine-crash
May 28, 2025
Merged

[GEN][ZH] Fix crash in StateMachine when a chained StateMachine update deletes the executor StateMachine#890
xezon merged 2 commits into
TheSuperHackers:mainfrom
xezon:xezon/fix-statemachine-crash

Conversation

@xezon

@xezon xezon commented May 20, 2025

Copy link
Copy Markdown

This change fixes a crash in StateMachine that happens when a chained StateMachine update destroys the prior executor StateMachine. This issue happened with #212, where the Ranger's StateMachine in the Bus would kill the Power Plant, and then the Power Plant weapon would kill the Bus, which would then delete the StateMachine of the Ranger.

@xezon xezon added this to the Stability fixes milestone May 20, 2025
@xezon xezon added Bug Something is not working right, typically is user facing Critical Severity: Minor < Major < Critical < Blocker Gen Relates to Generals ZH Relates to Zero Hour Crash This is a crash, very bad labels May 20, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This header was never used... so FAIL_IF failed to compile. No clue what that was supposed to be.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am guessing it was some older form of custom assert like the debug asserts.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be good to put a WWASSERT here

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@Mauller

Mauller commented May 21, 2025

Copy link
Copy Markdown

I wonder if this behaviour is also why i see leaked Ai states at times.

This will need some further testing for compat. It will definitely break compat in the situation it fixes, but then the vanilla game crashes at that point anyway.

@xezon

xezon commented May 21, 2025

Copy link
Copy Markdown
Author

I think this fix does not break compat at all. It does not change what the StateMachine does.

@Mauller

Mauller commented May 21, 2025

Copy link
Copy Markdown

I think this fix does not break compat at all. It does not change what the StateMachine does.

It probably doesn't, i was just considering if there are any other silent issues surrounding this same problem that affect the transfer still.

But the parent state machine should still be destroyed in the same frame so probably doesn't affect the transfer.

This fix just made me think of the fix for the garrison contain initialisation. That fix caused a mismatch in the end since it changed initial contain behaviour from being multi to single framed.

@xezon xezon force-pushed the xezon/fix-statemachine-crash branch from 98521c6 to 4addf04 Compare May 21, 2025 16:37

@Mauller Mauller left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, i tested VC6 with GR1 and it was fine.

I also tested without the fix and could replicate the issue with a battlebus filled with low health rebels, a humvee didn't seem to cause the same issue. So this must have some unique code path due to how the battlebus dies.

Tested locally with fix and all looked good.

@Mauller

Mauller commented May 21, 2025

Copy link
Copy Markdown

I think in the future we should ref count all states and state machines or have some form of dirty flag a state can set that can tell a state machine to clean itself up at the end of it's update.

@xezon

xezon commented May 21, 2025

Copy link
Copy Markdown
Author

I looked at State class, and I think these need no attention, because they are childs of StateMachine and bound to its life time. So as long as StateMachine lives, then so will all its contained States.

Comment on lines 487 to 489

@Mauller Mauller May 22, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be called "machine" rather than state to match the convention used within states.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@Mauller Mauller force-pushed the xezon/fix-statemachine-crash branch from 4addf04 to f2165ac Compare May 25, 2025 18:31
@Mauller

Mauller commented May 25, 2025

Copy link
Copy Markdown

Rebased with recent main for testing purposes.

@Mauller

Mauller commented May 27, 2025

Copy link
Copy Markdown

@helmutbuhler Shared a replay that crashed the game, it appears that it was also due to an attacking battlebus being hit by a nuclear weapon.

This PR allowed the replay to end gracefully with the scorescreen appearing.

@xezon xezon force-pushed the xezon/fix-statemachine-crash branch from f2165ac to 44ae554 Compare May 28, 2025 17:41
@xezon xezon merged commit c56e24d into TheSuperHackers:main May 28, 2025
18 checks passed
@xezon xezon deleted the xezon/fix-statemachine-crash branch May 28, 2025 18:21
fbraz3 pushed a commit to fbraz3/GeneralsX that referenced this pull request Nov 10, 2025
fbraz3 pushed a commit to fbraz3/GeneralsX that referenced this pull request Feb 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Something is not working right, typically is user facing Crash This is a crash, very bad Critical Severity: Minor < Major < Critical < Blocker Gen Relates to Generals ZH Relates to Zero Hour

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Game will crash if Battle Bus with Rebels destroys Nuke Battle Masters

2 participants