PropertyDescriptor.AddValueChanged is not thread-safe

[KalleOlaviNiemitalo]

Description

In the System.ComponentModel.PropertyDescriptor class, the AddValueChanged and RemoveValueChanged methods mutate private Dictionary<object, EventHandler?>? _valueChangedHandlers without locking. This causes errors if multiple threads add or remove value-changed event handlers for different components in parallel. Because TypeDescriptor caches the PropertyDescriptor instances, it is normal that multiple threads use the same PropertyDescriptor instance.

Reproduction Steps

This demo program attempts to trigger the bug in one of three ways, depending on the command line:

• PropertyDescriptor: Hits the bug, but might not be a realistic scenario.
• BindingSource: Hits the bug, but BindingSource is defined in Windows Forms and would typically be used from a UI thread only. In the Debug configuration, the dictionary corruption causes an assertion failure before the InvalidOperationException.
• BindingList<T>: Doesn't hit the bug because it checks for INotifyPropertyChange, rather than adding event handlers via PropertyDescriptor.

ConsoleApp1.csproj

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFrameworks>net48;net8.0-windows;net9.0-windows</TargetFrameworks>
    <UseWindowsForms>true</UseWindowsForms>

    <DefineConstants Condition="$(UseWindowsForms) == 'true'">$(DefineConstants);UseWindowsForms</DefineConstants>
  </PropertyGroup>

  <ItemGroup Condition="$(TargetFramework) == 'net48'">
    <Reference Include="System.Windows.Forms" Condition="$(UseWindowsForms) == 'true'" />
  </ItemGroup>

</Project>

Program.cs

using System;
using System.ComponentModel;
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Diagnostics;
using System.IO;
using System.Threading.Tasks;

#if UseWindowsForms
using System.Windows.Forms;
#endif

namespace ConsoleApp1
{
    internal static class Program
    {
        private static int Main(string[] args)
        {
            if (args.Length != 2
                || !long.TryParse(args[1], out long count))
            {
                ShowUsage(Console.Error);
                return 1;
            }

            Console.WriteLine("Initial check.");
            PropertyDescriptor property = TypeDescriptor.GetDefaultProperty(componentType: typeof(Notifier));
            Debug.Assert(property.SupportsChangeEvents, "Must support change events.");

            switch (args[0])
            {
                case "BindingList":
                    // No problem.
                    Console.WriteLine("Parallel looping with BindingList...");
                    Parallel.For(0, count, _ => PokeViaBindingList());
                    Console.WriteLine("Finished.");
                    break;

                case "BindingSource":
#if UseWindowsForms
                    Console.WriteLine("Parallel looping with BindingSource...");
                    Parallel.For(0, count, _ => PokeViaBindingSource());
                    Console.WriteLine("Finished.");
                    break;
#else
                    Console.Error.WriteLine("BindingSource requires Windows Forms.");
                    return 1;
#endif

                case "PropertyDescriptor":
                    // Not thread-safe.
                    Console.WriteLine("Parallel looping with PropertyDescriptor...");
                    Parallel.For(0, count, _ => PokeViaPropertyDescriptor());
                    Console.WriteLine("Finished.");
                    break;

                default:
                    ShowUsage(Console.Error);
                    return 1;
            }

            return 0;
        }

        private static void ShowUsage(TextWriter textWriter)
        {
            string appName = System.Reflection.Assembly.GetEntryAssembly().GetName().Name;
            textWriter.WriteLine($"Usage: {appName} (BindingList | BindingSource | PropertyDescriptor) count");
        }

        // No problems here.
        private static void PokeViaBindingList()
        {
            BindingList<Notifier> bindingList = new BindingList<Notifier>();
            Debug.Assert(((IRaiseItemChangedEvents)bindingList).RaisesItemChangedEvents, "Must raise ItemChanged events.");

            int itemChangeCount = 0;
            ListChangedEventHandler listChangedHandler = (sender, e) =>
            {
                if (e.ListChangedType == ListChangedType.ItemChanged)
                {
                    itemChangeCount++;
                }
            };
            bindingList.ListChanged += listChangedHandler;

            Notifier notifier = new Notifier();
            bindingList.Add(notifier);
            Debug.Assert(itemChangeCount == 0, "Expected itemChangeCount == 0");
            notifier.Number = 42;
            Debug.Assert(itemChangeCount == 1, "Expected itemChangeCount == 1");

            bindingList.ListChanged -= listChangedHandler;
            bindingList.Clear();
        }

#if UseWindowsForms
        private static void PokeViaBindingSource()
        {
            using (BindingSource bindingSource = new BindingSource())
            {
                // Because List<T> implements neither IRaiseItemChangedEvents nor IBindingList,
                // BindingSource will hook the property-change events of the current item only.
                bindingSource.DataSource = new List<Notifier>();

                bindingSource.CurrencyManager.PositionChanged += (sender, e) => { };

                int itemChangeCount = 0;
                ListChangedEventHandler listChangedHandler = (sender, e) =>
                {
                    if (e.ListChangedType == ListChangedType.ItemChanged)
                    {
                        itemChangeCount++;
                    }
                };
                bindingSource.ListChanged += listChangedHandler;

                Notifier notifier = new Notifier();
                bindingSource.Add(notifier);

                Debug.Assert(bindingSource.Current == notifier, "The item should have become current");
                Debug.Assert(itemChangeCount == 0, "Expected itemChangeCount == 0", $"Is actually {itemChangeCount}");
                notifier.Number = 42;
                Debug.Assert(itemChangeCount == 1, "Expected itemChangeCount == 1", $"Is actually {itemChangeCount}");

                bindingSource.ListChanged -= listChangedHandler;
            }
        }
#endif // UseWindowsForms

        private static void PokeViaPropertyDescriptor()
        {
            Notifier component = new Notifier();
            PropertyDescriptor property = TypeDescriptor.GetDefaultProperty(component: component);
            Debug.Assert(property.SupportsChangeEvents, "Must support change events.");

            EventHandler valueChangedHandler = (sender, e) => { };
            property.AddValueChanged(component, valueChangedHandler);
            property.RemoveValueChanged(component, valueChangedHandler);
        }
    }

    [DefaultProperty(nameof(Number))]
    internal sealed class Notifier : INotifyPropertyChanged
    {
        private int number;

        public int Number
        {
            get => this.number;

            set
            {
                this.number = value;
                this.PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(this.Number)));
            }
        }

        public event PropertyChangedEventHandler PropertyChanged;
    }
}

Run

dotnet run --configuration=Release --framework=net9.0-windows -- PropertyDescriptor 1000000

Expected behavior

Should not throw any exceptions. Indeed it doesn't throw, if the parallel loop count is only 1.

Initial check.
Parallel looping with PropertyDescriptor...
Finished.

Actual behavior

Throws InvalidOperationException because invalid parallel access corrupts the dictionary.

Initial check.
Parallel looping with PropertyDescriptor...
Unhandled exception. System.AggregateException: One or more errors occurred. (Operations that change non-concurrent collections must have exclusive access. A concurrent update was performed on this collection and corrupted its state. The collection's state is no longer correct.) (Operations that change non-concurrent collections must have exclusive access. A concurrent update was performed on this collection and corrupted its state. The collection's state is no longer correct.)
 ---> System.InvalidOperationException: Operations that change non-concurrent collections must have exclusive access. A concurrent update was performed on this collection and corrupted its state. The collection's state is no longer correct.
   at System.Collections.Generic.Dictionary`2.FindValue(TKey key)
   at System.Collections.Generic.Dictionary`2.TryGetValue(TKey key, TValue& value)
   at System.Collections.Generic.CollectionExtensions.GetValueOrDefault[TKey,TValue](IReadOnlyDictionary`2 dictionary, TKey key, TValue defaultValue)
   at System.ComponentModel.ReflectPropertyDescriptor.AddValueChanged(Object component, EventHandler handler)
   at ConsoleApp1.Program.PokeViaPropertyDescriptor() in [REDACTED]\ConsoleApp1\Program.cs:line 136
   at ConsoleApp1.Program.<>c.<Main>b__0_2(Int64 _) in [REDACTED]\ConsoleApp1\Program.cs:line 53
   at System.Threading.Tasks.Parallel.<>c__DisplayClass19_0`2.<ForWorker>b__1(RangeWorker& currentWorker, Int64 timeout, Boolean& replicationDelegateYieldedBeforeCompletion)
--- End of stack trace from previous location ---
   at System.Threading.Tasks.Parallel.<>c__DisplayClass19_0`2.<ForWorker>b__1(RangeWorker& currentWorker, Int64 timeout, Boolean& replicationDelegateYieldedBeforeCompletion)
   at System.Threading.Tasks.TaskReplicator.Replica.Execute()
   --- End of inner exception stack trace ---
   at System.Threading.Tasks.TaskReplicator.Run[TState](ReplicatableUserAction`1 action, ParallelOptions options, Boolean stopOnFirstFailure)
   at System.Threading.Tasks.Parallel.ForWorker[TLocal,TInt](TInt fromInclusive, TInt toExclusive, ParallelOptions parallelOptions, Action`1 body, Action`2 bodyWithState, Func`4 bodyWithLocal, Func`1 localInit, Action`1 localFinally)
--- End of stack trace from previous location ---
   at System.Threading.Tasks.Parallel.ForWorker[TLocal,TInt](TInt fromInclusive, TInt toExclusive, ParallelOptions parallelOptions, Action`1 body, Action`2 bodyWithState, Func`4 bodyWithLocal, Func`1 localInit, Action`1 localFinally)
   at System.Threading.Tasks.Parallel.For(Int64 fromInclusive, Int64 toExclusive, Action`1 body)
   at ConsoleApp1.Program.Main(String[] args) in [REDACTED]\ConsoleApp1\Program.cs:line 53
 ---> (Inner Exception #1) System.InvalidOperationException: Operations that change non-concurrent collections must have exclusive access. A concurrent update was performed on this collection and corrupted its state. The collection's state is no longer correct.
   at System.Collections.Generic.Dictionary`2.FindValue(TKey key)
   at System.Collections.Generic.Dictionary`2.TryGetValue(TKey key, TValue& value)
   at System.Collections.Generic.CollectionExtensions.GetValueOrDefault[TKey,TValue](IReadOnlyDictionary`2 dictionary, TKey key, TValue defaultValue)
   at System.ComponentModel.ReflectPropertyDescriptor.AddValueChanged(Object component, EventHandler handler)
   at ConsoleApp1.Program.PokeViaPropertyDescriptor() in [REDACTED]\ConsoleApp1\Program.cs:line 136
   at ConsoleApp1.Program.<>c.<Main>b__0_2(Int64 _) in [REDACTED]\ConsoleApp1\Program.cs:line 53
   at System.Threading.Tasks.Parallel.<>c__DisplayClass19_0`2.<ForWorker>b__1(RangeWorker& currentWorker, Int64 timeout, Boolean& replicationDelegateYieldedBeforeCompletion)
--- End of stack trace from previous location ---
   at System.Threading.Tasks.Parallel.<>c__DisplayClass19_0`2.<ForWorker>b__1(RangeWorker& currentWorker, Int64 timeout, Boolean& replicationDelegateYieldedBeforeCompletion)
   at System.Threading.Tasks.TaskReplicator.Replica.Execute()<---

Regression?

No, it doesn't work in .NET Framework either.

Known Workarounds

No response

Configuration

.NET 9.0.0-preview.3.24172.9 on Windows 10.

The thread-unsafety of PropertyDescriptor.AddValueChange is not specific to Windows. In the .NET Runtime however, only WPF and Windows Forms seem to call this method; that may make the bug less likely to affect applications on other operating systems.

Other information

Related to #30024 and #92394. The bug corrupts this dictionary:

runtime/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/PropertyDescriptor.cs

Line 19 in 9e6ba1f

private Dictionary<object, EventHandler?>? _valueChangedHandlers;

[KalleOlaviNiemitalo]

This should be area-System.ComponentModel.

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-componentmodel
See info in area-owners.md if you want to be subscribed.