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.

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

[ios] fix memory leaks in GestureRecognizers #21089

Closed

Conversation

jonathanpeppers
Copy link
Member

Context: #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.

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 added the memory-leak 💦 Memory usage grows / objects live forever (sub: perf) label Mar 7, 2024
@jonathanpeppers jonathanpeppers marked this pull request as ready for review March 8, 2024 14:23
@jonathanpeppers jonathanpeppers requested a review from a team as a code owner March 8, 2024 14:23
@jonathanpeppers
Copy link
Member Author

This one we probably need to go manually test all the recognizers. Is there a sample I can try that?

@PureWeen
Copy link
Member

This one we probably need to go manually test all the recognizers. Is there a sample I can try that?

I don't think so, Device Tests don't really support gestures so we haven't really used them very heavily other than basic logical tests.

@jonathanpeppers
Copy link
Member Author

Ok, it turns out in practice that GestureRecognizers do not leak. Because when controls' Window is lost, GesturePlatformManager.Dispose() is called. The HandlerDoesNotLeak test for RadioButton does not have a Window at all to unset.

Tested with dotnet/maui/main:

Something like this works in the sample:

new BoxView {
  GestureRecognizers = {
    new PanGestureRecognizer(),
    new DragGestureRecognizer(),
    new DropGestureRecognizer(),
    new PanGestureRecognizer(),
    new PinchGestureRecognizer(),
    new PointerGestureRecognizer(),
    new SwipeGestureRecognizer(),
    new TapGestureRecognizer(),
}
2024-03-11 16:33:49.851 GCTest[23861:336330] BoxView Page
2024-03-11 16:33:52.080 GCTest[23861:336340] ~BoxView Page

I will re-think this one.

There is still an unsolved issue with RadioButton in the above sample:

2024-03-11 16:38:49.099 GCTest[24155:340813] RadioButton Page

Note there is no ~RadioButton Page message.

jonathanpeppers added a commit to jonathanpeppers/maui that referenced this pull request Mar 12, 2024
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 added a commit that referenced this pull request Mar 18, 2024
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 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
memory-leak 💦 Memory usage grows / objects live forever (sub: perf)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants