Xamarin.forms: [Bug] UWP - ListView does not recycle cells [HUUGE MEMORY ISSUE]

[gentledepp]

Description

On UWP, the viewcells of a listview are not recycled. Rather, every time the datacontext changes, a new cell is created from the datatemplate!

Steps to Reproduce

1. Create a page with a Xamarin.ListView and set the itemssource to 200 items
2. Create a custom ViewCell (so that you have the xaml and the code behind)
3. Set the HeightRequest of the viewcell to 80 or more
4. Set a breakpoint in the ctor of your ViewCell
5. Scroll all the way up and all the way down

Expected Behavior

Constructor of the custom ViewCell should never be called during scrolling

Actual Behavior

Constructor is called all the time...

Basic Information

• Version with issue: 4.4.0 and all above
• Last known good version: no idea...
• IDE: VS 2019
• Platform Target Frameworks:
  
  
  
  • UWP: irrelevant
  
  
• Android Support Library Version:
• Nuget Packages: irrelevant
• Affected Devices: all UWP devices

Screenshots

Reproduction Link

Workaround

Solution

We already found the issue for this.
The culprit is the Xamarin.Forms.Platform.UAP.CellControl.SetCell(..) method:

The error is marked with a ⚠

void SetCell(object newContext)
{
    var cell = newContext as Cell;

    if (cell != null)
    {
        Cell = cell;
        return;
    }

    if (ReferenceEquals(Cell?.BindingContext, newContext))
        return;

    // If there is a ListView, load the Cell content from the ItemTemplate.
    // Otherwise, the given Cell is already a templated Cell from a TableView.
    ListView lv = _listView.Value;
    if (lv != null)
    {
        bool isGroupHeader = IsGroupHeader;
        DataTemplate template = isGroupHeader ? lv.GroupHeaderTemplate : lv.ItemTemplate;
        object bindingContext = newContext;

        if (template is DataTemplateSelector)
        {
            template = ((DataTemplateSelector)template).SelectTemplate(bindingContext, lv);
        }
                        // ⚠ ERROR: The code should now check if there already exists a Cell that
                        // could be recycled!
                        // Instead, however, it just creates a new cell using the template or the defaultcell (in case it is a groupheader)
        if (template != null)
        {
            cell = template.CreateContent() as Cell;
        }
        else
        {
            if (isGroupHeader)
                bindingContext = lv.GetDisplayTextFromGroup(bindingContext);

            cell = lv.CreateDefaultCell(bindingContext);
        }

        // A TableView cell should already have its parent,
        // but we need to set the parent for a ListView cell.
        cell.Parent = lv;

        // Set inherited BindingContext after setting the Parent so it won't be wiped out
        BindableObject.SetInheritedBindingContext(cell, bindingContext);

        // This provides the Group Header styling (e.g., larger font, etc.) when the
        // template is loaded later.
        cell.SetIsGroupHeader<ItemsView<Cell>, Cell>(isGroupHeader);
    }

    Cell = cell;
}

Now we patched this ourselves like the following:
(We marked our additions with rockeds: 🚀)

void SetCell(object newContext)
{
    var cell = newContext as Cell;

    if (cell != null)
    {
        Cell = cell;
        return;
    }

    if (ReferenceEquals(Cell?.BindingContext, newContext))
        return;

    // If there is a ListView, load the Cell content from the ItemTemplate.
    // Otherwise, the given Cell is already a templated Cell from a TableView.
    ListView lv = _listView.Value;
    if (lv != null)
    {
        // 🚀 If there is an old cell, check if it was a group header
        // we need this later to know whether we can recycle this cell
        bool? wasGroupHeader = null;
        var oldCell = Cell;
        if (oldCell != null)
        {
            wasGroupHeader = oldCell.GetIsGroupHeader<ItemsView<Cell>, Cell>();
        }

        bool isGroupHeader = IsGroupHeader;
        DataTemplate template = isGroupHeader ? lv.GroupHeaderTemplate : lv.ItemTemplate;
        object bindingContext = newContext;

        bool sameTemplate = false;
        if (template is DataTemplateSelector dataTemplateSelector)
        {
            template = dataTemplateSelector.SelectTemplate(bindingContext, lv);

             // 🚀 If there exists an old cell, get its data template and check
                // whether the new- and old template matches. In that case, we can recycle it
                if (oldCell?.BindingContext != null)
            {
                DataTemplate oldTemplate = dataTemplateSelector.SelectTemplate(oldCell?.BindingContext, lv);
                sameTemplate = oldTemplate == template;
            }
        }
        // 🚀 if there is no datatemplateselector, we now verify if the old cell
        // was a groupheader and whether the new one is as well.
        // Again, this is only to verify we can reuse this cell
        else if(wasGroupHeader.HasValue)
        {
            sameTemplate = wasGroupHeader == isGroupHeader;
        }

        // reuse cell
        var canReuseCell = Cell != null && sameTemplate;

        // 🚀 If we can reuse the cell, just reuse it...
        if (canReuseCell)
        {
            cell = Cell;
        }
        else if (template != null)
        {
            cell = template.CreateContent() as Cell;
        }
        else
        {
            if (isGroupHeader)
                bindingContext = lv.GetDisplayTextFromGroup(bindingContext);

            cell = lv.CreateDefaultCell(bindingContext);
        }

        // A TableView cell should already have its parent,
        // but we need to set the parent for a ListView cell.
        cell.Parent = lv;

        // Set inherited BindingContext after setting the Parent so it won't be wiped out
        BindableObject.SetInheritedBindingContext(cell, bindingContext);

        // This provides the Group Header styling (e.g., larger font, etc.) when the
        // template is loaded later.
        cell.SetIsGroupHeader<ItemsView<Cell>, Cell>(isGroupHeader);
    }

        // 🚀 Only set the cell if it DID change
    // Note: The cleanup (SendDisappearing(), etc.) is done by the Cell propertychanged callback so we do not need to do any cleanup ourselves.

    if(Cell != cell)
        Cell = cell;
    // 🚀 even if the cell did not change, we **must** call SendDisappearing() and SendAppearing()
    // because frameworks such as Reactive UI rely on this! (this.WhenActivated())
    else if(Cell != null)
    {
        Cell.SendDisappearing();
        Cell.SendAppearing();
    }

This works like a charm.
I do not know where to target our PR though as there are so many versions affected.
Maybe you just copy paste the fix yourself in all platforms that apply?

@bruzkovsky @BrayanKhosravian - fyi ;-)

Update:

It seems that the CellControl has another memory leak:

The Cell.PropertyChanged event is not unsubscribed when the cell is unloaded

        public CellControl()
        {
            _listView = new Lazy<ListView>(GetListView);

            DataContextChanged += OnDataContextChanged;

            Unloaded += (sender, args) =>
            {
                Cell?.SendDisappearing();
            };

            _propertyChangedHandler = OnCellPropertyChanged;
        }

We'd fix it like this:

        public CellControl()
        {
            _listView = new Lazy<ListView>(GetListView);

            DataContextChanged += OnDataContextChanged;

            Loaded += OnLoaded;
            Unloaded += OnUnloaded;

            _propertyChangedHandler = OnCellPropertyChanged;
        }

        void OnLoaded(object sender, RoutedEventArgs e)
        {
            if (Cell == null)
                return;

                        /// 🚀 subscribe topropertychanged
            // make sure we do not subscribe twice (because this could happen in SetSource(Cell oldCell, Cell newCell))
            Cell.PropertyChanged -= _propertyChangedHandler;
            Cell.PropertyChanged += _propertyChangedHandler;
        }

        void OnUnloaded(object sender, RoutedEventArgs e)
        {
            if (Cell == null)
                return;

            Cell.SendDisappearing();
                        /// 🚀 unsubscribe from propertychanged
            Cell.PropertyChanged -= _propertyChangedHandler;
        }

.....


        void SetSource(Cell oldCell, Cell newCell)
        {
            if (oldCell != null)
            {
                oldCell.PropertyChanged -= _propertyChangedHandler;
                oldCell.SendDisappearing();
            }

            if (newCell != null)
            {
                newCell.SendAppearing();

                UpdateContent(newCell);
                UpdateFlowDirection(newCell);
                SetupContextMenu();

                // 🚀 make sure we do not subscribe twice (OnLoaded!)
                newCell.PropertyChanged -= _propertyChangedHandler;
                newCell.PropertyChanged += _propertyChangedHandler;
            }
        }

[gentledepp]

Update: We have a fairly complex app which uses listview to the max:
Lazy loading, many data templates, sometimes datatemplateselectors and also groupings are used.
I now tested this thorougly and it works perfectly (although I had to fix my commented code 2 times 😁)

[nbevans]

Finally somebody has found the root cause of UWP's ListView memory leaks! You absolute legend.

Very much looking forward to these getting merged into master.

Question: what tool is that in your screenshot which you are using for memory analysis?

[gentledepp]

Well... don't be too happy. There is still some serious memory leaks, but we are currently not sure if they are within Xamarin.Forms for UWP or our own app

We are using Jetbrains dotMemory. Since the 2020 release it is really a great tool - even shows you the instantiation path of some object instance.

If you don't know Jetbrains - they are the brain behind the mother of tools: Resharper

[Pandoraman92]

Is there any updates about that topic? I think gentledepp provided a solution for at least one of the memory leaks of the listview. We in our company still use that listview on a fairly large installation base. So we would appreciate that this fix would be merged, sine this solution is pending since end of April!

[curtisconner]

Any update, seems like this was removed from 5.0.0. any idea when this will be fixed or are you providing another fix for this?