From 251b3167e3bdc5602289992b35aed4b7c9c69791 Mon Sep 17 00:00:00 2001
From: Jonathan Peppers <jonathan.peppers@microsoft.com>
Date: Mon, 13 Feb 2023 15:45:00 -0600
Subject: [PATCH] [controls] one less WeakReference in TypedBinding

Context: https://github.com/Vroomer/MAUI-master-detail-memory-leak
Context: https://github.com/dotnet/maui/issues/12039

When reviewing the memory snapshots in #12039, I noticed a
`WeakReference<T>` that could be completely removed in `TypedBinding`.
`BindingExpression.WeakPropertyChangedProxy` already had this exposed
as a `Source` property we can just use instead.

I also updated some `TypedBindingUnitTests` to assert more things are
GC'd properly.

I don't think this fixes an issue, except saving ~8 bytes per
`TypedBinding` and makes memory snapshots smaller/easier to read.
---
 src/Controls/src/Core/TypedBinding.cs         | 19 ++++-----
 .../Core.UnitTests/TypedBindingUnitTests.cs   | 42 +++++++++++++++----
 2 files changed, 42 insertions(+), 19 deletions(-)

diff --git a/src/Controls/src/Core/TypedBinding.cs b/src/Controls/src/Core/TypedBinding.cs
index e59383779169..732b1b8cd3cb 100644
--- a/src/Controls/src/Core/TypedBinding.cs
+++ b/src/Controls/src/Core/TypedBinding.cs
@@ -249,29 +249,28 @@ class PropertyChangedProxy
 			public Func<TSource, object> PartGetter { get; }
 			public string PropertyName { get; }
 			public BindingExpression.WeakPropertyChangedProxy Listener { get; }
-			WeakReference<INotifyPropertyChanged> _weakPart = new WeakReference<INotifyPropertyChanged>(null);
 			readonly BindingBase _binding;
 			PropertyChangedEventHandler handler;
 			public INotifyPropertyChanged Part
 			{
 				get
 				{
-					INotifyPropertyChanged target;
-					if (_weakPart.TryGetTarget(out target))
+					if (Listener != null && Listener.Source.TryGetTarget(out var target))
 						return target;
 					return null;
 				}
 				set
 				{
-					if (Listener != null && Listener.Source.TryGetTarget(out var source) && ReferenceEquals(value, source))
+					if (Listener != null)
+					{
 						//Already subscribed
-						return;
-
-					//clear out previous subscription
-					Listener?.Unsubscribe();
+						if (Listener.Source.TryGetTarget(out var source) && ReferenceEquals(value, source))
+							return;
 
-					_weakPart.SetTarget(value);
-					Listener.SubscribeTo(value, handler);
+						//clear out previous subscription
+						Listener.Unsubscribe();
+						Listener.SubscribeTo(value, handler);
+					}
 				}
 			}
 
diff --git a/src/Controls/tests/Core.UnitTests/TypedBindingUnitTests.cs b/src/Controls/tests/Core.UnitTests/TypedBindingUnitTests.cs
index abaadef6d66e..cbce71cb5e71 100644
--- a/src/Controls/tests/Core.UnitTests/TypedBindingUnitTests.cs
+++ b/src/Controls/tests/Core.UnitTests/TypedBindingUnitTests.cs
@@ -5,6 +5,7 @@
 using System.Globalization;
 using System.Linq;
 using System.Runtime.CompilerServices;
+using System.Threading.Tasks;
 using Microsoft.Maui.Controls.Internals;
 using Xunit;
 using Xunit.Sdk;
@@ -1376,7 +1377,20 @@ event PropertyChangedEventHandler INotifyPropertyChanged.PropertyChanged
 				remove { PropertyChanged -= value; }
 			}
 
-			public string Foo { get; set; }
+			string _foo;
+
+			public string Foo
+			{
+				get => _foo;
+				set
+				{
+					if (_foo != value)
+					{
+						_foo = value;
+						OnPropertyChanged(nameof(Foo));
+					}
+				}
+			}
 
 			public int InvocationListSize()
 			{
@@ -1392,9 +1406,10 @@ public virtual void OnPropertyChanged([CallerMemberName] string propertyName = n
 		}
 
 		[Fact]
-		public void BindingUnsubscribesForDeadTarget()
+		public async Task BindingUnsubscribesForDeadTarget()
 		{
 			var viewmodel = new TestViewModel();
+			WeakReference bindingRef = null, buttonRef = null;
 
 			int i = 0;
 			Action create = null;
@@ -1407,30 +1422,37 @@ public void BindingUnsubscribesForDeadTarget()
 				}
 
 				var button = new Button();
-				button.SetBinding(Button.TextProperty, new TypedBinding<TestViewModel, string>(vm => (vm.Foo, true), (vm, s) => vm.Foo = s, new[] {
+				var binding = new TypedBinding<TestViewModel, string>(vm => (vm.Foo, true), (vm, s) => vm.Foo = s, new[] {
 					new Tuple<Func<TestViewModel, object>, string>(vm=>vm,"Foo")
-				}));
+				});
+				button.SetBinding(Button.TextProperty, binding);
 				button.BindingContext = viewmodel;
+				bindingRef = new WeakReference(binding);
+				buttonRef = new WeakReference(button);
 			};
 
 			create();
+			Assert.Equal(viewmodel.Foo = "Bar", ((Button)buttonRef.Target).Text);
 
 			Assert.Equal(1, viewmodel.InvocationListSize());
 
+			await Task.Yield();
 			GC.Collect();
 			GC.WaitForPendingFinalizers();
-			GC.Collect();
 
 			viewmodel.OnPropertyChanged("Foo");
 
 			Assert.Equal(0, viewmodel.InvocationListSize());
+
+			Assert.False(bindingRef.IsAlive, "Binding should not be alive!");
+			Assert.False(buttonRef.IsAlive, "Button should not be alive!");
 		}
 
 		[Fact]
-		public void BindingDoesNotStayAliveForDeadTarget()
+		public async Task BindingDoesNotStayAliveForDeadTarget()
 		{
 			var viewModel = new TestViewModel();
-			WeakReference bindingRef = null;
+			WeakReference bindingRef = null, buttonRef = null;
 
 			int i = 0;
 			Action create = null;
@@ -1450,18 +1472,20 @@ public void BindingDoesNotStayAliveForDeadTarget()
 				button.BindingContext = viewModel;
 
 				bindingRef = new WeakReference(binding);
-				binding = null;
+				buttonRef = new WeakReference(button);
 			};
 
 			create();
+			Assert.Equal(viewModel.Foo = "Bar", ((Button)buttonRef.Target).Text);
 
 			Assert.Equal(1, viewModel.InvocationListSize());
 
+			await Task.Yield();
 			GC.Collect();
 			GC.WaitForPendingFinalizers();
-			GC.Collect();
 
 			Assert.False(bindingRef.IsAlive, "Binding should not be alive!");
+			Assert.False(buttonRef.IsAlive, "Button should not be alive!");
 		}
 
 		[Fact]