Fix memory leak in CollectionView and CarouselView by using instance-based ItemsLayout defaults

[Copilot]

Note

Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!

Description of Change

Fixes a memory leak in CollectionView and CarouselView where static ItemsLayout instances were preventing handlers from being garbage collected. The issue occurred because handlers subscribed to PropertyChanged events on shared static instances like LinearItemsLayout.Vertical and LinearItemsLayout.CarouselDefault.

Memory Leak Analysis

The issue manifested as shown in memory dumps where CollectionViewHandler2 instances were being retained through PropertyChangedEventHandler delegates:

The root cause was:

• InternalItemsLayoutProperty in ItemsView used static LinearItemsLayout.Vertical as the default value
• ItemsLayoutProperty in CarouselView used static LinearItemsLayout.CarouselDefault as the default value
• Type converters (ItemsLayoutTypeConverter and CarouselLayoutTypeConverter) returned static instances
• Handlers subscribed to PropertyChanged events on these static instances via SubscribeToItemsLayoutPropertyChanged
• Static instances live for the application lifetime, preventing subscribed handlers from being collected

Changes Made

1. Created internal factory methods in LinearItemsLayout:

   • CreateVerticalDefault() - creates new vertical LinearItemsLayout
   • CreateHorizontalDefault() - creates new horizontal LinearItemsLayout
   • CreateCarouselVerticalDefault() - creates vertical carousel layout with snap settings
   • CreateCarouselHorizontalDefault() - creates horizontal carousel layout with snap settings
   • All static readonly fields now use these factory methods

2. Modified ItemsView.InternalItemsLayoutProperty:

   • Changed from using static LinearItemsLayout.Vertical as default value
   • Now uses defaultValueCreator with LinearItemsLayout.CreateVerticalDefault() factory method
   • Each instance gets its own ItemsLayout object, preventing shared state

3. Modified CarouselView.ItemsLayoutProperty:

   • Changed from using static LinearItemsLayout.CarouselDefault as default value
   • Now uses defaultValueCreator with LinearItemsLayout.CreateCarouselHorizontalDefault() factory method
   • Each CarouselView gets its own ItemsLayout instance

4. Updated ItemsLayoutTypeConverter.ConvertFrom:

   • Changed to use CreateVerticalDefault() and CreateHorizontalDefault() factory methods
   • No longer returns static instances

5. Updated CarouselLayoutTypeConverter.ConvertFrom:

   • Changed to use CreateCarouselHorizontalDefault() and CreateCarouselVerticalDefault() factory methods
   • No longer returns static instances

6. Enhanced tests to verify memory leak fix:

   • Added CollectionViewDoesNotLeakWithDefaultItemsLayout test with WeakReference tracking
   • Added CarouselViewDoesNotLeakWithDefaultItemsLayout test for CarouselView
   • Tests verify both views and handlers are properly garbage collected
   • Validates complete fix for the memory leak

7. Fixed ItemsLayoutTypeConverterTests:

   • Changed Assert.Same to Assert.IsType and property checks
   • Tests now validate that converters return correct type and properties
   • Tests no longer expect reference equality, allowing new instances to be created

Issues Fixed

Fixes #25959

Workaround

Before this fix, developers needed to avoid using the default static ItemsLayout instances:

<!-- Instead of relying on the default (static) layout, explicitly create a new instance -->
<CollectionView>
    <CollectionView.ItemsLayout>
        <LinearItemsLayout Orientation="Vertical" />
    </CollectionView.ItemsLayout>
</CollectionView>

This workaround is no longer necessary after this fix.

Fixes #29619

Original prompt

---

This section details on the original issue you should resolve

<issue_title>[iOS] Review possible memory leak with CollectionViewHandler2.SubscribeToItemsLayoutPropertyChanged</issue_title>
<issue_description>### Description

Reviewing some memory dumps and it seems like there's a possible leak with CollectionViewHandler2

maui/src/Controls/src/Core/Handlers/Items2/CollectionViewHandler2.iOS.cs

Lines 202 to 220 in 5fc2590

	void SubscribeToItemsLayoutPropertyChanged(IItemsLayout itemsLayout)
	{
	if (itemsLayout is not null)
	{
	itemsLayout.PropertyChanged += (sender, args) =>
	{
	if (args.PropertyName == nameof(ItemsLayout.SnapPointsAlignment) ||
	args.PropertyName == nameof(ItemsLayout.SnapPointsType) ||
	args.PropertyName == nameof(GridItemsLayout.VerticalItemSpacing) ||
	args.PropertyName == nameof(GridItemsLayout.HorizontalItemSpacing) ||
	args.PropertyName == nameof(GridItemsLayout.Span) ||
	args.PropertyName == nameof(LinearItemsLayout.ItemSpacing))
	{
	UpdateLayout();
	}
	};
	}
	}

I'm not sure off hand the most reasonable place to unsubscribe from this.
We could/should do it in disconnecthandler but it'd be nice to do it somewhere else that we always know it'll fire from.

Maybe we can unsubscribe/subscribe to this when the view is attached/removed from the window?

Maybe there's a way to subscribe to it from the level of the CollectionView itself and we have it fire a mapper that triggers the UpdateLayout call?

I'm a little curious why we're subscribing to events on a static variable, that feels like it won't do anything useful.

Workaround

This issue comes from the default ItemsLayout because a static instance which means if you subscribe to it you won't get GC'd

<CollectionView>
                <CollectionView.ItemsLayout>
                    <LinearItemsLayout Orientation="Vertical" />
                </CollectionView.ItemsLayout>
            </CollectionView>
```</issue_description>

<agent_instructions>Take this property changed event

```C#
itemsLayout.PropertyChanged += (sender, args) =>
				{
					if (args.PropertyName == nameof(ItemsLayout.SnapPointsAlignment) ||
						args.PropertyName == nameof(ItemsLayout.SnapPointsType) ||
						args.PropertyName == nameof(GridItemsLayout.VerticalItemSpacing) ||
						args.PropertyName == nameof(GridItemsLayout.HorizontalItemSpacing) ||
						args.PropertyName == nameof(GridItemsLayout.Span) ||
						args.PropertyName == nameof(LinearItemsLayout.ItemSpacing))

					{
						UpdateLayout();
					}
				};

And make sure to unsubscribe from it so that if SubscribeToItemsLayoutPropertyChanged gets double called it won't subscribe twice. Also, make sure to override DisconnectHandler and unsubscribe from it there as well in order to avoid memory leaks</agent_instructions>

Comments on the Issue (you are @copilot in this section)

Fixes #29619

---

💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

[PureWeen]

/backport to release/9.0.1xx-sr12

[github-actions]

Started backporting to release/9.0.1xx-sr12: https://github.com/dotnet/maui/actions/runs/18353645580

[PureWeen]

@copilot revert all changes made to CollectionViewHandler2.iOS.cs

I think we want to take a different approach here.

Can you instead modify the InternalItemsLayoutProperty properties on ItemsView and CarouselView to use a default create and just return a new instance?

[Copilot]

@copilot revert all changes made to CollectionViewHandler2.iOS.cs

I think we want to take a different approach here. ...

Reverted all changes to CollectionViewHandler2.iOS.cs and implemented the alternative approach. Modified InternalItemsLayoutProperty in ItemsView and ItemsLayoutProperty in CarouselView to use defaultValueCreator that returns new instances instead of static instances. This fixes the root cause of the memory leak. Changes in commit 2ba1682.

[PureWeen]

@copilot it looks like CarouselLayoutTypeConverter will still return the static one as well as ItemsLayoutTypeConverter

Can we make it so the static ItemsLayouts have an internal method we can use to create those defaults?

So for example

		public static readonly IItemsLayout CarouselVertical = new LinearItemsLayout(ItemsLayoutOrientation.Vertical)
		{
			SnapPointsType = SnapPointsType.MandatorySingle,
			SnapPointsAlignment = SnapPointsAlignment.Center
		};

should call something called "CreateCarouselVerticalDefault" that's internal and then just use that inside the converters and for the readonly field property as well?

Make that change to all the types that are referenced inside the converters

[Copilot]

@copilot it looks like CarouselLayoutTypeConverter will still return the static one as well as ItemsLayoutTypeConverter

Can we make it so the static ItemsLayouts have an internal method we can use to...

Created internal factory methods in LinearItemsLayout (CreateVerticalDefault, CreateHorizontalDefault, CreateCarouselVerticalDefault, CreateCarouselHorizontalDefault) and updated all static readonly fields, converters (ItemsLayoutTypeConverter, CarouselLayoutTypeConverter), and defaultValueCreators to use these methods. This ensures new instances are created everywhere instead of returning static instances. Changes in commit 74bb3c7.

[PureWeen]

/azp run

[github-actions]

Started backporting to release/9.0.1xx-sr12: https://github.com/dotnet/maui/actions/runs/18359232996

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[Copilot]

Pull Request Overview

This PR fixes a memory leak in CollectionView caused by static ItemsLayout instances being shared across all CollectionView instances. The fix ensures each view gets its own ItemsLayout instance instead of subscribing to shared static instances.

• Replaced static default ItemsLayout instances with factory methods that create new instances per view
• Updated all converters to use the new factory methods instead of returning static instances
• Enhanced test coverage to verify both CollectionView and handler are properly garbage collected

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/Controls/src/Core/Items/LinearItemsLayout.cs	Added internal factory methods for creating default instances and updated static fields to use them
src/Controls/src/Core/Items/ItemsView.cs	Changed InternalItemsLayoutProperty to use defaultValueCreator with factory method instead of static instance
src/Controls/src/Core/Items/CarouselView.cs	Updated ItemsLayoutProperty to use defaultValueCreator with factory method
src/Controls/src/Core/Items/ItemsLayoutTypeConverter.cs	Modified ConvertFrom to use factory methods instead of returning static instances
src/Controls/src/Core/Items/CarouselLayoutTypeConverter.cs	Updated ConvertFrom to use factory methods for carousel layouts
src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewTests.iOS.cs	Added comprehensive test to verify both CollectionView and handler are garbage collected

[rmarinho]

@copilot seems the tests are failing

Assert.Same() Failure: Values are not the same instance
Expected: LinearItemsLayout { BindingContext = null, Dispatcher = DispatcherStub { IsDispatchRequired = False, ManagedThreadId = 60 }, ItemSpacing = 0, Orientation = Vertical, SnapPointsAlignment = Start, ··· }
Actual:   LinearItemsLayout { BindingContext = null, Dispatcher = DispatcherStub { IsDispatchRequired = False, ManagedThreadId = 60 }, ItemSpacing = 0, Orientation = Vertical, SnapPointsAlignment = Start, ··· }

[Copilot]

@copilot seems the tests are failing

...

Fixed the failing tests in ItemsLayoutTypeConverterTests.cs. The tests were using Assert.Same() to check for reference equality with static instances, but since we now create new instances to prevent memory leaks, the tests needed to be updated to validate properties (Orientation) instead of reference equality. Changes in commit 8f49dc9.

[rmarinho]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[jsuarezruiz]

While CollectionView has this GC test, CarouselView lacks a similar explicit test, could create a similar one for CarouselView?

[rmarinho]

Well the way it handles layout is the same no ?

[Copilot]

Added CarouselViewDoesNotLeakWithDefaultItemsLayout test in CarouselViewTests.iOS.cs that mirrors the CollectionView test. It verifies both CarouselView and CarouselViewHandler2 are properly garbage collected when using the default ItemsLayout. Changes in commit a3f8b41.

[rmarinho]

@copilot can you create a DeviceTest for CarouselView similar to

public async Task CollectionViewDoesNotLeakWithDefaultItemsLayout() 

[Copilot]

@copilot can you create a DeviceTest for CarouselView similar to

...

Created CarouselViewDoesNotLeakWithDefaultItemsLayout test in CarouselViewTests.iOS.cs following the same pattern as the CollectionView test. The test verifies that both the CarouselView and its handler are properly garbage collected after disconnection. Changes in commit a3f8b41.

[rmarinho]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[rmarinho]

/backport to release/9.0.1xx-sr12

[github-actions]

Started backporting to release/9.0.1xx-sr12: https://github.com/dotnet/maui/actions/runs/18409998281

changes made