From 585e2a6ace6f6cefa33c3fec0ed6cca0876c6206 Mon Sep 17 00:00:00 2001 From: Aaron Kimbrell Date: Tue, 29 Sep 2026 10:52:22 -0500 Subject: [PATCH] fix(inventory): a removed item is freed by its inventory, not by itself (issue 1568) Item::RemoveFromInventory ended with `delete this`, so every caller that still used the item afterwards (SetCount(0) returning into its caller, loops that remove several items, proxies purged while their parent is handled) touched freed memory. The inventory now takes the removed item and frees it at the inventory component's next update, or with the inventory. Check in game: sell, drop, delete, trade, mail and use up stacks (including the last of a stack); unequip and remove an item set piece and a proxy-bearing item (rocket, modular car); donate items; nothing crashes and the inventory shows the right counts after relogging. Co-Authored-By: Claude Opus 5.5 --- dGame/dComponents/InventoryComponent.cpp | 5 ++ dGame/dInventory/Inventory.cpp | 8 ++ dGame/dInventory/Inventory.h | 23 +++++ dGame/dInventory/Item.cpp | 4 +- .../dComponentsTests/CMakeLists.txt | 1 + .../dComponentsTests/ItemRemovalTests.cpp | 87 +++++++++++++++++++ 6 files changed, 127 insertions(+), 1 deletion(-) create mode 100644 tests/dGameTests/dComponentsTests/ItemRemovalTests.cpp diff --git a/dGame/dComponents/InventoryComponent.cpp b/dGame/dComponents/InventoryComponent.cpp index c40394c04..4ac5a7bde 100644 --- a/dGame/dComponents/InventoryComponent.cpp +++ b/dGame/dComponents/InventoryComponent.cpp @@ -964,6 +964,11 @@ void InventoryComponent::Update(float deltaTime) { for (auto* set : m_Itemsets) { set->Update(deltaTime); } + + // Items removed during the last frame are no longer in use + for (auto* const inventory : m_Inventories | std::views::values) { + inventory->FreeRetiredItems(); + } } void InventoryComponent::UpdateSlot(const std::string& location, const EquippedItem& item, bool keepCurrent) { diff --git a/dGame/dInventory/Inventory.cpp b/dGame/dInventory/Inventory.cpp index 363458e53..00851e3c2 100644 --- a/dGame/dInventory/Inventory.cpp +++ b/dGame/dInventory/Inventory.cpp @@ -236,6 +236,14 @@ void Inventory::RemoveManagedItem(Item* item) { free++; } +void Inventory::RetireItem(Item* item) { + m_RetiredItems.emplace_back(item); +} + +void Inventory::FreeRetiredItems() { + m_RetiredItems.clear(); +} + eInventoryType Inventory::FindInventoryTypeForLot(const LOT lot) { auto itemComponent = FindItemComponent(lot); diff --git a/dGame/dInventory/Inventory.h b/dGame/dInventory/Inventory.h index f07ec68e4..295e417cd 100644 --- a/dGame/dInventory/Inventory.h +++ b/dGame/dInventory/Inventory.h @@ -4,6 +4,7 @@ #define INVENTORY_H #include +#include #include #include "CDItemComponentTable.h" @@ -126,6 +127,23 @@ public: */ void RemoveManagedItem(Item* item); + /** + * Takes ownership of an item that was taken out of the inventory. Code still holding the item can use it until + * FreeRetiredItems runs (the inventory component's next update), so an item never frees itself. + * @param item the removed item + */ + void RetireItem(Item* item); + + /** + * Frees the items retired since the last call + */ + void FreeRetiredItems(); + + /** + * @return whether removed items are waiting to be freed + */ + bool HasRetiredItems() const { return !m_RetiredItems.empty(); } + /** * Returns the inventory type an item of the specified lot should be placed in * @param lot the lot to find the inventory type for @@ -183,6 +201,11 @@ private: */ std::map items; + /** + * Items taken out of this inventory, freed by FreeRetiredItems + */ + std::vector> m_RetiredItems; + /** * The inventory component this inventory belongs to */ diff --git a/dGame/dInventory/Item.cpp b/dGame/dInventory/Item.cpp index ec0a67de4..caff8ee22 100644 --- a/dGame/dInventory/Item.cpp +++ b/dGame/dInventory/Item.cpp @@ -580,7 +580,9 @@ void Item::RemoveFromInventory() { inventory->RemoveManagedItem(this); - delete this; + // The inventory frees the item at its next update, so callers still holding it (SetCount(0) and the loops that + // remove several items) are not left with a freed item + inventory->RetireItem(this); } Item::~Item() { diff --git a/tests/dGameTests/dComponentsTests/CMakeLists.txt b/tests/dGameTests/dComponentsTests/CMakeLists.txt index 97ccf063b..6950ce482 100644 --- a/tests/dGameTests/dComponentsTests/CMakeLists.txt +++ b/tests/dGameTests/dComponentsTests/CMakeLists.txt @@ -16,6 +16,7 @@ set(DCOMPONENTS_TESTS "QuickBuildCompleteTests.cpp" "RocketLaunchTests.cpp" "ReplicaConstructionTests.cpp" + "ItemRemovalTests.cpp" ) # Get the folder name and prepend it to the files above diff --git a/tests/dGameTests/dComponentsTests/ItemRemovalTests.cpp b/tests/dGameTests/dComponentsTests/ItemRemovalTests.cpp new file mode 100644 index 000000000..a946f7471 --- /dev/null +++ b/tests/dGameTests/dComponentsTests/ItemRemovalTests.cpp @@ -0,0 +1,87 @@ +#include "GameDependencies.h" + +#include "CDClientManager.h" +#include "CDComponentsRegistryTable.h" +#include "CDItemComponentTable.h" +#include "Entity.h" +#include "InventoryComponent.h" +#include "Item.h" +#include "eReplicaComponentType.h" + +#include + +// An item taken out of its inventory is not freed on the spot: the inventory keeps it until its component's next +// update, so code still holding the item can finish with it. +class ItemRemovalTests : public GameDependenciesTest { +protected: + std::unique_ptr entity; + InventoryComponent* inventoryComponent{}; + Inventory* items{}; + LWOOBJID nextId = 0x7000; + + void SetUp() override { + SetUpDependencies(); + RegisterLot(1000); + CDClientManager::GetEntriesMutable().insert_or_assign(static_cast(info.lot), 0); + entity = std::make_unique(1, info); + inventoryComponent = entity->AddComponent(-1); + items = inventoryComponent->GetInventory(eInventoryType::ITEMS); + } + + void TearDown() override { + entity.reset(); + TearDownDependencies(); + } + + static void RegisterLot(const LOT lot) { + const auto componentID = static_cast(96000 + lot); + auto& registry = CDClientManager::GetEntriesMutable(); + registry.insert_or_assign(static_cast(lot), componentID); + registry.insert_or_assign(static_cast(eReplicaComponentType::ITEM) << 32 | static_cast(lot), componentID); + CDItemComponent component{}; + component.id = componentID; + CDClientManager::GetEntriesMutable().insert_or_assign(componentID, component); + } + + Item* Give(const uint32_t count) { + return new Item(nextId++, 1000, items, static_cast(items->GetItems().size()), count, false, {}, LWOOBJID_EMPTY, LWOOBJID_EMPTY, eLootSourceType::NONE); + } +}; + +TEST_F(ItemRemovalTests, ARemovedItemIsUsableUntilTheNextUpdate) { + auto* const item = Give(1); + const auto id = item->GetId(); + + item->RemoveFromInventory(); + + EXPECT_EQ(items->GetItems().count(id), 0u); + EXPECT_TRUE(items->HasRetiredItems()); + // Still a valid object + EXPECT_EQ(item->GetId(), id); + EXPECT_EQ(item->GetCount(), 0u); + + inventoryComponent->Update(0.0f); + EXPECT_FALSE(items->HasRetiredItems()); +} + +TEST_F(ItemRemovalTests, RemovingSeveralItemsInALoop) { + Give(1); + Give(2); + Give(3); + + // Every item removed while the list is walked; each stays valid until the update + auto held = items->GetItems(); + for (auto* const item : held | std::views::values) item->RemoveFromInventory(); + for (auto* const item : held | std::views::values) EXPECT_EQ(item->GetCount(), 0u); + + EXPECT_TRUE(items->GetItems().empty()); + inventoryComponent->Update(0.0f); + EXPECT_FALSE(items->HasRetiredItems()); +} + +TEST_F(ItemRemovalTests, RetiredItemsAreFreedWithTheInventory) { + Give(1)->RemoveFromInventory(); + EXPECT_TRUE(items->HasRetiredItems()); + // Freed by the inventory's destructor (checked by sanitizer builds) + entity.reset(); +}