Skip to content

Commit

Permalink
[ios] fix memory leak in RadioButton (#21151)
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.

Other changes:

* `propertyChanging` seems wrong? Reading the existing code, it should
be checking `oldvalue` instead of `newvalue`?
  • Loading branch information
jonathanpeppers committed Mar 18, 2024
1 parent 8569185 commit 2007ab7
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 10 deletions.
13 changes: 9 additions & 4 deletions src/Controls/src/Core/Border/Border.cs
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ public Thickness Padding
BindableProperty.Create(nameof(StrokeShape), typeof(IShape), typeof(Border), new Rectangle(),
propertyChanging: (bindable, oldvalue, newvalue) =>
{
if (newvalue is not null)
if (oldvalue is not null)
(bindable as Border)?.StopNotifyingStrokeShapeChanges();
},
propertyChanged: (bindable, oldvalue, newvalue) =>
Expand All @@ -64,10 +64,13 @@ void NotifyStrokeShapeChanges()

if (strokeShape is VisualElement visualElement)
{
AddLogicalChild(visualElement);
SetInheritedBindingContext(visualElement, BindingContext);
_strokeShapeChanged ??= (sender, e) => OnPropertyChanged(nameof(StrokeShape));
_strokeShapeProxy ??= new();
_strokeShapeProxy.Subscribe(visualElement, _strokeShapeChanged);

OnParentResourcesChanged(this.GetMergedResources());
((IElementDefinition)this).AddResourcesChangedListener(visualElement.OnParentResourcesChanged);
}
}

Expand All @@ -77,7 +80,9 @@ void StopNotifyingStrokeShapeChanges()

if (strokeShape is VisualElement visualElement)
{
RemoveLogicalChild(visualElement);
((IElementDefinition)this).RemoveResourcesChangedListener(visualElement.OnParentResourcesChanged);

SetInheritedBindingContext(visualElement, null);
_strokeShapeProxy?.Unsubscribe();
}
}
Expand All @@ -87,7 +92,7 @@ void StopNotifyingStrokeShapeChanges()
BindableProperty.Create(nameof(Stroke), typeof(Brush), typeof(Border), null,
propertyChanging: (bindable, oldvalue, newvalue) =>
{
if (newvalue is not null)
if (oldvalue is not null)
(bindable as Border)?.StopNotifyingStrokeChanges();
},
propertyChanged: (bindable, oldvalue, newvalue) =>
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 2007ab7

Please sign in to comment.