Fix Android MediaPicker result recovery#35455
Conversation
|
Hey there @@AdamEssenmacher! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 35455Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 35455" |
github-actions
Bot
added
the
area-essentials
PureWeen
added
area-essentials-mediapicker
platform/android
t/bug
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/review -b feature/regression-check -p android |
MauiBot
added
s/agent-review-incomplete
s/agent-reviewed
|
/review -b feature/regression-check -p android |
1 similar comment
|
/review -b feature/regression-check -p android |
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 10 findings
See inline comments for details.
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
🤖 Automated review — alternative fix proposed
The expert-reviewer evaluation compared the PR fix against #1 automatically generated candidates and selected try-fix-1 as the strongest fix.
Why: try-fix-1 is the smallest correct fix: it ships the PR's Java pruning (the actual .NET 8->10.0.60 regression root cause) plus the AndroidX ActivityResultLauncher switch, while deleting the 3265-line speculative cross-process recovery layer (state machine, hand-rolled v1-v5 serializer, 3-method public API, 50+ device tests). It resolves all 10 reviewer findings (most as N/A), eliminates the gate's CS0260/CS0115 build errors and the failing MediaPickerRecovery_Tests, and ships zero new public API. The PR (and pr-plus-reviewer) failed the gate; try-fix candidates were predicted-pass (no Helix infra in this session, disclosed in the report).
Please consider applying the candidate diff below (or use it as guidance). Once you push an update, this workflow will re-trigger and re-evaluate.
Candidate diff (`try-fix-1`)
diff --git a/src/Core/AndroidNative/maui/src/main/java/com/microsoft/maui/PlatformMauiAppCompatActivity.java b/src/Core/AndroidNative/maui/src/main/java/com/microsoft/maui/PlatformMauiAppCompatActivity.java
index 3b2b9d5551..72dd2e65fa 100644
--- a/src/Core/AndroidNative/maui/src/main/java/com/microsoft/maui/PlatformMauiAppCompatActivity.java
+++ b/src/Core/AndroidNative/maui/src/main/java/com/microsoft/maui/PlatformMauiAppCompatActivity.java
@@ -1,7 +1,5 @@
package com.microsoft.maui;
-import android.content.Context;
-import android.content.res.Resources;
import android.content.res.TypedArray;
import androidx.appcompat.app.AppCompatActivity;
@@ -12,11 +10,28 @@ import android.os.Bundle;
* Class for batching native method calls within the MauiAppCompatActivity implementation
*/
public class PlatformMauiAppCompatActivity {
+ // These are Android framework / AndroidX saved-instance-state keys. MAUI does not create
+ // the bundles stored under these keys; it only removes or preserves them before AppCompat
+ // restores saved state. AndroidX does not expose public constants for these values.
+ //
+ // ComponentActivity saves pending ActivityResultRegistry state here. Preserving this bundle
+ // lets AndroidX replay pending activity results after activity or process recreation.
+ private static final String ACTIVITY_RESULT_REGISTRY_KEY = "android:support:activity-result";
+
+ // Framework FragmentManager and AndroidX FragmentManager saved-state keys. MAUI removes these
+ // when fragment restore is disabled because restoring old platform fragments can conflict with
+ // MAUI's own navigation/window reconstruction.
+ private static final String ANDROID_FRAGMENTS_KEY = "android:fragments";
+ private static final String SUPPORT_FRAGMENTS_KEY = "android:support:fragments";
+
+ // SavedStateRegistry's top-level bundle key. Older MAUI behavior removed this whole bundle to
+ // suppress fragment restore side effects, but that also discarded ActivityResultRegistry state.
+ private static final String SAVED_STATE_REGISTRY_KEY = "androidx.lifecycle.BundlableSavedStateRegistry.key";
+
public static void onCreate(AppCompatActivity activity, Bundle savedInstanceState, boolean allowFragmentRestore, int splashAttr, int mauiTheme)
{
if (!allowFragmentRestore && savedInstanceState != null) {
- savedInstanceState.remove("android:support:fragments");
- savedInstanceState.remove("androidx.lifecycle.BundlableSavedStateRegistry.key");
+ removeFragmentRestoreState(savedInstanceState);
}
boolean mauiSplashAttrValue = false;
@@ -33,4 +48,30 @@ public class PlatformMauiAppCompatActivity {
activity.setTheme(mauiTheme);
}
}
+
+ private static void removeFragmentRestoreState(Bundle savedInstanceState)
+ {
+ // First remove the direct fragment entries that may be present in the activity state.
+ savedInstanceState.remove(ANDROID_FRAGMENTS_KEY);
+ savedInstanceState.remove(SUPPORT_FRAGMENTS_KEY);
+
+ Bundle savedStateRegistry = savedInstanceState.getBundle(SAVED_STATE_REGISTRY_KEY);
+ if (savedStateRegistry != null) {
+ // The saved-state registry is a shared AndroidX container. Extract the activity-result
+ // entry before removing the container so pending activity results are not lost with the
+ // fragment-related providers.
+ Bundle activityResultRegistryState = savedStateRegistry.getBundle(ACTIVITY_RESULT_REGISTRY_KEY);
+
+ savedInstanceState.remove(SAVED_STATE_REGISTRY_KEY);
+
+ if (activityResultRegistryState != null) {
+ // Keep only the AndroidX ActivityResultRegistry state needed to replay pending
+ // results after activity/process recreation. Other saved-state providers may
+ // contain fragment state that MAUI cannot safely restore.
+ Bundle prunedSavedStateRegistry = new Bundle();
+ prunedSavedStateRegistry.putBundle(ACTIVITY_RESULT_REGISTRY_KEY, activityResultRegistryState);
+ savedInstanceState.putBundle(SAVED_STATE_REGISTRY_KEY, prunedSavedStateRegistry);
+ }
+ }
+ }
}
diff --git a/CustomAgentLogsTmp/PRState/35455/PRAgent/try-fix-1/csharp-sketch.md b/CustomAgentLogsTmp/PRState/35455/PRAgent/try-fix-1/csharp-sketch.md
new file mode 100644
index 0000000..1111111
--- /dev/null
+++ b/CustomAgentLogsTmp/PRState/35455/PRAgent/try-fix-1/csharp-sketch.md
@@ -0,0 +1,30 @@
+# try-fix-1 C# sketch (companion to Java diff)
+
+The Java hunk above IS the regression fix. The C# delta required to use AndroidX
+launchers (so AndroidX has a registered launcher to replay results into) is:
+
+1. Add three small files (no recovery coupling):
+ * `src/Essentials/src/Platform/CapturePhotoForResult.android.cs` -- thin
+ `ActivityForResultRequest<TakePicture, JavaBoolean>` singleton, registered
+ in `ActivityStateManager` at activity-create time.
+ * `src/Essentials/src/Platform/CaptureVideoForResult.android.cs` -- same
+ pattern for `CaptureVideo` contract.
+ * `src/Essentials/src/Platform/PickVisualMediaForResult.android.cs` /
+ `PickMultipleVisualMediaForResult.android.cs` -- thin AndroidX launcher
+ wrappers that materialize URIs **on Task.Run** (fixes finding F1/F2).
+
+2. Modify `src/Essentials/src/MediaPicker/MediaPicker.android.cs` to use
+ `CapturePhotoForResult.Instance.Launch(outputUri)` /
+ `CaptureVideoForResult.Instance.Launch(outputUri)` instead of the legacy
+ `IntermediateActivity.StartAsync(captureIntent, requestCode, ...)`. The
+ AndroidX launcher MUST be registered before STARTED, which is achieved by
+ adding a one-line registration in `ActivityStateManager.OnActivityCreated`.
+
+3. NO new public API. NO `MediaPickerRecovery.android.cs`. NO
+ `MediaPickerRecoveryManager.android.cs`. NO `MediaPickerRecoveryStore`.
+ NO new entries in `PublicAPI.Unshipped.txt`. NO `MediaPickerRecovery_Tests`.
+
+4. On orphaned-result (process death + AndroidX replay): the registered
+ launcher's no-op `OnActivityResultForOrphanedLaunch` swallows the boolean
+ result; the captured photo file is left on disk in the app's cache and the
+ user re-taps the camera button. Accepted UX trade-off.
|
/review -b feature/regression-check -p android |
🔬 Multimodal Expert Review — PR #354554 independent expert reviewers analyzed this PR across different dimensions using top-tier models:
Overall Assessment: The architecture is fundamentally sound — durable record + AndroidX replay + orphaned-result dispatch is the right pattern. The 🔴 Critical / High1. ARM Memory Model Issue in The double-checked pattern relies on Fix: Insert 2. Persistable URI Permissions Are Never Released
Fix: After 3. Saved-State Surgery Drops ALL Non-Activity-Result Providers
Suggestion: Invert the policy — drop only fragment-restore-related providers, preserve everything else. Or at minimum document the known limitation. 4. Pre-existing but now load-bearing: during configuration-change recreation (rotation, dark mode), the new activity's Fix: Compare 5. Between Suggestions:
6. Debug-Only Reflection Check for AndroidX Constant The hardcoded Suggestion: Run the check unconditionally. On mismatch, prefer the reflected key as source of truth, with the hardcoded constant as fallback. 7. Unbounded Recovery Result Accumulation SharedPreferences records grow indefinitely if the app never calls Fix: Add expiration/LRU eviction. In 8. Passing Fix: Either reject 🟡 Medium
📋 Test Coverage AssessmentVerdict: The 1527-line device test file provides substantial coverage of the state machine, waiters, and cancel/empty/missing-file scenarios. Identified gaps:
Flakiness risk: The 5-second 🟢 Positive Architecture NotesAll 4 reviewers independently recognized strong design decisions:
This review was generated by 4 independent AI reviewers (Claude Opus 4.7 xhigh, Opus 4.7 high, Opus 4.7, GPT 5.5). Findings were de-duplicated and cross-validated. The top recommendations for merge-readiness are #1 (ARM memory barrier), #2 (URI permission leak), #4 (Register early-return), and #5 (BeginOperation race). |
kubaflo
left a comment
kubaflo
left a comment
There was a problem hiding this comment.
Please look at the review comment
|
Review finding #1 was addressed in AdamEssenmacher@7dabf2f. |
|
/review -b feature/refactor-copilot-yml |
|
Addressed review finding #5 in commit AdamEssenmacher/maui@2327791. The live Android MediaPicker paths now use an atomic |
|
Addressed review finding #8 in 39a8529.\n\nThe wait API now preserves immediate-return behavior for |
🔍 Multimodal Code Review — PR #35455Reviewed with: Claude Opus 4.7 (expert reviewer + rubber-duck critique) Overall AssessmentThis is a well-architected solution to a genuinely hard Android problem (process death during camera/picker flows). The state machine is clean (Pending → ResultAccepted → Recovered), the durable SharedPreferences store is appropriate, and the separation between the recovery manager, public API surface, and ActivityResult infrastructure is thoughtful. The ~2000 lines of device tests show comprehensive coverage of the state machine, race conditions, and edge cases. The Java-side change to selectively preserve That said, both reviewers independently flagged several concrete issues worth addressing. 🔴 Issues to Address Before Merge1.
|
|
/review -b feature/refactor-copilot-yml |
|
|
|
I've re-reviewed all of the individual items listed above and think this PR is probably in a pretty good state in its current form. Re-opening. |
5 participants
Description of Change
Android can destroy or recreate an app process while another activity is in front. This is especially relevant for camera flows: the Android Activity Result documentation explicitly calls out memory-intensive operations such as camera usage as cases where the launching process/activity may be destroyed, and says result callbacks must be registered unconditionally when the activity is recreated:
https://developer.android.com/training/basics/intents/result
When this happens today, MAUI’s original
MediaPickertask is gone. The user can successfully finish the system camera or picker UI, but the app has no reliable way to receive the result. On affected device/app configurations, this effectively makes photo and/or video capture unusable throughMediaPicker.This change adds Android-only recovery support for AndroidX-backed
MediaPickeractivity results. MAUI now registers the relevant AndroidX activity-result launchers early, persists the active MediaPicker operation before launch, durably records accepted AndroidX callback results, and exposes an opt-in recovery surface so apps can retrieve results after process/activity recreation.The recovery surface is additive and Android-only:
MediaPicker.GetRecoveredMediaPickerResultsAsync()MediaPicker.WaitForRecoveredMediaPickerResultsAsync(CancellationToken)MediaPicker.ClearRecoveredMediaPickerResultAsync(string id)Recovery covers AndroidX-backed MediaPicker flows across the board:
Picker URI handling follows Android’s photo picker guidance around persisted media access:
https://developer.android.com/training/data-storage/shared/photo-picker#persist-media-file-access
Normal live-process behavior is unchanged: existing
MediaPickerandIMediaPickermethods still complete normally when the app survives, and no duplicate recovered result is queued.This also adds Android device-test coverage for the recovery state machine, callback routing, cancellation/wait behavior, duplicate prevention, picker URI materialization, and capture photo processing safety. I also smoke-tested the happy path manually on an API 36 AVD for photo capture, video capture, photo pick, video pick, and multi-photo pick.
Issues Fixed
Fixes #35308