SharedAsset improvements...

* Make SharedAsset constructor and destructor protected, so instances of
  this type can't be created directly.
* Maintain `_sizeInDepot` field on `SharedAsset` to defend against the
  asset size changing between when we add its size and when we remove
  its size.
* Add some doc comments.
* Change the staleAssetSizeLimit to 16 MiB.
* Add some defensive assertions.
* Fix use-after-free when removing deletion candidates.

CesiumGltf/include/CesiumGltf/SharedAsset.h

@@ -23,9 +23,6 @@ namespace CesiumGltf {
 template <typename T>
 class CESIUMGLTF_API SharedAsset : public CesiumUtility::ExtensibleObject {
 public:
-  SharedAsset() = default;
-  ~SharedAsset() { CESIUM_ASSERT(this->_referenceCount == 0); }
-
   // Assets can be copied, but the fresh instance has no references and is not
   // in the asset depot.
   SharedAsset(const SharedAsset& rhs)
@@ -99,12 +96,25 @@ class CESIUMGLTF_API SharedAsset : public CesiumUtility::ExtensibleObject {
    */
   bool isShareable() const { return this->_pDepot != nullptr; }
 
+  /**
+   * @brief Gets the unique ID of this asset, if it {@link isShareable}.
+   *
+   * If this asset is not shareable, this method will return an empty string.
+   */
   const std::string& getUniqueAssetId() const { return this->_uniqueAssetId; }
 
+protected:
+  SharedAsset() = default;
+  ~SharedAsset() { CESIUM_ASSERT(this->_referenceCount == 0); }
+
 private:
   mutable std::atomic<std::int32_t> _referenceCount{0};
-  SharedAssetDepot<T>* _pDepot;
-  std::string _uniqueAssetId;
+  SharedAssetDepot<T>* _pDepot{nullptr};
+  std::string _uniqueAssetId{};
+
+  // The size of this asset when it was counted by the depot. This is stored so
+  // that the exact same size can be subtracted later.
+  int64_t _sizeInDepot{0};
 
   // To allow the depot to modify _pDepot and _uniqueAssetId.
   friend class SharedAssetDepot<T>;

CesiumGltf/include/CesiumGltf/SharedAssetDepot.h

@@ -39,9 +39,9 @@ class SharedAssetDepot : public CesiumUtility::ReferenceCountedThreadSafe<
    * At that point, assets are cleaned up in the order that they were marked for
    * deletion until the total dips below this threshold again.
    *
-   * Default is 100MB.
+   * Default is 16MiB.
    */
-  int64_t staleAssetSizeLimit = 1000 * 1000 * 100;
+  int64_t staleAssetSizeLimit = 16 * 1024 * 1024;
 
   SharedAssetDepot() = default;
 
@@ -73,6 +73,11 @@ class SharedAssetDepot : public CesiumUtility::ReferenceCountedThreadSafe<
       return findIt->second.get();
     }
 
+    // If this asset ID is pending, but hasn't completed yet, where did this
+    // asset come from? It shouldn't happen.
+    CESIUM_ASSERT(
+        this->pendingAssets.find(assetId) == this->pendingAssets.end());
+
     pAsset->_pDepot = this;
     pAsset->_uniqueAssetId = assetId;
 
@@ -148,20 +153,19 @@ class SharedAssetDepot : public CesiumUtility::ReferenceCountedThreadSafe<
                 [uri,
                  this](CesiumUtility::IntrusivePointer<AssetType>&& pResult)
                     -> CesiumUtility::IntrusivePointer<AssetType> {
-                  // Get rid of our future.
+                  // Remove pending asset.
                   this->pendingAssets.erase(uri);
 
                   // Store the new asset in the depot.
                   return this->store(uri, pResult);
                 });
 
-    auto [it, ok] = this->pendingAssets.emplace(uri, std::move(future).share());
-    if (!ok) {
-      return asyncSystem
-          .createResolvedFuture<CesiumUtility::IntrusivePointer<AssetType>>(
-              nullptr)
-          .share();
-    }
+    auto [it, added] =
+        this->pendingAssets.emplace(uri, std::move(future).share());
+
+    // Should always be added successfully, because we checked above that the
+    // URI doesn't exist in the map yet.
+    CESIUM_ASSERT(added);
 
     return it->second;
   }
@@ -201,19 +205,28 @@ class SharedAssetDepot : public CesiumUtility::ReferenceCountedThreadSafe<
    */
   void markDeletionCandidate(const AssetType* pAsset) {
     std::lock_guard lock(this->deletionCandidatesMutex);
-    this->deletionCandidates.push_back(const_cast<AssetType*>(pAsset));
-    this->totalDeletionCandidateMemoryUsage += pAsset->getSizeBytes();
+
+    AssetType* pMutableAsset = const_cast<AssetType*>(pAsset);
+    pMutableAsset->_sizeInDepot = pMutableAsset->getSizeBytes();
+    this->totalDeletionCandidateMemoryUsage += pMutableAsset->_sizeInDepot;
+
+    this->deletionCandidates.push_back(pMutableAsset);
 
     if (this->totalDeletionCandidateMemoryUsage > this->staleAssetSizeLimit) {
       std::lock_guard assetsLock(this->assetsMutex);
+
+      // Delete the deletion candidates until we're below the limit.
       while (!this->deletionCandidates.empty() &&
              this->totalDeletionCandidateMemoryUsage >
                  this->staleAssetSizeLimit) {
         const AssetType* pOldAsset = this->deletionCandidates.front();
         this->deletionCandidates.pop_front();
+
+        this->totalDeletionCandidateMemoryUsage -= pOldAsset->_sizeInDepot;
+
+        // This will actually delete the asset.
         CESIUM_ASSERT(pOldAsset->_referenceCount == 0);
         this->assets.erase(pOldAsset->getUniqueAssetId());
-        this->totalDeletionCandidateMemoryUsage -= pOldAsset->getSizeBytes();
       }
     }
   }
@@ -224,15 +237,17 @@ class SharedAssetDepot : public CesiumUtility::ReferenceCountedThreadSafe<
    */
   void unmarkDeletionCandidate(const AssetType* pAsset) {
     std::lock_guard lock(this->deletionCandidatesMutex);
-    const std::string& assetId = pAsset->getUniqueAssetId();
-    for (auto it = this->deletionCandidates.begin();
-         it != this->deletionCandidates.end();
-         ++it) {
-      if ((*it)->getUniqueAssetId() == assetId) {
-        this->totalDeletionCandidateMemoryUsage -= (*it)->getSizeBytes();
-        this->deletionCandidates.erase(it);
-        break;
-      }
+
+    auto it = std::find(
+        this->deletionCandidates.begin(),
+        this->deletionCandidates.end(),
+        pAsset);
+
+    CESIUM_ASSERT(it != this->deletionCandidates.end());
+
+    if (it != this->deletionCandidates.end()) {
+      this->totalDeletionCandidateMemoryUsage -= (*it)->_sizeInDepot;
+      this->deletionCandidates.erase(it);
     }
   }