Reduce likelihood of binding leaks created by `PropertyDescriptor` fallback

[Nintynuts]

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. 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:

wpf/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/PropertyPath.cs

Lines 619 to 627 in 42f7d5d

	// 4. PropertyDescriptor (obtain from item - this is reputedly
	// slower than obtaining from type, but the latter doesn't
	// discover properties obtained from TypeDescriptorProvider -
	// see bug 1713000).
	// This supports the WinForms ValueChanged pattern.
	if (accessor == null && item != null)
	{
	accessor = TypeDescriptor.GetProperties(item)[propertyName];
	}

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.

[miloush]

It is a little bit unclear to me what exactly is being asked here, but the property descriptor can decide whenever it wants to announce a property change. Even when it represents an actual property, properties don't need to be settable in order to change.

[Nintynuts]

Please correct me if I'm wrong, but as I understand it the only way the PropertyDescriptor is able to notify about changes (if there's no WinForms ValueChanged event) is when more than one thing accesses the property via that descriptor and uses SetValue to change it, which can only work if the property is writable (as it's an external change).

[miloush]

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Threading;
using System.Windows;
using System.Windows.Controls;

class Program
{
    class NowTypeDescriptorProvider : TypeDescriptionProvider
    {
        public override ICustomTypeDescriptor GetTypeDescriptor(Type objectType, object instance) => new NowTypeDescriptor();
    }

    class NowTypeDescriptor : ICustomTypeDescriptor
    {
        public AttributeCollection GetAttributes() => default;
        public string GetClassName() => default;
        public string GetComponentName() => default;
        public TypeConverter GetConverter() => default;
        public EventDescriptor GetDefaultEvent() => default;
        public PropertyDescriptor GetDefaultProperty() => default;
        public object GetEditor(Type editorBaseType) => default;
        public EventDescriptorCollection GetEvents() => default;
        public EventDescriptorCollection GetEvents(Attribute[] attributes) => default;
        public PropertyDescriptorCollection GetProperties() => new PropertyDescriptorCollection([new NowPropertyDescriptor()]);
        public PropertyDescriptorCollection GetProperties(Attribute[] attributes) => default;
        public object GetPropertyOwner(PropertyDescriptor pd) => default;
    }

    class NowPropertyDescriptor : PropertyDescriptor
    {
        Dictionary<NowType, Timer> _timers = new();

        public NowPropertyDescriptor() : base("Now", []) { }
        private void OnTimerTick(object state) => OnValueChanged(state, EventArgs.Empty);
        public override Type ComponentType => typeof(NowType);
        public override bool IsReadOnly => true;
        public override Type PropertyType => typeof(DateTime);
        public override bool CanResetValue(object component) => false;
        public override object GetValue(object component)
        {
            NowType now = (NowType)component;
            if (!_timers.ContainsKey(now))
                _timers[now] = new Timer(OnTimerTick, now, 1000, 1000);

            return ((NowType)component).Now;
        }
        public override void ResetValue(object component) { }
        public override void SetValue(object component, object value) { }
        public override bool ShouldSerializeValue(object component) => false;
    }

    class NowType
    {
        public DateTime Now => DateTime.Now;
    }

    [STAThread]
    static void Main(string[] args)
    {
        TypeDescriptor.AddProvider(new NowTypeDescriptorProvider(), typeof(NowType));

        TextBlock t = new TextBlock();
        t.SetBinding(TextBlock.TextProperty, "Now");
        t.DataContext = new NowType();

        new Application().Run(new Window { Content = t });
    }
}

[Nintynuts]

I noticed the property path code also referred to ICustomTypeDescriptor, but I didn't think that was relevant in the case of most situations where you just want to bind to a read-only property and not cause a 'binding leak'. Is this a workaround to prevent it using a leaky PropertyDescriptor? It might be a short term solution if I can source generate it for model types, but it's a shame we need a workaround at all.

[MichaeIDietrich]

I wish there was an app switch to completely disable the support for TypeDescriptor in WPF.
This feature is so subtle, most devs are probably not even aware it exists, still it is a common cause of memory leaks in WPF.
Generally disabling it to not accidentally step into memory leaks would be great IMO.