fix: crash on cancelled YouTube HLS synthesis - #9759
Conversation
🚦 CI Status
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. Warnings count reduced: 13156 => 13148 Warnings/errors in files changed by this PR (18)All Unity tests passed ✅
|
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Code Review — PR #9759
STEP 2 — Root-cause check: PASS ✅
The PR fixes a fatal IL2CPP/MSVC crash on Windows triggered when OperationCanceledException unwinds through ~7 nested async state machines and hits TryWriteSynthesizedHlsAsync — whose catch/using funclets around the await produce a code shape that crashes IL2CPP's Class::IsAssignableFrom → Class::Init path.
Since the true root cause is an IL2CPP AOT compiler bug (outside the project's control), the fix correctly eliminates the code shape that triggers it:
- Async layer (
TryWriteSynthesizedHlsAsync) — deliberately exception-free; notry/catch/usingaroundawait. - Fetch (
FetchByteRangeAsync) — enforces the exception-free contract viaSuppressToResultAsync(), converting all exceptions (includingOperationCanceledException) intoResult<byte[]>values. - Sync layer (
WriteSynthesizedHls) — all throwable code (parsing, file I/O) lives here, wheretry/catchoperates on a normal stack frame, not an async state machine.
This addresses the cause from the application's perspective, not a symptom.
STEP 3 — Design & integration: PASS ✅
- No new long-lived units introduced. The PR restructures existing private methods within
YouTubeVideoClient. No new systems, plugins, managers, services, or controllers. - No lifecycle/ownership changes. No new subscriptions, events, connections, or buffers.
- Teardown trace: The
usingblocks forListPoolare preserved inWriteSynthesizedHls. No new resources requiring teardown are introduced. - No public API surface changes. The
IYouTubeVideoClientinterface is untouched.
STEP 4 — Member audit: PASS ✅
| Member | Visibility | Consumers | Assessment |
|---|---|---|---|
FetchByteRangeAsync |
private |
2 call sites (video + audio in UniTask.WhenAll) |
DRY extraction — eliminates duplicated request chain. Not single-use. ✅ |
WriteSynthesizedHls |
private static |
1 call site (TryWriteSynthesizedHlsAsync) |
Single-use, but extraction is mandatory — separating sync try/catch from async state machine is the core crash fix. Not gratuitous per CLAUDE.md §11. ✅ |
STEP 5 — Line-level review: PASS ✅
Pass A (blocking issues):
- ✅ Code quality —
SuppressToResultAsync()usage per CLAUDE.md §9.ct.IsCancellationRequested(notThrowIfCancellationRequested) in the exception-free flow per CLAUDE.md §9. - ✅ Bugs —
Result.Valueaccessed only afterResult.Successguard. Cancellation checked afterUniTask.WhenAllbefore proceeding. - ✅ Security — URLs from YouTube API, file writes to
Application.temporaryCachePath. No new attack surface. - ✅ Performance — No change to runtime characteristics. Parallel sidx fetch preserved.
- ✅ Error handling —
FetchByteRangeAsync→SuppressToResultAsynccatches everything.WriteSynthesizedHlshas its owncatch (Exception).TryWriteSynthesizedHlsAsyncis exception-free by contract with all paths covered. - ✅ Resource leaks — No new subscriptions/events/connections.
ListPoolusing-blocks preserved.Result<byte[]>does not require disposal. - ✅ Nullability — Return type
string?correctly nullable. No!operators added. - ✅ Logging — Correctly uses
ReportHub.Log(informational) andReportHub.LogWarning(unexpected failure) withReportCategory.MEDIA_STREAM. Improved diagnostics: fetch failures now distinguish video vs. audio sidx.
Pass B (design smells): No issues found.
- Doc comments explain "why" (IL2CPP crash avoidance, Sentry reference) — not narrating caller behavior.
staticonWriteSynthesizedHlscorrectly signals no instance-state dependency.- No magic values, no constructor issues, no encapsulation leaks.
Security review: No security issues found ✅
Reviewed against: secrets/credentials, injection, auth/authz, sensitive data exposure, path traversal, SSRF. No new attack surface — URLs sourced from YouTube API response, file operations in temp cache, no user-supplied input in paths or requests.
STEP 6 — Complexity
COMPLEX — Modifies async/UniTask cancellation flow and Result<T> patterns.
STEP 7 — QA
YES — Changes runtime code affecting video playback and crash behavior on Windows.
STEP 8 — Non-blocking warnings
None.
STEP 9 — Verdict
Clean, well-targeted fix. The async/sync split is the minimal structural change needed to eliminate the IL2CPP crash shape. All project conventions (async error handling, cancellation, naming, logging, resource cleanup) are followed. Doc comments provide essential guardrails against reintroducing the crash pattern.
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies async/UniTask cancellation flow, SuppressToResultAsync / Result<T> patterns in YouTube media stream handling
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
|
🔍 Jarvis reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging. |
This comment has been minimized.
This comment has been minimized.
Ludmilafantaniella
left a comment
Ludmilafantaniella
left a comment
There was a problem hiding this comment.
✅ Approve
Tested on Windows and Mac, using a scene with a Video Screen (Smart Item) pointing to a YouTube URL. Verified in two scenes: my own world and Genesis City.
- Teleported away within ~1s of the YouTube video starting to load - no crash on either platform
- Happy path: video plays via segmented HLS and starts within a few seconds on both platforms
Minor observation (not blocking): video start took a couple seconds longer on Windows in Genesis City specifically compared to my own world/Mac - likely just scene weight/network, not clearly tied to this change.
Note: the automated performance test on this PR shows a real CPU regression on both platforms (Intel ~14% slower average, up to 758% worse 1%-worst; M1 ~11% slower average, up to 526% worse 1%-worst) - worth a look from the dev/perf side, though it didn't surface as a noticeable issue in my manual testing.
No blockers on the crash fix itself.
14.08.2026_18.16.40_REC.mp4
|
@decentraland-bot rerun performance tests |
|
PR #9759, run #31845644851 Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Intel Core i5
Exception breakdown
Apple M1
|
4 participants
What
Fixes #9758
Fixes a fatal native crash on Windows that closes the client when a YouTube video
is being resolved and the player teleports or changes realm.
Why
Cancelling the byte-range requests in
YouTubeVideoClientthrows anOperationCanceledExceptionthat re-throws through ~7 nested async statemachines, each invoking the next continuation from inside its own catch handler.
By the time it reaches
TryWriteSynthesizedHlsAsync— which has both acatchand two
usingblocks around itsawait— IL2CPP crashes while matching thecatch clause (
Class::IsAssignableFrom→Class::Init, access violation).The root cause is in the IL2CPP/MSVC exception handling, which we can't fix, so
the fix removes the code shape that triggers it.
How
FetchByteRangeAsync(wasTryFetchByteRangeAsync) no longer rethrows oncancellation. It returns a
Result<byte[]>viaSuppressToResultAsync, so itcompletes through
SetResultinstead ofSetExceptionand the exception chainstops there.
TryWriteSynthesizedHlsAsyncno longer has anytry/catch/usingaround itsawait— the frame that crashed no longer exists.WriteSynthesizedHls,where
try/catchis safe.No behaviour change: cancellation and fetch failures still fall back to the
non-segmented playlist exactly as before.
QA
Requires a Windows build
VideoPlayerpointing at YouTube.Also verify the happy path still works: a YouTube plays with segmented HLS
and starts within a few seconds.