Merge pull request #2644 from joerih/fix-event-handler-leak-in-proper…

…ty-binding

Fix an event handler leak in `Binding.RemovePropertyEvent`

src/Eto/Forms/Binding/Binding.helpers.cs

@@ -189,12 +189,15 @@ public static IndirectBinding<TValue> Property<TValue>(string propertyName, bool
 	/// <param name="obj">INotifyPropertyChanged object to attach the event handler to</param>
 	/// <param name="propertyName">Name of the property to trigger the changed event.</param>
 	/// <param name="eh">Event handler delegate to trigger when the specified property changes</param>
-	/// <seealso cref="RemovePropertyEvent"/>
+	/// <seealso cref="RemovePropertyEvent(object,EventHandler{EventArgs})"/>
+	/// <seealso cref="RemovePropertyEvent(object,string,EventHandler{EventArgs})"/>
 	public static void AddPropertyEvent(object obj, string propertyName, EventHandler<EventArgs> eh)
 	{
-		var notifyObject = obj as INotifyPropertyChanged;
-		if (notifyObject != null)
-			new PropertyNotifyHelper(notifyObject, propertyName).Changed += eh;
+		if (obj is INotifyPropertyChanged notifyObject)
+		{
+			var helper = new PropertyNotifyHelper(notifyObject, propertyName);
+			helper.Changed += eh;
+		}
 	}
 
 	/// <summary>
@@ -208,32 +211,109 @@ public static void AddPropertyEvent(object obj, string propertyName, EventHandle
 	/// <param name="obj">INotifyPropertyChanged object to attach the event handler to</param>
 	/// <param name="propertyExpression">Expression to the property to trigger the changed event.</param>
 	/// <param name="eh">Event handler delegate to trigger when the specified property changes</param>
-	/// <seealso cref="RemovePropertyEvent"/>
+	/// <seealso cref="RemovePropertyEvent(object,EventHandler{EventArgs})"/>
+	/// <seealso cref="RemovePropertyEvent{T,TProperty}(T,Expression{Func{T, TProperty}},EventHandler{EventArgs})"/>
 	public static void AddPropertyEvent<T, TProperty>(T obj, Expression<Func<T, TProperty>> propertyExpression, EventHandler<EventArgs> eh)
 	{
-		var notifyObject = obj as INotifyPropertyChanged;
-		if (notifyObject != null)
+		var propertyInfo = propertyExpression.GetMemberInfo();
+		if (propertyInfo != null)
 		{
-			var propertyInfo = propertyExpression.GetMemberInfo();
-			if (propertyInfo != null)
-				new PropertyNotifyHelper(notifyObject, propertyInfo.Member.Name).Changed += eh;
+			AddPropertyEvent(obj, propertyInfo.Member.Name, eh);
 		}
 	}
 
 	/// <summary>
 	/// Removes an event handler previously attached with the AddPropertyEvent method.
 	/// </summary>
-	/// <param name="obj">INotifyPropertyChanged object to remove the event handler from</param>
+	/// <remarks>
+	/// Note that this will unsubscribe from all property handlers that point to the same delegate specified by <paramref name="eh"/>.
+	/// Use <see cref="RemovePropertyEvent(object, string, EventHandler{EventArgs})"/> to only unsubscribe for a single property.
+	/// </remarks>
+	/// <param name="obj">Object the event is subscribed to</param>
 	/// <param name="eh">Event handler delegate to remove</param>
 	/// <seealso cref="AddPropertyEvent(object,string,EventHandler{EventArgs})"/>
 	public static void RemovePropertyEvent(object obj, EventHandler<EventArgs> eh)
 	{
-		var helper = eh.Target as PropertyNotifyHelper;
-		if (helper != null)
+		if (obj is PropertyNotifyHelper helper)
 		{
 			helper.Changed -= eh;
 			helper.Unregister(obj);
 		}
+		else if (obj is INotifyPropertyChanged notifyObject)
+		{
+			var propertyChangedField = GetPropertyChangedField(notifyObject);
+			if (propertyChangedField == null)
+				return;
+
+			var propertyChangedDelegates = ((Delegate)propertyChangedField.GetValue(notifyObject))?.GetInvocationList().OfType<PropertyChangedEventHandler>() ?? Enumerable.Empty<PropertyChangedEventHandler>();
+			foreach (var del in propertyChangedDelegates)
+			{
+				// find ones hooked up to the PropertyNotifyHelper, regardless of property
+				if (del.Target is PropertyNotifyHelper h && h.IsHookedTo(eh))
+				{
+					h.Changed -= eh;
+					h.Unregister(obj);
+				}
+			}
+		}
+	}
+
+	/// <summary>
+	/// Removes an event handler previously attached with the AddPropertyEvent method.
+	/// </summary>
+	/// <param name="obj">INotifyPropertyChanged object to unsubscribe from</param>
+	/// <param name="propertyExpression">Expression for the property to remove the event handler for</param>
+	/// <param name="eh">Event handler delegate to remove</param>
+	/// <seealso cref="AddPropertyEvent{T,TProperty}(T,Expression{Func{T, TProperty}},EventHandler{EventArgs})"/>
+	public static void RemovePropertyEvent<T, TProperty>(T obj, Expression<Func<T, TProperty>> propertyExpression, EventHandler<EventArgs> eh)
+	{
+		var propertyInfo = propertyExpression.GetMemberInfo();
+		if (propertyInfo != null)
+		{
+			RemovePropertyEvent(obj, propertyInfo.Member.Name, eh);
+		}
+	}
+
+	/// <summary>
+	/// Removes an event handler previously attached with the AddPropertyEvent method.
+	/// </summary>
+	/// <param name="obj">INotifyPropertyChanged object to unsubscribe from</param>
+	/// <param name="propertyName">Name of the property to remove the event handler for</param>
+	/// <param name="eh">Event handler delegate to remove</param>
+	/// <seealso cref="AddPropertyEvent(object,string,EventHandler{EventArgs})"/>
+	public static void RemovePropertyEvent(object obj, string propertyName, EventHandler<EventArgs> eh)
+	{
+		if (obj is INotifyPropertyChanged notifyObject)
+		{
+			// this only works when PropertyChanged is a simple public event
+			// if it is implemented explicitly via the interface this won't work
+			var propertyChangedField = GetPropertyChangedField(notifyObject);
+			if (propertyChangedField == null)
+				return;
+
+			var propertyChangedDelegates = ((Delegate)propertyChangedField.GetValue(notifyObject))?.GetInvocationList().OfType<PropertyChangedEventHandler>() ?? Enumerable.Empty<PropertyChangedEventHandler>();
+			foreach (var del in propertyChangedDelegates)
+			{
+				// find ones hooked up to the PropertyNotifyHelper
+				if (del.Target is PropertyNotifyHelper h && h.PropertyName == propertyName && h.IsHookedTo(eh))
+				{
+					h.Changed -= eh;
+					h.Unregister(obj);
+				}
+			}
+		}
+	}
+
+	static FieldInfo GetPropertyChangedField(object obj)
+	{
+		FieldInfo field = null;
+		var type = obj?.GetType();
+		while (field == null && type != null)
+		{
+			field = type.GetField(nameof(INotifyPropertyChanged.PropertyChanged), BindingFlags.Instance | BindingFlags.NonPublic);
+			type = type.BaseType;
+		}
+		return field;
 	}
 
 	/// <summary>
@@ -289,4 +369,4 @@ public static void ExecuteCommand<T>(object dataContext, Expression<Func<T, ICom
 	{
 		ExecuteCommand(dataContext, Binding.Property(commandExpression), parameter);
 	}
-}
+}

src/Eto/Forms/Binding/DelegateBinding.cs

@@ -221,8 +221,9 @@ public class DelegateBinding<T, TValue> : IndirectBinding<TValue>
 		DefaultSetValue = defaultSetValue;
 		if (!string.IsNullOrEmpty(notifyProperty))
 		{
-			AddChangeEvent = (obj, eh) => { AddPropertyEvent(obj, notifyProperty, eh); return obj; };
-			RemoveChangeEvent = (obj, eh) => RemovePropertyEvent(obj, eh);
+			var binding = new PropertyBinding<object>(notifyProperty);
+			AddChangeEvent = (obj, eh) => binding.AddValueChangedHandler(obj, eh);
+			RemoveChangeEvent = (obj, eh) => binding.RemoveValueChangedHandler(obj, eh);
 		}
 	}
 

src/Eto/Forms/Binding/PropertyNotifyHelper.cs

@@ -4,15 +4,15 @@ namespace Eto.Forms;
 /// Helper to turn a property changed event to an EventHandler for binding
 /// </summary>
 /// <remarks>
-/// Use <see cref="Binding.AddPropertyEvent"/> and <see cref="Binding.RemovePropertyEvent"/> to access
-/// this functionality.
+/// Use <see cref="Binding.AddPropertyEvent"/> and <see cref="Binding.RemovePropertyEvent(object,string,EventHandler{EventArgs})"/> to access
+/// this functionality, or better yet use the <see cref="PropertyBinding{T}"/> class.
 /// </remarks>
 class PropertyNotifyHelper
 {
 	public string PropertyName { get; private set; }
 
 	public event EventHandler<EventArgs> Changed;
-
+	
 	public PropertyNotifyHelper(INotifyPropertyChanged obj, string propertyName)
 	{
 		PropertyName = propertyName;
@@ -35,4 +35,15 @@ void obj_PropertyChanged(object sender, PropertyChangedEventArgs e)
 		}
 	}
 
+	public bool IsHookedTo(EventHandler<EventArgs> eh)
+	{
+		foreach (var invocation in Changed.GetInvocationList())
+		{
+			if (invocation == (Delegate)eh)
+			{
+				return true;
+			}
+		}
+		return false;
+	}
 }

test/Eto.Test/UnitTests/Forms/Bindings/BindingHelpersTests.cs

@@ -0,0 +1,197 @@
+using NUnit.Framework;
+
+namespace Eto.Test.UnitTests.Forms.Bindings;
+
+[TestFixture]
+public class BindingHelpersTests
+{
+	[Test]
+	public void AddingAPropertyEventByNameShouldWork()
+	{
+		var propertyValueChanged = false;
+		void Handler(object sender, EventArgs e) => propertyValueChanged = true;
+
+		var bindObject = new BindObject { BoolProperty = true };
+		Binding.AddPropertyEvent(bindObject, "BoolProperty", Handler);
+
+		// Act
+		bindObject.BoolProperty = false;
+
+		// Assert
+		Assert.That(propertyValueChanged, Is.True);
+	}
+
+	[Test]
+	public void AddingAPropertyEventByExpressionShouldWork()
+	{
+		var propertyValueChanged = false;
+		void Handler(object sender, EventArgs e) => propertyValueChanged = true;
+
+		var bindObject = new BindObject { BoolProperty = true };
+		Binding.AddPropertyEvent(bindObject, obj => obj.BoolProperty, Handler);
+
+		// Act
+		bindObject.BoolProperty = false;
+
+		// Assert
+		Assert.That(propertyValueChanged, Is.True);
+	}
+
+	[Test]
+	public void AddingMultiplePropertyEventsShouldWork()
+	{
+		var boolPropertyValueChanged = false;
+		var intPropertyValueChanged = false;
+		var stringPropertyValueChanged = false;
+		void BoolHandler(object sender, EventArgs e) => boolPropertyValueChanged = true;
+		void IntHandler(object sender, EventArgs e) => intPropertyValueChanged = true;
+		void StringHandler(object sender, EventArgs e) => stringPropertyValueChanged = true;
+
+		var bindObject = new BindObject { BoolProperty = true, IntProperty = 3, StringProperty = "Test1" };
+		Binding.AddPropertyEvent(bindObject, obj => obj.BoolProperty, BoolHandler);
+		Binding.AddPropertyEvent(bindObject, obj => obj.IntProperty, IntHandler);
+		Binding.AddPropertyEvent(bindObject, obj => obj.StringProperty, StringHandler);
+
+		// Act
+		bindObject.BoolProperty = false;
+		bindObject.IntProperty = 4;
+		bindObject.StringProperty = "Test2";
+
+		// Assert
+		Assert.That(boolPropertyValueChanged, Is.True);
+		Assert.That(intPropertyValueChanged, Is.True);
+		Assert.That(stringPropertyValueChanged, Is.True);
+	}
+
+	[Test]
+	public void APropertyEventShouldNotRespondToADifferentProperty()
+	{
+		var propertyValueChanged = false;
+		void Handler(object sender, EventArgs e) => propertyValueChanged = true;
+
+		var bindObject = new BindObject { BoolProperty = true, IntProperty = 1 };
+		Binding.AddPropertyEvent(bindObject, obj => obj.BoolProperty, Handler);
+
+		// Act
+		bindObject.IntProperty = 2;
+
+		// Assert
+		Assert.That(propertyValueChanged, Is.False);
+	}
+
+	[Test]
+	public void RemovingAPropertyEventShouldWork()
+	{
+		var propertyValueChanged = false;
+		void Handler(object sender, EventArgs e) => propertyValueChanged = true;
+
+		var bindObject = new BindObject { BoolProperty = true };
+		Binding.AddPropertyEvent(bindObject, obj => obj.BoolProperty, Handler);
+
+		// Act
+		Binding.RemovePropertyEvent(bindObject, Handler);
+		bindObject.BoolProperty = false;
+
+		// Assert
+		Assert.That(propertyValueChanged, Is.False);
+	}
+
+	[Test]
+	public void RemovingAPropertyEventShouldKeepOtherEvents()
+	{
+		var boolPropertyValueChanged = false;
+		var intPropertyValueChanged = false;
+		var stringPropertyValueChanged = false;
+		void BoolHandler(object sender, EventArgs e) => boolPropertyValueChanged = true;
+		void IntHandler(object sender, EventArgs e) => intPropertyValueChanged = true;
+		void StringHandler(object sender, EventArgs e) => stringPropertyValueChanged = true;
+
+		var bindObject = new BindObject { BoolProperty = true, IntProperty = 3, StringProperty = "Test1" };
+		Binding.AddPropertyEvent(bindObject, obj => obj.BoolProperty, BoolHandler);
+		Binding.AddPropertyEvent(bindObject, obj => obj.IntProperty, IntHandler);
+		Binding.AddPropertyEvent(bindObject, obj => obj.StringProperty, StringHandler);
+
+		// Act
+		Binding.RemovePropertyEvent(bindObject, IntHandler);
+		bindObject.BoolProperty = false;
+		bindObject.IntProperty = 4;
+		bindObject.StringProperty = "Test2";
+
+		// Assert
+		Assert.That(boolPropertyValueChanged, Is.True);
+		Assert.That(intPropertyValueChanged, Is.False);
+		Assert.That(stringPropertyValueChanged, Is.True);
+	}
+
+	[Test]
+	public void RemovingAllPropertyEventsShouldWork()
+	{
+		var boolPropertyValueChanged = false;
+		var intPropertyValueChanged = false;
+		var stringPropertyValueChanged = false;
+		void BoolHandler(object sender, EventArgs e) => boolPropertyValueChanged = true;
+		void IntHandler(object sender, EventArgs e) => intPropertyValueChanged = true;
+		void StringHandler(object sender, EventArgs e) => stringPropertyValueChanged = true;
+
+		var bindObject = new BindObject { BoolProperty = true, IntProperty = 3, StringProperty = "Test1" };
+		Binding.AddPropertyEvent(bindObject, obj => obj.BoolProperty, BoolHandler);
+		Binding.AddPropertyEvent(bindObject, obj => obj.IntProperty, IntHandler);
+		Binding.AddPropertyEvent(bindObject, obj => obj.StringProperty, StringHandler);
+
+		// Act
+		Binding.RemovePropertyEvent(bindObject, StringHandler);
+		Binding.RemovePropertyEvent(bindObject, BoolHandler);
+		Binding.RemovePropertyEvent(bindObject, IntHandler);
+		bindObject.BoolProperty = false;
+		bindObject.IntProperty = 4;
+		bindObject.StringProperty = "Test2";
+
+		// Assert
+		Assert.That(boolPropertyValueChanged, Is.False);
+		Assert.That(intPropertyValueChanged, Is.False);
+		Assert.That(stringPropertyValueChanged, Is.False);
+	}
+
+	[Test]
+	public void UnsubscribingToSameEventShouldntUnsubscribeAllWhenPassingProperty()
+	{
+		var numchanged = 0;
+		void Handler(object sender, EventArgs e) => numchanged++;
+
+		var bindObject = new BindObject { BoolProperty = true, IntProperty = 3, StringProperty = "Test1" };
+		Binding.AddPropertyEvent(bindObject, obj => obj.BoolProperty, Handler);
+		Binding.AddPropertyEvent(bindObject, obj => obj.IntProperty, Handler);
+		Binding.AddPropertyEvent(bindObject, obj => obj.StringProperty, Handler);
+
+		// Act
+		Binding.RemovePropertyEvent(bindObject, obj => obj.BoolProperty, Handler);
+		bindObject.BoolProperty = false;
+		bindObject.IntProperty = 4;
+		bindObject.StringProperty = "Test2";
+
+		// Assert
+		Assert.That(numchanged, Is.EqualTo(2));
+	}
+
+	[Test]
+	public void UnsubscribingToSameEventWillUnsubscribeAll()
+	{
+		var numchanged = 0;
+		void Handler(object sender, EventArgs e) => numchanged++;
+
+		var bindObject = new BindObject { BoolProperty = true, IntProperty = 3, StringProperty = "Test1" };
+		Binding.AddPropertyEvent(bindObject, obj => obj.BoolProperty, Handler);
+		Binding.AddPropertyEvent(bindObject, obj => obj.IntProperty, Handler);
+		Binding.AddPropertyEvent(bindObject, obj => obj.StringProperty, Handler);
+
+		// Act
+		Binding.RemovePropertyEvent(bindObject, Handler);
+		bindObject.BoolProperty = false;
+		bindObject.IntProperty = 4;
+		bindObject.StringProperty = "Test2";
+
+		// Assert
+		// ambiguous which one to remove, so it removed all -- need to specify property or return value from AddPropertyEvent
+		Assert.That(numchanged, Is.EqualTo(0));
+	}
+}