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

Possible Memory leaks? #12048

Closed
Larhei opened this issue Dec 13, 2022 · 5 comments · Fixed by #14280
Closed

Possible Memory leaks? #12048

Larhei opened this issue Dec 13, 2022 · 5 comments · Fixed by #14280
Assignees
@jonathanpeppers
Labels
fixed-in-8.0.0-preview.4.8333 Look for this fix in 8.0.0-preview.4.8333! legacy-area-perf Startup / Runtime performance p/0 Work that we can't release without platform/iOS 🍎 t/bug Something isn't working t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.)
Milestone
.NET 8 Planning

Comments

@Larhei
Copy link

Larhei commented Dec 13, 2022 •
edited
Loading

Description

We are seeing crashes of our app on iOS because of what looks to us like a memory shortage. So we took some time to break down the things we are seeing.
The provided Repro is doing the following.
After starting the app we call GC.Collect(); and GC.WaitForPendingFinalizers(); every 2 Seconds;
When navigating away from the inital page, we stop calling GC.Collect and WaitForPendingFinalizers.
When navigating back to the start page we start calling GC.Collect(); and GC.WaitForPendingFinalizers(); every 2 Seconds; again.

So in theory because the page is not in the backstack any more, and because it is not single instance on the ioc side, it should get collected.

Of my knowledge it is not guaranteed to Finalize. when calling Collect() and WaitForPendingFinalizers();
This is all fine, but after some time it should kick in. Right?

That is why we added a Page that get´s Finalized after same time.
After making the changes described in the different views we can see the views get finalized also.
So we would say these are Memoryleaks that can be worked around?

The biggest things we are seeing is that on iOS non of our pages get finalized. Even in the provided Repro an empty Contentpage is not finalizing. This could be because this is expected from your side.. But not for me.
If it is expected. Please give some context why.
If this is not expected, what can we do? Is there a workaround?

Steps to Reproduce

  1. Checkout
  2. Run the Sample
  3. Navigate to a Sample Page
  4. Navagate Back
  5. Wait and watch the VS Output to see if Finalizer of the previously navigated page get´s called.

Link to public reproduction project repository

https://github.com/Larhei/Maui-Issues/tree/main/GarbageCollection

Version with bug

7.0 (current)

Last version that worked well

Unknown/Other

Affected platforms

iOS, Android, Windows

Affected platform versions

iOS, Android, Windows

Did you find any workaround?

For some things yes. Read the xaml ;-)

Relevant log output

No response
@Larhei Larhei added the t/bug Something isn't working label Dec 13, 2022
@jsuarezruiz jsuarezruiz added legacy-area-perf Startup / Runtime performance platform/iOS 🍎 labels Dec 13, 2022
@mattleibow mattleibow added this to the Backlog milestone Dec 13, 2022
@ghost
Copy link

ghost commented Dec 13, 2022

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@Vroomer
Copy link

Vroomer commented Dec 14, 2022

I suppose that this is the same problem I raised as an issue in #10578 and #12039.

I think that actual pages do get disposed as I tried to use weak reference for the pushed page in one of the sample and the reference is eventually broken, which probably means that the page was disposed. I'm also unable to see instances of page or view model in VS memory profiler. What I see is a lot of UI bits left behind - controls, bindings, handlers... to me it appears that those are never properly disposed. The problem also seems to be getting progressively worse with more controls on pushed page.

Unfortunately so far the MAUI team doesn't seem to care about this issue, even thought it probably affects all apps in serious manner.

@jfversluis
Copy link
Member

Sorry you feel that we don't care because we definitely do. It seems you are very experienced and knowledgeable in this area. We're more than happy to also take in any and all contributions you might have in this area to expedite things.

Thank you for your detailed reports which is very helpful!

@Larhei
Copy link
Author

Larhei commented Jan 9, 2023

Hi @jfversluis,

I agree with @Vroomer that a lot of UI bits are left behind. The question from my side would be when can we see a memory profiler on mac to better profile our apps and narrow down the problems? PerfView is not on Mac and what described at https://github.com/xamarin/xamarin-macios/wiki/Profiling is good for speed analysis but not for memory analysis. Xcode Profiler does not help me with that, because it does not seem to work with maui projects (on my machine).
Is it just me? If it´s just me, could someone please point me to some valid documentation how to do memory analysis on mac on maui apps?

@samhouts samhouts added the p/0 Work that we can't release without label Jan 27, 2023
@samhouts samhouts modified the milestones: Backlog, .NET 8 Planning Jan 27, 2023
@jonathanpeppers jonathanpeppers self-assigned this Feb 16, 2023
@jonathanpeppers
Copy link
Member

Ok, some good news. I think the following help this issue quite a bit:

I tested a local build of MAUI with #14108 and it appears all 4 scenarios in the above sample work on Windows & Android.

However, it appears that two of the scenarios still have an issue on iOS (and maybe also Catalyst).

I also made some changes to the sample:

  • BackButtonBehaviour was missing the logging
  • To test iOS, I used Console.WriteLine() instead of Debug.WriteLine()

@hartez hartez changed the title Possible Memoryleaks? Possible Memory leaks? Mar 22, 2023
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue Mar 29, 2023
@jonathanpeppers
[ios] fix memory leak in Button
08be7ab 
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.
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue Mar 29, 2023
@jonathanpeppers
[ios] fix memory leak in Button
986757a 
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.
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue Mar 29, 2023
@jonathanpeppers
[ios] fix memory leak in Button
f85d2b4 
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.
This was referenced Mar 29, 2023
Merged
Closed
PureWeen pushed a commit that referenced this issue Apr 4, 2023
@jonathanpeppers
[ios] fix memory leak in Button (#14280)
58e05d0 
Fixes: #12048
Context: https://github.com/jonathanpeppers/MemoryLeaksOniOS
Context: https://stackoverflow.com/a/13059140

Issue #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.
@ghost ghost locked as resolved and limited conversation to collaborators May 5, 2023
@samhouts samhouts added the fixed-in-8.0.0-preview.4.8333 Look for this fix in 8.0.0-preview.4.8333! label Jun 8, 2023
@Eilon Eilon added the t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) label May 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@jonathanpeppers jonathanpeppers

Labels
fixed-in-8.0.0-preview.4.8333 Look for this fix in 8.0.0-preview.4.8333! legacy-area-perf Startup / Runtime performance p/0 Work that we can't release without platform/iOS 🍎 t/bug Something isn't working t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.)
Projects
None yet
Milestone
.NET 8 Planning
Development

Successfully merging a pull request may close this issue.

[ios] fix memory leak in Button jonathanpeppers/maui
8 participants
@Eilon @jonathanpeppers @jfversluis @mattleibow @jsuarezruiz @samhouts @Vroomer @Larhei