[core] fix memory leaks in bindings (#13327)

Fixes: #12039
Fixes: #10560
Context: https://github.com/Vroomer/MAUI-master-detail-memory-leak
Context: https://github.com/symbiogenesis/Maui.DataGrid/tree/memory-leak

In investigating various MAUI samples, I found the following object
leaking:

    WeakPropertyChangedProxy (referenced by ->)
        TypedBinding
        Binding

In ~2016, I contributed a fix to Xamarin.Forms:

xamarin/Xamarin.Forms@f6febd4

Back then, this solved the follow case:

1. You have a long-lived `ViewModel` class. Could be a singleton, etc.

2. Data-bind a `View` to this `ViewModel`.

3. The `ViewModel` indefinitely held on to any object that subscribed
   to `PropertyChanged`.

At the time, this solved a huge memory leak, because a data-bound
`View` would have a reference to its parent, then to its parent, etc.
Effectively this was leaking entire `Page`'s at the time.

Unfortunately, there was still a flaw in this change...
`WeakPropertyChangedProxy` hangs around forever instead! I could
reproduce this problem in unit tests, by accessing various internal
members through reflection -- asserting they were alive or not.

We do have another layer of indirection, where other objects are GC'd
that can free the `WeakPropertyChangedProxy`, such as:

    // Regular Binding
    ~BindingExpressionPart() => _listener?.Unsubscribe();
    // TypedBinding
    ~PropertyChangedProxy() => Listener?.Unsubscribe();

This means it would take two GC's for these objects to go away, but it
is better than the alternative -- they *can* actually go away now.

After testing apps with this change, sometimes I would get an
`InvalidOperationException` in `WeakReference<T>`:

https://source.dot.net/#System.Private.CoreLib/src/libraries/System.Private.CoreLib/src/System/WeakReference.T.cs,103

So I added a parameter to `Unsubscribe(finalizer: true)`, to skip
`WeakReference<T>.SetTarget()` from finalizers.

After this change, I still found an issue! In my new unit test, the
following would hold onto a `ViemModel` object forever:

    VisualElement.BackgroundProperty.DefaultValue

This held the value of `Brush.Default`, in which
`Brush.Default.BindingContext` was the `ViewModel`!

My first thought was for `Brush.Default` to return `ImmutableBrush`:

    public static Brush Default => defaultBrush ??= new ImmutableBrush(null);

Because anyone could do `Brush.Default.Color = Colors.Red` if they liked.

When this didn't fix it, I found the underlying `_inheritedContext` is
what held a reference to my `ViewModel` object. I changed this value
to a `WeakReference`.

The types of leaks this fixes:

* Bindings to application-lifetime, singleton `ViewModel`s

* Scrolling `CollectionView`, `ListView`, etc. with data-bindings.

* Styles that were used on a `View` or `Page` that is now removed from
  the screen via navigation, de-parenting, etc.

Ok, I really think the leaks are gone now. Maybe?

src/Controls/src/Core/BindableObject.cs

@@ -31,7 +31,7 @@ public BindableObject()
 
 		readonly Dictionary<BindableProperty, BindablePropertyContext> _properties = new Dictionary<BindableProperty, BindablePropertyContext>(4);
 		bool _applying;
-		object _inheritedContext;
+		WeakReference _inheritedContext;
 
 		/// <include file="../../docs/Microsoft.Maui.Controls/BindableObject.xml" path="//Member[@MemberName='BindingContextProperty']/Docs/*" />
 		public static readonly BindableProperty BindingContextProperty =
@@ -41,7 +41,7 @@ public BindableObject()
 		/// <include file="../../docs/Microsoft.Maui.Controls/BindableObject.xml" path="//Member[@MemberName='BindingContext']/Docs/*" />
 		public object BindingContext
 		{
-			get => _inheritedContext ?? GetValue(BindingContextProperty);
+			get => _inheritedContext?.Target ?? GetValue(BindingContextProperty);
 			set => SetValue(BindingContextProperty, value);
 		}
 
@@ -238,7 +238,7 @@ public static void SetInheritedBindingContext(BindableObject bindable, object va
 			if (bpContext != null && ((bpContext.Attributes & BindableContextAttributes.IsManuallySet) != 0))
 				return;
 
-			object oldContext = bindable._inheritedContext;
+			object oldContext = bindable._inheritedContext?.Target;
 
 			if (ReferenceEquals(oldContext, value))
 				return;
@@ -253,7 +253,7 @@ public static void SetInheritedBindingContext(BindableObject bindable, object va
 			}
 			else
 			{
-				bindable._inheritedContext = value;
+				bindable._inheritedContext = new WeakReference(value);
 			}
 
 			bindable.ApplyBindings(skipBindingContext: false, fromBindingContextChanged: true);
@@ -557,7 +557,7 @@ internal void ApplyBindings(bool skipBindingContext, bool fromBindingContextChan
 
 		static void BindingContextPropertyBindingChanging(BindableObject bindable, BindingBase oldBindingBase, BindingBase newBindingBase)
 		{
-			object context = bindable._inheritedContext;
+			object context = bindable._inheritedContext?.Target;
 			var oldBinding = oldBindingBase as Binding;
 			var newBinding = newBindingBase as Binding;
 

src/Controls/src/Core/BindingExpression.cs

@@ -635,7 +635,7 @@ public void SubscribeTo(INotifyPropertyChanged source, PropertyChangedEventHandl
 				_listener.SetTarget(listener);
 			}
 
-			public void Unsubscribe()
+			public void Unsubscribe(bool finalizer = false)
 			{
 				INotifyPropertyChanged source;
 				if (_source.TryGetTarget(out source) && source != null)
@@ -644,6 +644,10 @@ public void Unsubscribe()
 				if (bo != null)
 					bo.BindingContextChanged -= _bchandler;
 
+				// If we are called from a finalizer, WeakReference<T>.SetTarget() can throw InvalidOperationException
+				if (finalizer)
+					return;
+
 				_source.SetTarget(null);
 				_listener.SetTarget(null);
 			}
@@ -668,6 +672,8 @@ class BindingExpressionPart
 			readonly PropertyChangedEventHandler _changeHandler;
 			WeakPropertyChangedProxy _listener;
 
+			~BindingExpressionPart() => _listener?.Unsubscribe(finalizer: true);
+
 			public BindingExpressionPart(BindingExpression expression, string content, bool isIndexer = false)
 			{
 				_expression = expression;

src/Controls/src/Core/Brush.cs

@@ -7,11 +7,9 @@ namespace Microsoft.Maui.Controls
 	[System.ComponentModel.TypeConverter(typeof(BrushTypeConverter))]
 	public abstract partial class Brush : Element
 	{
+		static ImmutableBrush defaultBrush;
 		/// <include file="../../docs/Microsoft.Maui.Controls/Brush.xml" path="//Member[@MemberName='Default']/Docs/*" />
-		public static Brush Default
-		{
-			get { return new SolidColorBrush(null); }
-		}
+		public static Brush Default => defaultBrush ??= new(null);
 
 		public static implicit operator Brush(Color color) => new SolidColorBrush(color);
 

src/Controls/src/Core/TypedBinding.cs

@@ -251,6 +251,9 @@ class PropertyChangedProxy
 			public BindingExpression.WeakPropertyChangedProxy Listener { get; }
 			readonly BindingBase _binding;
 			PropertyChangedEventHandler handler;
+
+			~PropertyChangedProxy() => Listener?.Unsubscribe(finalizer: true);
+
 			public INotifyPropertyChanged Part
 			{
 				get

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

@@ -1,8 +1,10 @@
 using System;
+using System.Collections;
 using System.Collections.Generic;
 using System.ComponentModel;
 using System.Globalization;
 using System.Linq;
+using System.Reflection;
 using System.Runtime.CompilerServices;
 using System.Threading.Tasks;
 using Xunit;
@@ -1185,6 +1187,81 @@ public void ValueUpdatedWithSelfPathOnTwoWayBinding(bool isDefault)
 			AssertNoErrorsLogged();
 		}
 
+
+		[Theory, Category("[Binding] Complex paths")]
+		[InlineData(BindingMode.OneWay)]
+		[InlineData(BindingMode.OneWayToSource)]
+		[InlineData(BindingMode.TwoWay)]
+		public async Task WeakPropertyChangedProxyDoesNotLeak(BindingMode mode)
+		{
+			var proxies = new List<WeakReference>();
+			WeakReference weakViewModel = null, weakBindable = null;
+
+			int i = 0;
+			void create ()
+			{
+				if (i++ < 1024)
+				{
+					create();
+					return;
+				}
+
+				var binding = new Binding("Model.Model[1]");
+				var bindable = new MockBindable();
+				weakBindable = new WeakReference(bindable);
+
+				var viewmodel = new ComplexMockViewModel
+				{
+					Model = new ComplexMockViewModel
+					{
+						Model = new ComplexMockViewModel()
+					}
+				};
+
+				weakViewModel = new WeakReference(viewmodel);
+
+				bindable.BindingContext = viewmodel;
+				bindable.SetBinding(MockBindable.TextProperty, binding);
+
+				// Access private members:
+				// WeakPropertyChangedProxy proxy = binding._expression._parts[i]._listener;
+				var flags = BindingFlags.NonPublic | BindingFlags.Instance;
+				var expression = binding.GetType().GetField("_expression", flags).GetValue(binding);
+				Assert.NotNull(expression);
+				var parts = expression.GetType().GetField("_parts", flags).GetValue(expression) as IEnumerable;
+				Assert.NotNull(parts);
+				foreach (var part in parts)
+				{
+					var listener = part.GetType().GetField("_listener", flags).GetValue(part);
+					if (listener == null)
+						continue;
+					proxies.Add(new WeakReference(listener));
+				}
+				Assert.NotEmpty(proxies); // Should be at least 1
+			};
+			create();
+
+			await Task.Yield();
+			GC.Collect();
+			GC.WaitForPendingFinalizers();
+
+			if (mode == BindingMode.TwoWay || mode == BindingMode.OneWay)
+				Assert.False(weakViewModel.IsAlive, "ViewModel wasn't collected");
+
+			if (mode == BindingMode.TwoWay || mode == BindingMode.OneWayToSource)
+				Assert.False(weakBindable.IsAlive, "Bindable wasn't collected");
+
+			// WeakPropertyChangedProxy won't go away until the second GC, BindingExpressionPart unsubscribes in its finalizer
+			await Task.Yield();
+			GC.Collect();
+			GC.WaitForPendingFinalizers();
+
+			foreach (var proxy in proxies)
+			{
+				Assert.False(proxy.IsAlive, "WeakPropertyChangedProxy wasn't collected");
+			}
+		}
+
 		[Theory, Category("[Binding] Complex paths")]
 		[InlineData(BindingMode.OneWay)]
 		[InlineData(BindingMode.OneWayToSource)]

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

@@ -4,6 +4,7 @@
 using System.Diagnostics;
 using System.Globalization;
 using System.Linq;
+using System.Reflection;
 using System.Runtime.CompilerServices;
 using System.Threading.Tasks;
 using Microsoft.Maui.Controls.Internals;
@@ -1452,7 +1453,7 @@ public async Task BindingUnsubscribesForDeadTarget()
 		public async Task BindingDoesNotStayAliveForDeadTarget()
 		{
 			var viewModel = new TestViewModel();
-			WeakReference bindingRef = null, buttonRef = null;
+			WeakReference bindingRef = null, buttonRef = null, proxyRef = null;
 
 			int i = 0;
 			Action create = null;
@@ -1473,6 +1474,16 @@ public async Task BindingDoesNotStayAliveForDeadTarget()
 
 				bindingRef = new WeakReference(binding);
 				buttonRef = new WeakReference(button);
+
+				// Access private members:
+				// WeakPropertyChangedProxy proxy = binding._handlers[0].Listener;
+				var flags = BindingFlags.NonPublic | BindingFlags.Instance;
+				var handlers = binding.GetType().GetField("_handlers", flags).GetValue(binding) as object[];
+				Assert.NotNull(handlers);
+				var handler = handlers[0];
+				var proxy = handler.GetType().GetProperty("Listener").GetValue(handler);
+				Assert.NotNull(proxy);
+				proxyRef = new WeakReference(proxy);
 			};
 
 			create();
@@ -1486,6 +1497,12 @@ public async Task BindingDoesNotStayAliveForDeadTarget()
 
 			Assert.False(bindingRef.IsAlive, "Binding should not be alive!");
 			Assert.False(buttonRef.IsAlive, "Button should not be alive!");
+
+			// WeakPropertyChangedProxy won't go away until the second GC, PropertyChangedProxy unsubscribes in its finalizer
+			await Task.Yield();
+			GC.Collect();
+			GC.WaitForPendingFinalizers();
+			Assert.False(proxyRef.IsAlive, "WeakPropertyChangedProxy should not be alive!");
 		}
 
 		[Fact]