Skip to content
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.

Sign up for GitHub

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

Jump to bottom

RadioButton is not currently being Garbage Collected #20023

Closed
PureWeen opened this issue Jan 20, 2024 · 2 comments · Fixed by #21151
Closed

RadioButton is not currently being Garbage Collected #20023

PureWeen opened this issue Jan 20, 2024 · 2 comments · Fixed by #21151
Assignees
@jonathanpeppers @jsuarezruiz
Labels
area-controls-radiobutton RadioButton, RadioButtonGroup fixed-in-8.0.20 fixed-in-8.0.40 fixed-in-9.0.0-preview.3.10457 t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.)
Milestone
.NET 8 SR4

Comments

@PureWeen
Copy link
Member

PureWeen commented Jan 20, 2024 •
edited

Description

Breaking out remaining work for: #18365

Add tests to ensure that RadioButton is GC'd.

Link to public reproduction project repository

https://github.com/heyThorsten/GCTest
@PureWeen PureWeen added legacy-area-perf Startup / Runtime performance area-controls-radiobutton RadioButton, RadioButtonGroup labels Jan 20, 2024
@PureWeen PureWeen added this to the .NET 8 SR3 milestone Jan 20, 2024
@ghost ghost added the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label Jan 20, 2024
@PureWeen PureWeen mentioned this issue Jan 20, 2024
Closed
21 tasks
@jonathanpeppers
Copy link
Member

@jsuarezruiz I was hoping someone else would take these over, and I would help?

@jsuarezruiz jsuarezruiz self-assigned this Feb 9, 2024
@jsuarezruiz jsuarezruiz mentioned this issue Feb 9, 2024
Closed
@PureWeen PureWeen modified the milestones: .NET 8 SR3, .NET 8 SR4 Mar 5, 2024
@jonathanpeppers
Copy link
Member

Posting here, @tj-devel709 and my findings.

#20472 does not seem like it would fix the problem here, as there are two different underlying problems:

  1. Adding a gesture recognizer to any control (even BoxView) causes a leak on iOS or Catalyst. There seems to be a general cycle when using gestures.

  2. Using Border.StrokeShape seems to be problematic, removing this line solves a leak:

maui/src/Controls/src/Core/Border/Border.cs

Line 67 in 51cabfd

AddLogicalChild(visualElement);

RadioButton uses gestures and Border shapes, so that is why this problem occurs for this control.

There may be a third leak in RadioButton we can solve by making this method static:

maui/src/Controls/src/Core/RadioButton/RadioButton.cs

Line 338 in 51cabfd

_tapGestureRecognizer.Tapped += SelectRadioButton;

maui/src/Controls/src/Core/RadioButton/RadioButton.cs

Line 368 in 51cabfd

void SelectRadioButton(object sender, EventArgs e)

(But we should retest after fixing 1 and 2 above).

jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue Mar 7, 2024
@jonathanpeppers
[ios] fix memory leaks in GestureRecognizers
8d65cde 
Context: dotnet#20023 (comment)

One of the underlying reasons `RadioButton` appears to leak, is that it
happens to use a `TapGestureRecognizer` internally. When testing,
@tj-devel709 and I found we could add pretty much *any* recognizer to
a simple control like `BoxView`, and it would cause the `BoxView` to
live forever.

I could reproduce this in a new unit test that I setup for each type of
gesture recognizer.

`GesturePlatformManager.iOS.cs` had two "cycles":

* `*Handler` -> ... -> `GesturePlatformManager` -> `*Handler`

* `*GestureRecognizer` -> `GesturePlatformManager` -> `*GestureRecognizer`

To fix these, we can make `GesturePlatformManager` not have strong
references pointing *back* to its parent classes:

* Make `_handler` a `WeakReference<IPlatformViewHandler>`

* Make `_gestureRecognizers` a `ConditionalWeakTable`

With these changes the tests pass, except for `PanGestureRecognizer`,
which had a "closure" in using the `panRecognizer` variable.

I fixed this one with:

* `((PanGestureRecognizer)panGestureRecognizer)` as `panGestureRecognizer`
  was backed by a `WeakReference`.
@jonathanpeppers jonathanpeppers mentioned this issue Mar 7, 2024
Closed
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue Mar 12, 2024
@jonathanpeppers
[ios] fix memory leak in RadioButton
2a7abf4 
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.
@jonathanpeppers jonathanpeppers mentioned this issue Mar 12, 2024
Merged
jonathanpeppers added a commit that referenced this issue Mar 18, 2024
@jonathanpeppers
[ios] fix memory leak in RadioButton (#21151)
2007ab7 
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`?
@github-actions github-actions bot locked and limited conversation to collaborators Apr 18, 2024
@Eilon Eilon added t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) and removed legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor legacy-area-perf Startup / Runtime performance labels May 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@jonathanpeppers jonathanpeppers

@jsuarezruiz jsuarezruiz

Labels
area-controls-radiobutton RadioButton, RadioButtonGroup fixed-in-8.0.20 fixed-in-8.0.40 fixed-in-9.0.0-preview.3.10457 t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.)
Projects
MAUI SDK Ongoing
Status: Done
Milestone
.NET 8 SR4
5 participants
@Eilon @jonathanpeppers @PureWeen @jsuarezruiz @samhouts