setElementModel - memory leaking

ds1-e commented 2 years ago

Describe the bug

Calling setElementModel would cause GTA process to claim memory and never free it. It happens for peds, didn't tested other types though.

Steps to reproduce

local playerSkins = {11, 9, 7}
local skinsCount = #playerSkins

function changeSkin()
    local randomSkin = math.random(1, skinsCount)
    local newSkin = playerSkins[randomSkin]

    setElementModel(localPlayer, newSkin)
end
setTimer(changeSkin, 1, 0)

Run this code and observe GTA process memory usage in task manager.

Version

Client/server: Multi Theft Auto v1.5.9-release-21261

Additional context

/

Relevant log output

No response

Security Policy

[X] I have read and understood the Security Policy and this issue is not security related.

PlatinMTA commented 2 years ago

I can reproduce this. A few things to note:

Restarting the resource does not clean up the memory.
Making the loop stop does not clean up the memory.
Using /showmemstat shows that only the Process Memory is going up.
The problem only occurs with peds,players. Tried with vehicles and objects and it doesn't happen with them.
Having the ped streamed off does seem to make the leak stop (but that could be because the ped I tried was clientsided and their model was being changed clientside aswell).
Disconnecting from the server does not clean up the memory (also I crashed with Offset = 0x00349EE4 after re-joining).

Lpsd commented 2 years ago

I did a little investigation on this and it seems somehow related to CAnimBlockSA - you can find the same by using VS 2022 memory profiling tools, creating various snapshots with & without the test script running and comparing object counts.

From a very quick glance at the code, suspicion is that we are losing reference to old CAnimBlockSA objects when a new model is applied to a ped, and therefore never destroying them when necessary?

Lpsd commented 2 years ago

Seems like this produces the same results too

local peds = {}
local playerSkins = {11, 9, 7}
local skinsCount = #playerSkins

function spawnPeds()
    local randomSkin = math.random(1, skinsCount)
    local newSkin = playerSkins[randomSkin]
    local x, y, z = getElementPosition(localPlayer)

    peds[#peds+1] = createPed(newSkin, x + math.random(5), y + math.random(5), z + math.random())

    if (#peds > 5) then
        local oldPed = table.remove(peds, 1)
        destroyElement(oldPed)
    end
end
setTimer(spawnPeds, 1, 0)

setElementModel actually causes ped to be destroyed and created again. I tried this to confirm it's something to do with that process & CAnimBlockSA (as mentioned above) - memory profiling seems to suggest it's the same issue.

JeViCo commented 2 years ago

Using setElementModel increases vehicle count in model cache section (showmemstat) up to it's cap and stays there forever. I'm not sure that it works as expected and it might be related to thia issue. I'm able to reproduce it on latest nightly build (client & server) using default admin panel resource with default SA vehicles.

Dutchman101 commented 1 year ago

Alleged culprits: https://github.com/multitheftauto/mtasa-blue/blob/08cd7480da6a2d5b4f00f1978d7ddc8c915b3206/Client/mods/deathmatch/logic/CClientPed.cpp#L3930

https://github.com/multitheftauto/mtasa-blue/blob/08cd7480da6a2d5b4f00f1978d7ddc8c915b3206/Client/mods/deathmatch/logic/CClientPed.cpp#L3922

Dutchman101 — Today at 2:39 PM [issue link] We need someone with enough GTA internals knownledge for this, but there's not many

TEDERIs — Today at 3:09 PM This is an old well known leak. And it was intentially allowed by commenting out some of code lines in the MTA source(see NO_CRASH_FIX_TEST and NO_CRASH_FIX_TEST2 macro). It seems that releasing ped geometries led to crashes in the past. When I was working on fork, we had implemented a garbage collector for this situations. But of course this is only a half measure. We're not using GTA models at all now, but I will try to look into it later.

Dutchman101 — Today at 3:10 PM oh, and how about other models like vehicle?

TEDERIs — Today at 3:41 PM I think in case of vehicles we have a similar problem with references. It should be checked in first of all. But it's just an assumption.

Pirulax commented 1 year ago

The commented out code seems to suggest that it only happens when the model is changed from CJ to a non-CJ model or vice versa, no?

tederis commented 1 year ago

The commented out code seems to suggest that it only happens when the model is changed from CJ to a non-CJ model or vice versa, no?

A simple check tells that this happens with all models. You can remove NO_CRASH_FIX_TEST directives and see that the leak no longer bothers(at least, the ref counting works as it should).

FileEX commented 4 weeks ago

Is there anything more known about the crash that occurs after decrementing extra reference geometry? I commented out NO_CRASH_FIX_TEST and NO_CRASH_FIX_TEST2 and I have been running this code for 10 minutes and there is no crash

crun local skins = {11,9,7} setTimer(function() local skin = math.random(1,#skins) setElementModel(localPlayer, skins[skin]) end, 1, 0)

multitheftauto / mtasa-blue