[Windows] GroupedItemTemplateCollection CollectionChanged event not unsubscribed, causes memory leak.

[jari-schuurman]

Description

CollectionView & ObservableCollections

If you use CollectionView with IsGrouped=true where your ItemsSource is of type ObservableCollection; congratulations you just introduced a memory leak in your application! Don't worry: the fix is easy! If it ever gets picked up by the MAUI team ;)
If you use grouping, the internal source for CollectionViewSource will be of type

maui/src/Controls/src/Core/Platform/Windows/CollectionView/GroupedItemTemplateCollection.cs

Line 9 in b182ffe

internal class GroupedItemTemplateCollection : ObservableCollection<GroupTemplateContext>

see:

maui/src/Controls/src/Core/Handlers/Items/GroupableItemsViewHandler.Windows.cs

Line 19 in b182ffe

protected override CollectionViewSource CreateCollectionViewSource()

Inside the GroupedItemTemplateCollection there is a check to see if the itemssource implements INotifyCollectionChanged:

if (_itemsSource is IList groupList && _itemsSource is INotifyCollectionChanged incc)
{
  _groupList = groupList;
  incc.CollectionChanged += GroupsChanged;
}

Maybe you already feel it coming or not... But yes indeed this event is never unsubscribed + the GroupsChanged event is not a static method. Which according to their docs does introduce a leak: https://github.com/dotnet/maui/wiki/Memory-Leaks#when-we-cant-use-weakeventmanager

I have spent many, way too many, hours testing, analyzing MAUI source code, verifying and more testing to confirm this is the root cause of the issue.

Test setup

I have a basic mainpage with a viewmodel. In the viewmodel I open a popup with a CollectionView as the content of the popup.
I have my own sub class of a CollectionView with a destructor added so I can verify if it is cleaned up or not (it is not).

~GroupedCollection()
{
    Debug.WriteLine("CLEANING");
}

Memory Usage/Snapshot & analysis

What I noticed is that the Count of the GroupedItemTemplateCollection is always 7x the Count of the CollectionView.
I guess it has to do with

maui/src/Controls/src/Core/Handlers/Items/GroupableItemsViewHandler.cs

Line 20 in b182ffe

public static PropertyMapper<TItemsView, GroupableItemsViewHandler<TItemsView>> GroupableItemsViewMapper = new(SelectableItemsViewMapper)

where it will trigger the UpdateItemsSource multiple times. In here the CleanUpCollectionViewSource is executed:

maui/src/Controls/src/Core/Handlers/Items/ItemsViewHandler.Windows.cs

Line 156 in b182ffe

protected virtual void CleanUpCollectionViewSource()

My idea is that the GroupableItemsViewHandler should override the CleanUpCollectionViewSource and check if the source is GroupedItemTemplateCollection. Since this handler is also responsible for creating this type of collection. Then do a CleanUp for example:

// inside GroupedItemTemplateCollection
public void CleanUp()
{
    if (_itemsSource is INotifyCollectionChanged incc)
    {
        incc.CollectionChanged -= GroupsChanged;
    }
}

However, since this is not happening at the moment. Every time the UpdateItemsSource is triggered, a new memory leak is introduced.

The solution

Either make the GroupsChanged static or follow https://github.com/dotnet/maui/wiki/Memory-Leaks#when-we-cant-use-weakeventmanager

I hope people can follow my explanation, I know I am not the best with explaining such things. Ill publish a repro repo soon, together with the workaround... well "workaround"

Steps to Reproduce

1. Navigate to a page/open a popup;
2. Create a CollectionView with IsGrouped=true
3. Make sure the ItemsSource binding is set to an ObservableCollection
4. Go back and forth between closing and opening your page
5. Observe the destructor is never called and memory usage is going up
6. Expected: destructor being called and memory usage to be stable

Link to public reproduction project repository

https://github.com/jari-schuurman/GroupFooterTemplateIssue

Version with bug

8.0.3 GA

Is this a regression from previous behavior?

Not sure, did not test other versions

Last version that worked well

Unknown/Other

Affected platforms

Windows

Affected platform versions

windows10.0.19401.0

Did you find any workaround?

In order to fix it temporarily, you will have to do some magic:

1. First create your own control and inherit from CollectionView: GroupedCollection. You do not need to add any custom properties.
2. Add a new Handler for this control that inherit from CollectionViewHandler, e.g.: GroupedCollectionHandler
3. Add the Handler to your maui program: handlers.AddHandler(typeof(GroupedCollection), typeof(GroupedCollectionHandler));
4. magic time
   All those handlers and classes are internal so we cannot easily change them:

public interface ICleanObservableCollection
{
    void Reset();
}

public class CleanObservableCollection<T> : ObservableCollection<T>, ICleanObservableCollection
{
    /// <summary>
    /// Initializes a new instance of CleanObservableCollection that is empty and has default initial capacity.
    /// </summary>
    public CleanObservableCollection()
    {
    }

    /// <summary>
    /// Initializes a new instance of the ObservableCollection class that contains
    /// elements copied from the specified collection and has sufficient capacity
    /// to accommodate the number of elements copied.
    /// </summary>
    /// <param name="collection">The collection whose elements are copied to the new list.</param>
    /// <remarks>
    /// The elements are copied onto the ObservableCollection in the
    /// same order they are read by the enumerator of the collection.
    /// </remarks>
    /// <exception cref="ArgumentNullException"> collection is a null reference </exception>
    public CleanObservableCollection(IEnumerable<T> collection) : base(new List<T>(collection ?? throw new ArgumentNullException(nameof(collection))))
    {
    }

    /// <summary>
    /// Initializes a new instance of the ObservableCollection class
    /// that contains elements copied from the specified list
    /// </summary>
    /// <param name="list">The list whose elements are copied to the new list.</param>
    /// <remarks>
    /// The elements are copied onto the ObservableCollection in the
    /// same order they are read by the enumerator of the list.
    /// </remarks>
    /// <exception cref="ArgumentNullException"> list is a null reference </exception>
    public CleanObservableCollection(List<T> list) : base(new List<T>(list ?? throw new ArgumentNullException(nameof(list))))
    {
    }

    public void Reset()
    {
        // the internal event handler of https://github.com/dotnet/maui/blob/b182ffef2c85a5681686732a81b0f572a86591cc/src/Controls/src/Core/Platform/Windows/CollectionView/GroupedItemTemplateCollection.cs#L56
        foreach (var handler in delegates.Where(h => h.Method.Name.Equals("GroupsChanged")).ToList())
        {
            CollectionChanged -= handler;
        }
    }

    private NotifyCollectionChangedEventHandler collectionChanged;
    private List<NotifyCollectionChangedEventHandler> delegates = new();

    public override event NotifyCollectionChangedEventHandler CollectionChanged
    {
        add
        {
            collectionChanged += value;
            delegates.Add(value);
        }
        remove
        {
            collectionChanged -= value;
            delegates.Remove(value);
        }
    }
}

Yes I know for now the method name check is hardcoded. Could probably make it more clean, but this is just POC.
Basically we keep track of all the handlers being added, then in our CollectionViewHandler we check if the type of the itemssource is of ICleanObservableCollection:

public partial class GroupedCollectionHandler : CollectionViewHandler
{
    protected override void UpdateItemsSource()
    {
        base.UpdateItemsSource();
    }

    protected override void DisconnectHandler(ListViewBase platformView)
    {
        Clean();
        base.DisconnectHandler(platformView);
    }

    protected override void CleanUpCollectionViewSource()
    {
        Clean();
        base.CleanUpCollectionViewSource();
    }

    private FieldInfo fieldInfo = null;

    private void Clean()
    {
        if (CollectionViewSource is not null)
        {
            dynamic observableItemTemplateCollection = CollectionViewSource.Source;
            if (observableItemTemplateCollection is IList list)
            {
                //Get the type of the class
                Type type = observableItemTemplateCollection.GetType();

                // Get the private field information
                fieldInfo ??= type.GetField("_itemsSource", BindingFlags.NonPublic | BindingFlags.Instance);

                if (fieldInfo != null)
                {
                    // Get the value of the private field
                    object fieldValue = fieldInfo.GetValue(observableItemTemplateCollection);
                    if (fieldValue is ICleanObservableCollection collection)
                    {
                        collection.Reset();
                    }
                }
            }
        }
    }
}

I didn't take the time to see if we can make this check more performant, but for now it works :)
6. Use the CleanObservableCollection for your ItemsSource binding and use the custom control created in step 1 instead of the CollectionView
7. NOTE: THIS IS MEANT FOR WINDOWS. IF you do cross platform, you probably want to wrap the addHandler step with the #if WINDOWS #endif tags
8. Confirm destructor is called & memory is stable
9. Please maui team solve this quickly. I do not want to add such code in my production repo, but now I am forced to do so

Relevant log output

No response

[github-actions]

Hi I'm an AI powered bot that finds similar issues based off the issue title.

Please view the issues below to see if they solve your problem, and if the issue describes your problem please consider closing this one and thumbs upping the other issue to help us prioritize it. Thank you!

Open similar issues:

• Memory Leak issues with CollectionView with elements with Behavior and VisualStateGroup (#21216), similarity score: 0.78
• Memory leak when scrolling a CollectionView with IsGrouped=true (#17698), similarity score: 0.78

Closed similar issues:

• Error "NSInternalInconsistencyException Reason: Invalid update: invalid number of sections" in CollectionView on iOS still not closed (#16438), similarity score: 0.79
• [CollectionView] [Windows] memory leaks when ItemsSource has changed (#13393), similarity score: 0.79
• Memory leaks EVERYWHERE (#12039), similarity score: 0.78

Note: You can give me feedback by thumbs upping or thumbs downing this comment.

[PureWeen]

Can you test with the latest nightly build?
https://github.com/dotnet/maui/wiki/Nightly-Builds

[jari-schuurman]

Can you test with the latest nightly build? https://github.com/dotnet/maui/wiki/Nightly-Builds

@PureWeen I have installed the 8.0.60-ci.net8.24309.1 nightly build:

As you can see in the screenshot, it still keeps a lot of references to the GroupedItemTemplateCollection and it is still a factor of 7 compared to my collection (GroupedCollection). I'd say the memory leak is still present in this nightly build.
This is the results after applying my workaround:

[jari-schuurman]

Added repro repo

[Zhanglirong-Winnie]

Verified this issue with Visual Studio 17.11.0 Preview 2.0 (8.0.60 & 8.0.40& 8.0.3). Can repro on Windows platform with sample project.

[jari-schuurman]

Any update on this? This basically makes grouped collections unusable on Windows platform :/

[jari-schuurman]

@PureWeen any estimation when this can be resolved?