bugfix(milesaudiomanager): Prevent dangling pointer to AudioEventRTS in PlayingAudio when handing it over to a new AudioRequest after a call to MilesAudioManager::startNextLoop()

[xezon]

Merge with Rebase

• Fixes Crash in MilesAudioManager::stopPlayingAudio() #2770
• Merge after fix(milesaudiomanager): Fix audio request kill condition in MilesAudioManager::killAudioEventImmediately() #2784

This change has 2 commits to work towards fixing race conditions in MilesAudioManager concerning a shared AudioEventRTS instance in classes AudioRequest and PlayingAudio.

The first commit implements a new RefCountMTClass which is fundamentally identical to RefCountClass, except it has a thread safe counter and all the debug functionality is omitted.

The first commit adds the RefCountMTClass RefCountClass to DynamicAudioEventRTS to allow for shared ownership. All existing users of DynamicAudioEventRTS accomodate it and will now use RefCountPtr for automatic reference counting.

The second commit replaces AudioEventRTS* with RefCountPtr<DynamicAudioEventRTS> in AudioRequest and PlayingAudio to allow sharing the audio event data between them. This is needed, because ownership will be shared in function MilesAudioManager::startNextLoop (or MilesAudioManager::stopPlayingAudio), where previously AudioRequest was given the sole authority to delete the AudioEventRTS while PlayingAudio still kept a pointer to it. Now both classes need to release their reference count before the audio event data is deleted.

This likely was also a problem in retail, because AudioEventRTS is heap allocated, not pool allocated.

TODO

• Add pull ids to commit titles
• Rename the second commit in regards to RefCountMTClass
• Rename the third commit because it is no longer accurate
• Fix condition in killAudioEventImmediately after fix(milesaudiomanager): Fix audio request kill condition in MilesAudioManager::killAudioEventImmediately() #2784 is merged
• Replicate in Generals

[greptile-apps]

Greptile Summary

This PR fixes a dangling pointer bug in MilesAudioManager::startNextLoop() where AudioRequest previously took sole ownership of the AudioEventRTS heap allocation while PlayingAudio still held a raw pointer to it. The fix introduces shared ownership by making DynamicAudioEventRTS inherit from RefCountClass and replacing all raw AudioEventRTS* / DynamicAudioEventRTS* holders in AudioRequest and PlayingAudio with RefCountPtr<DynamicAudioEventRTS>.

• DynamicAudioEventRTS now directly inherits from AudioEventRTS (removing the m_event wrapper member) and from RefCountClass, giving it proper shared-ownership semantics via RefCountPtr.
• The delayed-loop restart path in startNextLoop is reworked: instead of transferring raw pointer ownership to a new AudioRequest immediately (the root cause of the bug), it sets a m_rerequestOnNextUpdate flag and defers request creation to the main-thread processPlayingList call, at which point stopPlayingAudio atomically advances the status to PS_Stopped to prevent spurious rerequests if killAudioEventImmediately fires in between.
• Downstream callers across ThingTemplate, Drawable, PhysicsUpdate, GameSounds, GameMusic, and INI are updated to use RefCountPtr and the new flat DynamicAudioEventRTS interface.

Confidence Score: 5/5

The change is safe to merge; the shared-ownership refactor is mechanically consistent across all 23 files and the deferred-rerequest protocol correctly prevents the audio loop from firing after a kill.

The root cause (raw pointer transferred to AudioRequest while PlayingAudio kept a dangling copy) is eliminated by RefCountPtr throughout. The interlocked status CAS in stopPlayingAudio advancing PS_Stopping to PS_Stopped before processPlayingList can call rerequestPlayingAudioWhenSignalled is a correct guard. RefCountClass::operator= is a proper no-op so implicit copy-assignment of DynamicAudioEventRTS never corrupts the ref count. Ref count mutations happen exclusively on the main thread; the MSS timer thread only calls Peek(), so the non-atomic counter is safe. killAudioEventImmediately now correctly matches pending play requests via getPlayingHandle() and transitions stopping audio to PS_Stopped, both paths verified.

No files require special attention; the most complex logic is in MilesAudioManager.cpp around processPlayingList and startNextLoop, which has been carefully reviewed.

Important Files Changed

Filename	Overview
Core/GameEngine/Include/Common/AudioEventRTS.h	DynamicAudioEventRTS now directly inherits AudioEventRTS (removing the m_event wrapper) and RefCountClass; Delete_This() correctly routes deallocation through the memory pool.
Core/GameEngine/Include/Common/AudioRequest.h	Replaces the raw AudioEventRTS*/AudioHandle union with separate RefCountPtr m_pendingEvent and AudioHandle m_handleToInteractOn fields; adds an explicit constructor that zero-initialises both.
Core/GameEngineDevice/Source/MilesAudioDevice/MilesAudioManager.cpp	Core fix: PlayingAudio::m_audioEventRTS is now RefCountPtr; startNextLoop defers request creation to the main thread via m_rerequestOnNextUpdate; killAudioEventImmediately correctly uses getPlayingHandle() and stopPlayingAudio CAS to PS_Stopped blocks spurious rerequests.
Core/GameEngineDevice/Include/MilesAudioDevice/MilesAudioManager.h	Replaces m_cleanupAudioEventRTS with volatile Bool m_rerequestOnNextUpdate; adds rerequestPlayingAudio / rerequestPlayingAudioWhenSignalled declarations; PlayingAudio::m_audioEventRTS becomes RefCountPtr.
Core/GameEngine/Source/Common/Audio/GameAudio.cpp	addAudioEvent allocates DynamicAudioEventRTS via newInstance and wraps in RefCountPtr::Create_NoAddRef; removes manual releaseAudioEventRTS calls; properly handles both music and sound paths with RAII cleanup.
GeneralsMD/Code/GameEngine/Include/Common/ThingTemplate.h	AudioArray members converted from raw DynamicAudioEventRTS* to RefCountPtr; constructor/destructor simplified to defaults; copy-ctor/assign correctly deep-copy the underlying objects.
GeneralsMD/Code/GameEngine/Source/GameClient/Drawable.cpp	m_ambientSound converted to RefCountPtr; all manual deleteInstance/nullptr pairs replaced with .Clear(); direct m_event. member accesses removed since DynamicAudioEventRTS IS-A AudioEventRTS now.
Core/Libraries/Source/WWVegas/WWLib/ref_ptr.h	Minor fix: corrects include path from wwdebug.h to WWDebug/wwdebug.h; no semantic changes to the smart pointer implementation.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Main as Main Thread (processPlayingList)
    participant Timer as MSS Timer Thread (notifyOfAudioCompletion)
    participant PA as PlayingAudio
    participant Pool as AudioRequest Pool

    Note over Timer,PA: Audio loop ends - startNextLoop called on timer thread
    Timer->>PA: hasMoreLoops()?
    alt "has delay > 1 logic frame"
        Timer->>PA: "m_rerequestOnNextUpdate = true"
        Timer->>PA: InterlockedCAS(status, PS_Stopping, PS_Playing)
        Note over Timer: return (no AudioRequest created yet)
        Main->>PA: sees PS_Stopping
        Main->>PA: rerequestPlayingAudioWhenSignalled()
        PA-->>Pool: generateFilename() + allocate AudioRequest with shared ref
        Main->>PA: stopPlayingAudio() to PS_Stopped + releaseMilesHandles()
    else no delay
        Timer->>PA: generateFilename()
        Timer->>PA: playSample / playSample3D (same RefCountPtr)
    end

    Note over Main: killAudioEventImmediately path
    Main->>Pool: scan m_audioRequests via getPlayingHandle() match
    alt audio still in PS_Stopping list
        Main->>PA: stopPlayingAudio() CAS PS_Stopping to PS_Stopped
        Note over Main: processPlayingList sees PS_Stopped, skips rerequestPlayingAudioWhenSignalled
    end

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Main as Main Thread (processPlayingList)
    participant Timer as MSS Timer Thread (notifyOfAudioCompletion)
    participant PA as PlayingAudio
    participant Pool as AudioRequest Pool

    Note over Timer,PA: Audio loop ends - startNextLoop called on timer thread
    Timer->>PA: hasMoreLoops()?
    alt "has delay > 1 logic frame"
        Timer->>PA: "m_rerequestOnNextUpdate = true"
        Timer->>PA: InterlockedCAS(status, PS_Stopping, PS_Playing)
        Note over Timer: return (no AudioRequest created yet)
        Main->>PA: sees PS_Stopping
        Main->>PA: rerequestPlayingAudioWhenSignalled()
        PA-->>Pool: generateFilename() + allocate AudioRequest with shared ref
        Main->>PA: stopPlayingAudio() to PS_Stopped + releaseMilesHandles()
    else no delay
        Timer->>PA: generateFilename()
        Timer->>PA: playSample / playSample3D (same RefCountPtr)
    end

    Note over Main: killAudioEventImmediately path
    Main->>Pool: scan m_audioRequests via getPlayingHandle() match
    alt audio still in PS_Stopping list
        Main->>PA: stopPlayingAudio() CAS PS_Stopping to PS_Stopped
        Note over Main: processPlayingList sees PS_Stopped, skips rerequestPlayingAudioWhenSignalled
    end

Loading

_{Reviews (13): Last reviewed commit: "bugfix(milesaudiomanager): Prevent dangl..." | Re-trigger Greptile}

[xezon]

I revisited the implementation and added new fixup commits to simplify it, because I noticed that we can avoid adding the deferred audio requests container by using a new flag in PlayingAudio to tell main thread to create a new audio request when stopping the playing audio. This simplifies the whole thing.

The RefCountMTClass is now no longer used, but we can keep it anyway for future use cases.

[xezon]

Polished. Ready for review.

[githubawn]

I've been debugging this PR, and it sometimes softlocks when I exit near the factory and town in Alpine Assault. I didn't see anything strange in the debugger, but having a heightened camera increases the chances that it hangs. I think I found a race condition that explains it.

[githubawn]

Since processPlayingList() iterates without a lock, could a Miles background callback be modifying this object's state at the exact same time we read/overwrite it?

[xezon]

I don't think so, because this bool is set true only in that miles callback and the miles callback will not be called again.

[xezon]

I've been debugging this PR, and it sometimes softlocks when I exit near the factory and town in Alpine Assault. I didn't see anything strange in the debugger, but having a heightened camera increases the chances that it hangs. I think I found a race condition that explains it.

Break the debugger to see the callstack and where it hangs.

[Caball009]

I don't see where RefCountMTClass is used in this PR. Is this meant to be used in a follow-up PR?

Regardless, there are some thread safety issues with that class so I'd like to see that extracted to a separate PR.

[xezon]

I did use RefCountMTClass before. Then simplified code and it was no longer needed. I left it because it might be needed in future. I can remove it again.

[Caball009]

I did use RefCountMTClass before. Then simplified code and it was no longer needed.

Fair enough.

I left it because it might be needed in future. I can remove it again.

Please remove it from this PR at least. A reference counting class is quite tricky stuff for multi-threaded purposes. It should be added in a separate PR if we ever need it.

[xezon]

Please remove it from this PR at least.

First commit removed.

[xezon]

The bot had a lot of complaints here about my code. Pffft.

[Caball009]

What does "GameAudio.h" use from "mutex.h"?

[Caball009]

There are many cases of this throughout the code base. Perhaps this should be done in a separate PR?

[xezon]

This was needed here because this header was added to WWCommon.h

You are right this needs to be fixed at larger scale.

It is already split into a separate commit.

[Caball009]

Why doesn't this do m_audio[i] = that.m_audio[i];?

[xezon]

Because that would be not identical to what it did before.