fix(P76): guard GLTF instantiation from asset bundle

[eordano]

UNITY-EXPLORER-P76 (4911 evts/68 users — #1 issue): EcsSystemException [CreateGltfAssetFromAssetBundleSystem]. The success branch of ConvertFromAssetBundle calls Utils.TryCreateGltfObject (Object.Instantiate + GetComponentsInChildren), which can throw when the asset bundle's underlying Unity object was destroyed/unloaded (ref-counted bundle evicted) — TryGetAsset only checks the C# list, not Unity-liveness, so it hands back a fake-null object. The throw escaped Update and surfaced as a wrapped EcsSystemException. Wrap in try/catch and emit a failed StreamableLoadingResult (mirroring the existing else-branch), which downstream finalize systems already handle. Uses only symbols already in scope (no new usings). Unverified (no Unity build).

fixes #8845

---

Supersedes #9402: moved from the fork into the org repo so CI workflows receive repository secrets (fork PRs do not).

[github-actions]

New build in progress, come back later!

[github-actions]

Slack notification sent to #explorer-ext-contributions for external review.
To re-send, delete this comment and re-add the ext-contribution label.

[eordano]

Closed in favor of the compound PR #9430 (team decision — all fixes in this set land and iterate there; each fix remains individually reviewable as its own merge commit in #9430).

[decentraland-bot]

PR Review: fix(P76): guard GLTF instantiation from asset bundle

PR: #9416
Author: eordano
Files changed: 1 (+15 −4)
Target: dev

---

Step 2 — Root-cause check

Problem: EcsSystemException wrapping an unhandled throw in CreateGltfAssetFromAssetBundleSystem.Update — the #1 crash (UNITY-EXPLORER-P76, 4911 events / 68 users). The exception originates inside Utils.TryCreateGltfObject, which calls Object.Instantiate / GetComponentsInChildren on a Unity object that AssetBundleData.TryGetAsset returned. When the asset bundle's ref-counted native object has been evicted/destroyed, TryGetAsset still finds the C# wrapper in its Assets dictionary and returns it — a "fake-null" — because it only checks the managed-side collection, not Unity-liveness (asset == null with Unity's overloaded ==).

Does the diff fix the cause or a symptom? It fixes the symptom: the exception is caught and converted into a failed StreamableLoadingResult<GltfContainerAsset>, which downstream finalize systems already handle. The root cause — TryGetAsset at AssetBundleData.cs:97 returning a fake-null object — is not addressed. Adding if (asset == null) return false; after the cast in TryGetAsset would protect all callers (this system, TryDuplicateGltfAssetFromTemplate in Utils.cs, ResolveISSLODSystem, TexturesExtensions, WearablePolymorphicBehaviour, etc.), not just this one code path.

However, the fix is not swallowing the exception — it preserves it as an inner exception in the ArgumentException, and creates the standard failed-result that this ECS pipeline expects. For a top-crash hotfix this is a correct and pragmatic approach. Verdict: PASS, with a recommendation to also fix TryGetAsset as a follow-up.

Step 3 — Design & integration

No new long-lived units introduced. The diff adds a try/catch around the existing TryCreateGltfObject + World.Add block within the private ConvertFromAssetBundle query method. No lifecycle duplication, no per-frame reconciliation, no persistent state outside ECS, no new subscriptions or event hookups.

Teardown/consumption trace: No new subscriptions, callbacks, connections, buffers, or measurements added. Nothing to trace.

Verdict: PASS — no design concerns.

Step 4 — Member audit

No public properties or accessors were added or changed. The diff modifies only the body of a private query method. Step 4 does not apply.

Step 5 — Line-level review

Pass A (blocking issues): None found.

• The catch (Exception e) is broad but appropriate here: Unity fake-null access can throw MissingReferenceException, ArgumentException (from Object.Instantiate with null), or other platform-specific exceptions depending on which line inside TryCreateGltfObject is reached first. A narrow catch would risk missing some failure modes.
• The World.Add in the catch block is safe: if the try body throws before its own World.Add completes, the entity does not yet have the StreamableLoadingResult component, so the catch's World.Add is the first and only structural change — no double-add risk.
• GetReportData() in the catch matches the existing else-branch pattern. Consistent.
• The ArgumentException preserves the inner exception e for diagnostics. Good.
• No ref/in/out invalidation risk: no component references are held across the World.Add call.
• No nullability-contract violations.
• No LINQ in hot path.
• No detached async.

Pass B (design/encapsulation/resource smells): None found.

• The comment (lines 60–63) explains a non-obvious hidden constraint (fake-null from evicted AB), references the Sentry issue for traceability, and states what the code guarantees. It does not narrate external behavior. Acceptable per CLAUDE.md.

No line-level issues require fixes.

Step 6 — Complexity assessment

SIMPLE — Single file, 15 lines added / 4 removed, straightforward defensive try/catch around existing code. No ECS structural changes, no query modifications, no async patterns, no new components or systems.

Step 7 — QA assessment

YES — The change modifies runtime code in the asset loading pipeline (CreateGltfAssetFromAssetBundleSystem). While the change is defensive (catch + failed result), it affects what happens when a user loads a scene with an evicted asset bundle — the user will see a missing GLTF rather than a crash. This behavior change warrants manual verification.

Step 8 — Non-blocking warnings

None. Main scene not modified.

Security review

No security issues found. The change is a pure error-handling addition in an internal ECS system with no user input, no auth, no network operations, and no secrets.

CI status

Builds (macOS, Windows) are failing — these appear to be infrastructure/workflow issues (also failing on the Lint and watchdog checks), not caused by this 15-line diff. semantic/title-matches-convention passes. Tests are pending.

---

REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Single-file defensive try/catch in a GLTF-from-AB loading system; no ECS structural changes, query modifications, or async patterns.
QA_REQUIRED: YES

---

Reviewed by Jarvis 🤖 · Requested by eordano via Slack