Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
xezon merged 2 commits into from
May 28, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
942d3a6
[GEN][ZH] Fix crash in StateMachine when a chained StateMachine updat…
xezon May 20, 2025
44ae554
Fix review comments
xezon May 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions Core/Libraries/Source/WWVegas/WWLib/ref_ptr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,8 @@
};
*/

#define ALLOW_AUTOMATIC_REF_COUNT_PTR_CONSTRUCTION

class DummyPtrType;

template <class T>
Expand Down Expand Up @@ -416,7 +418,8 @@ bool operator <(const RefCountPtr<LHS> & lhs, const RefCountPtr<RHS> & rhs)
template <class RHS>
bool operator ==(DummyPtrType * dummy, const RefCountPtr<RHS> & rhs)
{
FAIL_IF(0 != dummy) {
if (0 != dummy) {
WWASSERT(0);
return false;
}

Expand All @@ -428,7 +431,8 @@ bool operator ==(DummyPtrType * dummy, const RefCountPtr<RHS> & rhs)
template <class RHS>
bool operator !=(DummyPtrType * dummy, const RefCountPtr<RHS> & rhs)
{
FAIL_IF(0 != dummy) {
if (0 != dummy) {

@xezon xezon 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.

@Mauller Mauller May 21, 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.

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

@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.

Might be good to put a WWASSERT here

@xezon xezon May 28, 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.

Fixed.

WWASSERT(0);
return true;
}

Expand Down
19 changes: 18 additions & 1 deletion Generals/Code/GameEngine/Include/Common/StateMachine.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@
#include "Common/Snapshot.h"
#include "Common/Xfer.h"

#include "refcount.h"

//-----------------------------------------------------------------------------
//-----------------------------------------------------------------------------
//-----------------------------------------------------------------------------
Expand Down Expand Up @@ -236,7 +238,7 @@ inline State::~State() { }
/**
* A finite state machine.
*/
class StateMachine : public MemoryPoolObject, public Snapshot
class StateMachine : public MemoryPoolObject, public Snapshot, public RefCountClass
{
MEMORY_POOL_GLUE_WITH_USERLOOKUP_CREATE( StateMachine, "StateMachinePool" );

Expand Down Expand Up @@ -340,6 +342,12 @@ class StateMachine : public MemoryPoolObject, public Snapshot
virtual void xfer( Xfer *xfer );
virtual void loadPostProcess();

// RefCountClass interface
virtual void Delete_This()
{
MemoryPoolObject::deleteInstanceInternal(this);
}

protected:

/**
Expand Down Expand Up @@ -473,4 +481,13 @@ class SleepState : public State
EMPTY_DTOR(SleepState)


//-----------------------------------------------------------------------------------------------------------
// TheSuperHackers @info Misappropriates deleteInstance to call Release_Ref to keep the StateMachine fix small.
// @todo Replace calls to deleteInstance with RefCountPtr<StateMachine> when so appropriate.
inline void deleteInstance(StateMachine* machine)
{
machine->Release_Ref();
}


#endif // _STATE_MACHINE_H_
10 changes: 9 additions & 1 deletion Generals/Code/GameEngine/Source/Common/StateMachine.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@
#include "GameLogic/GameLogic.h"
#include "GameLogic/Object.h"

#include "ref_ptr.h"

#ifdef RTS_INTERNAL
// for occasional debugging...

Expand Down Expand Up @@ -433,13 +435,19 @@ StateReturnType StateMachine::updateStateMachine()

if (m_currentState)
{
// TheSuperHackers @bugfix xezon 20/05/2025 Defer the deletion of this state machine.
// Calling m_currentState->update() can release this state machine in certain circumstances,
// for example if something kills the entity of this state machine as a result of this state update.
// See https://github.com/TheSuperHackers/GeneralsGameCode/issues/212
RefCountPtr<StateMachine> refThis(this);

// update() can change m_currentState, so save it for a moment...
State* stateBeforeUpdate = m_currentState;

// execute this state
StateReturnType status = m_currentState->update();

// it is possible that the state's update() method may cause the state to be destroyed
// it is possible that the state's update() method clears the state machine.
if (m_currentState == NULL)
{
return STATE_FAILURE;
Expand Down
19 changes: 18 additions & 1 deletion GeneralsMD/Code/GameEngine/Include/Common/StateMachine.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@
#include "Common/Snapshot.h"
#include "Common/Xfer.h"

#include "refcount.h"

//-----------------------------------------------------------------------------
//-----------------------------------------------------------------------------
//-----------------------------------------------------------------------------
Expand Down Expand Up @@ -237,7 +239,7 @@ inline State::~State() { }
/**
* A finite state machine.
*/
class StateMachine : public MemoryPoolObject, public Snapshot
class StateMachine : public MemoryPoolObject, public Snapshot, public RefCountClass
{
MEMORY_POOL_GLUE_WITH_USERLOOKUP_CREATE( StateMachine, "StateMachinePool" );

Expand Down Expand Up @@ -342,6 +344,12 @@ class StateMachine : public MemoryPoolObject, public Snapshot
virtual void xfer( Xfer *xfer );
virtual void loadPostProcess();

// RefCountClass interface
virtual void Delete_This()
{
MemoryPoolObject::deleteInstanceInternal(this);
}

protected:

/**
Expand Down Expand Up @@ -475,4 +483,13 @@ class SleepState : public State
EMPTY_DTOR(SleepState)


//-----------------------------------------------------------------------------------------------------------
// TheSuperHackers @info Misappropriates deleteInstance to call Release_Ref to keep the StateMachine fix small.
// @todo Replace calls to deleteInstance with RefCountPtr<StateMachine> when so appropriate.
inline void deleteInstance(StateMachine* machine)
{
machine->Release_Ref();
}


#endif // _STATE_MACHINE_H_
10 changes: 9 additions & 1 deletion GeneralsMD/Code/GameEngine/Source/Common/StateMachine.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@
#include "GameLogic/GameLogic.h"
#include "GameLogic/Object.h"

#include "ref_ptr.h"

#ifdef RTS_INTERNAL
// for occasional debugging...

Expand Down Expand Up @@ -433,13 +435,19 @@ StateReturnType StateMachine::updateStateMachine()

if (m_currentState)
{
// TheSuperHackers @bugfix xezon 20/05/2025 Defer the deletion of this state machine.
// Calling m_currentState->update() can release this state machine in certain circumstances,
// for example if something kills the entity of this state machine as a result of this state update.
// See https://github.com/TheSuperHackers/GeneralsGameCode/issues/212
RefCountPtr<StateMachine> refThis(this);

// update() can change m_currentState, so save it for a moment...
State* stateBeforeUpdate = m_currentState;

// execute this state
StateReturnType status = m_currentState->update();

// it is possible that the state's update() method may cause the state to be destroyed
// it is possible that the state's update() method clears the state machine.
if (m_currentState == NULL)
{
return STATE_FAILURE;
Expand Down
Loading