diff --git a/include/rive/lua/rive_lua_libs.hpp b/include/rive/lua/rive_lua_libs.hpp index cdd02878..3a3d1008 100644 --- a/include/rive/lua/rive_lua_libs.hpp +++ b/include/rive/lua/rive_lua_libs.hpp @@ -572,7 +572,8 @@ class ScriptedRenderer class ScriptReffedArtboard : public RefCnt { public: - ScriptReffedArtboard(rcp file, + ScriptReffedArtboard(lua_State* L, + rcp file, std::unique_ptr&& artboardInstance, rcp viewModelInstance, rcp parentDataContext); @@ -582,12 +583,18 @@ class ScriptReffedArtboard : public RefCnt Artboard* artboard(); StateMachineInstance* stateMachine(); rcp viewModelInstance() { return m_viewModelInstance; } + void releaseReferences(); + static void releaseAll(lua_State* state); private: rcp m_file; std::unique_ptr m_artboard; std::unique_ptr m_stateMachine; rcp m_viewModelInstance; + lua_State* m_luaState = nullptr; + ScriptReffedArtboard* m_trackNext = nullptr; + ScriptReffedArtboard* m_trackPrev = nullptr; + static ScriptReffedArtboard* s_head; }; class ScriptedArtboard @@ -630,7 +637,6 @@ class ScriptedArtboard private: lua_State* m_state = nullptr; rcp m_scriptReffedArtboard = nullptr; - rcp m_dataContext = nullptr; int m_dataRef = 0; }; @@ -704,11 +710,17 @@ class ScriptedViewModel return m_viewModelInstance; } + void releaseReferences(); + static void releaseAll(lua_State* state); + private: lua_State* m_state; rcp m_viewModel; rcp m_viewModelInstance; std::unordered_map m_propertyRefs; + ScriptedViewModel* m_trackNext = nullptr; + ScriptedViewModel* m_trackPrev = nullptr; + static ScriptedViewModel* s_head; }; class ScriptedPropertyViewModel : public ScriptedProperty diff --git a/include/rive/scripted/scripted_object.hpp b/include/rive/scripted/scripted_object.hpp index 2d507499..11ccd28f 100644 --- a/include/rive/scripted/scripted_object.hpp +++ b/include/rive/scripted/scripted_object.hpp @@ -59,6 +59,9 @@ class ScriptedObject : public FileAssetReferencer, lua_State* state() const { return m_vm ? m_vm->state() : nullptr; } #endif void scriptDispose(); +#ifdef WITH_RIVE_SCRIPTING + static void collectLuaGarbage(lua_State* state); +#endif virtual bool addScriptedDirt(ComponentDirt value, bool recurse = false) = 0; void setAsset(rcp asset) override; static ScriptedObject* from(Core* object); diff --git a/src/artboard.cpp b/src/artboard.cpp index a642d022..4ce72d76 100644 --- a/src/artboard.cpp +++ b/src/artboard.cpp @@ -141,6 +141,13 @@ Artboard::~Artboard() return vmObjects.count(object) != 0; }; +#ifdef WITH_RIVE_SCRIPTING + lua_State* luaState = nullptr; + if (!m_ScriptedObjects.empty()) + { + luaState = m_ScriptedObjects[0]->state(); + } +#endif // Second pass: delete non-VM objects. for (auto object : m_Objects) { @@ -154,6 +161,9 @@ Artboard::~Artboard() } delete object; } +#ifdef WITH_RIVE_SCRIPTING + ScriptedObject::collectLuaGarbage(luaState); +#endif for (auto object : m_invalidObjects) { if (object == nullptr) diff --git a/src/lua/lua_artboards.cpp b/src/lua/lua_artboards.cpp index dd83effc..f4b1a0cf 100644 --- a/src/lua/lua_artboards.cpp +++ b/src/lua/lua_artboards.cpp @@ -15,14 +15,18 @@ using namespace rive; +ScriptReffedArtboard* ScriptReffedArtboard::s_head = nullptr; + ScriptReffedArtboard::ScriptReffedArtboard( + lua_State* L, rcp file, std::unique_ptr&& artboardInstance, rcp viewModelInstance, rcp parentDataContext) : m_file(file), m_artboard(std::move(artboardInstance)), - m_stateMachine(m_artboard->defaultStateMachine()) + m_stateMachine(m_artboard->defaultStateMachine()), + m_luaState(L) { if (viewModelInstance) { @@ -45,18 +49,58 @@ ScriptReffedArtboard::ScriptReffedArtboard( m_stateMachine->bindViewModelInstance(m_viewModelInstance); } } + // Track this instance for cleanup during artboard destruction + m_trackNext = s_head; + m_trackPrev = nullptr; + if (s_head) + { + s_head->m_trackPrev = this; + } + s_head = this; } ScriptReffedArtboard::~ScriptReffedArtboard() { - // Make sure state machine is deleted before artboard since - // StateMachineInstance destructor accesses the artboard. + // Remove from tracking list + if (m_trackPrev) + { + m_trackPrev->m_trackNext = m_trackNext; + } + else if (s_head == this) + { + s_head = m_trackNext; + } + if (m_trackNext) + { + m_trackNext->m_trackPrev = m_trackPrev; + } + releaseReferences(); +} + +void ScriptReffedArtboard::releaseReferences() +{ + // StateMachine must be deleted before artboard since its destructor + // accesses the artboard. m_stateMachine = nullptr; - // Make sure artboard is deleted before file. m_artboard = nullptr; + m_viewModelInstance = nullptr; m_file = nullptr; } +void ScriptReffedArtboard::releaseAll(lua_State* state) +{ + ScriptReffedArtboard* current = s_head; + while (current) + { + ScriptReffedArtboard* next = current->m_trackNext; + if (current->m_luaState == state) + { + current->releaseReferences(); + } + current = next; + } +} + rive::rcp ScriptReffedArtboard::file() { return m_file; } Artboard* ScriptReffedArtboard::artboard() { return m_artboard.get(); } @@ -264,7 +308,7 @@ int ScriptedArtboard::instance(lua_State* L, m_scriptReffedArtboard->file(), std::move(artboardInstance), viewModelInstance, - m_dataContext); + artboard()->dataContext()); return 1; } @@ -384,11 +428,11 @@ ScriptedArtboard::ScriptedArtboard( rcp dataContext) : m_state(L), m_scriptReffedArtboard( - make_rcp(file, + make_rcp(L, + file, std::move(artboardInstance), viewModelInstance, - dataContext)), - m_dataContext(dataContext) + dataContext)) {} ScriptedArtboard::~ScriptedArtboard() diff --git a/src/lua/lua_properties.cpp b/src/lua/lua_properties.cpp index e9130678..391725e3 100644 --- a/src/lua/lua_properties.cpp +++ b/src/lua/lua_properties.cpp @@ -245,18 +245,61 @@ void ScriptedPropertyViewModel::setValue(ScriptedViewModel* scriptedViewModel) } } +ScriptedViewModel* ScriptedViewModel::s_head = nullptr; + ScriptedViewModel::ScriptedViewModel(lua_State* L, rcp viewModel, rcp viewModelInstance) : m_state(L), m_viewModel(viewModel), m_viewModelInstance(viewModelInstance) -{} +{ + m_trackNext = s_head; + m_trackPrev = nullptr; + if (s_head) + { + s_head->m_trackPrev = this; + } + s_head = this; +} ScriptedViewModel::~ScriptedViewModel() { + if (m_trackPrev) + { + m_trackPrev->m_trackNext = m_trackNext; + } + else if (s_head == this) + { + s_head = m_trackNext; + } + if (m_trackNext) + { + m_trackNext->m_trackPrev = m_trackPrev; + } for (auto itr : m_propertyRefs) { lua_unref(m_state, itr.second); } + releaseReferences(); +} + +void ScriptedViewModel::releaseReferences() +{ + m_viewModel = nullptr; + m_viewModelInstance = nullptr; +} + +void ScriptedViewModel::releaseAll(lua_State* state) +{ + ScriptedViewModel* current = s_head; + while (current) + { + ScriptedViewModel* next = current->m_trackNext; + if (current->m_state == state) + { + current->releaseReferences(); + } + current = next; + } } int ScriptedViewModel::instance(lua_State* L) diff --git a/src/scripted/scripted_object.cpp b/src/scripted/scripted_object.cpp index f046ad13..a90b06bd 100644 --- a/src/scripted/scripted_object.cpp +++ b/src/scripted/scripted_object.cpp @@ -355,14 +355,27 @@ void ScriptedObject::scriptDispose() lua_unref(L, m_self); disposeScriptedContext(); #ifdef TESTING - // Force GC to collect any ScriptedArtboard instances created via - // instance() lua_gc(L, LUA_GCCOLLECT, 0); #endif } m_vm = nullptr; m_self = 0; } + +void ScriptedObject::collectLuaGarbage(lua_State* state) +{ + if (state != nullptr) + { + // Null C++ pointers on all ScriptReffedArtboard and ScriptedViewModel + // instances for this lua_State. Sub-artboard scripts store orphaned + // registry refs (m_context, m_dataRef, m_propertyRefs) that keep C++ + // objects alive. Rather than walk the registry, we null the C++ side + // directly — the Lua userdata become harmless empty shells. + ScriptReffedArtboard::releaseAll(state); + ScriptedViewModel::releaseAll(state); + lua_gc(state, LUA_GCCOLLECT, 0); + } +} #else void ScriptedObject::setArtboardInput(std::string name, Artboard* artboard) {} diff --git a/tests/unit_tests/assets/blinko.riv b/tests/unit_tests/assets/blinko.riv new file mode 100644 index 00000000..5cbde7eb Binary files /dev/null and b/tests/unit_tests/assets/blinko.riv differ diff --git a/tests/unit_tests/runtime/blinko_minimal_leak_test.cpp b/tests/unit_tests/runtime/blinko_minimal_leak_test.cpp new file mode 100644 index 00000000..1d7efded --- /dev/null +++ b/tests/unit_tests/runtime/blinko_minimal_leak_test.cpp @@ -0,0 +1,207 @@ +/* + * Test: verify ViewModelInstance references don't leak across artboard + * creation/destruction cycles. + * Run with: ./test.sh -m "[blinko minimal]" + */ + +#include +#include +#include +#include +#include +#include "rive_file_reader.hpp" +#include +#include + +using namespace rive; + +static void printVMDeltas(File* file, + const std::vector& baseline, + const char* label) +{ + printf(" %s:", label); + bool any = false; + for (size_t i = 0; i < file->viewModelCount(); i++) + { + int32_t delta = file->viewModel(i)->debugging_refcnt() - baseline[i]; + if (delta != 0) + { + printf(" %s=%+d", file->viewModel(i)->name().c_str(), delta); + any = true; + } + } + if (!any) + printf(" (none)"); + printf("\n"); +} + +static bool hasVMLeak(File* file, const std::vector& baseline) +{ + for (size_t i = 0; i < file->viewModelCount(); i++) + { + if (file->viewModel(i)->debugging_refcnt() - baseline[i] != 0) + return true; + } + return false; +} + +static std::vector captureBaseline(File* file) +{ + std::vector v; + for (size_t i = 0; i < file->viewModelCount(); i++) + v.push_back(file->viewModel(i)->debugging_refcnt()); + return v; +} + +TEST_CASE("blinko: artboard instance only (no bind, no advance)", + "[blinko minimal]") +{ + auto file = ReadRiveFile("assets/blinko.riv"); + auto baseline = captureBaseline(file.get()); + + { + auto artboard = file->artboardDefault()->instance(); + } + printVMDeltas(file.get(), baseline, "After artboard create+destroy"); + CHECK(!hasVMLeak(file.get(), baseline)); +} + +TEST_CASE("blinko: artboard + VMI (no bind, no advance)", + "[blinko minimal]") +{ + auto file = ReadRiveFile("assets/blinko.riv"); + auto baseline = captureBaseline(file.get()); + + { + auto artboard = file->artboardDefault()->instance(); + auto vmi = file->createDefaultViewModelInstance(artboard.get()); + } + printVMDeltas(file.get(), baseline, "After artboard+VMI create+destroy"); + CHECK(!hasVMLeak(file.get(), baseline)); +} + +TEST_CASE("blinko: artboard + VMI + SM (no bind, no advance)", + "[blinko minimal]") +{ + auto file = ReadRiveFile("assets/blinko.riv"); + auto baseline = captureBaseline(file.get()); + + { + auto artboard = file->artboardDefault()->instance(); + auto vmi = file->createDefaultViewModelInstance(artboard.get()); + auto machine = artboard->defaultStateMachine(); + } + printVMDeltas(file.get(), baseline, "After create+destroy (no bind)"); + CHECK(!hasVMLeak(file.get(), baseline)); +} + +TEST_CASE("blinko: bind only (no advance)", + "[blinko minimal]") +{ + auto file = ReadRiveFile("assets/blinko.riv"); + auto baseline = captureBaseline(file.get()); + + { + auto artboard = file->artboardDefault()->instance(); + auto vmi = file->createDefaultViewModelInstance(artboard.get()); + auto machine = artboard->defaultStateMachine(); + machine->bindViewModelInstance(vmi); + } + printVMDeltas(file.get(), baseline, "After bind+destroy (no advance)"); + CHECK(!hasVMLeak(file.get(), baseline)); +} + +TEST_CASE("blinko: bind + 1 advance", + "[blinko minimal]") +{ + auto file = ReadRiveFile("assets/blinko.riv"); + auto baseline = captureBaseline(file.get()); + + { + auto artboard = file->artboardDefault()->instance(); + auto vmi = file->createDefaultViewModelInstance(artboard.get()); + auto machine = artboard->defaultStateMachine(); + machine->bindViewModelInstance(vmi); + machine->advanceAndApply(0.0f); + } + printVMDeltas(file.get(), baseline, "After bind+1advance+destroy"); + CHECK(!hasVMLeak(file.get(), baseline)); +} + +TEST_CASE("blinko: bind + 30 advances", + "[blinko minimal]") +{ + auto file = ReadRiveFile("assets/blinko.riv"); + auto baseline = captureBaseline(file.get()); + + { + auto artboard = file->artboardDefault()->instance(); + auto vmi = file->createDefaultViewModelInstance(artboard.get()); + auto machine = artboard->defaultStateMachine(); + machine->bindViewModelInstance(vmi); + for (int f = 0; f < 30; f++) + machine->advanceAndApply(0.016f); + } + printVMDeltas(file.get(), baseline, "After bind+30advance+destroy"); + CHECK(!hasVMLeak(file.get(), baseline)); +} + +TEST_CASE("blinko: repeated cycles against same File", + "[blinko minimal]") +{ + auto file = ReadRiveFile("assets/blinko.riv"); + auto baseline = captureBaseline(file.get()); + + for (int cycle = 0; cycle < 5; cycle++) + { + { + auto artboard = file->artboardDefault()->instance(); + auto vmi = file->createDefaultViewModelInstance(artboard.get()); + auto machine = artboard->defaultStateMachine(); + machine->bindViewModelInstance(vmi); + machine->advanceAndApply(0.0f); + } + char label[64]; + snprintf(label, sizeof(label), "After cycle %d", cycle + 1); + printVMDeltas(file.get(), baseline, label); + } + CHECK(!hasVMLeak(file.get(), baseline)); +} + +TEST_CASE("blinko: destroy order - machine first", + "[blinko minimal]") +{ + auto file = ReadRiveFile("assets/blinko.riv"); + auto baseline = captureBaseline(file.get()); + + auto artboard = file->artboardDefault()->instance(); + auto vmi = file->createDefaultViewModelInstance(artboard.get()); + auto machine = artboard->defaultStateMachine(); + machine->bindViewModelInstance(vmi); + machine->advanceAndApply(0.0f); + + machine.reset(); + artboard.reset(); + vmi = nullptr; + printVMDeltas(file.get(), baseline, "After machine->artboard->vmi destroy"); + CHECK(!hasVMLeak(file.get(), baseline)); +} + +TEST_CASE("blinko: destroy order - artboard first", + "[blinko minimal]") +{ + auto file = ReadRiveFile("assets/blinko.riv"); + auto baseline = captureBaseline(file.get()); + + auto artboard = file->artboardDefault()->instance(); + auto vmi = file->createDefaultViewModelInstance(artboard.get()); + auto machine = artboard->defaultStateMachine(); + machine->bindViewModelInstance(vmi); + machine->advanceAndApply(0.0f); + + artboard.reset(); + machine.reset(); + vmi = nullptr; + printVMDeltas(file.get(), baseline, "After artboard->machine->vmi destroy"); + CHECK(!hasVMLeak(file.get(), baseline)); +}