diff --git a/CHANGES.md b/CHANGES.md index a4142312f..e1c989de5 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -25,6 +25,7 @@ - Fixed a bug in `SubtreeFileReader::loadBinary` that prevented valid subtrees from loading if they did not contain binary data. - Fixed a bug in the `Tileset` selection algorithm that could cause detail to disappear during load in some cases. - Improved the "kicking" mechanism in the tileset selection algorithm. The new criteria allows holes in a `Tileset`, when they do occur, to be filled with loaded tiles more incrementally. +- Fixed a bug in `SharedAssetDepot` that could lead to crashes and other undefined behavior when an asset in the depot outlived the depot itself. ### v0.42.0 - 2024-12-02 diff --git a/CesiumAsync/include/CesiumAsync/SharedAssetDepot.h b/CesiumAsync/include/CesiumAsync/SharedAssetDepot.h index c74dbf727..fd7b0f37f 100644 --- a/CesiumAsync/include/CesiumAsync/SharedAssetDepot.h +++ b/CesiumAsync/include/CesiumAsync/SharedAssetDepot.h @@ -120,9 +120,18 @@ class CESIUMASYNC_API SharedAssetDepot int64_t getInactiveAssetTotalSizeBytes() const; private: + struct LockHolder; + // Disable copy void operator=(const SharedAssetDepot& other) = delete; + /** + * @brief Locks the shared asset depot for thread-safe access. It will remain + * locked until the returned object is destroyed or the `unlock` method is + * called on it. + */ + LockHolder lock() const; + /** * @brief Marks the given asset as a candidate for deletion. * Should only be called by {@link SharedAsset}. May be called from any thread. @@ -210,6 +219,23 @@ class CESIUMASYNC_API SharedAssetDepot CesiumUtility::ResultPointer toResultUnderLock() const; }; + // Manages the depot's mutex. Also ensures, via IntrusivePointer, that the + // depot won't be destroyed while the lock is held. + struct LockHolder { + LockHolder( + const CesiumUtility::IntrusivePointer& pDepot); + ~LockHolder(); + void unlock(); + + private: + // These two fields _must_ be declared in this order to guarantee that the + // mutex is released before the depot pointer. Releasing the depot pointer + // could destroy the depot, and that will be disastrous if the lock is still + // held. + CesiumUtility::IntrusivePointer pDepot; + std::unique_lock lock; + }; + // Maps asset keys to AssetEntry instances. This collection owns the asset // entries. std::unordered_map> @@ -287,7 +313,7 @@ SharedAssetDepot::getOrCreate( const TAssetKey& assetKey) { // We need to take care here to avoid two assets starting to load before the // first asset has added an entry and set its maybePendingAsset field. - std::unique_lock lock(this->_mutex); + LockHolder lock = this->lock(); auto existingIt = this->_assets.find(assetKey); if (existingIt != this->_assets.end()) { @@ -335,7 +361,7 @@ SharedAssetDepot::getOrCreate( [pDepot, pEntry](CesiumUtility::Result< CesiumUtility::IntrusivePointer>&& result) { - std::lock_guard lock(pDepot->_mutex); + LockHolder lock = pDepot->lock(); if (result.pValue) { result.pValue->_pDepot = pDepot.get(); @@ -377,19 +403,19 @@ SharedAssetDepot::getOrCreate( template size_t SharedAssetDepot::getAssetCount() const { - std::lock_guard lock(this->_mutex); + LockHolder lock = this->lock(); return this->_assets.size(); } template size_t SharedAssetDepot::getActiveAssetCount() const { - std::lock_guard lock(this->_mutex); + LockHolder lock = this->lock(); return this->_assets.size() - this->_deletionCandidates.size(); } template size_t SharedAssetDepot::getInactiveAssetCount() const { - std::lock_guard lock(this->_mutex); + LockHolder lock = this->lock(); return this->_deletionCandidates.size(); } @@ -397,10 +423,16 @@ template int64_t SharedAssetDepot::getInactiveAssetTotalSizeBytes() const { - std::lock_guard lock(this->_mutex); + LockHolder lock = this->lock(); return this->_totalDeletionCandidateMemoryUsage; } +template +SharedAssetDepot::LockHolder +SharedAssetDepot::lock() const { + return LockHolder{this}; +} + template void SharedAssetDepot::markDeletionCandidate( const TAssetType& asset, @@ -408,7 +440,7 @@ void SharedAssetDepot::markDeletionCandidate( if (threadOwnsDepotLock) { this->markDeletionCandidateUnderLock(asset); } else { - std::lock_guard lock(this->_mutex); + LockHolder lock = this->lock(); this->markDeletionCandidateUnderLock(asset); } } @@ -468,7 +500,7 @@ void SharedAssetDepot::unmarkDeletionCandidate( if (threadOwnsDepotLock) { this->unmarkDeletionCandidateUnderLock(asset); } else { - std::lock_guard lock(this->_mutex); + LockHolder lock = this->lock(); this->unmarkDeletionCandidateUnderLock(asset); } } @@ -514,4 +546,17 @@ SharedAssetDepot::AssetEntry::toResultUnderLock() const { return CesiumUtility::ResultPointer(p, errorsAndWarnings); } +template +SharedAssetDepot::LockHolder::LockHolder( + const CesiumUtility::IntrusivePointer& pDepot_) + : pDepot(pDepot_), lock(pDepot_->_mutex) {} + +template +SharedAssetDepot::LockHolder::~LockHolder() = default; + +template +void SharedAssetDepot::LockHolder::unlock() { + this->lock.unlock(); +} + } // namespace CesiumAsync diff --git a/CesiumAsync/test/TestSharedAssetDepot.cpp b/CesiumAsync/test/TestSharedAssetDepot.cpp index 87625fd6d..2a5eccb0f 100644 --- a/CesiumAsync/test/TestSharedAssetDepot.cpp +++ b/CesiumAsync/test/TestSharedAssetDepot.cpp @@ -124,4 +124,25 @@ TEST_CASE("SharedAssetDepot") { CHECK(pDepot->getActiveAssetCount() == 0); CHECK(pDepot->getInactiveAssetCount() == 1); } + + SECTION("is kept alive until all of its assets are unreferenced") { + auto pDepot = createDepot(); + SharedAssetDepot* pDepotRaw = pDepot.get(); + + ResultPointer assetOne = + pDepot->getOrCreate(asyncSystem, nullptr, "one").waitInMainThread(); + ResultPointer assetTwo = + pDepot->getOrCreate(asyncSystem, nullptr, "two!!").waitInMainThread(); + + pDepot.reset(); + + assetTwo.pValue.reset(); + + REQUIRE(assetOne.pValue->getDepot() == pDepotRaw); + CHECK( + pDepotRaw->getInactiveAssetTotalSizeBytes() == + int64_t(std::string("two!!").size())); + + assetOne.pValue.reset(); + } }