[leak-fix] Fix Picker.ItemsSource memory leak (Fixes #36272)

src/Controls/src/Core/Picker/Picker.cs

@@ -79,12 +79,17 @@ public partial class Picker : View, IFontElement, ITextElement, ITextAlignmentEl
 
 		readonly Lazy<PlatformConfigurationRegistry<Picker>> _platformConfigurationRegistry;
 
+		ItemsSourceCollectionChangedSubscription _itemsSourceCollectionChangedSubscription;
+		// WeakNotifyCollectionChangedProxy weakly references its handler, so the Picker must keep this delegate alive.
+		NotifyCollectionChangedEventHandler _collectionChangedEventHandler;
+
 		/// <summary>Initializes a new instance of the Picker class.</summary>
 		public Picker()
 		{
 			((INotifyCollectionChanged)Items).CollectionChanged += OnItemsCollectionChanged;
 			_platformConfigurationRegistry = new Lazy<PlatformConfigurationRegistry<Picker>>(() => new PlatformConfigurationRegistry<Picker>(this));
 		}
+
 		/// <summary>Gets a value that indicates whether the font for the searchbar text is bold, italic, or neither. This is a bindable property.</summary>
 		public FontAttributes FontAttributes
 		{
@@ -333,14 +338,18 @@ static void OnItemsSourceChanged(BindableObject bindable, object oldValue, objec
 
 		void OnItemsSourceChanged(IList oldValue, IList newValue)
 		{
-			var oldObservable = oldValue as INotifyCollectionChanged;
-			if (oldObservable != null)
-				oldObservable.CollectionChanged -= CollectionChanged;
+			_itemsSourceCollectionChangedSubscription?.Unsubscribe();
 
 			var newObservable = newValue as INotifyCollectionChanged;
 			if (newObservable != null)
 			{
-				newObservable.CollectionChanged += CollectionChanged;
+				var subscription = _itemsSourceCollectionChangedSubscription ??= new ItemsSourceCollectionChangedSubscription();
+				_collectionChangedEventHandler ??= CollectionChanged;
+				subscription.Subscribe(newObservable, _collectionChangedEventHandler);
+			}
+			else
+			{
+				_itemsSourceCollectionChangedSubscription = null;
 			}
 
 			if (newValue != null)
@@ -356,6 +365,24 @@ void OnItemsSourceChanged(IList oldValue, IList newValue)
 			}
 		}
 
+		sealed class ItemsSourceCollectionChangedSubscription
+		{
+			readonly WeakNotifyCollectionChangedProxy _proxy = new();
+
+			~ItemsSourceCollectionChangedSubscription() => _proxy.Unsubscribe();
+
+			public void Subscribe(INotifyCollectionChanged source, NotifyCollectionChangedEventHandler handler)
+			{
+				_proxy.Subscribe(source, handler);
+			}
+
+			public void Unsubscribe()
+			{
+				// The subscription is reused across ItemsSource changes, so its safety-net finalizer must remain registered.
+				_proxy.Unsubscribe();
+			}
+		}
+
 		readonly Queue<Action> _pendingIsOpenActions = new Queue<Action>();
 
 		void OnIsOpenPropertyChanged(bool oldValue, bool newValue)

src/Controls/tests/Core.UnitTests/PickerTests.cs

@@ -5,6 +5,9 @@
 using System.ComponentModel;
 using System.Globalization;
 using System.Linq;
+using System.Reflection;
+using System.Runtime.CompilerServices;
+using System.Threading.Tasks;
 using Microsoft.Maui.Graphics;
 using NSubstitute;
 using Xunit;
@@ -1040,5 +1043,134 @@ public void PickerPreservesSelectedItemAfterInsertingItemBeforeSelection()
 			Assert.Equal("Dog", picker.SelectedItem);
 			Assert.Equal(2, picker.SelectedIndex);
 		}
+
+		[Fact, Category(TestCategory.Memory)]
+		public async Task PickerItemsSourceDoesNotLeak()
+		{
+			// A long-lived / shared collection (for example a ViewModel-owned ObservableCollection)
+			// must not keep every Picker it was assigned to alive.
+			var sharedItemsSource = new ObservableCollection<string> { "a", "b", "c" };
+
+			var references = CreatePickerReferences(sharedItemsSource);
+
+			Assert.False(await references.Picker.WaitForCollect(), "Picker should not be alive!");
+			Assert.False(await references.Subscription.WaitForCollect(), "ItemsSource subscription should not be alive!");
+			Assert.False(await references.Proxy.WaitForCollect(), "WeakNotifyCollectionChangedProxy should not be alive!");
+			GC.KeepAlive(sharedItemsSource);
+		}
+
+		[Fact, Category(TestCategory.Memory)]
+		public async Task PickerItemsSourceChangesStillApplyAfterGc()
+		{
+			var itemsSource = new ObservableCollection<string> { "a", "b", "c" };
+			var picker = new Picker { ItemsSource = itemsSource };
+
+			await TestHelpers.Collect();
+
+			itemsSource.Add("d");
+
+			Assert.Equal(4, picker.Items.Count);
+			Assert.Equal("d", picker.Items[3]);
+			GC.KeepAlive(picker);
+		}
+
+		[Fact, Category(TestCategory.Memory)]
+		public async Task PickerItemsSourceClearReleasesCollectionChangedSubscription()
+		{
+			var sharedItemsSource = new ObservableCollection<string> { "a", "b", "c" };
+			var picker = new Picker { ItemsSource = sharedItemsSource };
+			var references = GetItemsSourceSubscriptionReferences(picker);
+
+			picker.ItemsSource = null;
+
+			Assert.False(await references.Subscription.WaitForCollect(), "ItemsSource subscription should not be alive!");
+			Assert.False(await references.Proxy.WaitForCollect(), "WeakNotifyCollectionChangedProxy should not be alive!");
+			GC.KeepAlive(picker);
+			GC.KeepAlive(sharedItemsSource);
+		}
+
+		[Fact, Category(TestCategory.Memory)]
+		public async Task PickerItemsSourceReassignmentMovesCollectionChangedSubscription()
+		{
+			var replacementItemsSource = new ObservableCollection<string> { "replacement" };
+			var picker = new Picker();
+			var oldItemsSourceReference = AssignAndReplaceItemsSource(picker, replacementItemsSource);
+
+			replacementItemsSource.Add("added");
+
+			Assert.Equal(2, picker.Items.Count);
+			Assert.Equal("added", picker.Items[1]);
+			Assert.False(await oldItemsSourceReference.WaitForCollect(), "Old ItemsSource should not be alive!");
+
+			replacementItemsSource.Add("added after GC");
+
+			Assert.Equal(3, picker.Items.Count);
+			Assert.Equal("added after GC", picker.Items[2]);
+			GC.KeepAlive(picker);
+			GC.KeepAlive(replacementItemsSource);
+		}
+
+		[Fact]
+		public void PickerItemsSourceReassignmentIgnoresOldCollectionChanges()
+		{
+			var oldItemsSource = new ObservableCollection<string> { "old" };
+			var replacementItemsSource = new ObservableCollection<string> { "replacement" };
+			var picker = new Picker { ItemsSource = oldItemsSource };
+
+			picker.ItemsSource = replacementItemsSource;
+			oldItemsSource.Add("ignored");
+
+			Assert.Single(picker.Items);
+			Assert.Equal("replacement", picker.Items[0]);
+
+			replacementItemsSource.Add("added");
+
+			Assert.Equal(2, picker.Items.Count);
+			Assert.Equal("added", picker.Items[1]);
+		}
+
+		[Fact]
+		public void PickerItemsSourceCollectionChangedHandlerIsLazy()
+		{
+			const BindingFlags flags = BindingFlags.NonPublic | BindingFlags.Instance;
+			var handlerField = typeof(Picker).GetField("_collectionChangedEventHandler", flags);
+			Assert.NotNull(handlerField);
+			var picker = new Picker { ItemsSource = new List<string> { "item" } };
+
+			Assert.Null(handlerField.GetValue(picker));
+
+			picker.ItemsSource = new ObservableCollection<string> { "observable item" };
+
+			Assert.NotNull(handlerField.GetValue(picker));
+		}
+
+		[MethodImpl(MethodImplOptions.NoInlining)]
+		static (WeakReference Picker, WeakReference Subscription, WeakReference Proxy) CreatePickerReferences(
+			ObservableCollection<string> itemsSource)
+		{
+			var picker = new Picker { ItemsSource = itemsSource };
+			var (subscription, proxy) = GetItemsSourceSubscriptionReferences(picker);
+			return (new WeakReference(picker), subscription, proxy);
+		}
+
+		[MethodImpl(MethodImplOptions.NoInlining)]
+		static WeakReference AssignAndReplaceItemsSource(Picker picker, ObservableCollection<string> replacementItemsSource)
+		{
+			var oldItemsSource = new ObservableCollection<string> { "old" };
+			picker.ItemsSource = oldItemsSource;
+			picker.ItemsSource = replacementItemsSource;
+			return new WeakReference(oldItemsSource);
+		}
+
+		[MethodImpl(MethodImplOptions.NoInlining)]
+		static (WeakReference Subscription, WeakReference Proxy) GetItemsSourceSubscriptionReferences(Picker picker)
+		{
+			const BindingFlags flags = BindingFlags.NonPublic | BindingFlags.Instance;
+			var subscription = typeof(Picker).GetField("_itemsSourceCollectionChangedSubscription", flags)?.GetValue(picker);
+			Assert.NotNull(subscription);
+			var proxy = subscription.GetType().GetField("_proxy", flags)?.GetValue(subscription);
+			Assert.NotNull(proxy);
+			return (new WeakReference(subscription), new WeakReference(proxy));
+		}
 	}
 }