Skip to content

[ios] fix memory leaks in GestureRecognizers#21089

Closed
jonathanpeppers wants to merge 1 commit intodotnet:mainfrom
jonathanpeppers:ios-gestures-leak
Closed

[ios] fix memory leaks in GestureRecognizers#21089
jonathanpeppers wants to merge 1 commit intodotnet:mainfrom
jonathanpeppers:ios-gestures-leak

Conversation

@jonathanpeppers
Copy link
Copy Markdown
Member

@jonathanpeppers jonathanpeppers commented Mar 7, 2024

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.

@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 added the perf/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
Copy Markdown
Member Author

jonathanpeppers commented Mar 8, 2024

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

@PureWeen
Copy link
Copy Markdown
Member

PureWeen commented Mar 11, 2024

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
Copy Markdown
Member Author

jonathanpeppers commented Mar 11, 2024

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
@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 pull request Mar 12, 2024
Merged
jonathanpeppers added a commit that referenced this pull request 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 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@mattleibow mattleibow Awaiting requested review from mattleibow mattleibow was automatically assigned from dotnet/dotnet-maui-reviewers

@rmarinho rmarinho Awaiting requested review from rmarinho rmarinho was automatically assigned from dotnet/dotnet-maui-reviewers

Assignees

No one assigned

Labels

perf/memory-leak 💦 Memory usage grows / objects live forever (sub: perf)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jonathanpeppers @PureWeen