Reduce likelihood of binding leaks created by `PropertyDescriptor` fallback
- Dominant language
- C#
- Stars
- 7.7k
- Forks
- 1.3k
- Avg merge
- 1d 11h
- Merged PRs (30d)
- 61
Description
In WPF you can bind to a control's `DependencyProperty` or a property on a view model (implementing `INotifyPropertyChanged`), which are fine, but if you bind to a property on an immutable model (not implementing `INotifyPropertyChanged`) it falls back to using a `PropertyDescriptor.AddValueChanged` listener, which causes a binding leak (reported by Snoop, documented [here](https://blog.jetbrains.com/dotnet/2014/09/04/fighting-common-wpf-memory-leaks-with-dotmemory/#binding-leak). As far as I can tell, this only makes some sense if it follows the WinForms pattern (a specific change event is present) or the property is writable (so multiple controls setting the value via the `PropertyDescriptor` will be notified about those changes, but not when changed directly in M/VM code).
Here's where the `PropertyDescriptor` accessor is chosen:
https://github.com/dotnet/wpf/blob/42f7d5d89b211409b9c11a294bfcd78cbe748664/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/PropertyPath.cs#L619-L627
I've only recently become aware of this issue, but clearly it's been around a long time, and I wonder if it can be mostly fixed just by adding some checks before choosing to use a `PropertyDescriptor.AddValueChanged` listener, firstly looking for the WinForms event, and secondly checking for a public setter on the `PropertyInfo` for the instance being accessed. If neither of these are present, then it seems better to fall through to the `PropertyInfo` option below, which hopefully makes it effectively a one-time binding, but that can still be re-evaluated if a parent property that does notify changes.
I'm sure there are people who know this part of the framework much better than I, so please let me know if there is a fundamental problem with my suggestion, or if there is a better solution for avoiding this problem without requiring everything to implement `INotifyPropertyChanged`.
Contributor guide
Assessment
This issue has not been assessed yet.