Fix xmlLoadString memory leak

[Lpsd]

Resolves #1373

[ghost]

Thanks for the PR, mate.

[qaisjp]

Possible to incorporate smart pointers?

[Pirulax]

Well, kinda? But I woudn't, because it woudnt be proper use of them IMHO. Lua itself won't hold the reference count up, so its kinda mix and match.

[Lpsd]

Possible to incorporate smart pointers?

I'd have to actually learn about smart pointers - eek!

If someone else wants to add that to this PR very, very soon then go ahead - otherwise I think we should just merge as is (1.5.8 overdue!)

[ghost]

Does this really fix the memory leak? The memory gets freed when the destructor of CLuaMain is called, and CLuaMain is only called when the client exits the server. So if the client never leaves the server, then the memory will keep on stacking up.

[Lpsd]

I monitored the memory usage on both the client and server using the following script (edit: just added an updated version which appears to work the same)

local xml = {}
local instances = 10000

function xmlStringTest()
	for i=1, instances do
		xml[#xml+1] = xmlLoadString("<animals><animal type='monkey'></animal></animals>")
	end
end
addCommandHandler(triggerClientEvent and "sxml" or "cxml", xmlStringTest)

When restarting/stopping the resource, the memory is freed on both client and server (and freed for the client when leaving the server). I guess one thing I didn't think of is how you can free the memory from Lua without needing to restart/stop the resource.

In that case do we need to add a function such as xmlUnloadString?

[ghost]

I see. CLuaMain is per resource.

In that case do we need to add a function such as xmlUnloadString?

I think extending xmlUnloadFile to support unloading of loaded strings would be nice.

[Lpsd]

Would it not make more sense (naming wise) to create xmlUnload which supports nodes created via xmlLoadString and xmlLoadFile (and optionally deprecate/add warning for xmlUnloadFile)

[ghost]

Would it not make more sense (naming wise) to create xmlUnload which supports nodes created via xmlLoadString and xmlLoadFile (and optionally deprecate/add warning for xmlUnloadFile)

Technically, a loaded string is also a file. To be honest, I would've just added an extra optional paramter to xmlLoadFile for loading xml files as strings, but now since we have a function, I don't think it matters that much if we use xmlUnloadFile for unloading xml files that were loaded as strings.

[qaisjp]

For what it's worth @saml1er I'd argue we can still remove xmlLoadString and replace with xmlLoadFile as it's a beta function and not officially released yet.

[ghost]

For what it's worth @saml1er I'd argue we can still remove xmlLoadString and replace with xmlLoadFile as it's a beta function and not officially released yet.

That makes sense. We haven't officially released it. We can remove xmlLoadString and extend xmlLoadFile.

[qaisjp]

To be honest, I would've just added an extra optional paramter to xmlLoadFile for loading xml files as strings

Possible to keep it consistent with other functions (e.g. engineLoadTXD/playSound/dxCreateShader) and use a heuristic?

The first two examples above are binary but this is the heuristic used for dxCreateShader:

mtasa-blue/Client/mods/deathmatch/logic/luadefs/CLuaDrawingDefs.cpp

Lines 1171 to 1184 in 36efacb

	bool bIsRawData = false;
	const bool bValidFilePath = CResourceManager::ParseResourcePathInput(strFile, pFileResource, &strPath, &strMetaPath);
	if (!bValidFilePath || (strFile[0] != '@' && strFile[0] != ':'))
	{
	bIsRawData = strFile.find("\n") != std::string::npos;
	if (!bIsRawData)
	{
	bIsRawData = (strFile.find("technique ") != std::string::npos) && (strFile.find("pass ") != std::string::npos) &&
	(strFile.find('{') != std::string::npos) && (strFile.find('}') != std::string::npos);
	}
	}

Perhaps if it satisfies any of the following properties:

• contains \n, OR
• contains two < and two >, OR
• contains < and />

Maybe a heuristic is a bad idea in this case, since on Linux, < and > are perfectly valid filename characters.

[Pirulax]

If it's longer than 256 characters it must raw data*. If that fails, check for other things, that'd be the fastest IMHO

*Well, kinda. Windows now supports more than 256 afaik, but, nobody uses more than that.

[Lpsd]

We would need to use existing MTA file functions to verify the path/file exists and is a valid resource file.

Character length and/or checking included characters isn't good enough in this situation.

[Lpsd]

The suggested changes have deviated too much from what the initial PR intended to do - I'll close this and submit a new PR soon which re-implements XML string loading into xmlLoadFile (and removes xmlLoadString)

[qaisjp]

I mean, we can merge this (once it uses smart pointers), and you can do the xmlLoadFile/xmlLoadString merge in a separate PR. That way there's one PR for the bugfix and a separate PR for the refactor.

(If you prefer to do it in one PR, please use separate commits for the bugfix and the refactor.)

[qaisjp]

I'm happy to just let this one through in favour of getting this fixed ASAP. This is due a refactor anyway