fix(misc): scattered null/dispose/cancellation robustness - #9428
Conversation
…, map, plugins, init Production evidence (decentraland Sentry, archive snapshot 2026-07-17): - UNITY-TEST-ENVIRONMENT-16E: 60 events / 1 users, last seen 2026-04-25 - UNITY-TEST-ENVIRONMENT-18V: 5 events / 4 users, last seen 2026-05-14
decentraland-bot
added
the
ext-contribution
|
New build in progress, come back later! |
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review: fix(misc): scattered null/dispose/cancellation robustness
This PR is closed — superseded by #9430 (compound PR). Findings below should be addressed there.
STEP 2 — Root-cause check
The PR fixes scattered robustness issues backed by Sentry production evidence. Most fixes address root causes:
- CancellationToken.None → proper tokens (auth views, ChatPlugin, InWorldCameraPlugin, LoadSmartWearablePreviewSceneSystem, InWorldCameraController): Correctly fixes async tasks outliving their owners. PASS.
- Event unsubscription leaks (ChatPlugin, InWorldCameraPlugin, AnnouncementCreationCardView): Correctly wires teardown at disposal points. PASS.
- Empty-avatar (0-vertex) skinning pipeline (ComputeShaderSkinning → AvatarCustomSkinningComponent → FixedComputeBufferHandler → AvatarInstantiatorSystem → MakeVertsOutBufferDefragmentationSystem): Comprehensive fix across the full Initialize → Skinning → BufferRent → Defragmentation chain. PASS.
Partial FAIL — AvatarBase.cs: Adds if (AvatarAnimator == null) return; in 12+ methods instead of investigating why the animator is null. If AvatarAnimator can legitimately be null (e.g., during teardown when Unity destroys the component), declare it as Animator? so the compiler enforces null-handling at all call sites (CLAUDE.md §11). If it should never be null, investigate the root cause from the Sentry traces. For void methods, prefer null-conditional (AvatarAnimator?.Method()) over explicit guard-and-return.
STEP 3 — Design & integration
No new systems, plugins, or persistent-state units introduced. New CancellationTokenSource fields in InWorldCameraPlugin and LoadSmartWearablePreviewSceneSystem are properly disposed via SafeCancelAndDispose(). The subscribedClipboardManager capture in AnnouncementCreationCardView correctly decouples subscribe/unsubscribe from ViewDependencies lifetime. PASS.
STEP 4 — Member audit
No new public properties or accessors added. PrivateVoiceChatCallStatusServiceNull.PrivateVoiceChatUpdateReceived changed from field-like to accessor-like event ({ add { } remove { } }) — correct null-object pattern suppressing CS0067. No findings.
STEP 5 — Line-level findings
| # | Sev | File | Issue |
|---|---|---|---|
| 1 | P1 | RealUserInAppInitializationFlow.cs:239-243 |
Null-conditional inconsistency creates NRE path |
| 2 | P2 | AvatarBase.cs (12 sites) |
Blanket null-checks on non-nullable field (CLAUDE.md §11) |
| 3 | P2 | 5 files | GetInstanceID() → GetEntityId() — verify semantic equivalence for Physics.BakeMesh(int instanceID, ...) and identity comparison |
| 4 | P2 | Multiple files | Redundant comments that restate what the code does (CLAUDE.md: default to no comments) |
See inline comments for details and suggestions.
Security review: No security issues found. No secrets exposed, no auth changes, no input validation concerns. Exception details logged via ReportHub (internal) are appropriate.
STEP 6 — Complexity
COMPLEX — touches avatar rendering pipeline (GPU skinning/compute buffers), async cancellation patterns, plugin disposal lifecycle, and spans auth, map, camera, chat, voice chat, and initialization subsystems across 41 files.
STEP 7 — QA assessment
YES — changes affect runtime code across auth flow, avatar rendering, map markers, in-world camera, chat, voice chat, scene loading, and user initialization.
STEP 8 — Non-blocking warnings
No Main Scene modification. No additional warnings.
STEP 9 — Verdict
REVIEW_RESULT: FAIL \u274c
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Touches avatar rendering pipeline (skinning/compute buffers), async cancellation patterns, plugin lifecycle, and map/auth/camera subsystems across 41 files.
QA_REQUIRED: YES
Reviewed by Jarvis \U0001f916 \u00b7 Requested by eordano via Slack
| // is a propagated TaskError.Cancelled, not a genuine auth failure. | ||
| if (result.Error?.State != TaskError.Cancelled && !ct.IsCancellationRequested) | ||
| { | ||
| string message = result.Error.AsMessage(); | ||
| ReportHub.LogError(ReportCategory.AUTHENTICATION, message); | ||
| } | ||
| } |
There was a problem hiding this comment.
[P1] Null-conditional inconsistency → NRE path. result.Error?.State on line 239 uses null-conditional, admitting Error could be null. But line 241 calls result.Error.AsMessage() unconditionally — if Error is null and ct.IsCancellationRequested is false, this throws NRE.
Also, the 2-line comment restates what the conditional already expresses (CLAUDE.md: default to no comments).
| // is a propagated TaskError.Cancelled, not a genuine auth failure. | |
| if (result.Error?.State != TaskError.Cancelled && !ct.IsCancellationRequested) | |
| { | |
| string message = result.Error.AsMessage(); | |
| ReportHub.LogError(ReportCategory.AUTHENTICATION, message); | |
| } | |
| } | |
| if (!ct.IsCancellationRequested && result.Error is { } error && error.State != TaskError.Cancelled) | |
| ReportHub.LogError(ReportCategory.AUTHENTICATION, error.AsMessage()); |
| public void SetPointAtLayerWeight(float weight) | ||
| { | ||
| if (AvatarAnimator == null) return; | ||
| AvatarAnimator.SetLayerWeight(rightPointAtLayerIndex, weight); | ||
| } |
There was a problem hiding this comment.
[P2] CLAUDE.md §11: Defensive null-checks against non-nullable declarations. This guard-and-return pattern is repeated in 12+ methods across this file. For void methods, null-conditional (?.) is more concise and idiomatic. The same simplification applies to SetRotationLayerWeight, ResetAnimatorTrigger, SetLayerWeight, and other void methods here.
The broader fix: if AvatarAnimator can legitimately be null (teardown ordering), declare it as Animator? at the field/property site so the compiler enforces handling everywhere.
| public void SetPointAtLayerWeight(float weight) | |
| { | |
| if (AvatarAnimator == null) return; | |
| AvatarAnimator.SetLayerWeight(rightPointAtLayerIndex, weight); | |
| } | |
| public void SetPointAtLayerWeight(float weight) => | |
| AvatarAnimator?.SetLayerWeight(rightPointAtLayerIndex, weight); |
| // an empty avatar (no vertices) has nothing to skin | ||
| if (VertCount == 0) | ||
| return Result.SuccessResult(); |
There was a problem hiding this comment.
[P2] Redundant comment. The comment restates what if (VertCount == 0) return SuccessResult() already says. CLAUDE.md: default to no comments; only add one when the WHY is non-obvious.
| // an empty avatar (no vertices) has nothing to skin | |
| if (VertCount == 0) | |
| return Result.SuccessResult(); | |
| if (VertCount == 0) | |
| return Result.SuccessResult(); |
| // A null profile must fail the operation: adding it to the world poisons every | ||
| // Profile-querying system with a per-frame NRE (observed when the profiles endpoint | ||
| // returns no avatars for the signed-in wallet). | ||
| if (profile == null) | ||
| throw new System.InvalidOperationException("Self profile could not be resolved (profiles endpoint returned no avatar for the signed-in wallet) — cannot create the player avatar."); |
There was a problem hiding this comment.
[P2] Redundant comment. The 3-line comment is redundant with the exception message, which already explains the scenario and impact. CLAUDE.md: default to no comments.
| // A null profile must fail the operation: adding it to the world poisons every | |
| // Profile-querying system with a per-frame NRE (observed when the profiles endpoint | |
| // returns no avatars for the signed-in wallet). | |
| if (profile == null) | |
| throw new System.InvalidOperationException("Self profile could not be resolved (profiles endpoint returned no avatar for the signed-in wallet) — cannot create the player avatar."); | |
| if (profile == null) | |
| throw new System.InvalidOperationException("Self profile could not be resolved (profiles endpoint returned no avatar for the signed-in wallet) \u2014 cannot create the player avatar."); |
| // ENABLE_SDK_OBSERVABLES is a deliberate compile-time toggle; the else branch is kept intact | ||
| // so the SDK-observables feature can be disabled by flipping the constant. Suppress the | ||
| // unreachable-code warning for the currently-disabled branch without removing it. | ||
| #pragma warning disable CS0162 |
There was a problem hiding this comment.
[P2] Over-commented pragma. Three comment lines to explain a standard #pragma warning disable. A short inline comment suffices.
| // ENABLE_SDK_OBSERVABLES is a deliberate compile-time toggle; the else branch is kept intact | |
| // so the SDK-observables feature can be disabled by flipping the constant. Suppress the | |
| // unreachable-code warning for the currently-disabled branch without removing it. | |
| #pragma warning disable CS0162 | |
| #pragma warning disable CS0162 // compile-time toggle: both branches kept |
2 participants
across auth, map, plugins, init
Production evidence (decentraland Sentry, archive snapshot 2026-07-17):
Supersedes #9414: moved from the fork into the org repo so CI workflows receive repository secrets (fork PRs do not).