Ensure SharedAssetDepot stays alive while lock is held

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
 

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<TAssetType, TAssetKey>& 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<TAssetType> 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<const SharedAssetDepot>& 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<const SharedAssetDepot> pDepot;
+    std::unique_lock<std::mutex> lock;
+  };
+
   // Maps asset keys to AssetEntry instances. This collection owns the asset
   // entries.
   std::unordered_map<TAssetKey, CesiumUtility::IntrusivePointer<AssetEntry>>
@@ -287,7 +313,7 @@ SharedAssetDepot<TAssetType, TAssetKey>::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<TAssetType, TAssetKey>::getOrCreate(
               [pDepot,
                pEntry](CesiumUtility::Result<
                        CesiumUtility::IntrusivePointer<TAssetType>>&& result) {
-                std::lock_guard lock(pDepot->_mutex);
+                LockHolder lock = pDepot->lock();
 
                 if (result.pValue) {
                   result.pValue->_pDepot = pDepot.get();
@@ -377,38 +403,44 @@ SharedAssetDepot<TAssetType, TAssetKey>::getOrCreate(
 
 template <typename TAssetType, typename TAssetKey>
 size_t SharedAssetDepot<TAssetType, TAssetKey>::getAssetCount() const {
-  std::lock_guard lock(this->_mutex);
+  LockHolder lock = this->lock();
   return this->_assets.size();
 }
 
 template <typename TAssetType, typename TAssetKey>
 size_t SharedAssetDepot<TAssetType, TAssetKey>::getActiveAssetCount() const {
-  std::lock_guard lock(this->_mutex);
+  LockHolder lock = this->lock();
   return this->_assets.size() - this->_deletionCandidates.size();
 }
 
 template <typename TAssetType, typename TAssetKey>
 size_t SharedAssetDepot<TAssetType, TAssetKey>::getInactiveAssetCount() const {
-  std::lock_guard lock(this->_mutex);
+  LockHolder lock = this->lock();
   return this->_deletionCandidates.size();
 }
 
 template <typename TAssetType, typename TAssetKey>
 int64_t
 SharedAssetDepot<TAssetType, TAssetKey>::getInactiveAssetTotalSizeBytes()
     const {
-  std::lock_guard lock(this->_mutex);
+  LockHolder lock = this->lock();
   return this->_totalDeletionCandidateMemoryUsage;
 }
 
+template <typename TAssetType, typename TAssetKey>
+typename SharedAssetDepot<TAssetType, TAssetKey>::LockHolder
+SharedAssetDepot<TAssetType, TAssetKey>::lock() const {
+  return LockHolder{this};
+}
+
 template <typename TAssetType, typename TAssetKey>
 void SharedAssetDepot<TAssetType, TAssetKey>::markDeletionCandidate(
     const TAssetType& asset,
     bool threadOwnsDepotLock) {
   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<TAssetType, TAssetKey>::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<TAssetType, TAssetKey>::AssetEntry::toResultUnderLock() const {
   return CesiumUtility::ResultPointer<TAssetType>(p, errorsAndWarnings);
 }
 
+template <typename TAssetType, typename TAssetKey>
+SharedAssetDepot<TAssetType, TAssetKey>::LockHolder::LockHolder(
+    const CesiumUtility::IntrusivePointer<const SharedAssetDepot>& pDepot_)
+    : pDepot(pDepot_), lock(pDepot_->_mutex) {}
+
+template <typename TAssetType, typename TAssetKey>
+SharedAssetDepot<TAssetType, TAssetKey>::LockHolder::~LockHolder() = default;
+
+template <typename TAssetType, typename TAssetKey>
+void SharedAssetDepot<TAssetType, TAssetKey>::LockHolder::unlock() {
+  this->lock.unlock();
+}
+
 } // namespace CesiumAsync

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<TestAsset, std::string>* pDepotRaw = pDepot.get();
+
+    ResultPointer<TestAsset> assetOne =
+        pDepot->getOrCreate(asyncSystem, nullptr, "one").waitInMainThread();
+    ResultPointer<TestAsset> 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();
+  }
 }