decentraland / decentraland/unity-explorer
GltfContainerAsset.Root can be destroyed while still referenced by a live promise result (root cause of #9452)
- Dominant language
- C#
- Stars
- 23
- Forks
- 17
- Avg merge
- 2d 16h
- Merged PRs (30d)
- 101
Description
## Summary
Follow-up to #9452. That crash (`NullReferenceException` in `ResetGltfContainerSystem` → `GltfContainerAssetsCache.DereferenceFinalOperation`) is being fixed with a defensive guard, but the guard only stops the crash. It does not address the root cause: a `GltfContainerAsset` whose `Root` GameObject is already destroyed is still reachable through a live promise result and gets dereferenced again. This tracks that root cause.
## What actually crashes
`DereferenceFinalOperation` touches `asset.Root` (`SetActive` / `transform.SetParent`). The null is a **destroyed Unity GameObject**, not a managed null:
- `asset` is non-null — `TryReleaseAsset` calls `entityCollidersSceneCache.Remove(result.Asset)` one line earlier and would throw first otherwise.
- `Root` is a `readonly` field assigned non-null in the constructor, so never managed-null.
- So `Root` was destroyed by `GltfContainerAsset.Dispose()` (via `Unload`). Under IL2CPP, calling a method on a destroyed object surfaces as an NRE (matches the `il2cpp-codegen.h Dereference` frames).
## Root cause: use-after-free
1. `GltfContainerComponent.Promise` resolves with `Succeeded == true` and a valid `Asset`.
2. The asset is pooled via `cache.Dereference`, but the promise result referencing it is not always cleared: `CleanUpGltfContainerSystem.DestroyGLTFContainer` calls `Promise.ForgetLoading`, which does **not** clear the cached `Result`. The component still points at the pooled asset.
4. `Unload` later disposes that pooled asset, destroying `Root`.
5. A later release path reads the stale `Result`, sees `Succeeded == true`, and dereferences the disposed asset → touches the destroyed `Root` → crash (and re-pools a dead instance).
Contributing detail: `StreamableLoadingResult(T? asset)` sets `Succeeded = true` even for a null asset, and `AssetPromise.TryGetResult` caches `Result`, so it survives `ForgetLoading`.
## Why the PR fix is not enough
The PR guards `Dereference` by skipping when `asset.Root` compares null. It stops the crash and avoids re-pooling a dead instance, but the stale promise result still outlives its asset, and consumers like `EntityCollidersCacheExtensions.Remove` can still read collider lists already released back to the pool by `Dispose()`.
## Repro validation
A temporary `ReportHub.LogWarning` in the guard (fires in all builds — `LogWarning` has no `[Conditional]`) confirms the destroyed-`Root` path is hit:
```
[GLTF_CONTAINER] Skipped dereferencing GltfContainerAsset '' with an already destroyed Root
```
Marked `remove before merge`.
## References
- Crash issue: #9452
- Sentry: [UNITY-EXPLORER-P83](https://decentraland.sentry.io/issues/7503170033/)
- Files: `GltfContainerAssetsCache`, `ResetGltfContainerSystem`, `CleanUpGltfContainerSystem.DestroyGLTFContainer`, `GltfContainerAsset.Dispose`, `StreamableLoadingResult`, `AssetPromise`
Contributor guide
Research direction
Start by tracing the promise and asset lifecycle through CleanUpGltfContainerSystem.DestroyGLTFContainer, StreamableLoadingResult, AssetPromise, GltfContainerAsset.Dispose, and GltfContainerAssetsCache. Check how ForgetLoading and cached Result interact with Unload and the release paths in ResetGltfContainerSystem. Done means a live promise result cannot retain a disposed asset, and later dereference or collider-cache removal cannot access a destroyed Root.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp, unity
- Domain
- game-dev
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100