Fix an event handler leak in `Binding.RemovePropertyEvent`

[joerih]

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.

fixes #2446

[cwensley]

Hey @joerih, thanks for submitting the fix and especially for the unit tests.

[joerih]

@cwensley Hi Curtis. Is there any chance this fix can be included in the upcoming release?

[cwensley]

Hey @joerih, sorry for the late response. There are a few problems I noticed with this, namely the existing API is actually not complete enough to do this properly, as calling RemovePropertyEvent for the same method (but different properties) would remove it for all properties, not just the one you specify (even though your implementation only removes the first one).

This may mean an API break to return a value for AddPropertyEvent, or possibly by adding a AddPropertyEventEx that returns an object that you would then pass to RemovePropertyEvent (this was actually the original intent of these methods, as it is done like this internally in some cases in Eto).

I've done some changes locally for this and it seems to work. I'll try to push some changes for you to review shortly.

[cwensley]

Ok I've added some changes. Please take a look to see if I've missed anything. In particular, the UnsubscribingToSameEventWillUnsubscribeAll test would not work as expected previously, but now will remove all bindings to that delegate. I've changed AddPropertyEvent and added a few new helper APIs:

~object AddPropertyEvent(object obj, string propertyName, EventHandler<EventArgs> eh)
~object AddPropertyEvent<T, TProperty>(T obj, Expression<Func<T, TProperty>> propertyExpression, EventHandler<EventArgs> eh)
+RemovePropertyEvent<T, TProperty>(T obj, Expression<Func<T, TProperty>> propertyExpression, EventHandler<EventArgs> eh)
+RemovePropertyEvent(object obj, string propertyName, EventHandler<EventArgs> eh)

The return value for AddPropertyEvent can be passed into RemovePropertyEvent(object, EventHandler<EventArgs>) to specifically remove that binding, or you can use one of the other RemovePropertyEvent that specifies the property so it only removes the binding for that particular property.

E.g. one would do something like this:

var boolObj = Binding.AddPropertyEvent(bindObject, obj => obj.BoolProperty, Handler);
// event is active
Binding.RemovePropertyEvent(boolObj, Handler);
// event is inactive

This is also totally valid:

Binding.AddPropertyEvent(bindObject, obj => obj.BoolProperty, Handler);
// event is active
Binding.RemovePropertyEvent(bindObject, obj => obj.BoolProperty, Handler);
// event is inactive

The previous behaviour that you fixed up would still work:

Binding.AddPropertyEvent(bindObject, "MyProperty", Handler);
// event is active
Binding.RemovePropertyEvent(bindObject, Handler);
// event is inactive

However it was ambiguous in this scenario:

Binding.AddPropertyEvent(bindObject, "MyProperty", Handler);
Binding.AddPropertyEvent(bindObject, "MyOtherProperty", Handler);
// MyProperty and MyOtherProperty event is active
Binding.RemovePropertyEvent(bindObject, Handler);
// MyProperty event is inactive.  Now with the added changes both property change events are inactive.

[joerih]

@cwensley Hi Curtis. Thanks for your response and for improving the fix.

Indeed, I struggled to get this working without changing the API - something I didn't want to do because I was not sure about the consequences. But this way it is even better, as far as I can tell it should cover all scenarios.

[cwensley]

Indeed, I struggled to get this working without changing the API - something I didn't want to do because I was not sure about the consequences. But this way it is even better, as far as I can tell it should cover all scenarios.

Yeah, I am hesitant to make ABI breaking changes as well. I am thinking instead to obsolete the RemovePropertyEvent(object obj, EventHandler<EventArgs> eh) API as it is ambiguous, and require you to specify the property when removing it. We could then change the signature of the AddPropertyEvent back to its original state as there would be no need to retain the return value.

[joerih]

I am thinking instead to obsolete the RemovePropertyEvent(object obj, EventHandler eh) API as it is ambiguous, and require you to specify the property when removing it. We could then change the signature of the AddPropertyEvent back to its original state as there would be no need to retain the return value.

Yes, that would also work well I think. At the moment we implemented a workaround to avoid the memory leak in our application, and it would not be a problem to switch to a new API when we remove this workaround in the future.