bugfix(contain): Restore retail compatibility after crash fix in OpenContain::killAllContained

[Caball009]

• Closes Restore retail compatibility after crash fix in OpenContain::killAllContained #2215
• Follow up for [ZH] Fix crash in OpenContain::killAllContained() when killed occupants kill the host container #921

The way the crash fix was implemented in #921 assumes that the logic behavior doesn't change if m_containList / m_containListSize is already empty with the first iteration. This is not the case in the attached replay in #2215.

If m_containList / m_containListSize is empty, it'll make garrisoned civilian buildings unoccupied on the first iteration instead of the last iteration, and the GLA Demolition upgrade would cause damage to the building for each occupant instead of just the last one when hit with a neutron shell.

garrisoned_gla_demolition_upgrade_neutron_shell_incorrect.mp4

garrisoned_gla_demolition_upgrade_neutron_shell_correct.mp4

Pay attention to the health of the civilian building. In the correct version, only the detonation of the last GLA RPG Trooper causes damage instead of every one.

TODO:

• Polish comments.

[greptile-apps]

Greptile Summary

This PR fixes a behaviour regression introduced by PR #921's crash fix in OpenContain::killAllContained. PR #921 guarded against recursive list modification by swapping m_containList out immediately, but that caused GarrisonContain::onRemoving to always see an empty list, making every occupant appear to be the "last" one. This broke civilian-building garrison ownership and made the GLA Demolition upgrade deal full damage for every occupant instead of only the last.

The new approach erase-before-notify: each element is erased from m_containList (and m_containListSize is decremented) before onRemoving is called, so the callback sees the correct shrinking list size. After rider->kill(), the iterator is unconditionally reset to m_containList.begin(), which is safe regardless of whether kill() recurses into the same function or not. A final safety clear() and m_containListSize = 0 guard against any unexpected leftover state.

Key points:

• The iterator invalidation concern raised on PR [ZH] Fix crash in OpenContain::killAllContained() when killed occupants kill the host container #921 is now fully resolved: it is never used between erase(it) and it = m_containList.begin().
• The sequential order (erase → onRemoving → onRemovedFrom → kill() → reset iterator) correctly mirrors the pattern already used in harmAndForceExitAllContained.
• Null elements (should never occur in practice) are skipped with ++it and cleaned up by the final clear(); m_containListSize is reset to 0 unconditionally, so no size mismatch persists.

Confidence Score: 4/5

• This PR is safe to merge; it correctly restores intended retail behaviour while preserving the original crash fix's safety guarantees.
• The logic is sound: erasing each element before invoking callbacks gives onRemoving the correct list size at every step, and resetting the iterator to begin() after kill() handles all recursive-drain scenarios without touching an invalidated iterator. The final clear() / m_containListSize = 0 are harmless safety resets. No custom instruction violations were found in the changed lines.
• No files require special attention.

Important Files Changed

Filename	Overview
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp	Reverts the swap-to-empty approach from PR #921 and replaces it with an in-place erase-then-reset-iterator loop, so that onRemoving sees the live (shrinking) list size during each iteration — restoring correct "last occupant" semantics for GarrisonContain. Logic is sound; null-element cleanup is deferred to the final safety clear().

Sequence Diagram

sequenceDiagram
    participant KC as killAllContained
    participant CL as m_containList
    participant OC as onRemoving
    participant RD as rider

    KC->>CL: it = begin()
    loop while it != end()
        KC->>CL: rider = *it
        KC->>CL: erase(it) / --m_containListSize
        KC->>OC: onRemoving(rider)
        Note over OC: Sees updated list size<br/>(correct "last occupant" check)
        KC->>RD: onRemovedFrom(container)
        KC->>RD: kill()
        Note over KC,RD: kill() may recursively<br/>drain m_containList
        KC->>CL: it = begin()
    end
    KC->>CL: ASSERT empty, then clear() / size=0

Loading

_{Last reviewed commit: "Updated comments."}

[Mauller]

Does this still prevent the crashing with tech terrors?

[Caball009]

Does this still prevent the crashing with tech terrors?

Yes, the original crash is still fixed. I made sure of that, and also created a reproduction of it in the crash fix PR.

[Skyaero42]

This looks correct to me.

[Mauller]

Looks good to me too

[xezon]

Logic looks plausible. Comments need polishing.

[xezon]

Too much additional information in my opinion.

[Caball009]

I removed most of it.

[xezon]

I suggest merge this description with the one above.

[Caball009]

I could use some suggestions as to what information you want to retain from your own comment.

[xezon]

The important part is the repro and the technical error it caused.

[Caball009]

Updated the comments.

I got rid of the old 'Neutron Shells + GLA Technical containing GLA Terrorists' issue reproduction description that seems too detailed to me.