[ios] fix memory leak in TimePicker #16265
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
jonathanpeppers
added
the
memory-leak 💦
jonathanpeppers
added a commit
to jonathanpeppers/maui
that referenced
this pull request
Jul 21, 2023
This is an alternate idea to: dotnet#16265
|
We are discussing if there could be a handler lifecycle change to make this easier. For now, I might rework this change to be smaller and only fix |
rmarinho
reviewed
Jul 24, 2023
rmarinho
reviewed
Jul 24, 2023
jonathanpeppers
commented
Jul 25, 2023
jonathanpeppers
force-pushed
the
TimePickerOhNo
branch
from
827be04
to
5c9ffd3
Compare
jonathanpeppers
force-pushed
the
TimePickerOhNo
branch
from
5c9ffd3
to
706ccdd
Compare
jonathanpeppers
commented
Aug 1, 2023
Comment on lines
+42
to
43
| [InlineData(typeof(TimePicker))] | ||
| public async Task HandlerDoesNotLeak(Type type) |
There was a problem hiding this comment.
I reworked this PR to use the new test pattern in 0e27ca4.
The test was failing without these changes, so it makes these PRs easier.
jonathanpeppers
commented
Aug 1, 2023
Comment on lines
7
to
+9
| public partial class TimePickerHandler : ViewHandler<ITimePicker, UIDatePicker> | ||
| { | ||
| readonly UIDatePickerProxy _proxy = new(); |
There was a problem hiding this comment.
I found MacCatalyst has a different handler than iOS, so I found I had to make fixes in here as well.
jonathanpeppers
force-pushed
the
TimePickerOhNo
branch
from
706ccdd
to
9079f91
Compare
After working on some tooling to proactively find memory leaks, I found
a few in the `TimePicker`. This kind of highlights the pattern:
* `MauiTimePicker` is a subclass of `UIDatePicker`
* `TimePickerHandler` has a strong reference to `MauiTimePicker`
as the `PlatformView`.
* `TimePickerHandler` subscribes to various events and has a strong
reference to `MauiTimePicker`: creating a cycle.
This effectively causes entire pages to leak, because
`TimePicker.Parent.Parent`, etc. will hold a strong reference up to the
entire `Page`.
It is not enough to unsubscribe from these events in `DisconnectHandler`,
because it is not called in many cases.
For example, take the test:
await InvokeOnMainThreadAsync(() =>
{
var layout = new Grid();
var picker = new TimePicker();
layout.Add(picker);
var handler = CreateHandler<LayoutHandler>(layout);
viewReference = new WeakReference(handler);
handlerReference = new WeakReference(picker.Handler);
platformViewReference = new WeakReference(picker.Handler.PlatformView);
});
await AssertionExtensions.WaitForGC(viewReference, handlerReference, platformViewReference);
Assert.False(viewReference.IsAlive, "TimePicker should not be alive!");
Assert.False(handlerReference.IsAlive, "Handler should not be alive!");
Assert.False(platformViewReference.IsAlive, "PlatformView should not be alive!");
In this case, `Grid`'s `LayoutHandler.DisconnectHandler` is called, but
not the `TimePickerHandler`.
To solve this problem, I moved 5 events to the `MauiTimePicker` class
and was able to remove one.
I also found a similar problem with `MauiDoneAccessoryView` and converted
various `Action` callbacks to use `WeakReference` instead.
The test did not pass until solving all of the above.
jsuarezruiz
approved these changes
Aug 7, 2023
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
After working on some tooling to proactively find memory leaks, I found a few in the
TimePicker. This kind of highlights the pattern:MauiTimePickeris a subclass ofUIDatePickerTimePickerHandlerhas a strong reference toMauiTimePickeras thePlatformView.TimePickerHandlersubscribes to various events and has a strong reference toMauiTimePicker: creating a cycle.This effectively causes entire pages to leak, because
TimePicker.Parent.Parent, etc. will hold a strong reference up to the entirePage.It is not enough to unsubscribe from these events in
DisconnectHandler, because it is not called in many cases.For example, take the test:
In this case,
Grid'sLayoutHandler.DisconnectHandleris called, but not theTimePickerHandler.To solve this problem, I moved 5 events to the
MauiTimePickerclass and was able to remove one.I also found a similar problem with
MauiDoneAccessoryViewand converted variousActioncallbacks to useWeakReferenceinstead.The test did not pass until solving all of the above.