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

[net7.0 / net8.0] [iOS] Memory leak when CollectionView.ItemsSource changes #14664

Closed
4 tasks done
Sergtek opened this issue Apr 19, 2023 · 10 comments
Closed
4 tasks done
Assignees
Labels
area-controls-collectionview CollectionView, CarouselView, IndicatorView memory-leak 💦 Memory usage grows / objects live forever (sub: perf) p/1 Work that is important, and has been scheduled for release in this or an upcoming sprint partner/cat 😻 this is an issue that impacts one of our partners or a customer our advisory team is engaged with platform/iOS 🍎 s/triaged Issue has been reviewed s/try-latest-version Please try to reproduce the potential issue on the latest public version t/bug Something isn't working t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf)
Milestone

Comments

@Sergtek
Copy link

Sergtek commented Apr 19, 2023

Description

I am aware of the existence of the following bug #13530 but only mention is made of Windows, however in iOS the same memory leak problem also occurs when updating the ItemsSource of a CollectionView several times and the RAM memory is not released even by browsing backwards:

issue1

I leave a video playing an example repository where I can reproduce this problem in iOS until the application crashes due to lack of memory:
https://www.youtube.com/watch?v=_UTEGbE4-ug

I leave the repository that I show in the video so you can do the test:
https://github.com/nacompllo/MemoryLeakEverywhere/tree/bugfix/memoryLeakItemsSource

Steps to Reproduce

Link to public reproduction project repository

https://github.com/nacompllo/MemoryLeakEverywhere/tree/bugfix/memoryLeakItemsSource

Version with bug

7.0 (current)

Last version that worked well

Unknown/Other

Affected platforms

iOS

Affected platform versions

iOS 16

Did you find any workaround?

No response

Relevant log output

No response

Depends on

@Sergtek Sergtek added the t/bug Something isn't working label Apr 19, 2023
@drasticactions
Copy link
Contributor

How is this different from #14654, which is not closed?

@Sergtek
Copy link
Author

Sergtek commented Apr 19, 2023

How is this different from #14654, which is not closed?

The #14654 is about a memory leak when repeatedly navigating between pages and #14664 is a memory leak when the ItemsSource from CollectionView changes.

@Eilon Eilon added legacy-area-perf Startup / Runtime performance area-controls-collectionview CollectionView, CarouselView, IndicatorView labels Apr 19, 2023
@samhouts samhouts added the partner/cat 😻 this is an issue that impacts one of our partners or a customer our advisory team is engaged with label May 9, 2023
@jonathanpeppers
Copy link
Member

@Nacompllo and you were testing this on .NET 7?

It might be fixed by this one, which is only in main/.NET 8 currently:

I will test your above sample, though, thanks.

@jonathanpeppers
Copy link
Member

Note that my comment here:

image

Is specifically talking about a list of "logical children" that grew indefinitely on Windows.

@jonathanpeppers jonathanpeppers self-assigned this May 9, 2023
@jonathanpeppers
Copy link
Member

There appears to be an issue with images on iOS, a failing test:

[Fact("Image Does Not Leak")]
public async Task DoesNotLeak()
{
	SetupBuilder();
	WeakReference reference = null;

	await InvokeOnMainThreadAsync(async () =>
	{
		var layout = new VerticalStackLayout();
		var image = new Image
		{
			Background = Colors.Black,
			Source = "red.png",
		};
		layout.Add(image);

		var handler = CreateHandler<LayoutHandler>(layout);
		await image.Wait();
		reference = new WeakReference(image.Handler.PlatformView);
	});

	await Task.Yield();
	GC.Collect();
	GC.WaitForPendingFinalizers();
	
	Assert.NotNull(reference);
	Assert.False(reference.IsAlive, "PlatformView should not be alive!");
}

Still investigating what the fix would be.

@samhouts samhouts added the p/1 Work that is important, and has been scheduled for release in this or an upcoming sprint label May 10, 2023
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue May 12, 2023
Context: dotnet#14664 (comment)

`Image` has two different "cycles" on iOS:

* `ImageHandler` -> `MauiImageView` -> `ImageHandler`

  * via the `WindowChanged` event

* `ImageHandler` -> `ImageSourcePartLoader` -> `ImageHandler`

    * via `Handler`, `Func<IImageSourcePart?>`, and `Action<PlatformImage?>`

This causes any `MauiImageView` to live forever.

To solve these issues:

* Get rid of the `MauiImageView.WindowChanged` event, and use a
`WeakReference` to the handler instead.

* `ImageSourcePartLoader` now only has a `WeakReference` to the handler.

* This requires a new `ISetImageHandler` interface to be used by
several handler types involving images.

Hopefully the changes here are using `[Obsolete]` correctly to make
things backwards compatible & less breaking changes.

Unsure yet if this fully solves dotnet#14664, but at least one part of it.
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue May 12, 2023
Context: dotnet#14664 (comment)

`Image` has two different "cycles" on iOS:

* `ImageHandler` -> `MauiImageView` -> `ImageHandler`

  * via the `WindowChanged` event

* `ImageHandler` -> `ImageSourcePartLoader` -> `ImageHandler`

    * via `Handler`, `Func<IImageSourcePart?>`, and `Action<PlatformImage?>`

This causes any `MauiImageView` to live forever.

To solve these issues:

* Get rid of the `MauiImageView.WindowChanged` event, and use a
`WeakReference` to the handler instead.

* `ImageSourcePartLoader` now only has a `WeakReference` to the handler.

* This requires a new `ISetImageHandler` interface to be used by
several handler types involving images.

Hopefully the changes here are using `[Obsolete]` correctly to make
things backwards compatible & less breaking changes.

Unsure yet if this fully solves dotnet#14664, but at least one part of it.
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue May 15, 2023
Context: dotnet#14664 (comment)

`Image` has two different "cycles" on iOS:

* `ImageHandler` -> `MauiImageView` -> `ImageHandler`

  * via the `WindowChanged` event

* `ImageHandler` -> `ImageSourcePartLoader` -> `ImageHandler`

    * via `Handler`, `Func<IImageSourcePart?>`, and `Action<PlatformImage?>`

This causes any `MauiImageView` to live forever.

To solve these issues:

* Get rid of the `MauiImageView.WindowChanged` event, and use a
`WeakReference` to the handler instead.

* `ImageSourcePartLoader` now only has a `WeakReference` to the handler.

* This requires a new `ISetImageHandler` interface to be used by
several handler types involving images.

Hopefully the changes here are using `[Obsolete]` correctly to make
things backwards compatible & less breaking changes.

Unsure yet if this fully solves dotnet#14664, but at least one part of it.
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue May 19, 2023
Context: https://github.com/jonathanpeppers/MemoryLeaksOniOS
Related to: dotnet#14664

There are a couple cycles on iOS that causes memory leaks in all
controls based on `ContentView`:

* `Microsoft.Maui.Controls.ContentView` ->
    * `ContentViewHandler` ->
    * `Microsoft.Maui.Platform` ->
    * `CrossPlatformArrange/Measure` property ->
* `Microsoft.Maui.Controls.ContentView` cycle

To fix this, I made `CrossPlatformArrange/Measure` properties obsolete,
using the weak `View` property directly instead. This applies to various
other controls like `Border`, `RadioButton`, `SwipeItemView`, `SwipeView`.

I did not yet fix `ScrollView` that appears to be more complicated to
solve. For now, it supports both code paths.

`ScrollView` will continue to have leaks in:

* Closures with captured variables
    * https://github.com/dotnet/maui/blob/3b620952425ca3c4542c4632e05d09eb4f583e22/src/Core/src/Handlers/ScrollView/ScrollViewHandler.iOS.cs#L195
    * https://github.com/dotnet/maui/blob/3b620952425ca3c4542c4632e05d09eb4f583e22/src/Core/src/Handlers/ScrollView/ScrollViewHandler.iOS.cs#L227

* `CrossPlatformArrange/Measure`
    * https://github.com/dotnet/maui/blob/3b620952425ca3c4542c4632e05d09eb4f583e22/src/Core/src/Handlers/ScrollView/ScrollViewHandler.iOS.cs#L182-L186
@samhouts samhouts added this to the .NET 8 milestone May 25, 2023
github-actions bot pushed a commit to jonathanpeppers/maui that referenced this issue May 28, 2023
Context: https://github.com/jonathanpeppers/MemoryLeaksOniOS
Related to: dotnet#14664

There are a couple cycles on iOS that causes memory leaks in all
controls based on `ContentView`:

* `Microsoft.Maui.Controls.ContentView` ->
    * `ContentViewHandler` ->
    * `Microsoft.Maui.Platform` ->
    * `CrossPlatformArrange/Measure` property ->
* `Microsoft.Maui.Controls.ContentView` cycle

To fix this, I made `CrossPlatformArrange/Measure` properties obsolete,
using the weak `View` property directly instead. This applies to various
other controls like `Border`, `RadioButton`, `SwipeItemView`, `SwipeView`.

I did not yet fix `ScrollView` that appears to be more complicated to
solve. For now, it supports both code paths.

`ScrollView` will continue to have leaks in:

* Closures with captured variables
    * https://github.com/dotnet/maui/blob/3b620952425ca3c4542c4632e05d09eb4f583e22/src/Core/src/Handlers/ScrollView/ScrollViewHandler.iOS.cs#L195
    * https://github.com/dotnet/maui/blob/3b620952425ca3c4542c4632e05d09eb4f583e22/src/Core/src/Handlers/ScrollView/ScrollViewHandler.iOS.cs#L227

* `CrossPlatformArrange/Measure`
    * https://github.com/dotnet/maui/blob/3b620952425ca3c4542c4632e05d09eb4f583e22/src/Core/src/Handlers/ScrollView/ScrollViewHandler.iOS.cs#L182-L186
@QuiqueLargachaGil
Copy link

Hi everyone,

I can see that the issue is still in an Open status, but I am writing to provide some feedback just in case it helps.
I have forked the repository indicated by @Nacompllo and I have tested the same application compiling with the version released a few days ago of .Net 8 preview 4. The changes to make the compilation are isolated in two independent branches (one for #14654 and another for #14664). Here is a link to the repository:

https://github.com/QuiqueLargachaGil/MAUI-POC.MemoryLeakEverywhere/tree/bugfix/updated-to-Net-8-preview-4-in-memoryLeakItemsSource-branch

And the result for the case of this incident is the same. There are still memory leaks when reproducing the issue. I share with you a youtube link to watch the video that shows it.

https://www.youtube.com/watch?v=EAxcUQE6wLE

Many thanks!

@jonathanpeppers
Copy link
Member

Yes, this is not fixed until we get these merged:

But then we'll need to retest the above sample app, to see if there are further issues.

jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue May 30, 2023
Context: https://github.com/jonathanpeppers/MemoryLeaksOniOS
Related to: dotnet#14664

There are a couple cycles on iOS that causes memory leaks in all
controls based on `ContentView`:

* `Microsoft.Maui.Controls.ContentView` ->
    * `ContentViewHandler` ->
    * `Microsoft.Maui.Platform` ->
    * `CrossPlatformArrange/Measure` property ->
* `Microsoft.Maui.Controls.ContentView` cycle

To fix this, I made `CrossPlatformArrange/Measure` properties obsolete,
using the weak `View` property directly instead. This applies to various
other controls like `Border`, `RadioButton`, `SwipeItemView`, `SwipeView`.

I did not yet fix `ScrollView` that appears to be more complicated to
solve. For now, it supports both code paths.

`ScrollView` will continue to have leaks in:

* Closures with captured variables
    * https://github.com/dotnet/maui/blob/3b620952425ca3c4542c4632e05d09eb4f583e22/src/Core/src/Handlers/ScrollView/ScrollViewHandler.iOS.cs#L195
    * https://github.com/dotnet/maui/blob/3b620952425ca3c4542c4632e05d09eb4f583e22/src/Core/src/Handlers/ScrollView/ScrollViewHandler.iOS.cs#L227

* `CrossPlatformArrange/Measure`
    * https://github.com/dotnet/maui/blob/3b620952425ca3c4542c4632e05d09eb4f583e22/src/Core/src/Handlers/ScrollView/ScrollViewHandler.iOS.cs#L182-L186
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue May 30, 2023
Context: dotnet#14664 (comment)

`Image` has two different "cycles" on iOS:

* `ImageHandler` -> `MauiImageView` -> `ImageHandler`

  * via the `WindowChanged` event

* `ImageHandler` -> `ImageSourcePartLoader` -> `ImageHandler`

    * via `Handler`, `Func<IImageSourcePart?>`, and `Action<PlatformImage?>`

This causes any `MauiImageView` to live forever.

To solve these issues:

* Get rid of the `MauiImageView.WindowChanged` event, and use a
`WeakReference` to the handler instead.

* `ImageSourcePartLoader` now only has a `WeakReference` to the handler.

* This requires a new `ISetImageHandler` interface to be used by
several handler types involving images.

Hopefully the changes here are using `[Obsolete]` correctly to make
things backwards compatible & less breaking changes.

Unsure yet if this fully solves dotnet#14664, but at least one part of it.
github-actions bot pushed a commit to jonathanpeppers/maui that referenced this issue May 30, 2023
Context: https://github.com/jonathanpeppers/MemoryLeaksOniOS
Related to: dotnet#14664

There are a couple cycles on iOS that causes memory leaks in all
controls based on `ContentView`:

* `Microsoft.Maui.Controls.ContentView` ->
    * `ContentViewHandler` ->
    * `Microsoft.Maui.Platform` ->
    * `CrossPlatformArrange/Measure` property ->
* `Microsoft.Maui.Controls.ContentView` cycle

To fix this, I made `CrossPlatformArrange/Measure` properties obsolete,
using the weak `View` property directly instead. This applies to various
other controls like `Border`, `RadioButton`, `SwipeItemView`, `SwipeView`.

I did not yet fix `ScrollView` that appears to be more complicated to
solve. For now, it supports both code paths.

`ScrollView` will continue to have leaks in:

* Closures with captured variables
    * https://github.com/dotnet/maui/blob/3b620952425ca3c4542c4632e05d09eb4f583e22/src/Core/src/Handlers/ScrollView/ScrollViewHandler.iOS.cs#L195
    * https://github.com/dotnet/maui/blob/3b620952425ca3c4542c4632e05d09eb4f583e22/src/Core/src/Handlers/ScrollView/ScrollViewHandler.iOS.cs#L227

* `CrossPlatformArrange/Measure`
    * https://github.com/dotnet/maui/blob/3b620952425ca3c4542c4632e05d09eb4f583e22/src/Core/src/Handlers/ScrollView/ScrollViewHandler.iOS.cs#L182-L186
github-actions bot pushed a commit to jonathanpeppers/maui that referenced this issue May 30, 2023
Context: dotnet#14664 (comment)

`Image` has two different "cycles" on iOS:

* `ImageHandler` -> `MauiImageView` -> `ImageHandler`

  * via the `WindowChanged` event

* `ImageHandler` -> `ImageSourcePartLoader` -> `ImageHandler`

    * via `Handler`, `Func<IImageSourcePart?>`, and `Action<PlatformImage?>`

This causes any `MauiImageView` to live forever.

To solve these issues:

* Get rid of the `MauiImageView.WindowChanged` event, and use a
`WeakReference` to the handler instead.

* `ImageSourcePartLoader` now only has a `WeakReference` to the handler.

* This requires a new `ISetImageHandler` interface to be used by
several handler types involving images.

Hopefully the changes here are using `[Obsolete]` correctly to make
things backwards compatible & less breaking changes.

Unsure yet if this fully solves dotnet#14664, but at least one part of it.
rmarinho pushed a commit that referenced this issue Jun 14, 2023
* [ios] fix memory leak in Image

Context: #14664 (comment)

`Image` has two different "cycles" on iOS:

* `ImageHandler` -> `MauiImageView` -> `ImageHandler`

  * via the `WindowChanged` event

* `ImageHandler` -> `ImageSourcePartLoader` -> `ImageHandler`

    * via `Handler`, `Func<IImageSourcePart?>`, and `Action<PlatformImage?>`

This causes any `MauiImageView` to live forever.

To solve these issues:

* Get rid of the `MauiImageView.WindowChanged` event, and use a
`WeakReference` to the handler instead.

* `ImageSourcePartLoader` now only has a `WeakReference` to the handler.

* This requires a new `ISetImageHandler` interface to be used by
several handler types involving images.

Hopefully the changes here are using `[Obsolete]` correctly to make
things backwards compatible & less breaking changes.

Unsure yet if this fully solves #14664, but at least one part of it.

* Update src/Core/src/Platform/ImageSourcePartLoader.cs

Co-authored-by: Matthew Leibowitz <mattleibow@live.com>

* Update MauiImageView.cs

* Update MauiImageView.cs

* ISetImageHandler -> IImageSourcePartSetter

* IImageHandler.OnWindowChanged() is now public

* Fix IImageSourcePartSetter namespace

* MauiImageView._handler can be readonly

---------

Co-authored-by: Matthew Leibowitz <mattleibow@live.com>
StephaneDelcroix pushed a commit that referenced this issue Jun 19, 2023
* [ios] fix memory leak in Image

Context: #14664 (comment)

`Image` has two different "cycles" on iOS:

* `ImageHandler` -> `MauiImageView` -> `ImageHandler`

  * via the `WindowChanged` event

* `ImageHandler` -> `ImageSourcePartLoader` -> `ImageHandler`

    * via `Handler`, `Func<IImageSourcePart?>`, and `Action<PlatformImage?>`

This causes any `MauiImageView` to live forever.

To solve these issues:

* Get rid of the `MauiImageView.WindowChanged` event, and use a
`WeakReference` to the handler instead.

* `ImageSourcePartLoader` now only has a `WeakReference` to the handler.

* This requires a new `ISetImageHandler` interface to be used by
several handler types involving images.

Hopefully the changes here are using `[Obsolete]` correctly to make
things backwards compatible & less breaking changes.

Unsure yet if this fully solves #14664, but at least one part of it.

* Update src/Core/src/Platform/ImageSourcePartLoader.cs

Co-authored-by: Matthew Leibowitz <mattleibow@live.com>

* Update MauiImageView.cs

* Update MauiImageView.cs

* ISetImageHandler -> IImageSourcePartSetter

* IImageHandler.OnWindowChanged() is now public

* Fix IImageSourcePartSetter namespace

* MauiImageView._handler can be readonly

---------

Co-authored-by: Matthew Leibowitz <mattleibow@live.com>
PureWeen pushed a commit that referenced this issue Jun 30, 2023
Context: #14664
Context: https://github.com/nacompllo/MemoryLeakEverywhere/tree/bugfix/memoryLeakItemsSource

While looking at the customer sample, we found an issue with
`BorderHandler`:

* `ContentView` -> via `LayoutSubviewsChanged`
* `BorderHandler` ->
* `ContentView`

Creating a cycle & memory leak on iOS and Catalyst. We could reproduce
this in a device test.

It appears the event only did this:

    void OnLayoutSubviewsChanged(object? sender, EventArgs e)
    {
        PlatformView?.UpdateMauiCALayer();
    }

And so instead, we can just call the extension method directly:

    this.UpdateMauiCALayer();

And the leak is gone!

Co-authored-by: Haritha Mohan <harithamohan@microsoft.com>
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue Jul 6, 2023
I added some logging for the sample:

* dotnet#14664
* https://github.com/nacompllo/MemoryLeakEverywhere/tree/bugfix/memoryLeakItemsSource

I found the "logical children" grow indefinitely on iOS/Catalyst, after
adding some logging it quickly got up to 71 items:

    2023-07-06 09:04:27.745 MemoryLeakEverywhere[93127:7440192] Microsoft.Maui.Controls.CollectionView added child -> Microsoft.Maui.Controls.Image, count: 71

I was able to reproduce in a test, after I added some code to allow the
`CollectionView` to create items (using `WidthRequest`/`HeightRequest`).

When you replace an `ItemsSource` on iOS, in this way:

    var newCollection = new ObservableCollection<string>();
    collectionView.ItemsSource = newCollection;
    foreach (var item in data)
    {
        newCollection.Add(item);
    }

It appears that the old items are not removed, the test fails with:

    Expected: 3
    Actual: 6

But passes on Windows & Android.

Somewhere, we need to call one of:

* `ItemsView.ClearLogicalChildren()`
* `ItemsView.RemoveLogicalChild()`

But I've not been successful yet.
@jonathanpeppers
Copy link
Member

The list of linked PRs above address various memory issues found the in above sample. However, the memory still appears to grow in the sample with dotnet/maui/main at this time.

Latest Issue

I found the "logical children" grow indefinitely on iOS/Catalyst, after adding some logging the sample above quickly got up to 71 items:

2023-07-06 09:04:27.745 MemoryLeakEverywhere[93127:7440192] Microsoft.Maui.Controls.CollectionView added child -> Microsoft.Maui.Controls.Image, count: 71

I can illustrate the issue in this test:

jonathanpeppers@bbea059

It passes on Windows & Android, but on iOS & Catalyst the list of logical children grows.

It appears to be due to this pattern:

var newCollection = new ObservableCollection<string>();
collectionView.ItemsSource = newCollection;
foreach (var item in data)
{
    newCollection.Add(item);
}

If I change it to set ItemsSource last, it also works around this issue:

var newCollection = new ObservableCollection<string>();
foreach (var item in data)
{
    newCollection.Add(item);
}
collectionView.ItemsSource = newCollection;

This version is faster anyway, but the issue seems to be related to the CollectionChanged event firing.

Workarounds for Above Sample

@samhouts samhouts modified the milestones: .NET 8, .NET 8 GA Jul 12, 2023
@samhouts samhouts added the memory-leak 💦 Memory usage grows / objects live forever (sub: perf) label Aug 1, 2023
@homeyf homeyf added s/triaged Issue has been reviewed s/try-latest-version Please try to reproduce the potential issue on the latest public version labels Jan 4, 2024
@ghost
Copy link

ghost commented Jan 4, 2024

Hi @Nacompllo. We have added the "s/try-latest-version" label to this issue, which indicates that we'd like you to try and reproduce this issue on the latest available public version. This can happen because we think that this issue was fixed in a version that has just been released, or the information provided by you indicates that you might be working with an older version.

You can install the latest version by installing the latest Visual Studio (Preview) with the .NET MAUI workload installed. If the issue still persists, please let us know with any additional details and ideally a reproduction project provided through a GitHub repository.

This issue will be closed automatically in 7 days if we do not hear back from you by then - please feel free to re-open it if you come back to this issue after that time.

@homeyf
Copy link

homeyf commented Jan 4, 2024

This is a Memory leak issue, we tried to reproduce it with a user-provided project, and it took over an hour to run. But the memory
consumption is still less than 1GB, so it is marked as not reproduce.

@ghost ghost closed this as completed Jan 11, 2024
@github-project-automation github-project-automation bot moved this from Todo to Done in MAUI SDK Ongoing Jan 11, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Mar 3, 2024
@Eilon Eilon added t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf) and removed legacy-area-perf Startup / Runtime performance labels May 10, 2024
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-controls-collectionview CollectionView, CarouselView, IndicatorView memory-leak 💦 Memory usage grows / objects live forever (sub: perf) p/1 Work that is important, and has been scheduled for release in this or an upcoming sprint partner/cat 😻 this is an issue that impacts one of our partners or a customer our advisory team is engaged with platform/iOS 🍎 s/triaged Issue has been reviewed s/try-latest-version Please try to reproduce the potential issue on the latest public version t/bug Something isn't working t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf)
Projects
Status: Done
Development

No branches or pull requests

7 participants