-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[Android] NET10 - Exception on quit - fix #32647
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Deleted the OnDestroy method and its dispatcher-based disposal logic from ShellFragmentContainer. Cleanup is unnecessary in this context and causes Android.Runtime.JavaProxyThrowable exception
|
Hey there @@kubaflo! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
Co-authored-by: kubaflo <[email protected]>
PR #32647 Review SummaryOverviewPR Title: [Android] NET10 - Exception on quit - fix Problem StatementAn unhandled Error DetailsFrom the issue logs: Root Cause AnalysisWhen an Android app quits, the following sequence occurs:
This throws an PR ChangesThe PR removes the OnDestroy override entirely: - public override void OnDestroy()
- {
- _mauiContext
- .GetDispatcher()
- .Dispatch(Dispose);
-
- base.OnDestroy();
- }Files Changed:
Lines Changed: -9 (deletions only) Analysis1. Is the Fix Safe?✅ YES - The fix is safe for the following reasons: Cleanup is Already HandledThe public override void OnDestroyView()
{
base.OnDestroyView();
((IShellContentController)ShellContentTab).RecyclePage(_page);
_page = null;
}This recycles the page and nulls out the reference, which is the primary cleanup needed. No Custom Destroy LogicUnlike other similar fragments (ShellContentFragment, ShellItemRenderer, ShellSectionRenderer), Fragment Lifecycle ManagementThe Android framework handles Fragment disposal through its lifecycle. The Fragment base class's 2. Comparison with Other FragmentsShellContentFragment: public override void OnDestroy()
{
base.OnDestroy();
Destroy(); // Calls custom Destroy() method with cleanup logic
}
void Destroy()
{
// Actual cleanup: AnimationFinished events, MauiWindowInsetListener, etc.
...
}ShellItemRenderer: public override void OnDestroy()
{
Destroy(); // Calls custom Destroy() method with cleanup logic
base.OnDestroy();
}
void Destroy()
{
// Actual cleanup: Fragments, handlers, etc.
...
}ShellFragmentContainer (BEFORE fix): public override void OnDestroy()
{
_mauiContext.GetDispatcher().Dispatch(Dispose); // Just calls base Dispose
base.OnDestroy();
}
// No custom Destroy() method!ShellFragmentContainer (AFTER fix): // No OnDestroy override - relies on base Fragment lifecycle3. Memory Leak Analysis✅ No memory leaks expected
4. Historical ContextThe OnDestroy method was introduced in commit Original Change (March 2022): - Activity.RunOnUiThread(Dispose);
+ _mauiContext.GetDispatcher().Dispatch(Dispose);The change was made to use the dispatcher instead of Why this worked before .NET 10: Testing RecommendationsManual Testing
Regression Testing
Review Comments✅ ApprovedReasoning:
Potential Concerns AddressedQ: Won't removing OnDestroy prevent proper cleanup? Q: Could this cause memory leaks? Q: Why was the Dispose call added in the first place? Q: Should we use a try-catch instead?
Alternative Solutions ConsideredAlternative 1: Try-Catch Blockpublic override void OnDestroy()
{
try
{
_mauiContext?.GetDispatcher()?.Dispatch(Dispose);
}
catch (ObjectDisposedException)
{
// Service provider already disposed, fragment disposal will be handled by Android
}
base.OnDestroy();
}Why rejected: Adds complexity without benefit. The disposal is unnecessary. Alternative 2: Check Service Providerpublic override void OnDestroy()
{
var dispatcher = _mauiContext?.GetOptionalDispatcher();
if (dispatcher != null)
{
dispatcher.Dispatch(Dispose);
}
base.OnDestroy();
}Why rejected: Still unnecessary since there's no custom cleanup logic. Alternative 3: Remove Entirely (CHOSEN)// No OnDestroy overrideWhy chosen: Simplest and safest solution. Removes unnecessary code that depends on disposed resources. RecommendationsFor Merge✅ APPROVE - This PR should be merged. Justification:
Additional Suggestions
ConclusionThe fix is correct and safe. It removes unnecessary code that was causing an exception during app shutdown. The cleanup that was being performed (calling Fragment.Dispose()) is not needed because:
This is a minimal, surgical fix that addresses the root cause without introducing new issues. Review Checklist
Final Recommendation✅ APPROVE AND MERGE This PR correctly fixes a critical regression in .NET MAUI 10 with a minimal, safe change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes an ObjectDisposedException that occurs during Android app shutdown when ShellFragmentContainer.OnDestroy() attempts to dispatch Dispose() via the dispatcher after the MauiContext service provider has already been disposed.
Key Changes:
- Removed the
OnDestroy()override fromShellFragmentContainerthat was callingDispose()via the dispatcher - Cleanup continues to be handled by the existing
OnDestroyView()implementation
f07c16b to
30b11a2
Compare
Deleted the OnDestroy method and its dispatcher-based disposal logic from ShellFragmentContainer. Cleanup is unnecessary in this context and causes Android.Runtime.JavaProxyThrowable exception
Deleted the OnDestroy method and its dispatcher-based disposal logic from ShellFragmentContainer. Cleanup is unnecessary in this context and causes Android.Runtime.JavaProxyThrowable exception
Deleted the OnDestroy method and its dispatcher-based disposal logic from ShellFragmentContainer. Cleanup is unnecessary in this context and causes Android.Runtime.JavaProxyThrowable exception
Deleted the OnDestroy method and its dispatcher-based disposal logic from ShellFragmentContainer. Cleanup is unnecessary in this context and causes Android.Runtime.JavaProxyThrowable exception
Deleted the OnDestroy method and its dispatcher-based disposal logic from ShellFragmentContainer. Cleanup is unnecessary in this context and causes Android.Runtime.JavaProxyThrowable exception
Deleted the OnDestroy method and its dispatcher-based disposal logic from ShellFragmentContainer. Cleanup is unnecessary in this context and causes Android.Runtime.JavaProxyThrowable exception
Deleted the OnDestroy method and its dispatcher-based disposal logic from ShellFragmentContainer. Cleanup is unnecessary in this context and causes Android.Runtime.JavaProxyThrowable exception
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!
Description of Change
ShellFragmentContainer.OnDestroy()attempts to dispatchDispose()via the dispatcher during app shutdown. TheMauiContextservice provider is already disposed at this point, causingObjectDisposedException→JavaProxyThrowable.Fix: Remove the
OnDestroy()override entirely.Why safe:
OnDestroyView()already handles cleanup (page recycling, null references)Destroy()logic exists (unlikeShellContentFragment/ShellItemRenderer)Root cause: Commit 0e86703 (PR #5064) switched from
Activity.RunOnUiThread(Dispose)to dispatcher-based disposal, introducing a dependency on the service provider being available—not guaranteed during shutdown.Issues Fixed
Fixes #32600
Original prompt