LootList userdata now properly owns the contents of the deque, + garbage collection

Author: ConnorForan

changelog.txt

@@ -191,6 +191,8 @@ Fixes:
 	- Fixed MC_GET_STATUS_EFFECT_TARGET not running for the "shrink" status effect.
 * EntityPlayer:
 	- Fixed SetPoopSpell & RemovePoopSpell not working correctly and messing up the poop mana value.
+* EntityPickup
+	- Fix some crashes related to GetLootList
 * CharacterMenu:
 	- Fixed GetCharacterMenuIDFromPlayerType not returning the correct value for Tainted Isaac
 * Fixed modded characters' locked/door sprites rendering on the "continue" widget if they are locked behind an achievement.

repentogon/LuaInterfaces/LuaLootList.cpp

@@ -3,9 +3,8 @@
 #include "HookSystem.h"
 
 LUA_FUNCTION(Lua_LootListConstructor) {
-	LootList* loot = nullptr;
-	loot = lua::place<LootList>(L, lua::metatables::LootListMT);
-
+    // Initializes a new LootList (std::deque) under lua ownership and pushes a reference to the stack.
+	lua::place<LootList>(L, lua::metatables::LootListMT);
 	return 1;
 }
 
@@ -30,46 +29,46 @@ LUA_FUNCTION(Lua_PickupGetLootList) {
 	Entity_Pickup* pickup = lua::GetLuabridgeUserdata<Entity_Pickup*>(L, 1, lua::Metatables::ENTITY_PICKUP, "EntityPickup");
 	bool shouldAdvance = lua::luaL_optboolean(L, 2, false);
 
-	LootList loot = pickup->GetLootList(shouldAdvance, nullptr);
 	LootList* toLua = (LootList*)lua_newuserdata(L, sizeof(LootList));
+    luaL_setmetatable(L, lua::metatables::LootListMT);
+    // Initialize a new LootList (std::deque) in the memory allocated for the userdata, using "placement new".
+    // Using the move constructor, we can "steal" the contents of the deque returned by the function without having to copy everything.
+    // If we just copied the raw bytes of the deque, the allocated contents would still be freed alongside the original deque.
+    // This allows lua to properly take ownership of the deque. Destroying the deque later is handled by `Lua_LootList__gc`.
+    new (toLua) LootList(std::move(pickup->GetLootList(shouldAdvance, nullptr)));
 
-	luaL_setmetatable(L, lua::metatables::LootListMT);
-	memcpy(toLua, &loot, sizeof(LootList));
 	return 1;
 }
 
 LUA_FUNCTION(Lua_NPCGetFireplaceLoot) {
 	auto* npc = lua::GetLuabridgeUserdata<Entity_NPC*>(L, 1, lua::Metatables::ENTITY_NPC, "EntityNPC");
 	bool shouldAdvance = lua::luaL_optboolean(L, 2, false);
 
-	LootList loot = npc->fireplace_get_loot(shouldAdvance);
-	LootList* toLua = (LootList*)lua_newuserdata(L, sizeof(LootList));
+    LootList* toLua = (LootList*)lua_newuserdata(L, sizeof(LootList));
+    luaL_setmetatable(L, lua::metatables::LootListMT);
+    new (toLua) LootList(std::move(npc->fireplace_get_loot(shouldAdvance)));
 
-	luaL_setmetatable(L, lua::metatables::LootListMT);
-	memcpy(toLua, &loot, sizeof(LootList));
 	return 1;
 }
 
 LUA_FUNCTION(Lua_NPCGetShopkeeperLoot) {
 	auto* npc = lua::GetLuabridgeUserdata<Entity_NPC*>(L, 1, lua::Metatables::ENTITY_NPC, "EntityNPC");
 	bool shouldAdvance = lua::luaL_optboolean(L, 2, false);
 
-	LootList loot = npc->shopkeeper_get_loot(shouldAdvance);
-	LootList* toLua = (LootList*)lua_newuserdata(L, sizeof(LootList));
+    LootList* toLua = (LootList*)lua_newuserdata(L, sizeof(LootList));
+    luaL_setmetatable(L, lua::metatables::LootListMT);
+    new (toLua) LootList(std::move(npc->shopkeeper_get_loot(shouldAdvance)));
 
-	luaL_setmetatable(L, lua::metatables::LootListMT);
-	memcpy(toLua, &loot, sizeof(LootList));
 	return 1;
 }
 
 LUA_FUNCTION(Lua_LootListGetEntries)
 {
 	LootList* lootList = lua::GetRawUserdata<LootList*>(L, 1, lua::metatables::LootListMT);
-	auto& entries = *lootList; // Assuming lootList is a std::deque<LootListEntry>
 
 	lua_newtable(L);
 	int idx = 1;
-	for (LootListEntry& item : entries) {
+	for (LootListEntry& item : *lootList) {
 		LootListEntry** ud = (LootListEntry**)lua_newuserdata(L, sizeof(LootListEntry*));
 		*ud = &item;
 		luaL_setmetatable(L, lua::metatables::LootListEntryMT);
@@ -80,13 +79,12 @@ LUA_FUNCTION(Lua_LootListGetEntries)
 	return 1;
 }
 
-/*LUA_FUNCTION(Lua_LootList__gc) {
-	LootList* lootlist = lua::GetRawUserdata<LootList*>(L, 1, lua::metatables::LootListMT);
-	lootlist->~LootList();
-
+LUA_FUNCTION(Lua_LootList__gc) {
+    if (LootList* lootlist = lua::GetRawUserdata<LootList*>(L, 1, lua::metatables::LootListMT)) {
+        lootlist->~LootList();
+    }
 	return 0;
 }
-*/
 
 static void RegisterLootList(lua_State* L) {
 
@@ -100,7 +98,7 @@ static void RegisterLootList(lua_State* L) {
 		{ "PushEntry", Lua_LootListPushEntry },
 		{ NULL, NULL }
 	};
-	lua::RegisterNewClass(L, lua::metatables::LootListMT, lua::metatables::LootListMT, functions);
+	lua::RegisterNewClass(L, lua::metatables::LootListMT, lua::metatables::LootListMT, functions, Lua_LootList__gc);
 	lua_register(L, lua::metatables::LootListMT, Lua_LootListConstructor);
 }