fix: crash on scene teardown after changeMaterialTextureMap (MaterialInstance destroyed while attached)#343
Open
codydorsettlynn wants to merge 1 commit into
Conversation
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>
Author
|
Claude Fable 5 was responsible for identifying and patching this in my project after some recurrent crashes were identified. This PR implements that patch. Take it or leave it, I just thought I would elevate it here. |
Member
|
thanks for sharing, will try to take a look soon! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Fixes a fatal abort (SIGABRT) on scene teardown after
RenderableManager.changeMaterialTextureMap()has been used, and fixes a permanent GPU memory leak of theTexturecreated per call (plausibly a contributor to #297). The teardown crash is the same class of teardown-order problem as #130.Affects 1.11.0 (shared C++, so both Android and iOS). The bug class exists in earlier versions too, but the filament core bundled with 1.11.0 turned it from a silent leak into a fatal precondition abort.
The bug
changeMaterialTextureMap()duplicates the entity's currentMaterialInstance, binds the duplicate viasetMaterialInstanceAt, and accumulates every duplicate in_materialInstances, which is only released whenRenderableManagerImplis destroyed. TheTexture*created per call is never destroyed at all.At scene teardown the JS wrappers release in nondeterministic order (
withCleanupScopedefers releases throughInteractionManager+setTimeout). WhenRenderableManagerWrapperreleases beforeFilamentAssetWrapper, the duplicate instance is destroyed while still attached to the renderable, and filament aborts:Simply skipping the destroy at teardown is not a fix — it trips filament's other precondition when gltfio's ubershader material provider destroys the parent
Materialduring scene destruction (tested, verified in crash logs):So the duplicates must genuinely be destroyed before
destroyMaterials()runs, but never while attached.Repro
<FilamentScene>+<FilamentView>+<ModelRenderer>with any GLB.renderableManager.changeMaterialTextureMap(entity, materialName, buffer, 'sRGB')at least once (e.g. via<EntitySelector textureMap={...}>).FilamentRenderethread.The
🎨 Change Materialsexample screen reproduces this: press "Change Color", then navigate back. It crashes before this fix and tears down cleanly after (loggingRestoring original material instance.../Destroying material instance...).The fix
Two parts, both in
RNFRenderableManagerImpl.{h,cpp}:Detach-then-destroy deleter. The duplicate's
shared_ptrdeleter dispatches to the renderer thread and, if the instance is still attached to the renderable (teardown path), first restores the slot's original asset-ownedMaterialInstance, then destroys the duplicate. This satisfies both preconditions and is safe in any teardown order:hasComponent(entity)is false → destroy duplicate directly.All destruction work runs on the single renderer dispatcher (FIFO), so the destroys land before the scene deleter runs
materialProvider->destroyMaterials().Per-slot tracking replaces the grow-forever vector. Keyed by
(entity id, primitive index):_slotMaterialInstances— holds only the currently-attached duplicate per slot; replacing the entry releases (→ destroys) the superseded one._slotOriginalInstances— the asset-owned instance each slot had before the first replacement (the restore target)._slotTextures— textures created per slot; a superseded texture is destroyed eagerly (fixes the permanent leak).Verification
externalNativeBuildDebug(NDK/CMake).🎨 Change Materialsexample screen was extended to serve as the regression repro: every press now re-applies the texture map (exercising the superseded-instance/texture destroy path), and navigating back exercises the teardown path.🤖 Generated with Claude Code