Skip to content

Commit

Permalink
[ios] fix memory leak in RadioButton
Browse files Browse the repository at this point in the history
Context: https://github.com/heyThorsten/GCTest
Fixes: #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):

* #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.
  • Loading branch information
jonathanpeppers committed Mar 12, 2024
1 parent df5061e commit 2a7abf4
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 8 deletions.
4 changes: 2 additions & 2 deletions src/Controls/src/Core/Border/Border.cs
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ void NotifyStrokeShapeChanges()

if (strokeShape is VisualElement visualElement)
{
AddLogicalChild(visualElement);
SetInheritedBindingContext(visualElement, BindingContext);
_strokeShapeChanged ??= (sender, e) => OnPropertyChanged(nameof(StrokeShape));
_strokeShapeProxy ??= new();
_strokeShapeProxy.Subscribe(visualElement, _strokeShapeChanged);
Expand All @@ -77,7 +77,7 @@ void StopNotifyingStrokeShapeChanges()

if (strokeShape is VisualElement visualElement)
{
RemoveLogicalChild(visualElement);
SetInheritedBindingContext(visualElement, null);
_strokeShapeProxy?.Unsubscribe();
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@
using System.Text;
using System.Threading.Tasks;
using Microsoft.Maui.Controls;
using Microsoft.Maui.Controls.Handlers;
using Microsoft.Maui.Controls.Handlers.Compatibility;
using Microsoft.Maui.Controls.Handlers.Items;
using Microsoft.Maui.Controls.Shapes;
using Microsoft.Maui.DeviceTests.Stubs;
using Microsoft.Maui.Handlers;
using Microsoft.Maui.Hosting;
Expand Down Expand Up @@ -33,15 +35,19 @@ void SetupBuilder()
handlers.AddHandler(typeof(TabbedPage), typeof(TabbedViewHandler));
#endif
handlers.AddHandler(typeof(FlyoutPage), typeof(FlyoutViewHandler));
handlers.AddHandler<Page, PageHandler>();
handlers.AddHandler<Window, WindowHandlerStub>();
handlers.AddHandler<Frame, FrameRenderer>();
handlers.AddHandler<Label, LabelHandler>();
handlers.AddHandler(typeof(ScrollView), typeof(ScrollViewHandler));
handlers.AddHandler<Border, BorderHandler>();
handlers.AddHandler<Button, ButtonHandler>();
handlers.AddHandler<CarouselView, CarouselViewHandler>();
handlers.AddHandler<CollectionView, CollectionViewHandler>();
handlers.AddHandler(typeof(Controls.ContentView), typeof(ContentViewHandler));
handlers.AddHandler(typeof(ScrollView), typeof(ScrollViewHandler));
handlers.AddHandler<Frame, FrameRenderer>();
handlers.AddHandler<IContentView, ContentViewHandler>();
handlers.AddHandler<Label, LabelHandler>();
handlers.AddHandler<Layout, LayoutHandler>();
handlers.AddHandler<Page, PageHandler>();
handlers.AddHandler<RadioButton, RadioButtonHandler>();
handlers.AddHandler<Shape, ShapeViewHandler>();
handlers.AddHandler<Window, WindowHandlerStub>();
});
});
}
Expand Down Expand Up @@ -323,6 +329,7 @@ public async Task DoesNotLeak()
new ContentView(),
new Label(),
new ScrollView(),
new RadioButton(),
}
};
pageReference = new WeakReference(page);
Expand Down

0 comments on commit 2a7abf4

Please sign in to comment.