Delete assets after artboards to fix race condition with FileAssetReferencers. I caught a random crash in Unity. Our unreferencing logic has a race condition! This is probably causing crashes in a lot of the runtimes, randomly. We attempt to unregister a file asset referencer when the FileAssetReferencer is destructed here: https://github.com/rive-app/rive/blob/d708e6a07b454ce0fc20d9432172f2b58c9f29f7/packages/runtime/src/assets/file_asset_referencer.cpp#L10-L13 This causes invalid memory access if m_fileAsset has already been deleted. This can happen when the source artboards stored in a file get deleted. Doesn't affect instances, but any-time a whole File is destructed this race condition can occur. We store our artboards and file assets as lists in our File object. Because they are stored in vectors as unique ptrs, we aren't guaranteed destructor order (some compilers will destruct the list of artboards first and some will destruct the files first). I've seen Clang do both (in Windows it was destroying the assets first which causes the problem as the references in the artboard then try to access the file assets). We should be really cautious when accessing bare pointers in destructors and make sure we understand the lifecycle of any objects we're referencing during destruction. For now this is the cleanest immediate fix, ensure that destruction is done in order: 1. Artboards and their components first 2. File assets 3. Backboard We introduced this here: https://github.com/rive-app/rive/pull/6068 Diffs= 18ae32102 Delete assets after artboards to fix race condition with FileAssetReferencers. (#6223) Co-authored-by: Luigi Rosso <luigi-rosso@users.noreply.github.com>
diff --git a/.rive_head b/.rive_head index 9266ff8..a5ecfc8 100644 --- a/.rive_head +++ b/.rive_head
@@ -1 +1 @@ -252100f48a796caa0199b65339e0def4d2eb676c +18ae3210288344019381946136b5dc0474c3eb35
diff --git a/include/rive/assets/file_asset.hpp b/include/rive/assets/file_asset.hpp index 8091299..f1d587b 100644 --- a/include/rive/assets/file_asset.hpp +++ b/include/rive/assets/file_asset.hpp
@@ -35,13 +35,18 @@ void removeFileAssetReferencer(FileAssetReferencer* referencer) { - m_fileAssetReferencers.erase( - std::remove_if(m_fileAssetReferencers.begin(), - m_fileAssetReferencers.end(), - [&referencer](FileAssetReferencer* otherReferencer) { - return otherReferencer == referencer; - }), - m_fileAssetReferencers.end()); + auto itr = m_fileAssetReferencers.begin(); + while (itr != m_fileAssetReferencers.end()) + { + if (*itr == referencer) + { + itr = m_fileAssetReferencers.erase(itr); + } + else + { + itr++; + } + } } std::string uniqueFilename() const;
diff --git a/include/rive/file.hpp b/include/rive/file.hpp index b1b75b5..9f6659d 100644 --- a/include/rive/file.hpp +++ b/include/rive/file.hpp
@@ -41,24 +41,6 @@ /// Minor version number supported by the runtime. static const int minorVersion = 0; -private: - /// The file's backboard. All Rive files have a single backboard - /// where the artboards live. - std::unique_ptr<Backboard> m_Backboard; - - /// We just keep these alive for the life of this File - std::vector<std::unique_ptr<FileAsset>> m_FileAssets; - - /// List of artboards in the file. Each artboard encapsulates a set of - /// Rive components and animations. - std::vector<std::unique_ptr<Artboard>> m_Artboards; - - Factory* m_Factory; - - /// The helper used to load assets when they're not provided in-band - /// with the file. - FileAssetLoader* m_AssetLoader; - File(Factory*, FileAssetLoader*); public: @@ -77,13 +59,13 @@ FileAssetLoader* assetLoader = nullptr); /// @returns the file's backboard. All files have exactly one backboard. - Backboard* backboard() const { return m_Backboard.get(); } + Backboard* backboard() const { return m_backboard; } /// @returns the number of artboards in the file. - size_t artboardCount() const { return m_Artboards.size(); } + size_t artboardCount() const { return m_artboards.size(); } std::string artboardNameAt(size_t index) const; - std::vector<const FileAsset*> assets() const; + const std::vector<FileAsset*>& assets() const; // Instances std::unique_ptr<ArtboardInstance> artboardDefault() const; @@ -113,6 +95,23 @@ private: ImportResult read(BinaryReader&, const RuntimeHeader&); + + /// The file's backboard. All Rive files have a single backboard + /// where the artboards live. + Backboard* m_backboard; + + /// We just keep these alive for the life of this File + std::vector<FileAsset*> m_fileAssets; + + /// List of artboards in the file. Each artboard encapsulates a set of + /// Rive components and animations. + std::vector<Artboard*> m_artboards; + + Factory* m_factory; + + /// The helper used to load assets when they're not provided in-band + /// with the file. + FileAssetLoader* m_assetLoader; }; } // namespace rive #endif
diff --git a/src/assets/file_asset_referencer.cpp b/src/assets/file_asset_referencer.cpp index 6c3fee2..11b7996 100644 --- a/src/assets/file_asset_referencer.cpp +++ b/src/assets/file_asset_referencer.cpp
@@ -27,6 +27,13 @@ void FileAssetReferencer::setAsset(FileAsset* asset) { + if (m_fileAsset != nullptr) + { + m_fileAsset->removeFileAssetReferencer(this); + } m_fileAsset = asset; - asset->addFileAssetReferencer(this); + if (asset != nullptr) + { + asset->addFileAssetReferencer(this); + } }; \ No newline at end of file
diff --git a/src/file.cpp b/src/file.cpp index cf77acb..a6dee57 100644 --- a/src/file.cpp +++ b/src/file.cpp
@@ -121,12 +121,24 @@ } File::File(Factory* factory, FileAssetLoader* assetLoader) : - m_Factory(factory), m_AssetLoader(assetLoader) + m_factory(factory), m_assetLoader(assetLoader) { assert(factory); } -File::~File() {} +File::~File() +{ + for (auto artboard : m_artboards) + { + delete artboard; + } + // Assets delete after artboards as they reference them. + for (auto asset : m_fileAssets) + { + delete asset; + } + delete m_backboard; +} std::unique_ptr<File> File::import(Span<const uint8_t> bytes, Factory* factory, @@ -188,20 +200,20 @@ switch (object->coreType()) { case Backboard::typeKey: - m_Backboard.reset(object->as<Backboard>()); + m_backboard = object->as<Backboard>(); break; case Artboard::typeKey: { Artboard* ab = object->as<Artboard>(); - ab->m_Factory = m_Factory; - m_Artboards.push_back(std::unique_ptr<Artboard>(ab)); + ab->m_Factory = m_factory; + m_artboards.push_back(ab); } break; case ImageAsset::typeKey: case FontAsset::typeKey: { auto fa = object->as<FileAsset>(); - m_FileAssets.push_back(std::unique_ptr<FileAsset>(fa)); + m_fileAssets.push_back(fa); } break; } @@ -277,7 +289,7 @@ case ImageAsset::typeKey: case FontAsset::typeKey: stackObject = - new FileAssetImporter(object->as<FileAsset>(), m_AssetLoader, m_Factory); + new FileAssetImporter(object->as<FileAsset>(), m_assetLoader, m_factory); stackType = FileAsset::typeKey; break; } @@ -301,11 +313,11 @@ Artboard* File::artboard(std::string name) const { - for (const auto& artboard : m_Artboards) + for (const auto& artboard : m_artboards) { if (artboard->name() == name) { - return artboard.get(); + return artboard; } } return nullptr; @@ -313,20 +325,20 @@ Artboard* File::artboard() const { - if (m_Artboards.empty()) + if (m_artboards.empty()) { return nullptr; } - return m_Artboards[0].get(); + return m_artboards[0]; } Artboard* File::artboard(size_t index) const { - if (index >= m_Artboards.size()) + if (index >= m_artboards.size()) { return nullptr; } - return m_Artboards[index].get(); + return m_artboards[index]; } std::string File::artboardNameAt(size_t index) const @@ -353,15 +365,7 @@ return ab ? ab->instance() : nullptr; } -std::vector<const FileAsset*> File::assets() const -{ - std::vector<const FileAsset*> assets; - for (auto itr = m_FileAssets.begin(); itr != m_FileAssets.end(); itr++) - { - assets.push_back(itr->get()); - } - return assets; -} +const std::vector<FileAsset*>& File::assets() const { return m_fileAssets; } #ifdef WITH_RIVE_TOOLS const std::vector<uint8_t> File::stripAssets(Span<const uint8_t> bytes,