fix(misc): scattered null/dispose/cancellation robustness across auth, map, plugins, init#9378
fix(misc): scattered null/dispose/cancellation robustness across auth, map, plugins, init#9378eordano wants to merge 1 commit into
Conversation
|
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. |
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review: fix(misc): scattered null/dispose/cancellation robustness across auth, map, plugins, init
STEP 2 — Root-cause check
This PR addresses multiple independent robustness issues: fire-and-forget async flows using CancellationToken.None (symptom of missing lifecycle-scoped cancellation), Unity teardown crashes from accessing destroyed objects, swallowed exceptions hiding diagnostic info, and compiler warnings from Unity 6000.4 API deprecations. Each fix targets the actual cause — properly scoping CTS to owner lifetimes, guarding against Unity's destroyed-object state, surfacing exception details, and modernizing API calls. PASS.
STEP 3 — Design & integration
CTS ownership: The two new CancellationTokenSource instances (InWorldCameraPlugin.pluginCts, LoadSmartWearablePreviewSceneSystem.systemCts) are correctly owned by their respective classes and cancelled+disposed in the matching Dispose()/OnDispose() lifecycle methods. The auth screen views correctly use destroyCancellationToken (MonoBehaviour lifecycle) rather than introducing a new CTS. PASS.
AnnouncementCreationCardView.subscribedClipboardManager: Correctly captures the ClipboardManager reference at subscription time so that OnDestroy unsubscribes from the same instance, avoiding the bug where ViewDependencies.ClipboardManager could return a different instance during teardown. PASS.
ComputeShaderSkinning empty guard: See P1 finding below — the returned component has a Buffers struct whose computeSkinningBufferContainer is never initialized, causing a deferred NRE on disposal. FAIL.
STEP 4 — Member audit
No new public properties or accessors added. The subscribedClipboardManager field in AnnouncementCreationCardView is private, single-use (captures the subscription target for matching unsubscribe). PASS.
STEP 5 — Line-level findings
See inline comments below.
STEP 6 — Complexity
Touches 37 files across auth flow, avatar rendering pipeline (compute shader skinning), async/UniTask patterns, plugin disposal, and map layer controllers. Multiple subsystems affected.
STEP 7 — QA assessment
Changes affect runtime code: auth screen transitions, avatar skinning, camera plugin, map layers, input system disposal, voice chat.
STEP 8 — Non-blocking warnings
No Main.unity changes detected.
Security review
No hardcoded secrets, credentials, or API keys found. No auth bypass risks — the CancellationToken.None → destroyCancellationToken changes improve teardown safety. The exception message in LoadPlayerAvatarStartupOperation contains no sensitive data (no wallet address, no endpoint URL). No security issues found.
REVIEW_RESULT: FAIL ❌
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Touches avatar GPU skinning pipeline, auth screen async flows, plugin lifecycle/disposal, and map layer controllers across 37 files.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by Esteban Ordano (<@U0AJ91P1KBP>) via Slack
4b280f8 to
e15a8cc
Compare
c93a652 to
e15a8cc
Compare
…, map, plugins, init
decentraland-bot
left a comment
There was a problem hiding this comment.
Re-review: fix(misc): scattered null/dispose/cancellation robustness across auth, map, plugins, init
Previous findings — status
-
[P1] ComputeShaderSkinning NRE on disposal✅ Fixed ine0d6d1cc. Empty guard now usesdefault(Buffers)(no orphaned ComputeBuffer),DisposeBuffers()is null-safe, materials list is pool-rented viaUSED_SLOTS_POOL.Get(). Additionally, thorough pipeline guards added:ComputeSkinningearly-outs onVertCount == 0,AvatarInstantiatorSystemskipsRentfor zero VertCount,FixedComputeBufferHandler.Releaseguards zero-length slices, andMakeVertsOutBufferDefragmentationSystemskips defrag for zero-length regions. Well-handled. -
[P2] AvatarBase inconsistent null guards✅ Fixed ine0d6d1cc. AllAvatarAnimator-accessing methods now have null guards:SetPointAtLayerWeight,SetRotationLayerWeight,IsAnimatorInTag,GetAnimatorCurrentStateTag,ResetAnimatorTrigger,ResetState,SetLayerWeight,GetAnimatorBool,SetAnimatorBool,GetAnimatorFloat,ReplaceEmoteAnimation, andStartMaskedLegacyEmote.
New changes in this revision
- InWorldCameraPlugin.Dispose(): Now unsubscribes
OnIdentityChangedbefore disposing, and usesSafeCancelAndDispose(). Correct ordering — prevents the handler from firing on a cancelled CTS. ✅ - ChatPlugin.Dispose(): Now unsubscribes
OnIdentityClearedandOnIdentityChanged(lines 196-197), matching subscriptions at lines 433-434. Fixes event leak. ✅ - VoiceChatParticipantEntryPresenter.OnOpenPassport(): Correctly keeps
CancellationToken.Nonewith a comment explaining the passport UI must outlive the pooled list entry. ✅ - LoadSmartWearablePreviewSceneSystem: Uses
SafeCancelAndDispose()— consistent with project convention. ✅
STEP 2 — Root-cause check
All fixes target root causes. The empty-avatar pipeline guards are thorough — every touchpoint (skinning, buffer rent, defrag) is protected. PASS.
STEP 3 — Design & integration
CTS ownership is correct. Event subscription symmetry verified in both InWorldCameraPlugin and ChatPlugin. The VoiceChatParticipantEntryPresenter keeps CancellationToken.None with clear justification — the passport action must survive entry recycling. PASS.
STEP 5 — Line-level review
No blocking issues found. All changes are clean and well-commented.
Security review
No security issues found (unchanged from first review).
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Touches avatar GPU skinning pipeline, auth screen async flows, plugin lifecycle/disposal, and map layer controllers across 40+ files.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by Esteban Ordano (<@U0AJ91P1KBP>) via Slack
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review — #9378 fix(misc): scattered null/dispose/cancellation robustness
STEP 2 — Root-cause check
PASS. The PR addresses genuine root causes across multiple subsystems:
- Missing unsubscribes (ChatPlugin, InWorldCameraPlugin): event handlers outlived their owners → real leak.
CancellationToken.Nonein lifecycle-scoped flows: auth views, camera plugin, smart-wearable system all fired detached async that couldn't be cancelled on teardown → real resource waste / post-dispose callbacks.- Null self-profile poisoning ECS (LoadPlayerAvatarStartupOperation):
profile!on a nullable result passed a null into every Profile-querying system → per-frame NRE. - Empty avatar mesh crashing skinning pipeline (ComputeShaderSkinning): zero verts/bones hit
FixedComputeBufferHandler.Rentwhich throws on length 0. - Swallowed exceptions in IGetHash: exception type/message discarded → impossible to diagnose hybrid-scene fetch failures.
The AvatarBase null-checks are defensive against Unity's teardown behavior (serialized references become "Unity null" after Destroy). The behavior is real, but the fix is treating a symptom — see P2 finding below.
STEP 3 — Design & integration
PASS. No new long-lived units introduced. All changes modify existing classes at their existing lifecycle boundaries:
pluginCtsin InWorldCameraPlugin /systemCtsin LoadSmartWearablePreviewSceneSystem — scoped to the owning plugin/system, cancelled inDispose()/OnDispose(). Correct lifecycle pattern.- Empty-avatar sentinel (
VertCount == 0,default(Buffers)) — defense-in-depth across the GPU skinning pipeline. Guards are placed at every entry point that touches the buffer (ComputeSkinning,DisposeBuffers,Rent-skip,Release-skip, defragmentation-skip). TheUSED_SLOTS_POOL.Get()on the empty path keeps pool balance. Sound design for an edge case observed in production. subscribedClipboardManagerin AnnouncementCreationCardView — captures the exact instance subscribed to, ensuringOnDestroyunsubscribes from the same object even if the static accessor changes. Correct pattern.- ChatPlugin identity-cache unsubscribes — subscriptions at lines 430–431 (
OnIdentityCleared,OnIdentityChanged) were the only two missing mirrors. Other subscriptions go throughpluginScope(disposed) or are explicitly unsubscribed. Complete fix.
TEARDOWN TRACE:
InWorldCameraPlugin: ctor subscribesOnIdentityChanged += FetchCameraReelStorage→Dispose()now unsubscribes ✅ + cancelspluginCts✅ + existingperformed -=✅.ChatPlugin:InitializeAsyncsubscribesOnIdentityCleared +=andOnIdentityChanged +=→Dispose()now unsubscribes both ✅ + existingpluginCts.SafeCancelAndDispose()✅.LoadSmartWearablePreviewSceneSystem:systemCtscreated →OnDispose()callsSafeCancelAndDispose()✅.AnnouncementCreationCardView:Awake()subscribesOnPaste +=on capturedsubscribedClipboardManager→OnDestroy()unsubscribes via same reference ✅.
STEP 4 — Member audit
No new public properties or accessors introduced. The ComputeSkinning early-return for VertCount == 0 is an existing public method gaining a guard — single consumer is the ECS query, count: 1. No single-use / absent-≠-false issues.
STEP 5 — Line-level findings
See inline comments. Summary of findings:
| # | Severity | File | Issue |
|---|---|---|---|
| 1 | P2 | AvatarBase.cs |
Null-checks on non-nullable AvatarAnimator — declaration should be Animator? if null is a legitimate teardown state (CLAUDE.md §11) |
| 2 | P2 | RealUserInAppInitializationFlow.cs |
Inconsistent null-safety: result.Error?.State (null-conditional) on L236 but result.Error.AsMessage() (non-conditional) on L239 |
| 3 | P2 | ContextualImage.cs |
Same pattern as AvatarBase: null-check on non-nullable image field |
STEP 6 — Complexity
COMPLEX — touches avatar rendering pipeline (GPU skinning, compute shaders, buffer handlers), async/cancellation flows across auth/plugins/systems, 41 files across 10+ subsystems.
STEP 7 — QA assessment
YES — changes runtime code in auth views, avatar rendering, voice chat, map renderer, in-world camera, input handling, and startup flow.
STEP 8 — Non-blocking warnings
No Main Scene modified. No warnings.
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Touches avatar GPU skinning pipeline, async/cancellation flows across auth/plugins/systems, and plugin disposal across 41 files.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by unknown (<@unknown>) via Slack
|
Warnings count reduced: 19134 => 19102 |
Pull Request Description
What does this PR change?
A batch of small, scattered robustness fixes — no feature changes. Grouped by theme:
Lifecycle-scoped cancellation (replaces
CancellationToken.Nonein fire-and-forget flows)LobbyForExistingAccount,LobbyForNewAccount,LoginSelection,ProfileFetching,VerificationDapp,VerificationOTP) now passdestroyCancellationTokento their show/hide animations.InWorldCameraPluginandLoadSmartWearablePreviewSceneSystemown a CTS cancelled+disposed on teardown;InWorldCameraControllerandChatPluginuse their existing lifecycle tokens; plugins unsubscribe their identity-cache event handlers on dispose so no handler can outlive its token source.VoiceChatParticipantEntryPresenterdeliberately keeps the passport popup onCancellationToken.Noneso it survives the pooled entry being recycled.Null/dispose hardening in Unity teardown paths
ContextualImage.OnDisable/OnDestroyguard against already-destroyedimage/asset.AvatarBase.SetAnimatorFloat/Int/Triggerearly-out when the animator is destroyed.AnnouncementCreationCardView.OnDestroyguards destroyed serialized references and unsubscribes from the sameClipboardManagerinstance it subscribed to.LoadPlayerAvatarStartupOperationnow throws when the self profile resolves to null instead of adding a null profile to the world (which NRE'd every Profile-querying system per frame).RealUserInAppInitializationFlowno longer logs shutdown cancellation as an authentication error.ComputeShaderSkinningguards empty avatar mesh data (vertCount/boneCount == 0): the component is created in an explicit empty state (default buffers, pool-rented materials list) thatComputeSkinning/Disposeno-op on, and zero-length regions are excluded fromFixedComputeBufferHandlerrent/release and defragmentation remapping.InputPlugin.DisposedisablesDCLInputbeforeReset().Diagnostics
IGetHash(hybrid scenes) logs the actual exception type/message instead of silently swallowing it.Warning cleanup / Unity 6000.4 API modernization
GetInstanceID()→GetEntityId(),FindObjectOfType→FindAnyObjectByType.asyncmethods withoutawait→UniTask.CompletedTask(map layer controllers).#pragma warning disable CS0162around the deliberateENABLE_SDK_OBSERVABLEScompile-time branch inSceneFactory.DCLVersion.inEditorVersionreturns null only in non-editor builds (#elseinstead of unreachable code).null!/Array.Empty) inSkyboxRenderController,SettingsModuleBindings,DebugSettings; no-op event accessors inPrivateVoiceChatCallStatusServiceNull; removed unreachable code inEventListItemViewand unused locals inAvatarShapeVisibilitySystemShould.Test Instructions
Steps (standard run):
Expected result: Client boots, logs in, and runs with no new NREs or
ObjectDisposedExceptions — especially during screen transitions and shutdown.Steps (fresh account):
Expected result: Full new-account auth flow (login selection → OTP/dapp verification → lobby) shows and hides each screen cleanly; no errors logged when screens are dismissed mid-animation.
Automation (if applicable):
metaforge explorer test XXXXPrerequisites
Test Steps
AUTHENTICATIONerrors caused by shutdown cancellation.Additional Testing Notes
async→UniTask.CompletedTaskconversions must not change behavior).Quality Checklist