[iOS] Fix HEIC images picked via PickPhotosAsync not displayed#34954
Conversation
|
Hey there @@HarishwaranVijayakumar! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
24efe7c to
1cd828b
Compare
🚦 Gate — Test Before and After Fix
🚦 Gate Session —
|
🤖 AI Summary
📊 Review Session —
|
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #34954 | Null out pd.Handler before DismissViewController in DidFinishPicking |
MediaPicker.ios.cs |
Original PR |
🔧 Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix (opus-4.6) | Remove Handler?.Invoke() from PhotoPickerPresentationControllerDelegate.Dispose() entirely |
MediaPicker.ios.cs |
Relies solely on DidDismiss for swipe-cancel; may lose safety net |
|
| 2 | try-fix (sonnet-4.6) | Add _pickerCompleted flag + MarkCompleted() to delegate; gate Handler?.Invoke() in both DidDismiss and Dispose |
MediaPicker.ios.cs |
Explicit state; encapsulates cancel-vs-complete within delegate; more verbose | |
| 3 | try-fix (gpt-5.3-codex) | Hold strong reference to PhotoPickerPresentationControllerDelegate on PhotoPickerDelegate to prevent premature GC |
❌ FAIL (MSB3644 build env blocker) | MediaPicker.ios.cs |
Prevents GC race by extending lifetime; build env blocked runtime test |
| 4 | try-fix (gpt-5.4) | Interlocked.Exchange atomic one-shot callback consumption |
MediaPicker.ios.cs |
Thread-safe but more complex than needed (iOS main-thread only) | |
| PR | PR #34954 | Null pd.Handler in DidFinishPicking before DismissViewController |
MediaPicker.ios.cs |
Original PR; minimal, targeted, preserves cancel safety net |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| claude-opus-4.6 | 2 | No | NO NEW IDEAS — all approaches target same fundamental race |
Exhausted: Yes (4/4 models, 1 cross-pollination round)
Selected Fix: PR's fix — null pd.Handler before DismissViewController — smallest, most targeted, preserves swipe-cancel safety net, no additional complexity
📋 Report — Final Recommendation
✅ Final Recommendation: APPROVE
Phase Status
| Phase | Status | Notes |
|---|---|---|
| Pre-Flight | ✅ COMPLETE | Issue #34953 — iOS regression in PickPhotosAsync for HEIC images |
| Gate | No tests in PR — recommend author adds tests | |
| Try-Fix | ✅ COMPLETE | 4 attempts, 0 PASS (all BLOCKED by env / no iOS device); PR's fix selected as best |
| Report | ✅ COMPLETE |
Summary
PR #34954 fixes a GC race condition in MediaPicker.ios.cs that causes HEIC images picked via PickPhotosAsync to return blank. The fix is minimal, correctly targeted, and well-commented. All 4 alternative approaches explored via try-fix were either more complex or equivalent — none outperformed the PR's one-liner. Gate was SKIPPED because no tests were added; recommending tests via write-tests-agent.
Root Cause
PhotoPickerPresentationControllerDelegate.Dispose() invokes Handler?.Invoke() → tcs.TrySetResult([]) (empty list). This fires during GC after DismissViewController is called but before the async CompletedHandler finishes HEIC transcoding via NSItemProvider.LoadDataRepresentationAsync. HEIC is particularly vulnerable because transcoding is significantly slower than JPEG/PNG, widening the GC collection window.
Regression introduced by PR #34250 which moved CompletedHandler invocation into the DismissViewController completion callback.
Fix Quality
The fix is correct and minimal:
- Nulls
pd.HandlerinDidFinishPickingbeforeDismissViewController— prevents the race at the exact right moment - Preserves the swipe-cancel behavior:
DidDismissstill firesHandlerwhen user swipes down (the_pickerCompletedpath is unaffected) - Well-commented explaining the race condition
Minor observation (non-blocking): The single-photo PickPhotoAsync path (line ~133–162) has the same PhotoPickerPresentationControllerDelegate pattern. However, its CompletedHandler calls GetFileResult (synchronous), so the race window is much smaller. Out of scope for this PR.
Missing tests: Gate flagged no tests added. A device test verifying HEIC photo pick results is recommended.
…t#34954) <!-- Please let the below note in for people that find this PR --> > [!NOTE] > Are you waiting for the changes in this PR to be merged? > It would be very helpful if you could [test the resulting artifacts](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! <!-- !!!!!!! MAIN IS THE ONLY ACTIVE BRANCH. MAKE SURE THIS PR IS TARGETING MAIN. !!!!!!! --> ### Issue Details - HEIC images picked via PickPhotosAsync are not displayed — the image appears blank, whereas other image formats (e.g., JPEG, PNG) display correctly. ### Root Cause of the issue - PR [34250](dotnet#34250) moved CompletedHandler invocation into the DismissViewController completion callback. This introduced a GC race condition where PhotoPickerPresentationControllerDelegate.Dispose() fires tcs.TrySetResult([]) before the async CompletedHandler finishes HEIC transcoding. - HEIC is particularly affected because NSItemProvider.LoadDataRepresentationAsync transcoding is significantly slower than JPEG/PNG loading, widening the GC window. ### Description of Change **Race condition prevention:** * In `PhotoPickerDelegate.DidFinishPicking`, the `Handler` property of `PhotoPickerPresentationControllerDelegate` is set to `null` before dismissing the picker to avoid a garbage collection race condition that could interfere with the async completion handler, especially during slow operations like HEIC transcoding. <!-- Enter description of the fix in this section --> ### Issues Fixed Fixes dotnet#34953 ### Tested the behaviour in the following platforms - [ ] - Windows - [ ] - Android - [x] - iOS - [ ] - Mac | Before | After | |----------|----------| | <video src="https://github.com/user-attachments/assets/368192e4-57ea-4732-82df-3e3ca386ab35"> | <video src="https://github.com/user-attachments/assets/0e3a5066-e092-4461-9199-1730475307d8"> | <!-- Are you targeting main? All PRs should target the main branch unless otherwise noted. -->
<!-- Please let the below note in for people that find this PR --> > [!NOTE] > Are you waiting for the changes in this PR to be merged? > It would be very helpful if you could [test the resulting artifacts](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! <!-- !!!!!!! MAIN IS THE ONLY ACTIVE BRANCH. MAKE SURE THIS PR IS TARGETING MAIN. !!!!!!! --> ### Issue Details - HEIC images picked via PickPhotosAsync are not displayed — the image appears blank, whereas other image formats (e.g., JPEG, PNG) display correctly. ### Root Cause of the issue - PR [34250](#34250) moved CompletedHandler invocation into the DismissViewController completion callback. This introduced a GC race condition where PhotoPickerPresentationControllerDelegate.Dispose() fires tcs.TrySetResult([]) before the async CompletedHandler finishes HEIC transcoding. - HEIC is particularly affected because NSItemProvider.LoadDataRepresentationAsync transcoding is significantly slower than JPEG/PNG loading, widening the GC window. ### Description of Change **Race condition prevention:** * In `PhotoPickerDelegate.DidFinishPicking`, the `Handler` property of `PhotoPickerPresentationControllerDelegate` is set to `null` before dismissing the picker to avoid a garbage collection race condition that could interfere with the async completion handler, especially during slow operations like HEIC transcoding. <!-- Enter description of the fix in this section --> ### Issues Fixed Fixes #34953 ### Tested the behaviour in the following platforms - [ ] - Windows - [ ] - Android - [x] - iOS - [ ] - Mac | Before | After | |----------|----------| | <video src="https://github.com/user-attachments/assets/368192e4-57ea-4732-82df-3e3ca386ab35"> | <video src="https://github.com/user-attachments/assets/0e3a5066-e092-4461-9199-1730475307d8"> | <!-- Are you targeting main? All PRs should target the main branch unless otherwise noted. -->
<!-- Please let the below note in for people that find this PR --> > [!NOTE] > Are you waiting for the changes in this PR to be merged? > It would be very helpful if you could [test the resulting artifacts](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! <!-- !!!!!!! MAIN IS THE ONLY ACTIVE BRANCH. MAKE SURE THIS PR IS TARGETING MAIN. !!!!!!! --> ### Issue Details - HEIC images picked via PickPhotosAsync are not displayed — the image appears blank, whereas other image formats (e.g., JPEG, PNG) display correctly. ### Root Cause of the issue - PR [34250](#34250) moved CompletedHandler invocation into the DismissViewController completion callback. This introduced a GC race condition where PhotoPickerPresentationControllerDelegate.Dispose() fires tcs.TrySetResult([]) before the async CompletedHandler finishes HEIC transcoding. - HEIC is particularly affected because NSItemProvider.LoadDataRepresentationAsync transcoding is significantly slower than JPEG/PNG loading, widening the GC window. ### Description of Change **Race condition prevention:** * In `PhotoPickerDelegate.DidFinishPicking`, the `Handler` property of `PhotoPickerPresentationControllerDelegate` is set to `null` before dismissing the picker to avoid a garbage collection race condition that could interfere with the async completion handler, especially during slow operations like HEIC transcoding. <!-- Enter description of the fix in this section --> ### Issues Fixed Fixes #34953 ### Tested the behaviour in the following platforms - [ ] - Windows - [ ] - Android - [x] - iOS - [ ] - Mac | Before | After | |----------|----------| | <video src="https://github.com/user-attachments/assets/368192e4-57ea-4732-82df-3e3ca386ab35"> | <video src="https://github.com/user-attachments/assets/0e3a5066-e092-4461-9199-1730475307d8"> | <!-- Are you targeting main? All PRs should target the main branch unless otherwise noted. -->
<!-- Please let the below note in for people that find this PR --> > [!NOTE] > Are you waiting for the changes in this PR to be merged? > It would be very helpful if you could [test the resulting artifacts](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! <!-- !!!!!!! MAIN IS THE ONLY ACTIVE BRANCH. MAKE SURE THIS PR IS TARGETING MAIN. !!!!!!! --> ### Issue Details - HEIC images picked via PickPhotosAsync are not displayed — the image appears blank, whereas other image formats (e.g., JPEG, PNG) display correctly. ### Root Cause of the issue - PR [34250](#34250) moved CompletedHandler invocation into the DismissViewController completion callback. This introduced a GC race condition where PhotoPickerPresentationControllerDelegate.Dispose() fires tcs.TrySetResult([]) before the async CompletedHandler finishes HEIC transcoding. - HEIC is particularly affected because NSItemProvider.LoadDataRepresentationAsync transcoding is significantly slower than JPEG/PNG loading, widening the GC window. ### Description of Change **Race condition prevention:** * In `PhotoPickerDelegate.DidFinishPicking`, the `Handler` property of `PhotoPickerPresentationControllerDelegate` is set to `null` before dismissing the picker to avoid a garbage collection race condition that could interfere with the async completion handler, especially during slow operations like HEIC transcoding. <!-- Enter description of the fix in this section --> ### Issues Fixed Fixes #34953 ### Tested the behaviour in the following platforms - [ ] - Windows - [ ] - Android - [x] - iOS - [ ] - Mac | Before | After | |----------|----------| | <video src="https://github.com/user-attachments/assets/368192e4-57ea-4732-82df-3e3ca386ab35"> | <video src="https://github.com/user-attachments/assets/0e3a5066-e092-4461-9199-1730475307d8"> | <!-- Are you targeting main? All PRs should target the main branch unless otherwise noted. -->
<!-- Please let the below note in for people that find this PR --> > [!NOTE] > Are you waiting for the changes in this PR to be merged? > It would be very helpful if you could [test the resulting artifacts](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! <!-- !!!!!!! MAIN IS THE ONLY ACTIVE BRANCH. MAKE SURE THIS PR IS TARGETING MAIN. !!!!!!! --> ### Issue Details - HEIC images picked via PickPhotosAsync are not displayed — the image appears blank, whereas other image formats (e.g., JPEG, PNG) display correctly. ### Root Cause of the issue - PR [34250](#34250) moved CompletedHandler invocation into the DismissViewController completion callback. This introduced a GC race condition where PhotoPickerPresentationControllerDelegate.Dispose() fires tcs.TrySetResult([]) before the async CompletedHandler finishes HEIC transcoding. - HEIC is particularly affected because NSItemProvider.LoadDataRepresentationAsync transcoding is significantly slower than JPEG/PNG loading, widening the GC window. ### Description of Change **Race condition prevention:** * In `PhotoPickerDelegate.DidFinishPicking`, the `Handler` property of `PhotoPickerPresentationControllerDelegate` is set to `null` before dismissing the picker to avoid a garbage collection race condition that could interfere with the async completion handler, especially during slow operations like HEIC transcoding. <!-- Enter description of the fix in this section --> ### Issues Fixed Fixes #34953 ### Tested the behaviour in the following platforms - [ ] - Windows - [ ] - Android - [x] - iOS - [ ] - Mac | Before | After | |----------|----------| | <video src="https://github.com/user-attachments/assets/368192e4-57ea-4732-82df-3e3ca386ab35"> | <video src="https://github.com/user-attachments/assets/0e3a5066-e092-4461-9199-1730475307d8"> | <!-- Are you targeting main? All PRs should target the main branch unless otherwise noted. -->
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Issue Details
Root Cause of the issue
Description of Change
Race condition prevention:
PhotoPickerDelegate.DidFinishPicking, theHandlerproperty ofPhotoPickerPresentationControllerDelegateis set tonullbefore dismissing the picker to avoid a garbage collection race condition that could interfere with the async completion handler, especially during slow operations like HEIC transcoding.Issues Fixed
Fixes #34953
Tested the behaviour in the following platforms
Before_.34786.mov
After_.34786.mov