-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[android] fix memory leak in Window #16045
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes: dotnet#15972 Context: https://github.com/markusroessler/MauiWindowScopeSample In the above sample, `android.permission.FOREGROUND_SERVICE` is utilized to keep the app running even when terminated by the user. Unfortunately, in this case `Window` & various other objects live forever. When running the sample, the `InstanceCount` grows each time you close & launch the app again: 07-07 16:08:32.281 22562 22562 I DOTNET : ScopedService InstanceCount: 3 07-07 16:08:32.291 22562 22562 I DOTNET : MainWindow InstanceCount: 3 The first issue I noticed was `IWindow.Destroying()` was never called because of this code in `OnDestroy()`: if (!activity.IsFinishing) { var window = activity.GetWindow(); window?.Destroying(); } `IsFinishing` was always `true` from my debugging in this spot. `OnStop()` had the code: // As of Ice Cream Sandwich, Stopped is guaranteed to be called // even when the activity is finishing or being destroyed // We check for finishing and call destroying here if so if (activity.IsFinishing) window?.Destroying(); In this case `IsFinishing` was `false`. I think the solution here is that `OnDestroy()` should attempt to destroy the `Window` in all cases. There isn't a case you would *not* want to destroy it. The only problem being we can now destroy it twice -- and there is no `IWindow.IsDestroyed` value to check. We can simply handle `InvalidOperationException`, as this should be rare. With this change in place, I still saw a leak. Reviewing a `.gcdump` of the app, I saw the object tree: Microsoft.Maui.Controls.Platform.AlertManager.AlertRequestHelper List<Microsoft.Maui.Controls.Platform.AlertManager.AlertRequestHelper> Microsoft.Maui.Controls.Platform.AlertManager MauiWindowScopeSample.MainWindow Microsoft.Maui.Controls.MessagingCenter.MaybeWeakReference Microsoft.Maui.Controls.MessagingCenter.Subscription List<Microsoft.Maui.Controls.MessagingCenter.Subscription> Dictionary<Microsoft.Maui.Controls.MessagingCenter.Sender, List<Microsoft.Maui.Controls.MessagingCenter.Subscription>> Microsoft.Maui.Controls.MessagingCenter Other Roots Where `MessagingCenter` appears to keep `AlertRequestHelper` alive, which keeps the `Window` alive. After these changes, we instead get this every time: 07-07 16:08:38.967 22562 22562 I DOTNET : ScopedService InstanceCount: 2 07-07 16:08:38.978 22562 22562 I DOTNET : MainWindow InstanceCount: 2 07-07 16:08:39.015 22562 22562 I DOTNET : Lifecycle event: OnStart 07-07 16:08:39.072 22562 22562 I DOTNET : Lifecycle event: OnPostCreate 07-07 16:08:39.073 22562 22562 I DOTNET : Lifecycle event: OnResume 07-07 16:08:39.074 22562 22562 I DOTNET : Lifecycle event: OnPostResume 07-07 16:08:39.411 22562 22585 I DOTNET : MainWindow InstanceCount: 1 07-07 16:08:39.414 22562 22585 I DOTNET : ScopedService InstanceCount: 1 The `InstanceCount` returns to 1 as the finalizers run for these types. I have yet to figure out how to write a test for this scenario -- I've only been able to validate this fix manually testing the app.
jonathanpeppers
commented
Jul 7, 2023
src/Core/src/Hosting/LifecycleEvents/AppHostBuilderExtensions.Android.cs
Outdated
Show resolved
Hide resolved
jonathanpeppers
commented
Jul 7, 2023
@@ -506,6 +506,7 @@ void IWindow.Destroying() | |||
Destroying?.Invoke(this, EventArgs.Empty); | |||
OnDestroying(); | |||
|
|||
AlertManager.Unsubscribe(); |
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.
This call unsubscribes from MessagingCenter
things on Android.
Redth
approved these changes
Jul 12, 2023
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Labels
backport/approved
After some discussion or review, this PR or change was approved to be backported.
backport/suggested
The PR author or issue review has suggested that the change should be backported.
fixed-in-8.0.0-preview.7.8842
Look for this fix in 8.0.0-preview.7.8842!
memory-leak 💦
Memory usage grows / objects live forever (sub: perf)
platform/android 🤖
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #15972
Context: https://github.com/markusroessler/MauiWindowScopeSample
In the above sample,
android.permission.FOREGROUND_SERVICE
is utilized to keep the app running even when terminated by the user.Unfortunately, in this case
Window
& various other objects live forever. When running the sample, theInstanceCount
grows each time you close & launch the app again:The first issue I noticed was
IWindow.Destroying()
was never called because of this code inOnDestroy()
:IsFinishing
was alwaystrue
from my debugging in this spot.OnStop()
had the code:In this case
IsFinishing
wasfalse
.I think the solution here is that
OnDestroy()
should attempt to destroy theWindow
in all cases. There isn't a case you would not want to destroy it. The only problem being we can now destroy it twice -- and there is noIWindow.IsDestroyed
value to check. We can simply handleInvalidOperationException
, as this should be rare.With this change in place, I still saw a leak. Reviewing a
.gcdump
of the app, I saw the object tree:Where
MessagingCenter
appears to keepAlertRequestHelper
alive, which keeps theWindow
alive.After these changes, we instead get this every time:
The
InstanceCount
returns to 1 as the finalizers run for these types.I have yet to figure out how to write a test for this scenario -- I've only been able to validate this fix manually testing the app.