[ios] fix memory leak in RadioButton #21151
Conversation
Context: https://github.com/heyThorsten/GCTest Fixes: dotnet#20023 In testing the above sample, a page with a `RadioButton` has a memory leak due to the usage of `Border.StrokeShape`. There was also a slight "rabbit" hole, where we thought there was also an issue with `GestureRecognizers`. But this was not the case in a real app (just unit tests): * dotnet#21089 It turns out that when `GestureRecognizers` are used in `MemoryTests`, they will leak if there is no `Window` involved. Instead of using `MemoryTests` like I would traditionally, we should instead use `NavigationPageTests.DoesNotLeak()`. I can reproduce the problem with `RadioButton` *and* a `Window` exists in the test. ~~ Underlying issue ~~ It appears that `Border.StrokeShape` does not use the same pattern as other properties like `Border.Stroke`: * On set: `SetInheritedBindingContext(visualElement, BindingContext);` * On unset: `SetInheritedBindingContext(visualElement, null);` It instead used: * On set: `AddLogicalChild(visualElement);` * On unset: `RemoveLogicalChild(visualElement);` 6136a8a that introduced these does not mention why one was used over another. I am unsure if this will cause a problem, but it fixes the leak.
|
/cc @tj-devel709 |
| AddLogicalChild(visualElement); | ||
| SetInheritedBindingContext(visualElement, BindingContext); |
There was a problem hiding this comment.
@PureWeen maybe you remember if there is something this change would break? This makes Border.StrokeShape and Border.Stroke the same now.
There was a problem hiding this comment.
This should be fine
Prior to converting this to AddLogicalChild we were already setting the parent
https://github.com/dotnet/maui/blame/cd7a0e021d99be3ef8c7d83437fb66f5080cd5af/src/Controls/src/Core/Border/Border.cs#L67-L68
Which is why I converted it over to AddLogicalChild during that big push
Stephanes changes with these changes should make this all work alright.
The only weird thing here is if the strokes are static resources this code is still weird, but it's already weird and this makes it less weird at least.
There was a problem hiding this comment.
do we need to copy the concept with parent resource propagation that's part of stroke?
maui/src/Controls/src/Core/Border/Border.cs
Lines 113 to 114 in 2a7abf4
There was a problem hiding this comment.
For Stroke, I guess you could have a GradientBrush that uses a {StaticResource} for a color inside. That seems like it would be required.
For StrokeShape, I guess you can do the exact thing for something like a double value on some shape, so I'll make the properties the same.
This makes `Border.StrokeShape` closer to existing code in `Border.Stroke`.
jonathanpeppers
added
memory-leak 💦
Context: https://github.com/heyThorsten/GCTest
Fixes: #20023
In testing the above sample, a page with a
RadioButtonhas a memory leak due to the usage ofBorder.StrokeShape.There was also a slight "rabbit" hole, where we thought there was also an issue with
GestureRecognizers. But this was not the case in a real app (just unit tests):GestureRecognizers#21089It turns out that when
GestureRecognizersare used inMemoryTests, they will leak if there is noWindowinvolved.Instead of using
MemoryTestslike I would traditionally, we should instead useNavigationPageTests.DoesNotLeak(). I can reproduce the problem withRadioButtonand aWindowexists in the test.Underlying issue
It appears that
Border.StrokeShapedoes not use the same pattern as other properties likeBorder.Stroke:SetInheritedBindingContext(visualElement, BindingContext);SetInheritedBindingContext(visualElement, null);It instead used:
AddLogicalChild(visualElement);RemoveLogicalChild(visualElement);6136a8a that introduced these does not mention why one was used over another. I am unsure if this will cause a problem, but it fixes the leak.