fix: bugsweep aug16 - #9765
Conversation
All 24 branches of the aug16 bug-sweep campaign combined into one branch for batch testing/QA. Every fix carries its own regression test; the full set passed batched EditMode RED/GREEN validation (25/25 tests) plus 3-lens adversarial review. Individual branches: bugsweep/<slug>. Ledger: workspace INDEX.md (forge mine/bugsweep-aug16). ## Included fixes - bugsweep/ab-archive-cache-corrupt — fix: evict corrupt Unity AB cache entries and retry the download once - bugsweep/auth-profile-fetch-timeout — fix: cancel and retry stalled login profile fetch instead of abandoning it - bugsweep/chat-mention-analytics-userid-json — fix: send mention wallet strings, not UserId objects, in chat analytics - bugsweep/comms-topic-string-alloc — perf: eliminate per-message topic string alloc in comms receive path - bugsweep/crdt-syncbuffer-semaphore-leak — fix: release CRDT sync buffer rent slot on failed batches - bugsweep/credits-topup-stuck-spinner — fix: auto-cancel credits top-up when focus returns without payment - bugsweep/debug-console-loghistory-thread-safety — fix: make debug console log ingestion thread-safe via pending queue - bugsweep/emote-equip-null-geturn-nre — fix: don't hand unresolved avatar elements to backpack equip flows - bugsweep/eventbus-offthread-closure-alloc — perf: pool the off-main-thread EventBus publish continuation - bugsweep/finalize-gltf-container-nre — fix: keep checked-out GLTF clones alive when AssetPreLoadCache clears - bugsweep/genesis-ram-climb-emotes — fix: dereference emote assets after promise consumption so memory pressure can unload them - bugsweep/ktx-unity-dll-load — fix: fall back to unconverted textures when the ktx native decoder cannot load - bugsweep/mainui-plugin-dispose-nre — fix: dispose never-shown AuthenticationScreenController without NRE - bugsweep/minimap-pixelated-fullscreen — fix: resize map render texture when screen resolution changes - bugsweep/notifications-poll-list-alloc — perf: remove per-poll List allocation in notifications polling - bugsweep/private-conversation-userstate-nre — fix: resolve private conversation user state without friends service - bugsweep/rusteth-privatekey-pad32 — fix: left-pad private key bytes to 32 before rust sign-server init - bugsweep/scenescache-duplicate-parcel-add — fix: discard duplicate scene facade for already-cached parcels and serialize realm changes - bugsweep/segment-network-errors — fix: stop double-reporting Segment transport errors and downgrade lossless retries to warnings - bugsweep/thumbnail-sticky-failure-rethrow — fix: retry avatar thumbnail loads after a failed attempt instead of rethrowing forever - bugsweep/trigger-area-layer-update — fix: apply TriggerArea collisionMask and mesh updates after creation - bugsweep/unload-thumbnail-nre-blocks-memory-release — fix: guard thumbnail disposal on Succeeded so failed results don't NRE and abort cache cleanup - bugsweep/wearable-mainfile-insecure-url — fix: allow cleartext http to loopback only, upgrade other http to https - bugsweep/websocket-closeasync-nre — fix: abandon the parked connect await when an unestablished close aborts Fixes #3661, #6484, #7792, #7832, #7907, #8023, #8884, #8891, #8902, #8911, #9182, #9206, #9263, #9346, #9447, #9451, #9531, #9665, #9692, #9737, #9738, #9741
eordano
added
force-build
🚦 CI Status
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. Warnings count reduced: 13116 => 12925 Warnings/errors in files changed by this PR (51)Some Unity tests failed ❌
Failed tests (2)
|
eordano
removed
the
force-lint
Warning-ratchet compliance for the Lint gate (count must be strictly below baseline): fixes the InspectCode findings in every file this PR touches — nullability made honest (pattern-matched bindings, no null-forgiving in production code), redundant usings/suppressions removed, naming aligned, wire-format DTOs given the file-level ReSharper convention. Also pauses Unity logging around the KTX capability probe so its expected error code cannot leak into strict test log asserts. Full 138-test EditMode suite re-validated after the cleanup.
This comment has been minimized.
This comment has been minimized.
eordano
added
the
force-lint
Full 9-step automated-review protocol run over every lane, then a fix round: 29 blocking findings resolved (root-cause honesty in comments, lifecycle mirrors, silent-failure paths surfaced, dead code removed, PR-body accuracy), plus three cross-lane semantic conflicts caught and reconciled by the combined validation (singleton test double-init, a WithFallback API collapse consumed by a sibling lane, a renamed preload-cache method with an out-of-diff caller). All 24 lanes end at REVIEW_RESULT: PASS. Re-validated on the v16 EditMode lane: 178/178 tests pass (tag aug17-final-green6).
eordano
added
force-lint
The auto-cancel-on-focus-return approach is withdrawn: the missing feature needs backend and Stripe-integration changes that are being discussed, and a client-side timer/focus heuristic is not the real solution. All credits-topup files return to their base state; the other 23 fixes are untouched (their files are disjoint).
The suite moved to Infrastructure/Utility/Tests during the review round; file-content pushes cannot express moves, so the old copy lingered.
eordano
added
force-lint
This comment has been minimized.
This comment has been minimized.
… storage hits The in-world suite caught a regression in the outfit-equip path: awaiting the full wearable fetch (asset bundles included) before firing the equip event stalled outfit application and silently dropped failed-DTO pointers. Equip now settles on DTO resolution only - the full fetch continues in the background - and re-appends recovered entries, preserving the resolved-only guarantee. RED/GREEN validated on the EditMode lane; in-world hair-revert flow re-verified on the rig.
eordano
added
force-lint
…t read a pooled list after release Sturdiness hardening on the outfit fetch: the detached background provider fetch and the settle predicate now read an owned URN[] snapshot instead of the pooled missingUrns list (released when ExecuteAsync returns), so a future provider that does not copy pointers before its first await cannot turn this into a pooled-list use-after-free. Discriminating EditMode test added; 9/9 green on the rig.
eordano
changed the title
decentraland-bot
added
the
ext-contribution
|
Slack notification sent to #explorer-ext-contributions for external review. |
This comment has been minimized.
This comment has been minimized.
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review — #9765 fix: bugsweep aug16
Branch: bugsweep/aug16-consolidated → dev
Files changed: 100+ (+5584 −444)
CI: No checks reported yet
STEP 2 — Root-cause check
All 24 fixes address root causes, not symptoms:
| Fix | Root-cause? | Evidence |
|---|---|---|
| ab-archive-cache-corrupt | ✅ | Evicts the poison Unity cache entry so the next load retries over network |
| auth-profile-fetch-timeout | ✅ | Linked CTS per attempt cancels the stuck profile fetch |
| chat-mention-analytics-userid-json | ✅ | Extracts .Value wallet string before JArray.Add |
| comms-topic-string-alloc | ✅ | Byte-keyed copy-on-write snapshot eliminates per-message string materialization |
| crdt-syncbuffer-semaphore-leak | ✅ | try/catch + AbortSyncCommandBuffer ensures exactly-one-of abort-or-apply |
| debug-console-loghistory-thread-safety | ✅ | Pending queue + drain pattern, lock-guarded cross-thread |
| emote-equip-null-geturn-nre | ✅ | Guards element.DTO == null before invoking callback |
| eventbus-offthread-closure-alloc | ✅ | PooledContinuation<T> copy-to-locals-then-recycle eliminates closure alloc |
| finalize-gltf-container-nre | ✅ | Ownership corrected: cache owns template, containers own clones |
| genesis-ram-climb-emotes | ✅ | Dereference() after promise consumption drops ref-count |
| ktx-unity-dll-load | ✅ | Probe-at-startup + runtime MarkUnsupported() degrades to unconverted URLs |
| mainui-plugin-dispose-nre | ✅ | Null-conditional on fsm?.Dispose() + try/catch per controller |
| minimap-pixelated-fullscreen | ✅ | Polls resolution in LateUpdate (comment explains why callback alone is insufficient) |
| notifications-poll-list-alloc | ✅ | Loop-local list reused across iterations |
| private-conversation-userstate-nre | ✅ | friendsService made nullable, guarded throughout |
| rusteth-privatekey-pad32 | ✅ | LeftPad to 32 bytes, immutable copy of source array |
| scenescache-duplicate-parcel-add | ✅ | AnyParcelHasLiveScene guard + realmChangeSemaphore serializes realm changes |
| segment-network-errors | ✅ | Debounced LogWarning for retriable send-loop; loss-signal errors stay LogException |
| thumbnail-sticky-failure-rethrow | ✅ | Guards disposal on Succeeded |
| trigger-area-layer-update | ✅ | Synthetic ENTER/EXIT events when mask changes |
| unload-thumbnail-nre-blocks-memory-release | ✅ | Guards on Succeeded before .Asset |
| wearable-mainfile-insecure-url | ✅ | EnforceSecureScheme fails closed; loopback exemption via IPAddress.IsLoopback |
| websocket-closeasync-nre | ✅ | State check before close + connectAbort CTS unparks pending connect |
STEP 3 — Design & integration
Owner search results (new units):
CorruptAbCacheEvictor— statelessstatichelper with two pure functions. Called inline fromLoadAssetBundleSystemat the detection site. Justified extraction for testability. PASS.PooledContinuation<T>(nested inEventBus) — ephemeral pool entries, not persistent state. Payload copied to locals and recycled before handlers run. PASS.KtxNativeSupport— static capability probe, cachedbool?. Not a lifecycle manager. PASS.TopicLookupEntry(nested inCommsApiWrap) — readonly struct for copy-on-write snapshot. Ownership stays withCommsApiWrap. PASS.
Teardown trace: EventBus Subscribe → Unsubscriber.Dispose() ✅. CommsApiWrap subscribe → RemoveSceneMessageHandler in Dispose() ✅. DCLWebSocket.connectAbort → SafeCancelAndDispose in Dispose() ✅. PrivateConversationUserStateService subscribe → UnsubscribeFromEvents in Dispose() ✅.
STEP 4 — Member audit
SDKEntityTriggerAreaComponent.UpdateMaskAndMeshType(1 consumer:TriggerAreaHandlerSystem.UpdateTriggerArea) — encapsulates mask+meshType+dirty+targetOnlyMainPlayer invariant. Not single-use extraction. PASS.ICRDTWorldSynchronizer.AbortSyncCommandBuffer(2 call sites) — documents exactly-one-of contract. PASS.EmoteReferences.sourceAsset— set on acquire, cleared on pool release. Balanced. PASS.
STEP 5 — Line-level findings
See inline comments below.
Non-diff note: DebugMenuConsoleLogHistory.ApplyFilter (line 94) still uses LINQ .Where() — an iterator allocation per CLAUDE.md §4. The PR commendably removed LINQ from the LogEntryCount/ErrorEntryCount properties (which were called every frame), but the remaining .Where() in the user-triggered ApplyFilter could be replaced with a manual loop. Low priority since the call is infrequent.
STEP 6 — Complexity
COMPLEX — touches ECS systems, CRDT synchronization, async/UniTask patterns, asset loading pipeline, memory management, and networking across 24 fixes.
STEP 7 — QA assessment
QA_REQUIRED: YES — 24 runtime bug fixes across avatar rendering, scene lifecycle, authentication, chat, minimap, emotes, trigger areas, and networking.
STEP 8 — Non-blocking warnings
No Main.unity modification. ✅
ProjectSettings.asset modified — verify the change is intentional.
STEP 9 — Verdict
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Touches ECS systems, CRDT synchronization, async/UniTask patterns, asset loading pipeline, memory management, and networking across 24 interleaved fixes
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by unknown via Slack
| foreach (URN urn in promise.LoadingIntention.SuccessfulPointers) | ||
| { | ||
| if (!storage.TryGetElement(urn, out IEmote emote)) continue; | ||
|
|
||
| emote.AssetResults[promise.LoadingIntention.BodyShape]?.Asset?.Dereference(); | ||
| } |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
There was a problem hiding this comment.
The suggestion is self-inconsistent. It says the null-Asset-on-Succeeded case is "worth a diagnostic," but the suggested code doesn't add one — it keeps assetResult.Asset?.Dereference(), which still silently skips a null Asset on a Succeeded
result. So it doesn't deliver the thing it argues for. And a diagnostic for an impossible state on a hot per-frame deref path is exactly the defensive noise the repo's own bar discourages.
The real issue: it breaks a deliberate mirror and can reintroduce the leak. This deref must exactly balance the reference side in LoadEmotesByPointersSystem:
if (emote.AssetResults[bodyShape] is { Succeeded: true })
if (!intention.SuccessfulPointers.Contains(urn)) {
intention.SuccessfulPointers.Add(urn);
emote.AssetResults[bodyShape]?.Asset?.AddReference(); // <- ?.Asset?.
}
Two things follow:
- A pointer lands in SuccessfulPointers only inside the Succeeded: true block, and gets referenced there with the identical ?.Asset?. idiom. The deref loop iterates exactly that set with the identical ?.Asset?.Dereference() — it's the faithful
structural mirror. Same null-skip on both sides ⟹ the same (result, asset) pairs referenced and dereferenced. That symmetry is the correctness argument. - The suggested change re-reads Succeeded at deref time, which is a different, temporal condition from "was Succeeded when referenced." In the only scenario where it changes behavior — the emote's cached result having flipped away from Succeeded
between load and finalize — the suggestion skips the dereference. That leaves the reference added earlier permanently unbalanced, i.e. it reintroduces the exact ref-count leak this branch exists to fix. In every normal state (Succeeded ⟺ Asset !=
null, stable) it's behaviorally identical, so it's not a fix — just a stylistic change carrying a leak risk on the downside.
Co-authored-by: Muna <44584806+decentraland-bot@users.noreply.github.com> Signed-off-by: Esteban Ordano <42750+eordano@users.noreply.github.com>
Security-focused dependency review — PR #9765
View job run · branch |
Reverts the accepted suggestion. returns an oversized array unchanged for value.Length > size, silently handing the rust sign-server a wrong-length key instead of the 32-byte scalar the contract requires — a silent contract violation worse than the BlockCopy throw it avoids. The over-length case is unreachable anyway (secp256k1 scalars are <=32 bytes; the bug this fixes was under-length). Restores the exact-length fast path.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…tch is not broken Scopes the cleartext-scheme policy so it no longer over-upgrades unsigned local-scene-development scene fetches (the Creator Hub preview / SDK network-testing flow). - RequestEnvelope: EnforceSecureScheme now runs only on signed requests (identity auth never travels over cleartext); unsigned wire URLs pass through — their scheme is decided at the infra resolution site or the fetch module's dev-mode gate. - WebRequestController: redirect guard changed to IsCleartextDowngrade(sentUrl, finalUrl) so a deliberately-sent cleartext request (dev fetch) is not misread as a mid-flight downgrade. - WebRequestUtils: adds IsCleartextDowngrade. v16 EditMode RED/GREEN (DCL.WebRequests.Tests.InsecureSchemePolicyShould): over-scoped global hook = 34 total / 3 failed (the non-loopback-http + texture passthrough guards); scoped fix = 34/34 passed. Loopback preview still works, production scene fetch still blocked by the module dev-mode gate, signed/infra requests still upgraded.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…-in (not signed-only) Responds to the security review of the prior commit (1a375d4): the signed-only scoping left ALL unsigned envelope traffic (textures, asset bundles, GLTFs, wearable/emote main files, catalyst content, NFT images) neither upgraded by the app nor blocked by Unity — a net weakening and a coverage gap for exactly this lane's traffic. Rescope to an explicit dev-mode opt-in instead: - CommonArguments gains AllowInsecureCleartext (default false). - SimpleFetchApiImplementation sets it = isLocalSceneDevelopment (the only opt-in site; its own line-63 gate already blocks non-https outside dev mode). - RequestEnvelope enforces EnforceSecureScheme on the wire URL whenever !AllowInsecureCleartext — signed AND unsigned — restoring app-level default-deny (justifies ProjectSettings insecureHttpOption 0->2 AlwaysAllowed), still above signing so signatures cover the upgraded URL. - The IsCleartextDowngrade redirect guard now blocks http->http downgrades for all non-opted-in requests too; only the dev opt-in send skips it. NOTE for reviewers: this consolidated PR also flips ProjectSettings.asset insecureHttpOption 0 (NotAllowed) -> 2 (AlwaysAllowed); the client-side default-deny policy above is its compensating control. v16 EditMode DCL.WebRequests.Tests.InsecureSchemePolicyShould: GREEN 40/40; RED (signed-only guard) 40/37/3 failing the default-deny coverage locks (unsigned non-loopback http x2 + texture).
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🐛 With Issues
Tested on Windows and Mac.
🟢 Regression testing
Went through general in-world testing (login, chat, friends panel, wearables, minimap, notifications, comms) looking for issues tied to the 24 bugsweep fixes. No errors found in the logs tied to the listed lanes (outfits, thumbnails, trigger areas, notifications-poll, etc.) during my session.
🔴 Critical: Fatal crash opening a friend's profile/passport
STR:
- Open friends panel
- Search for a profile that doesn't fully load (placeholder image or blank)
- Click on it to open the user's profile
Result: Client crashes fatally, requires force close, does not recover.
Confirmed NOT reproducible on prod or latest dev with the same repro - only happens on this PR's build, so this looks like a regression introduced by one of the 24 fixes here, not a pre-existing issue.
Evidence from Player-prev.log:
Right before the crash: Graphics device is null., followed immediately by a TextMeshProUGUI.Awake() call inside the passport's friendship-status flow:
Graphics device is null. TMPro.TextMeshProUGUI:Awake() DCL.Passport.<<ShowFriendshipInteraction>g__FetchFriendshipStatusAndShowInteractionAsync|0>d:MoveNext() (PassportController.cs:926) ... DCL.Friends.<GetFriendshipStatusAsync>d__31:MoveNext() (RPCFriendsService.cs:380)
Followed by Crash!!! and a fatal native stack trace (IL2CPP-level, in GameAssembly).
Evidence from crash.dmp:
- Valid Sentry-generated minidump. Exception is
EXCEPTION_BREAKPOINTinsideUnityPlayer.dll- this is Sentry's handler capturing state, not the root cause itself. - Found the string
ALLOC_TEMP_UnityGfxDeviceWorkerin the dump, which lines up with the "Graphics device is null" line - reinforces that this is a graphics-device issue, not a friends/networking one. - Couldn't reconstruct the full native stack without Unity's
.pdbsymbols - happy to share the raw dump for WinDbg if useful.
Hypothesis: something in this PR's changes appears to invalidate/interact badly with the graphics device around the passport's friendship-status flow, causing a fatal crash instead of a recoverable managed exception.
Given this is a confirmed regression (not reproducible on prod/dev) and results in an unrecoverable crash, I don't think this should merge as-is until it's addressed. Happy to share the full crash report folder (C:/Users/ludmi/AppData/Local/Temp/Decentraland/Explorer/Crashes) or retest once there's a fix.
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Code Review — PR #9765: fix: bugsweep aug16
Overview
Consolidated PR merging 24 bug fixes into one branch for batch QA. Touches ECS systems, async flows, asset lifecycle (reference counting), networking (WebSocket, comms), transport security (HTTP→HTTPS enforcement), and several UI fixes. The engineering discipline across this sweep is strong — balanced reference counting, correct threading patterns, good test coverage, and defensive guards.
Root-Cause Check
Each fix addresses a distinct root cause. Highlights:
- Emote asset leaks — loading system added references per successful pointer but never released them; consumers now dereference on promise consumption.
- CRDT semaphore leak — an exception during message processing could skip
ApplySyncCommandBuffer, leaving the rent slot permanently held; now caught and routed toAbortSyncCommandBuffer. - WebSocket stall — mono's
ConnectAsyncblocks indefinitely when the remote drops the TCP handshake; a newconnectAbortCTS unparks it. - Transport security — cleartext HTTP to non-loopback hosts was previously allowed by Unity's
AlwaysAllowedsetting; a newEnforceSecureScheme+ redirect downgrade guard establishes default-deny at the app level. - Duplicate scene facades — two definition entities claiming the same parcel could both get facades;
AnyParcelHasLiveScenediscards the latecomer. - Static JArray race —
ChatMessagesBusAnalyticsDecoratorshared a singleMENTION_WALLET_IDSacross concurrent sends. - Corrupt AB cache — a null
assetBundlewith a valid cache hash now evicts the corrupt entry and retries once.
QA Note — Crash Regression
QA (Ludmila) reported a native crash when opening a friend's passport in-world (Graphics device is null in TextMeshProUGUI.Awake()). PassportController is NOT in this diff. The crash is likely caused indirectly by timing changes from the WebSocket, EventBus, or async infrastructure fixes. This should be a priority investigation during QA.
Findings Summary
| # | Severity | File | Finding |
|---|---|---|---|
| 1 | P1 | ControlSceneUpdateLoopSystem.cs | TOCTOU race: async cache population leaves a window for duplicate facades |
| 2 | P1 | FinalizeEmoteLoadingSystem.cs | Scene-emote consumers omit CancellationTokenSource cleanup |
| 3 | P1 | FinalizeGltfContainerLoadingSystem.cs | Destroyed-Root guard does not release asset ref-count |
| 4 | P2 | DCLWebSocket.cs | ws.Abort() not guarded against ObjectDisposedException from racing Dispose() |
| 5 | P2 | EmoteReferences.cs | // ReSharper disable InconsistentNaming on non-DTO MonoBehaviour |
| 6 | P2 | ReportsHandlingSettings assets | Duplicate ANALYTICS category entries with different severities |
Positive Highlights
- Transport security is well-designed with defense-in-depth: envelope enforcement, redirect downgrade guard, and per-site enforcement at bypass locations. Test coverage (
InsecureSchemePolicyShould) is thorough, including thelocalhost.evil.comnegative case. - CRDT
delegatedflag pattern inApplySyncCommandBufferis elegant — prevents both double-release and missed-release on all exception paths. - EventBus
PooledContinuation<T>correctly handles cross-thread scheduling, reentrant invocation, and GC rooting. The allocation-regression test with canary validation is exemplary. - CommsApiWrap copy-on-write topic lookup eliminates per-message string allocation on the hot path. The byte-level linear scan with volatile snapshot is well-designed.
- KtxNativeSupport capability probe with runtime trip-wire is a robust fallback for broken native installs.
- ProfileFetchingAuthState retry loop with per-attempt linked CTS correctly distinguishes timeout (retry) from genuine null profile (return null) from external cancellation (throw).
- MVCManager try/catch dispose loop correctly logs via
ReportHubwithout swallowing or aborting remaining controllers.
Review Agents Used
- Security sentinel
- Architecture strategist
- Pattern recognition specialist
- Emote lifecycle specialist
- AssetPreLoadCache specialist
- RealmController / ControlScene specialist
- CommsApiWrap / EventBus specialist
- MVCManager / remaining fixes specialist
REVIEW_RESULT: PASS
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Touches ECS systems, async CancellationToken flows, asset lifecycle reference counting, networking (WebSocket + comms), CRDT synchronization, transport security, and MVC teardown across 90+ files.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by Esteban Ordano (<@U9DLN0485>) via Slack
| if (!definitionComponent.IsPortableExperience && AnyParcelHasLiveScene(definitionComponent.Parcels)) | ||
| { | ||
| ReportHub.LogWarning(GetReportData(), $"Duplicate scene definition for '{definitionComponent.Definition.GetLogSceneName()}': discarding its facade"); | ||
| scene.DisposeAsync().Forget(); | ||
| return; | ||
| } | ||
|
|
||
| StartAndUpdateSceneAsync(definitionComponent, partition, scene).Forget(); |
There was a problem hiding this comment.
[P1] TOCTOU race in duplicate scene detection. AnyParcelHasLiveScene reads from scenesCache, but the cache is populated asynchronously inside StartAndUpdateSceneAsync(...).Forget(). If two definition entities for the same parcel are both present in the same query pass, and the cache isn't yet written when the second entity is processed, a duplicate facade can slip through.
The test covers this with Task.Delay(100) between updates, but in production under load the window is theoretically exploitable.
Consider adding synchronous bookkeeping to close the TOCTOU gap — e.g., claim parcels in the cache (or a local set) immediately before firing the async:
| if (!definitionComponent.IsPortableExperience && AnyParcelHasLiveScene(definitionComponent.Parcels)) | |
| { | |
| ReportHub.LogWarning(GetReportData(), $"Duplicate scene definition for '{definitionComponent.Definition.GetLogSceneName()}': discarding its facade"); | |
| scene.DisposeAsync().Forget(); | |
| return; | |
| } | |
| StartAndUpdateSceneAsync(definitionComponent, partition, scene).Forget(); | |
| if (!definitionComponent.IsPortableExperience && AnyParcelHasLiveScene(definitionComponent.Parcels)) | |
| { | |
| ReportHub.LogWarning(GetReportData(), $"Duplicate scene definition for '{definitionComponent.Definition.GetLogSceneName()}': discarding its facade"); | |
| scene.DisposeAsync().Forget(); | |
| return; | |
| } | |
| // Claim parcels synchronously before the async kick so a second entity | |
| // in the same query pass sees the cache populated. | |
| scenesCache.ClaimParcels(definitionComponent.Parcels, scene); | |
| StartAndUpdateSceneAsync(definitionComponent, partition, scene).Forget(); |
(Adapt ClaimParcels to whichever API scenesCache exposes for synchronous parcel registration.)
| private void ConsumeAndDisposeFinishedSceneEmoteFromRealmPromise(in Entity entity, ref SceneEmoteFromRealmPromise promise) | ||
| { | ||
| if (!promise.SafeTryConsume(World, GetReportData(), out StreamableLoadingResult<EmotesResolution> result)) return; | ||
|
|
||
| DereferenceSceneEmote(promise.LoadingIntention.NewSceneEmoteURN(), promise.LoadingIntention.BodyShape, in result); | ||
|
|
||
| World.Destroy(entity); | ||
| } |
There was a problem hiding this comment.
[P1] Scene-emote consumer omits CancellationTokenSource cleanup. Both GetSceneEmoteFromRealmIntention and GetSceneEmoteFromLocalSceneIntention create a new CancellationTokenSource() in their constructor. The regular emote consumer (ConsumeAndDisposeFinishedEmotePromise) calls promise.LoadingIntention.Dispose() which cancels and returns pooled resources; these scene-emote consumers skip that step.
For the realm variant the CTS is passed to the downstream AB promise system, so it may be managed there — but an explicit cancel is good hygiene (mirrors the regular consumer). For the local variant the CTS is never passed downstream and genuinely leaks.
| private void ConsumeAndDisposeFinishedSceneEmoteFromRealmPromise(in Entity entity, ref SceneEmoteFromRealmPromise promise) | |
| { | |
| if (!promise.SafeTryConsume(World, GetReportData(), out StreamableLoadingResult<EmotesResolution> result)) return; | |
| DereferenceSceneEmote(promise.LoadingIntention.NewSceneEmoteURN(), promise.LoadingIntention.BodyShape, in result); | |
| World.Destroy(entity); | |
| } | |
| private void ConsumeAndDisposeFinishedSceneEmoteFromRealmPromise(in Entity entity, ref SceneEmoteFromRealmPromise promise) | |
| { | |
| if (!promise.SafeTryConsume(World, GetReportData(), out StreamableLoadingResult<EmotesResolution> result)) return; | |
| DereferenceSceneEmote(promise.LoadingIntention.NewSceneEmoteURN(), promise.LoadingIntention.BodyShape, in result); | |
| promise.LoadingIntention.CancellationTokenSource.SafeCancelAndDispose(); | |
| World.Destroy(entity); | |
| } |
| private void ConsumeAndDisposeFinishedSceneEmoteFromLocalPromise(in Entity entity, ref SceneEmoteFromLocalPromise promise) | ||
| { | ||
| if (!promise.SafeTryConsume(World, GetReportData(), out StreamableLoadingResult<EmotesResolution> result)) return; | ||
|
|
||
| DereferenceSceneEmote(promise.LoadingIntention.NewSceneEmoteURN(), promise.LoadingIntention.BodyShape, in result); | ||
|
|
||
| World.Destroy(entity); | ||
| } |
There was a problem hiding this comment.
[P1] Same pattern as the realm consumer above — the local-emote intention's CancellationTokenSource is never passed to the downstream GLTF promise system and leaks on every scene-emote load cycle.
| private void ConsumeAndDisposeFinishedSceneEmoteFromLocalPromise(in Entity entity, ref SceneEmoteFromLocalPromise promise) | |
| { | |
| if (!promise.SafeTryConsume(World, GetReportData(), out StreamableLoadingResult<EmotesResolution> result)) return; | |
| DereferenceSceneEmote(promise.LoadingIntention.NewSceneEmoteURN(), promise.LoadingIntention.BodyShape, in result); | |
| World.Destroy(entity); | |
| } | |
| private void ConsumeAndDisposeFinishedSceneEmoteFromLocalPromise(in Entity entity, ref SceneEmoteFromLocalPromise promise) | |
| { | |
| if (!promise.SafeTryConsume(World, GetReportData(), out StreamableLoadingResult<EmotesResolution> result)) return; | |
| DereferenceSceneEmote(promise.LoadingIntention.NewSceneEmoteURN(), promise.LoadingIntention.BodyShape, in result); | |
| promise.LoadingIntention.CancellationTokenSource.SafeCancelAndDispose(); | |
| World.Destroy(entity); | |
| } |
| if (result.Asset is not { } asset || asset.Root == null) | ||
| { | ||
| ReportHub.LogError(GetReportData(), $"GltfContainerAsset '{component.Name}' ({component.Hash}) resolved with a destroyed Root"); | ||
| component.State = LoadingState.FinishedWithError; | ||
| component.RootGameObject = null; | ||
| eventsBuffer.Add(entity, component); | ||
| return; | ||
| } |
There was a problem hiding this comment.
[P1] Destroyed-Root guard does not release the asset's ref count. When the guard detects a destroyed Root, the component transitions to FinishedWithError but the asset itself is not disposed or dereferenced. The IStreamableRefCountData reference from the loading system is never decremented, which can prevent the cache from reclaiming the entry.
If the downstream error-handling system already cleans up FinishedWithError assets, please document that here with a comment. Otherwise:
| if (result.Asset is not { } asset || asset.Root == null) | |
| { | |
| ReportHub.LogError(GetReportData(), $"GltfContainerAsset '{component.Name}' ({component.Hash}) resolved with a destroyed Root"); | |
| component.State = LoadingState.FinishedWithError; | |
| component.RootGameObject = null; | |
| eventsBuffer.Add(entity, component); | |
| return; | |
| } | |
| if (result.Asset is not { } asset || asset.Root == null) | |
| { | |
| ReportHub.LogError(GetReportData(), $"GltfContainerAsset '{component.Name}' ({component.Hash}) resolved with a destroyed Root"); | |
| result.Asset?.Dispose(); // Release the ref-count the loading system added | |
| component.State = LoadingState.FinishedWithError; | |
| component.RootGameObject = null; | |
| eventsBuffer.Add(entity, component); | |
| return; | |
| } |
| // Abort must tolerate a racing Dispose() (scene teardown vs a JS close()) | ||
| } | ||
|
|
||
| ws.Abort(); |
There was a problem hiding this comment.
[P2] ws.Abort() not guarded against ObjectDisposedException from racing Dispose(). connectAbort.Cancel() is correctly guarded on the line above, but ws.Abort() is not. If Dispose() has already called ws.Dispose() on another thread, this can throw during teardown.
| ws.Abort(); | |
| try { ws.Abort(); } | |
| catch (ObjectDisposedException) { /* same Dispose race as connectAbort above */ } |
| @@ -1,5 +1,8 @@ | |||
| using DCL.AvatarRendering.Loading.Assets; | |||
| using UnityEngine; | |||
|
|
|||
There was a problem hiding this comment.
[P2] // ReSharper disable InconsistentNaming on a non-DTO MonoBehaviour. CLAUDE.md restricts this suppression to deserialized DTO fields ("suppress per file with // ReSharper disable InconsistentNaming above the namespace"). EmoteReferences is a MonoBehaviour with Unity-serialized fields, not a JSON DTO. Consider renaming the fields to match PascalCase conventions instead, or adding a comment explaining why the suppression is justified for serialized Unity fields.
…nnect Fixes a fatal regression the earlier websocket-closeasync-nre commit introduced into this consolidation: DCLWebSocket.ConnectAsync forced useCurrentSynchronizationContext:false on every connect, dropping the connecting thread's SynchronizationContext. The social-service RPC transport starts its receive loop in the connect continuation (RPCSocialServices.InitializeConnectionAsync -> ListenForIncomingData), and DCLWebSocket.ReceiveAsync re-captures whatever thread that loop runs on. So a main-thread connect that lost its context moved every RPC response -- and the passport friendship-status continuation that flips UI buttons active (PassportController.GetFriendshipStatusAsync -> gameObject.SetActive) -- onto a background thread, touching Unity graphics off the main thread: 'Graphics device is null' + native crash (repro: open a searched stranger's passport; reproduced on Mac and Windows; clean on prod/dev). Fix: marshal back only when a SynchronizationContext exists (marshalBackToIssuingContext = SynchronizationContext.Current != null). Restores the main-thread receive loop for social/comms transports (and every other DCLWebSocket consumer that shared this regression), while preserving the original NRE fix and the no-throw behaviour on the context-less V8 script-invoke thread (TaskScheduler.FromCurrentSynchronizationContext throws with no current context). The connectAbort/linked-CTS/AttachExternalCancellation handling is untouched. Regression test ResumeOnTheCallerSynchronizationContextAfterConnecting. v16 EditMode Utility.Tests.DCLWebSocketCloseAsyncShould: RED (unconditional false) 6/5/1 (only the thread-affinity test fails), GREEN (fix) 6/6.
…fety dev independently fixed the same never-shown-dispose NRE (fsm/audio nullable, ?.-guarded) that bugsweep/mainui-plugin-dispose-nre addressed. Adopt dev's version of this file verbatim so merging dev is conflict-free; the fix's intent (no NRE disposing a never-instantiated controller) is fully preserved by dev's ?. Dispose guard.
This comment has been minimized.
This comment has been minimized.
…nController dispose test The dev merge added an IMVCManager mvcManager constructor parameter (19th positional) to AuthenticationScreenController; the mainui-plugin-dispose-nre regression test NotThrowOnDisposeWhenViewWasNeverShown still constructed it with the pre-merge 18-arg signature, breaking the Windows/macOS Cloud build with CS7036. Pass null! for mvcManager (a stored, never-dereferenced dependency in the dispose-before-view lifecycle, consistent with the test's other null! args).
|
Claude finished @eordano's task in 2m 8s —— View job Security-focused dependency review — PR #9765 (re-run after push
|
| Commit | Change |
|---|---|
213268a |
AuthenticationScreenControllerShould.cs — one extra null! argument |
I verified the build fix is actually correct rather than taking the message at its word. dev added IMVCManager mvcManager as the 19th parameter of AuthenticationScreenController (AuthenticationScreenController.cs:117), ahead of the optional string? referrer = null. The test now passes exactly 19 arguments (() => null!, eight null!, string.Empty, nine null!), matching viewFactory … mvcManager. Signature-only fix, no behavioural change, no dependency impact.
The merge base is still f41270d, so git diff origin/dev...HEAD remains the authoritative PR-side surface. Against it, the dependency picture is byte-for-byte identical to the last two runs:
-
Explorer/Packages/— empty diff. Nomanifest.jsonentry, nopackages-lock.jsonentry. -
Binary files — zero.
git diff --numstatshows a numeric pair on all 129 rows; no- -row exists. No.dll/.so/.dylib/.bundle, no native plugin, no install hook, no build/editor script, no.yml/.sh/.py. -
Assembly graph — 4 test-scoped entries, and I resolved the GUIDs this run rather than assuming:
- the three new
.asmreffiles all point atGUID:da80994a…=DCL.EditMode.Tests(Explorer/Assets/DCL/Tests/Editor/DCL.EditMode.Tests.asmdef.meta); - the one added reference in
DCL.EditMode.Tests.asmdefisGUID:1d75b3d8…= the existingDCL.Settingsassembly (Explorer/Assets/DCL/Settings/Settings.asmdef.meta).
That's an
Editor-only test assembly (includePlatforms: ["Editor"]) taking a reference on an existing production assembly — normal test wiring. NoprecompiledReferences, nooverrideReferences, noInternalsVisibleTowidening, no new runtime assembly. - the three new
-
The only file under
Explorer/Assets/Plugins/isRustSegment/SegmentServerWrap/RustSegmentAnalyticsService.cs— managed C# source, not a binary. The nativeSegmentServerWraplibrary is untouched.
STEP 1.5 has no subjects; the new-dependency label remains a false positive. STEP W does not apply — no .github/** path in the diff.
The only dependency-adjacent file is still ProjectSettings.asset:1063, insecureHttpOption: 0 → 2.
Transport security — re-verified against the current tree, unchanged and still correct
- Default-deny.
RequestEnvelope.cs:110enforces on the wire URL of every envelope unless the request opted in. Exactly one non-test opt-in site:SimpleFetchApiImplementation.cs:71, bound toisLocalSceneDevelopment. - Ordering. Enforcement precedes
AssignHeaders → SignRequest, so the signature and identity auth chain cover the upgraded URL. - Streamable loading covered.
CommonLoadingArguments's implicit conversion buildsCommonArgumentswithout the flag → defaultsfalse, so ABs, GLTFs, textures, audio, and wearable/emote main files are enforced. - Redirect guard.
WebRequestController.cs:76capturessentUrlper attempt inside the retry loop;IsCleartextDowngradeblocks on both the success and exception paths, and the exception-paththrowprevents a retry re-sending headers over cleartext. - The compensating control is pinned.
InsecureSchemePolicyShould.KeepPlayerSettingAlwaysAllowed:160assertsPlayerSettings.insecureHttpOption == AlwaysAllowed, so the app-level policy can't be silently decoupled from the setting it replaces.
Residual — carried over for the third run, still the only transport-security gap
MEDIUM — WebContentInfo.FetchAsync bypasses the envelope on a third-party URL. Confirmed still present: WebRequests/WebContentInfo.cs:21 builds a raw UnityWebRequest.Head(url), so it gets neither the app-level EnforceSecureScheme policy nor Unity's platform block (now AlwaysAllowed). Its caller LoadNFTTypeSystem.cs:41-42 passes convertUrl, which is the https media-converter URL only when ktxEnabled; otherwise it is imageUrl verbatim from NFT metadata — so http://third-party/… goes out as a cleartext HEAD from the user's IP. This PR raises how often that branch is taken, since NFTShapePlugin.cs:79 and RequestHub.cs:82 now AND KtxNativeSupport.IsSupported into the flag, and the baseline perf run's 192 [UI] DllNotFoundException shows the fallback firing on real hardware. The ktx lane is the right fix; the gap is WebContentInfo's.
One line, the same shape already applied at UrlResolverService.cs:106 and AbgenSidecar.cs:89:
var request = UnityWebRequest.Head(WebRequestUtils.EnforceSecureScheme(url));LOW — other raw non-envelope sites, all first-party URLs. DeepLinkWorldWhitelistProvider.cs:36, ReelCommonActions.cs:46, SceneFactory.cs:155, and AbgenSidecar's sidecar/loopback calls. InnerTubeClient is hardcoded https. Unchanged.
LOW — converter-mediated cleartext. PreserveHttpOriginEmbeddedInConverterUrl pins that an http:// origin stays http inside the escaped converter query parameter. Correct call — it's data, not a wire URL, and the client's own hop is https — but the origin is then fetched in cleartext by the converter. Pre-existing.
Still open, third run: the insecureHttpOption NOTE lives only in 22a1a3d's commit message. Please mirror it into the PR body. Nobody reviewing a 129-file diff reads commit messages to discover that a platform-level transport guard was turned off and replaced by an app-level one.
Summary
| Item | Risk | Evidence confidence |
|---|---|---|
| Dependency graph (packages, lockfile, binaries, native plugins, asmdefs, workflows) | unchanged | HIGH |
New commit 213268a — dependency impact |
none (test-only, signature verified) | HIGH |
| Transport-security design (default-deny, single opt-in, enforce-before-sign, redirect guard, setting pinned by test) | correct | HIGH |
Raw UnityWebRequest.Head on third-party NFT image URL (WebContentInfo ← LoadNFTTypeSystem) |
MEDIUM | HIGH |
| Other raw non-envelope call sites (first-party URLs) | LOW | HIGH |
http origin embedded in the https converter query parameter |
LOW | HIGH |
No new dependency, no new binary, no unpinned reference, no dynamic assembly loading, no workflow or prompt change. I did not inspect any binary internals — none were added.
DEPENDENCY_REVIEW: NEEDS_ATTENTION
NEEDS_ATTENTION is for the one carried-over transport-security gap, not for anything in the supply chain. The ask has not changed in size across three runs: one EnforceSecureScheme wrap at WebContentInfo.cs:21 plus a test case, and mirroring the insecureHttpOption NOTE into the PR body.
Outside my review scope, but relevant to the merge decision:
213268a only fixes the test compile against dev's new constructor parameter — it does not touch the crash lane. The two things I flagged last run are still unconfirmed and neither is provable from the diff:
- Re-run QA's exact repro on a build that includes
34a937e. Ludmila'sGraphics device is null→ native crash was reported against1a375d4/22a1a3d, i.e. before the websocket fix. Opening a non-loading stranger's passport should be re-tested, not assumed fixed. marshalBackToIssuingContext = SynchronizationContext.Current != nullis thread-affinity at the point of call. AnyDCLWebSocketconsumer that awaits onto the thread pool before callingConnectAsyncstill gets a background receive loop. Worth confirming the archipelago (WebSocketArchipelagoLiveConnection) and dapp (DappWeb3EthereumApi) connects are issued on the main thread, since they shared the regression.
The perf numbers in this thread (CPU 1% worst +773% on i5, +536% on M1) were also measured on 22a1a3d, before the websocket fix. Off-main-thread UI work is exactly the shape that produces multi-hundred-millisecond spikes alongside a modest average delta, so a fresh perf run on 213268a would show whether those spikes shared the crash's root cause.
· branch bugsweep/aug16-consolidated
|
PR #9765, run #32202105101 Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Intel Core i5
Exception breakdown
Apple M1
|
4 participants
24 branches of an aug/16 bug-sweep campaign combined into one branch for batch testing/QA. Every fix carries its own regression test; the full set passed batched EditMode RED/GREEN validation (25/25 tests) plus 3-lens adversarial review. Individual branches: bugsweep/. Ledger: workspace INDEX.md (forge mine/bugsweep-aug16).
Included fixes
Fixes #3661, #6484, #7792, #7832, #7907, #8023, #8884, #8891, #8902, #8911, #9182, #9206, #9263, #9346, #9447, #9451, #9531, #9665, #9692, #9738, #9741
Pull Request Description
What does this PR change?
A lot of under-the-hood errors. Triggering some of the errors is pretty hard -- I would do the full InWorld battery of tests and the release test checklist.