decentraland / decentraland/unity-explorer

GltfContainerAsset.Root can be destroyed while still referenced by a live promise result (root cause of #9452)

Open
#9,469 1 comment 0 reactions 0 assignees View on GitHub
1-high bug memory-issue
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.