From 832e13a2beb8492468e1b650e6c860df8c4ada77 Mon Sep 17 00:00:00 2001
From: codydorsettlynn <cody@drs.com.au>
Date: Mon, 13 Jul 2026 12:43:42 +0800
Subject: [PATCH] fix: crash on scene teardown after changeMaterialTextureMap
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

changeMaterialTextureMap duplicated the entity's MaterialInstance and
accumulated every duplicate until the RenderableManager was destroyed.
Because JS wrappers release in nondeterministic order at scene teardown,
a duplicate could be destroyed while still attached to its renderable,
aborting with filament's "destroying MaterialInstance which is still in
use by Renderable" precondition. The Texture created per call was never
destroyed at all (permanent GPU memory leak).

Track instances and textures per (entity, primitive) slot instead. The
duplicate's deleter now detaches a still-attached instance first by
restoring the slot's original asset-owned instance, then destroys the
duplicate on the renderer thread — satisfying both filament teardown
preconditions in any release order. Superseded instances and textures
of a slot are destroyed eagerly when replaced.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---
 examples/Shared/src/ChangeMaterials.tsx       | 14 +++--
 package/cpp/core/RNFRenderableManagerImpl.cpp | 58 ++++++++++++++++---
 package/cpp/core/RNFRenderableManagerImpl.h   | 16 ++++-
 3 files changed, 74 insertions(+), 14 deletions(-)

diff --git a/examples/Shared/src/ChangeMaterials.tsx b/examples/Shared/src/ChangeMaterials.tsx
index f3a715c1..22a09029 100644
--- a/examples/Shared/src/ChangeMaterials.tsx
+++ b/examples/Shared/src/ChangeMaterials.tsx
@@ -9,7 +9,11 @@ import RocketGlb from '@assets/rocket.glb'
 const baseColorBlueImage = require('@assets/rocket_BaseColor_Blue.png')
 
 function Renderer() {
-  const [showBlue, setShowBlue] = React.useState(false)
+  // Regression test for a crash on scene teardown: after changing a texture map, navigating back
+  // from this screen must not abort with filament's "destroying MaterialInstance which is still
+  // in use by Renderable" precondition. Every press re-applies the texture map, which also
+  // exercises destroying the superseded material instance and texture of a slot.
+  const [applyCount, setApplyCount] = React.useState(0)
   const blueBaseColorBuffer = useBuffer({ source: baseColorBlueImage })
   const materialName = 'Toy Ship'
 
@@ -20,18 +24,18 @@ function Renderer() {
         <DefaultLight />
 
         <Model source={RocketGlb} translate={[0, -1, 0]}>
-          {blueBaseColorBuffer != null && showBlue && (
-            <>
+          {blueBaseColorBuffer != null && applyCount > 0 && (
+            <React.Fragment key={applyCount}>
               <EntitySelector byName="Tip" textureMap={{ materialName, textureSource: blueBaseColorBuffer }} />
               <EntitySelector byName="Wings" textureMap={{ materialName, textureSource: blueBaseColorBuffer }} />
-            </>
+            </React.Fragment>
           )}
         </Model>
       </FilamentView>
       <Button
         title="Change Color"
         onPress={() => {
-          setShowBlue(true)
+          setApplyCount((count) => count + 1)
         }}
       />
     </SafeAreaView>
diff --git a/package/cpp/core/RNFRenderableManagerImpl.cpp b/package/cpp/core/RNFRenderableManagerImpl.cpp
index b86b4a3f..a493b77a 100644
--- a/package/cpp/core/RNFRenderableManagerImpl.cpp
+++ b/package/cpp/core/RNFRenderableManagerImpl.cpp
@@ -137,22 +137,66 @@ void RenderableManagerImpl::changeMaterialTextureMap(std::shared_ptr<EntityWrapp
   // This material instance still belongs to the original asset and will be cleaned up when the asset is destroyed
   MaterialInstance* materialInstance = renderableManager.getMaterialInstanceAt(instance, primitiveIndex);
 
+  std::pair<uint32_t, size_t> slotKey = {entityInstance.getId(), primitiveIndex};
+
+  // Remember the asset-owned instance that was attached before our first replacement on this slot.
+  // The deleter below restores it if the duplicate is destroyed while still attached (teardown),
+  // so the renderable never references a destroyed instance and the duplicate can be destroyed
+  // without hitting filament's "still in use by Renderable" precondition. The original is only
+  // touched when the entity still exists, which implies the asset (its owner) is still alive.
+  MaterialInstance* originalInstance;
+  auto originalIt = _slotOriginalInstances.find(slotKey);
+  if (originalIt == _slotOriginalInstances.end()) {
+    originalInstance = materialInstance;
+    _slotOriginalInstances[slotKey] = originalInstance;
+  } else {
+    originalInstance = originalIt->second;
+  }
+
   // The texture might not be loaded yet, but we can already set it on the material instance
   auto engine = _engine;
   auto dispatcher = _rendererDispatcher;
+  Entity slotEntity = entityInstance;
+  size_t slotIndex = primitiveIndex;
   std::shared_ptr<MaterialInstance> newInstance =
-      std::shared_ptr<MaterialInstance>(MaterialInstance::duplicate(materialInstance), [engine, dispatcher](MaterialInstance* instance) {
-        dispatcher->runAsync([engine, instance]() {
-          Logger::log(TAG, "Destroying material instance %p", instance);
-          engine->destroy(instance);
-        });
-      });
+      std::shared_ptr<MaterialInstance>(MaterialInstance::duplicate(materialInstance),
+                                        [engine, dispatcher, slotEntity, slotIndex, originalInstance](MaterialInstance* instance) {
+                                          dispatcher->runAsync([engine, slotEntity, slotIndex, originalInstance, instance]() {
+                                            RenderableManager& renderableManager = engine->getRenderableManager();
+                                            if (renderableManager.hasComponent(slotEntity)) {
+                                              RenderableManager::Instance renderable = renderableManager.getInstance(slotEntity);
+                                              if (slotIndex < renderableManager.getPrimitiveCount(renderable) &&
+                                                  renderableManager.getMaterialInstanceAt(renderable, slotIndex) == instance) {
+                                                // Still attached (teardown path) — detach by restoring the asset's original instance.
+                                                // Destroying an attached instance, or letting the material provider destroy the parent
+                                                // material while duplicates are alive, is a fatal precondition in filament.
+                                                Logger::log(TAG, "Restoring original material instance on entity before destroy");
+                                                renderableManager.setMaterialInstanceAt(renderable, slotIndex, originalInstance);
+                                              }
+                                            }
+                                            Logger::log(TAG, "Destroying material instance %p", instance);
+                                            engine->destroy(instance);
+                                          });
+                                        });
 
   auto sampler = TextureSampler(TextureSampler::MinFilter::LINEAR, TextureSampler::MagFilter::LINEAR, TextureSampler::WrapMode::REPEAT);
   Texture* texture = createTextureFromBuffer(textureBuffer, textureFlags);
   newInstance->setParameter("baseColorMap", texture, sampler);
   renderableManager.setMaterialInstanceAt(instance, primitiveIndex, newInstance.get());
-  _materialInstances.push_back(newInstance);
+
+  // Replacing the slot entry releases the superseded instance — it is detached now (the renderable
+  // points at newInstance), so its deleter can safely destroy it.
+  _slotMaterialInstances[slotKey] = newInstance;
+  // The superseded texture is no longer sampled by any attached instance; destroy it.
+  auto previousTexture = _slotTextures.find(slotKey);
+  if (previousTexture != _slotTextures.end()) {
+    Texture* oldTexture = previousTexture->second;
+    dispatcher->runAsync([engine, oldTexture]() {
+      Logger::log(TAG, "Destroying texture %p", oldTexture);
+      engine->destroy(oldTexture);
+    });
+  }
+  _slotTextures[slotKey] = texture;
 
   // Load the texture
   startUpdateResourceLoading();
diff --git a/package/cpp/core/RNFRenderableManagerImpl.h b/package/cpp/core/RNFRenderableManagerImpl.h
index 5b968c7f..3a94c03e 100644
--- a/package/cpp/core/RNFRenderableManagerImpl.h
+++ b/package/cpp/core/RNFRenderableManagerImpl.h
@@ -15,6 +15,9 @@
 #include <filament/RenderableManager.h>
 #include <gltfio/TextureProvider.h>
 
+#include <map>
+#include <utility>
+
 namespace margelo {
 using namespace filament;
 using namespace gltfio;
@@ -91,8 +94,17 @@ class RenderableManagerImpl {
   std::shared_ptr<Engine> _engine;
   std::shared_ptr<Dispatcher> _rendererDispatcher;
   std::shared_ptr<TextureProvider> _textureProvider;
-  // Keep a list of all material instances the RenderableManager creates, so we can clean them up when the RenderableManager is
-  std::vector<std::shared_ptr<MaterialInstance>> _materialInstances;
+  // Material instances created by changeMaterialTextureMap, keyed by (entity id, primitive index).
+  // Only the latest instance per slot is attached to the renderable; replacing a slot entry
+  // releases the superseded (now detached) instance, whose deleter destroys it. At manager
+  // teardown the deleter detaches a still-attached instance first (restoring the slot's original
+  // asset-owned instance) — see changeMaterialTextureMap.
+  std::map<std::pair<uint32_t, size_t>, std::shared_ptr<MaterialInstance>> _slotMaterialInstances;
+  // The asset-owned instance each slot had before our first replacement (restore target on teardown).
+  std::map<std::pair<uint32_t, size_t>, MaterialInstance*> _slotOriginalInstances;
+  // Textures created by changeMaterialTextureMap, keyed the same way. A superseded texture is no
+  // longer sampled by the replacement instance and is destroyed eagerly (previously these leaked).
+  std::map<std::pair<uint32_t, size_t>, Texture*> _slotTextures;
   std::vector<std::unique_ptr<DebugVertex[]>> _debugVerticesList;
 
 private: