Skip to content

Commit

Permalink
Fix an event handler leak in Binding.RemovePropertyEvent
Browse files Browse the repository at this point in the history 
Removing the handler in `RemovePropertyEvent` doesn't work, as the
`Target` of the handler is not the `PropertyNotifyHelper` instance; it
is the object that the handler is bound to.

Instead, find the correct handler to remove by checking all invocations
of the `PropertyChanged` event of the bound object, and comparing the
internal handler of the invocation with the method's argument.

Also, add unit tests to verify the correct behavior.
  • Loading branch information
@joerih
joerih committed Apr 16, 2024
1 parent 4721256 commit 19baf47
Show file tree
Hide file tree
Showing 3 changed files with 195 additions and 53 deletions.
56 changes: 41 additions & 15 deletions src/Eto/Forms/Binding/Binding.helpers.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -192,9 +192,10 @@ public static IndirectBinding<TValue> Property<TValue>(string propertyName, bool
/// <seealso cref="RemovePropertyEvent"/>
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 not INotifyPropertyChanged notifyObject)
return;

notifyObject.PropertyChanged += (s, e) => OnPropertyChanged(s, e, propertyName, eh);
}

/// <summary>
Expand All @@ -211,13 +212,11 @@ public static void AddPropertyEvent(object obj, string propertyName, EventHandle
/// <seealso cref="RemovePropertyEvent"/>
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)
new PropertyNotifyHelper(notifyObject, propertyInfo.Member.Name).Changed += eh;
}
var propertyInfo = propertyExpression.GetMemberInfo();
if (propertyInfo == null)
return;

AddPropertyEvent(obj, propertyInfo.Member.Name, eh);
}

/// <summary>
Expand All @@ -228,12 +227,39 @@ public static void AddPropertyEvent<T, TProperty>(T obj, Expression<Func<T, TPro
/// <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 not INotifyPropertyChanged notifyObject)
return;

var propertyChangedField = GetPropertyChangedField(notifyObject);
if (propertyChangedField == null)
return;

var propertyChangedDelegates = ((Delegate)propertyChangedField.GetValue(notifyObject))?.GetInvocationList().OfType<PropertyChangedEventHandler>() ?? Enumerable.Empty<PropertyChangedEventHandler>();
var delegateToRemove = propertyChangedDelegates.FirstOrDefault(d => d.Target?.GetType().GetField("eh")?.GetValue(d.Target)?.Equals(eh) ?? false);
if (delegateToRemove == null)
return;

notifyObject.PropertyChanged -= delegateToRemove;
}

static void OnPropertyChanged(object sender, PropertyChangedEventArgs args, string propertyName, EventHandler<EventArgs> handler)
{
if (args.PropertyName != propertyName)
return;

handler?.Invoke(sender, EventArgs.Empty);
}

static FieldInfo GetPropertyChangedField(object obj)
{
FieldInfo field = null;
var type = obj?.GetType();
while (field == null && type != null)
{
helper.Changed -= eh;
helper.Unregister(obj);
field = type.GetField(nameof(INotifyPropertyChanged.PropertyChanged), BindingFlags.Instance | BindingFlags.NonPublic);
type = type.BaseType;
}
return field;
}

/// <summary>
Expand Down Expand Up @@ -289,4 +315,4 @@ public static void ExecuteCommand<T>(object dataContext, Expression<Func<T, ICom
{
ExecuteCommand(dataContext, Binding.Property(commandExpression), parameter);
}
}
}
38 changes: 0 additions & 38 deletions src/Eto/Forms/Binding/PropertyNotifyHelper.cs
View file

This file was deleted.

154 changes: 154 additions & 0 deletions test/Eto.Test/UnitTests/Forms/Bindings/BindingHelpersTests.cs
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,154 @@
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);
}
}

0 comments on commit 19baf47

Please sign in to comment.