[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?

[gentledepp]

I do not understand how such an important fix is pushed back further and further...
in 4.8.x this critical bug is still there.
We gave up on waiting for fixes a long time ago and are maintaining our own Xamarin.Forms fork.
I strongly encourage you to do so as

• known bugs tend to not be fixed no matter how critical
• new versions of XF tend to introduce and reintroduce bugs all the time

This really is a maintenance nightmare, but thankfully it is open source so you can do something about it.

[CliffAgius]

If there is a fix that @gentledepp has implemented that looks like it works and merged into their private fork of XF any chance one of the team @jsuarezruiz @samhouts @rmarinho etc can look at merging the same fix and resolve this MASSIVE memory issue that plagues the use of CollectionViews on the UWP platform....

[rmarinho]

Hi @CliffAgius i will ping @hartez he knows very well inner works of CollectionView.
Is it possible @gentledepp to make a PR with all the changes he implemented so it easier for us to look and check if makes sense and fixes the issues?

Thanks

[CliffAgius]

@rmarinho if you scroll up slightly you can see they merged a PR into thier own fork but have not done a PR into the XF Repo.

If needed I'm happy to create the PR obviously attributing it to @gentledepp if it will speed things up.

[CliffAgius]

@rmarinho I have created a PR from @gentledepp work and having tested here it seems to work really well. Not sure if this will make it into XF5 or if it's going to get pushed in MAUI but a project I am working on really needs this so hopefully the former.

I know there are other Issues based on the same fault that this could cross off as well.

Let me know if you need anything else from me to push this along.