Skip to content

[GEN][ZH] Fix freeing reference-counted-pointer based memory leaks. #623

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.

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

Mauller
Copy link

@Mauller Mauller commented Apr 7, 2025

Merge with Rebase

Fix freeing some reference counted pointers so that underlying memory is properly freed.

Will be checking for more so left this as draft for the moment.

Fixes:

  • Gen + ZH - W3DLaserDraw was leaking materials.
  • Gen + ZH - Production update was leaking production queue items if game exited before buildings sold or destroyed.
  • ZH - Snow system was leaking on shutdown, wouldn't affect runtime but means a clean shutdown.
  • Gen + ZH - W3DTankDraw was leaking particle systems, therefore leaking particles as well.
  • Gen + ZH - MeshModel had an extra call to free directx8 references in the wrong place, this could cause other dependent assets to not be deallocated properly within reset(). And Generals behaviour was freeing data in the wrong order.
  • Gen - Fix W3DWaypointBuffer not correctly handling it's materials internally, also matched and synchronised memory and initialisation related code changes to ZH. Resolved in another commit so removed other changes for now.
  • Gen + ZH - Win32LocalFileSystem was leaking Win32File handlers due to creating a handler and returning early on a null filename before deleting it. Moved the creation of the file handler after the early return and where it is needed / used.
  • Gen - MeshModel class was calling the wrong delete method when deallocating GapFiller data. Fixed already in ZH.
  • Gen + ZH - Win32BigFileSystem was leaking Big32File handlers due to creating a handler and returning early on a null filename before deleting it. Moved the creation of the file handler after the early return and where it is needed / used.
  • Gen + ZH - W3DDisplay was not releasing its debug display string so it could prevent DisplayStringManager from being cleanly destroyed.

@Mauller Mauller self-assigned this Apr 7, 2025
@Mauller Mauller added Major Severity: Minor < Major < Critical < Blocker ZeroHour Relates to Zero Hour Memory Is memory related Generals Related Generals only Stability Concerns stability of the runtime labels Apr 7, 2025
@Mauller
Copy link
Author

Mauller commented Apr 13, 2025

Small update, found that production items in a production queue could leak if there were production queues in progress.
This would happen if a game was exited or finished before the queue completed.

Selling or destroying the building appeared to deallocate the queue normally.

@Mauller Mauller force-pushed the fix-memory-leaks-genzh branch from 3da01e7 to 269babe Compare April 16, 2025 16:07
@Mauller
Copy link
Author

Mauller commented Apr 16, 2025

Rebased with Main and added a new leak fix, Tank like vehicles were leaking particle systems when their W3DTankDraw object was destroyed.

@Mauller
Copy link
Author

Mauller commented Apr 20, 2025

Small update where some references could be removed too early and some W3D asset manager glue was missing on mech models.

@Mauller Mauller force-pushed the fix-memory-leaks-genzh branch 2 times, most recently from 79535d8 to f483373 Compare April 26, 2025 07:26
@Mauller Mauller marked this pull request as ready for review April 26, 2025 21:49
@Mauller Mauller force-pushed the fix-memory-leaks-genzh branch 2 times, most recently from 21a8f33 to 2fe6a0d Compare April 27, 2025 09:59
@Mauller
Copy link
Author

Mauller commented Apr 27, 2025

Removed the MeshGeometryClass change as it was not necessary in the end.
Removed the W3DWaypoints change as i think the main thing i was originaly fixing in generals variant was fixed with a backport by Tomsons26.

The rest should be good to go now.

@tomsons26
Copy link

tomsons26 commented Apr 28, 2025

DisplayString instances may be leaking all over
neither m_benchmarkDisplayString or m_nativeDebugDisplay are even nulled in dtor..
m_displayStrings is just nulled but isn't deallocated..

assert( m_stringList == NULL ); will throw in DisplayStringManager DTOR if you play around with debug keys and exit game, appears to be m_benchmarkDisplayString instance it's holding on to as it contains "FPS 31.25"

@Mauller Mauller force-pushed the fix-memory-leaks-genzh branch from 2fe6a0d to ed3592c Compare April 28, 2025 19:49
@Mauller
Copy link
Author

Mauller commented Apr 28, 2025

Rebased with Main after the recent inclusions. Just need to look at what Tomson26 found today.

@xezon xezon added this to the Stability fixes milestone Apr 28, 2025
@Mauller Mauller force-pushed the fix-memory-leaks-genzh branch from 83c9d86 to 1275b51 Compare May 1, 2025 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Generals Related Generals only Major Severity: Minor < Major < Critical < Blocker Memory Is memory related Stability Concerns stability of the runtime ZeroHour Relates to Zero Hour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants