Fix for BindingExpression memory leak (#279)

* Unit test proving a memory leak with Binding

What we were seeing in our app was that Binding objects stay around when
bound to long-lived ViewModels, even when the View is long gone

* BindingExpression - INotifyPropertyChanged should use WeakReference

I had to make a WeakPropertyChangedProxy class for this, I could not
think of a way to get around creating a new object for this

Xamarin.Forms.Core.UnitTests/BindingUnitTests.cs

@@ -5,6 +5,7 @@
 using System.ComponentModel;
 using System.Linq;
 using System.Runtime.CompilerServices;
+using System.Threading.Tasks;
 using NUnit.Framework;
 using CategoryAttribute=NUnit.Framework.CategoryAttribute;
 using DescriptionAttribute=NUnit.Framework.DescriptionAttribute;
@@ -2591,6 +2592,32 @@ public void BindingUnsubscribesForDeadTarget ()
 			Assert.AreEqual (0, viewmodel.InvocationListSize ());
 		}
 
+		[Test]
+		public async Task BindingDoesNotStayAliveForDeadTarget()
+		{
+			TestViewModel viewModel = new TestViewModel();
+			WeakReference bindingRef;
+
+			{ 
+				var binding = new Binding("Foo");
+
+				var button = new Button();
+				button.SetBinding(Button.TextProperty, binding);
+				button.BindingContext = viewModel;
+
+				bindingRef = new WeakReference(binding);
+			}
+
+			Assume.That(viewModel.InvocationListSize(), Is.EqualTo(1));
+
+			//NOTE: this was the only way I could "for sure" get the binding to get GC'd
+			GC.Collect();
+			await Task.Delay(10);
+			GC.Collect();
+
+			Assert.IsFalse(bindingRef.IsAlive, "Binding should not be alive!");
+		}
+
 		[Test]
 		public void BindingCreatesSingleSubscription ()
 		{

Xamarin.Forms.Core/BindingExpression.cs

@@ -90,9 +90,7 @@ internal void Unapply()
 						part.TryGetValue(sourceObject, out sourceObject);
 					}
 
-					var inpc = sourceObject as INotifyPropertyChanged;
-					if (inpc != null)
-						inpc.PropertyChanged -= part.ChangeHandler;
+					part.Unsubscribe();
 				}
 			}
 
@@ -147,9 +145,7 @@ void ApplyCore(object sourceObject, BindableObject target, BindableProperty prop
 					var inpc = current as INotifyPropertyChanged;
 					if (inpc != null && !ReferenceEquals(current, previous))
 					{
-						// If we're reapplying, we don't want to double subscribe
-						inpc.PropertyChanged -= part.ChangeHandler;
-						inpc.PropertyChanged += part.ChangeHandler;
+						part.Subscribe(inpc);
 					}
 				}
 
@@ -410,11 +406,57 @@ public BindingPair(BindingExpressionPart part, object source, bool isLast)
 			public object Source { get; private set; }
 		}
 
+		class WeakPropertyChangedProxy
+		{
+			WeakReference _source, _listener;
+
+			public WeakPropertyChangedProxy(INotifyPropertyChanged source, PropertyChangedEventHandler listener)
+			{
+				source.PropertyChanged += OnPropertyChanged;
+				_source = new WeakReference(source);
+				_listener = new WeakReference(listener);
+			}
+
+			public void Unsubscribe()
+			{
+				if (_source != null)
+				{
+					var source = _source.Target as INotifyPropertyChanged;
+					if (source != null)
+					{
+						source.PropertyChanged -= OnPropertyChanged;
+					}
+					_source = null;
+					_listener = null;
+				}
+			}
+
+			private void OnPropertyChanged(object sender, PropertyChangedEventArgs e)
+			{
+				if (_listener != null)
+				{
+					var handler = _listener.Target as PropertyChangedEventHandler;
+					if (handler != null)
+					{
+						handler(sender, e);
+					}
+					else
+					{
+						Unsubscribe();
+					}
+				}
+				else
+				{
+					Unsubscribe();
+				}
+			}
+		}
+
 		class BindingExpressionPart
 		{
 			readonly BindingExpression _expression;
-
-			public readonly PropertyChangedEventHandler ChangeHandler;
+			readonly PropertyChangedEventHandler _changeHandler;
+			WeakPropertyChangedProxy _listener;
 
 			public BindingExpressionPart(BindingExpression expression, string content, bool isIndexer = false)
 			{
@@ -423,7 +465,25 @@ public BindingExpressionPart(BindingExpression expression, string content, bool
 				Content = content;
 				IsIndexer = isIndexer;
 
-				ChangeHandler = PropertyChanged;
+				_changeHandler = PropertyChanged;
+			}
+
+			public void Subscribe(INotifyPropertyChanged handler)
+			{
+				// If we're reapplying, we don't want to double subscribe
+				Unsubscribe();
+
+				_listener = new WeakPropertyChangedProxy(handler, _changeHandler);
+			}
+
+			public void Unsubscribe()
+			{
+				var listener = _listener;
+				if (listener != null)
+				{
+					listener.Unsubscribe();
+					_listener = null;
+				}
 			}
 
 			public object[] Arguments { get; set; }