dotnet / dotnet/wpf

Reduce likelihood of binding leaks created by `PropertyDescriptor` fallback

Open
#11,236 5 comments 2 reactions 0 assignees View on GitHub
Investigate
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.