[ios] fix memory leak in Button
#14280
Conversation
Fixes: dotnet#12048 Context: https://github.com/jonathanpeppers/MemoryLeaksOniOS Context: https://stackoverflow.com/a/13059140 Issue dotnet#12048 has four scenarios that are currently fixed in `main` for Android and Windows. Unfortunately, one of the four scenarios fails on iOS due to its usage of `Button`. I could reproduce the issue by simply adding a `Button` to existing memory leak tests. `ButtonHandler.iOS.cs` has a "cycle": * `ButtonHandler` subscribes to events like `UIButton.TouchUpInside` making `UIButton` hold a strong reference to `ButtonHandler`. * `ButtonHandler` (like all handlers) has a strong reference to the `Microsoft.Maui.Button`. * `ButtonHandler` has a strong reference to `UIButton`. * `Microsoft.Maui.Button` has a strong reference to `ButtonHandler`. To solve this issue, we can make a nested classed named `ButtonProxy`: * Its purpose is subscribe to `UIButton.TouchUpInside` and invoke `Microsoft.Maui.Button.Clicked()`. * `ButtonProxy` has a weak reference to the `Microsoft.Maui.Button`. Now the `UIButton`, `ButtonProxy`, `ButtonHandler`, and `Microsoft.Maui.Button` can all be collected.
| @@ -166,20 +164,48 @@ static void SetControlPropertiesFromProxy(UIButton platformView) | |||
| } | |||
| } | |||
|
|
|||
| void OnButtonTouchUpInside(object? sender, EventArgs e) | |||
| class ButtonEventProxy | |||
There was a problem hiding this comment.
There are several controls that can benefit from a similar change. I can create a list with the ones with the greatest impact.
There was a problem hiding this comment.
Can you file a new issue with the list? I'll be going on vacation soon, so probably want to look at it when I get back.
|
I heard close/reopen PR is a way to fix the
Trying it, but I wonder if it will rerun all the CI again... |
Eilon
added
the
legacy-area-controls
| { | ||
| VirtualView?.Released(); | ||
| } | ||
| IButton? VirtualView => _virtualView is not null && _virtualView.TryGetTarget(out var v) ? v : null; |
There was a problem hiding this comment.
@PureWeen should this rather be at the base level for all handlers? Why are handlers holding strong references to virtual views when the virtual view decides what hander it is? Or is it the other way around?
At least for iOS, we should do this at the base level so that we can avoid this since all controls with events will do this.
There was a problem hiding this comment.
I think we should perhaps do both? From other cases where we've passed something into these special classes, we usually pass the handler in and then make it a weak reference to the handler.
For example this.
This special class is going to have to hold a weak reference to something. So, I don't think generalizing this inside the base of the handler will really change much of the code here.
jonathanpeppers
added
memory-leak 💦
Eilon
added
area-controls-button
samhouts
added
the
fixed-in-8.0.0-preview.4.8333
Fixes #12048
Context: https://github.com/jonathanpeppers/MemoryLeaksOniOS
Context: https://stackoverflow.com/a/13059140
Issue #12048 has four scenarios that are currently fixed in
mainfor Android and Windows. Unfortunately, one of the four scenarios fails on iOS due to its usage ofButton. I could reproduce the issue by simply adding aButtonto existing memory leak tests.ButtonHandler.iOS.cshas a "cycle":ButtonHandlersubscribes to events likeUIButton.TouchUpInsidemakingUIButtonhold a strong reference toButtonHandler.ButtonHandler(like all handlers) has a strong reference to theMicrosoft.Maui.Button.ButtonHandlerhas a strong reference toUIButton.Microsoft.Maui.Buttonhas a strong reference toButtonHandler.To solve this issue, we can make a nested classed named
ButtonProxy:Its purpose is subscribe to
UIButton.TouchUpInsideand invokeMicrosoft.Maui.Button.Clicked().ButtonProxyhas a weak reference to theMicrosoft.Maui.Button.Now the
UIButton,ButtonProxy,ButtonHandler, andMicrosoft.Maui.Buttoncan all be collected.