Skip to content

Fix a crash generating thumbnails for self-hosted videos - #25977

Draft
jkmassel wants to merge 1 commit into
trunkfrom
jkmassel/selfhosted-video-thumbnail-crash
Draft

Fix a crash generating thumbnails for self-hosted videos#25977
jkmassel wants to merge 1 commit into
trunkfrom
jkmassel/selfhosted-video-thumbnail-crash

Conversation

@jkmassel

@jkmassel jkmassel commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Fixes an intermittent SIGSEGV in the video-thumbnail async bridge. It surfaces as a hard crash of MediaImageServiceTests.testSmallThumbnailForRemoteSelfHostedVideo, and this bridge runs in production every time the app generates a thumbnail for a self-hosted video — so it is a shipping bug, not just a test artifact.
  • The test is retained deliberately as the regression guard: it exercises the real AVFoundation path and is the only thing that caught this.

Root Cause

Self-hosted videos have no remoteThumbnailURL, so the app generates the thumbnail from the video itself via live AVFoundation: MediaImageService.generateThumbnailForVideoMediaThumbnailExporter.exportThumbnail(forVideoURL:)MediaVideoExporter.exportPreviewImageForVideo.

The callback-to-async bridge was unsafe in two compounding ways:

  1. Unsafe continuation, resumed more than once. The bridge used withUnsafeThrowingContinuation. AVAssetImageGenerator.generateCGImagesAsynchronously can invoke its completion handler a second time (a .cancelled callback on teardown, or racing the cancellation handler), resuming the same continuation twice. A double-resume of an unsafe continuation is undefined behavior — a raw SIGSEGV — instead of the deterministic trap a checked continuation gives.
  2. Progress mutated after the resume. In exportPreviewImageForVideo the image export is synchronous, so exporter.export(onCompletion:onError:) resumes the continuation inline. The next line — progress.addChild(imageProgress, …) — then mutated the Progress object graph on AVFoundation's queue after the awaiting task had resumed and could be racing forward on another thread.

Fix

1. Make the async bridges resume-once and checked

MediaExporter.export(), MediaThumbnailExporter.exportThumbnail(forFileURL:) and exportThumbnail(forVideoURL:) now use withCheckedThrowingContinuation routed through a small NSLock-backed ResumeOnce<T> that nils the continuation on first resume. A stray second callback degrades to a no-op. The checked variant stays permanently — the unsafe one buys nothing here and hides exactly this class of bug.

2. Stop mutating progress after the resume

exportPreviewImageForVideo finalizes progress.completedUnitCount = .done inside the inner export's onCompletion/onError, before the resuming callback fires. The post-resume addChild is removed. Because the image export is synchronous and returns an already-complete Progress, this is equivalent for progress reporting while removing the cross-thread mutation.

What We Explored

  • Reproducing the SEGV locally. Ran the pre-fix binary 200× with per-iteration relaunch on an iOS 26 simulator: 200/200 passed, 0 crashes. The crash does not reproduce in an isolated local loop — it needs CI-level executor contention (the test runs alongside hundreds of others). The test never cancels, so the operative mechanism is the double-resume (1), which is not a TSan-detectable data race — so TSan is clean on this path and adds nothing here. Part 1 makes the crash impossible regardless of the exact micro-mechanism.

Test Plan

  • rake lint clean on all changed files.
  • WordPressUnitTests builds; testSmallThumbnailForRemoteSelfHostedVideo green across 121 executions of the real AVFoundation path (0 crashes / 0 failures).
  • Pre-fix SEGV confirmed not locally reproducible (200/200 clean) — documented above.
  • Full WordPressUnitTests suite on CI (running the pipeline repeatedly to confirm the flake is gone).

Related

  • The sibling testSmallThumbnailForRemoteVideo ~3% flake is a separate MemoryCache.shared / ImageDownloader.shared test-isolation issue, out of scope here.

The video-thumbnail async bridge used withUnsafeThrowingContinuation, so a double-resume (AVFoundation firing its image-generation completion handler a second time on teardown) was undefined behavior — an intermittent SIGSEGV — instead of a deterministic trap. It also mutated the Progress object graph after the awaiting task had already resumed and could be racing forward on another thread.

Switch the three async bridges (MediaExporter.export(), MediaThumbnailExporter.exportThumbnail(forFileURL:/forVideoURL:)) to withCheckedThrowingContinuation routed through a resume-once guard so a stray second callback degrades to a no-op, and finalize the Progress before the resuming callback fires in MediaVideoExporter.exportPreviewImageForVideo.
@dangermattic

Copy link
Copy Markdown
Collaborator
1 Message
📖 This PR is still a Draft: some checks will be skipped.

Generated by 🚫 Danger

@jkmassel jkmassel self-assigned this Sep 2, 2026
@jkmassel jkmassel added this to the 27.3 milestone Sep 2, 2026
@wpmobilebot

Copy link
Copy Markdown
Contributor
App Icon📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress
ConfigurationRelease-Alpha
Build Number34194
VersionPR #25977
Bundle IDorg.wordpress.alpha
Commitb4d61a7
Installation URL1opihf56h1aog
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@wpmobilebot

Copy link
Copy Markdown
Contributor
App Icon📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack
ConfigurationRelease-Alpha
Build Number34194
VersionPR #25977
Bundle IDcom.jetpack.alpha
Commitb4d61a7
Installation URL07kchdd5jo0c8
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@wpmobilebot

Copy link
Copy Markdown
Contributor

🤖 Build Failure Analysis

This build has failures. Claude has analyzed them - check the build annotations for details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants