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 by replacing raw AudioEventRTS* ownership in both PlayingAudio and AudioRequest with a shared RefCountPtr<DynamicAudioEventRTS>. DynamicAudioEventRTS is promoted to a first-class reference-counted type by adding RefCountClass as a base and overriding Delete_This() to use the memory pool.

• PlayingAudio::m_audioEventRTS and AudioRequest::m_pendingEvent are now RefCountPtr<DynamicAudioEventRTS>, preventing the use-after-free that occurred when startNextLoop handed the raw pointer to a new AudioRequest while PlayingAudio still held it.
• m_cleanupAudioEventRTS is removed; cleanup is now entirely automatic through ref-counting. A new volatile m_rerequestOnNextUpdate flag signals the main thread to re-queue the loop request via the new rerequestPlayingAudio / rerequestPlayingAudioWhenSignalled helpers.
• All callers of DynamicAudioEventRTS across ThingTemplate, Drawable, PhysicsBehavior, and INI are updated to use RefCountPtr for RAII ownership.

Confidence Score: 4/5

The core fix is mechanically sound and all ~30 call sites that touch m_audioEventRTS are updated consistently.

The concurrent design relies on x86 TSO ordering guarantees that are correct for the target platform but not expressed through standard synchronization primitives. No new P0 or P1 bugs were found beyond concerns already noted in previous review threads.

MilesAudioManager.cpp — the new rerequestPlayingAudio/rerequestPlayingAudioWhenSignalled helpers and their interaction with the PS_Stopping branch, particularly the double-stopPlayingAudio path that can arise when killLowestPrioritySoundImmediately and processPlayingList both act on the same entry.

Important Files Changed

Filename	Overview
Core/GameEngine/Include/Common/AudioEventRTS.h	DynamicAudioEventRTS now directly inherits AudioEventRTS and RefCountClass, removes the m_event member, adds forwarding constructors, and overrides Delete_This() for memory-pool correctness.
Core/GameEngineDevice/Include/MilesAudioDevice/MilesAudioManager.h	PlayingAudio updated: m_audioEventRTS is RefCountPtr, m_cleanupAudioEventRTS replaced with volatile m_rerequestOnNextUpdate; new helper declarations added.
Core/GameEngineDevice/Source/MilesAudioDevice/MilesAudioManager.cpp	Core fix: rerequestPlayingAudio/rerequestPlayingAudioWhenSignalled added; startNextLoop delay path signals the main thread instead of transferring raw ownership; killAudioEventImmediately and isCurrentlyPlaying correctly use req->m_pendingEvent non-null as the AR_Play discriminator.
Core/GameEngine/Include/Common/AudioRequest.h	Union replaced by separate m_pendingEvent (RefCountPtr) and m_handleToInteractOn fields; m_usePendingEvent removed; explicit constructor added initializing all members.
GeneralsMD/Code/GameEngine/Include/Common/ThingTemplate.h	AudioArray updated to hold RefCountPtr; copy-ctor allocates new instances; operator= re-uses existing in-place via compiler-generated copy-assign which correctly skips RefCountClass via its no-op operator=.
Core/GameEngine/Source/Common/Audio/GameAudio.cpp	addAudioEvent creates DynamicAudioEventRTS via newInstance with Create_NoAddRef; early-exit paths let the local RefCountPtr destructor handle cleanup automatically.
Core/GameEngine/Source/Common/Audio/GameSounds.cpp	addAudioEvent return type changed from void to Bool; Create_AddRef used when assigning to AudioRequest::m_pendingEvent. Correct.
GeneralsMD/Code/GameEngine/Source/GameClient/Drawable.cpp	m_ambientSound updated to RefCountPtr; all manual deleteInstance/nullptr assignments replaced with .Clear() calls; m_event. accesses replaced with direct member access.
Core/Libraries/Source/WWVegas/WWLib/ref_ptr.h	Only change is fixing the wwdebug.h include path to use the subdirectory prefix. No functional changes.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant MT as Main Thread
    participant PA as PlayingAudio
    participant DAERTS as DynamicAudioEventRTS (refcount)
    participant AR as AudioRequest
    participant MSS as MSS Timer Thread

    Note over PA,DAERTS: audio starts - refcount=1
    PA->>DAERTS: "m_audioEventRTS RefCountPtr rc=1"

    MSS->>PA: "notifyOfAudioCompletion -> startNextLoop"
    Note over MSS: getDelay() > threshold
    MSS->>PA: "m_rerequestOnNextUpdate = true"
    MSS->>PA: "InterlockedCmpXchg -> PS_Stopping"

    MT->>PA: processPlayingList sees PS_Stopping
    MT->>MT: rerequestPlayingAudio(playing)
    MT->>AR: "req->m_pendingEvent = playing->m_audioEventRTS"
    Note over DAERTS: refcount = 2 (PA + AR)
    MT->>PA: "stopPlayingAudio -> releaseMilesHandles"
    MT->>PA: "releasePlayingAudio -> delete playing"
    Note over DAERTS: refcount = 1 (only AR)

    MT->>AR: "processRequest -> playAudioEvent"
    AR->>PA: "new PlayingAudio m_audioEventRTS = req->m_pendingEvent"
    Note over DAERTS: refcount = 2 (new PA + AR)
    MT->>AR: deleteInstance(req)
    Note over DAERTS: refcount = 1 (new PA only)

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 MT as Main Thread
    participant PA as PlayingAudio
    participant DAERTS as DynamicAudioEventRTS (refcount)
    participant AR as AudioRequest
    participant MSS as MSS Timer Thread

    Note over PA,DAERTS: audio starts - refcount=1
    PA->>DAERTS: "m_audioEventRTS RefCountPtr rc=1"

    MSS->>PA: "notifyOfAudioCompletion -> startNextLoop"
    Note over MSS: getDelay() > threshold
    MSS->>PA: "m_rerequestOnNextUpdate = true"
    MSS->>PA: "InterlockedCmpXchg -> PS_Stopping"

    MT->>PA: processPlayingList sees PS_Stopping
    MT->>MT: rerequestPlayingAudio(playing)
    MT->>AR: "req->m_pendingEvent = playing->m_audioEventRTS"
    Note over DAERTS: refcount = 2 (PA + AR)
    MT->>PA: "stopPlayingAudio -> releaseMilesHandles"
    MT->>PA: "releasePlayingAudio -> delete playing"
    Note over DAERTS: refcount = 1 (only AR)

    MT->>AR: "processRequest -> playAudioEvent"
    AR->>PA: "new PlayingAudio m_audioEventRTS = req->m_pendingEvent"
    Note over DAERTS: refcount = 2 (new PA + AR)
    MT->>AR: deleteInstance(req)
    Note over DAERTS: refcount = 1 (new PA only)

Loading

_{Reviews (14): 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]

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

[xezon]

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