[release/8.0] ComponentModel threading fixes (#107353)

Author: steveharter

src/libraries/System.ComponentModel.TypeConverter/src/System.ComponentModel.TypeConverter.csproj

@@ -253,6 +253,7 @@
                       SetTargetFramework="TargetFramework=netstandard2.0"
                       OutputItemType="Analyzer" />
     <Reference Include="System.Collections" />
+    <Reference Include="System.Collections.Concurrent" />
     <Reference Include="System.Collections.NonGeneric" />
     <Reference Include="System.Collections.Specialized" />
     <Reference Include="System.ComponentModel" />

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

@@ -2,9 +2,11 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System.Collections;
+using System.Collections.Concurrent;
 using System.Collections.Generic;
 using System.Diagnostics.CodeAnalysis;
 using System.Reflection;
+using System.Threading;
 
 namespace System.ComponentModel
 {
@@ -16,10 +18,11 @@ public abstract class PropertyDescriptor : MemberDescriptor
         internal const string PropertyDescriptorPropertyTypeMessage = "PropertyDescriptor's PropertyType cannot be statically discovered.";
 
         private TypeConverter? _converter;
-        private Dictionary<object, EventHandler?>? _valueChangedHandlers;
+        private ConcurrentDictionary<object, EventHandler?>? _valueChangedHandlers;
         private object?[]? _editors;
         private Type[]? _editorTypes;
         private int _editorCount;
+        private object? _syncObject;
 
         /// <summary>
         /// Initializes a new instance of the <see cref='System.ComponentModel.PropertyDescriptor'/> class with the specified name and
@@ -49,6 +52,8 @@ protected PropertyDescriptor(MemberDescriptor descr, Attribute[]? attrs) : base(
         {
         }
 
+        private object SyncObject => LazyInitializer.EnsureInitialized(ref _syncObject);
+
         /// <summary>
         /// When overridden in a derived class, gets the type of the
         /// component this property is bound to.
@@ -124,10 +129,11 @@ public virtual void AddValueChanged(object component, EventHandler handler)
             ArgumentNullException.ThrowIfNull(component);
             ArgumentNullException.ThrowIfNull(handler);
 
-            _valueChangedHandlers ??= new Dictionary<object, EventHandler?>();
-
-            EventHandler? h = _valueChangedHandlers.GetValueOrDefault(component, defaultValue: null);
-            _valueChangedHandlers[component] = (EventHandler?)Delegate.Combine(h, handler);
+            lock (SyncObject)
+            {
+                _valueChangedHandlers ??= new ConcurrentDictionary<object, EventHandler?>(concurrencyLevel: 1, capacity: 0);
+                _valueChangedHandlers.AddOrUpdate(component, handler, (k, v) => (EventHandler?)Delegate.Combine(v, handler));
+            }
         }
 
         /// <summary>
@@ -392,15 +398,18 @@ public virtual void RemoveValueChanged(object component, EventHandler handler)
 
             if (_valueChangedHandlers != null)
             {
-                EventHandler? h = _valueChangedHandlers.GetValueOrDefault(component, defaultValue: null);
-                h = (EventHandler?)Delegate.Remove(h, handler);
-                if (h != null)
-                {
-                    _valueChangedHandlers[component] = h;
-                }
-                else
+                lock (SyncObject)
                 {
-                    _valueChangedHandlers.Remove(component);
+                    EventHandler? h = _valueChangedHandlers.GetValueOrDefault(component, defaultValue: null);
+                    h = (EventHandler?)Delegate.Remove(h, handler);
+                    if (h != null)
+                    {
+                        _valueChangedHandlers[component] = h;
+                    }
+                    else
+                    {
+                        _valueChangedHandlers.Remove(component, out EventHandler? _);
+                    }
                 }
             }
         }

src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.cs

@@ -3,6 +3,7 @@
 
 using System.Collections;
 using System.Collections.Generic;
+using System.Collections.Concurrent;
 using System.ComponentModel.Design;
 using System.Diagnostics;
 using System.Diagnostics.CodeAnalysis;
@@ -22,10 +23,8 @@ namespace System.ComponentModel
     /// </summary>
     internal sealed partial class ReflectTypeDescriptionProvider : TypeDescriptionProvider
     {
-        // Hastable of Type -> ReflectedTypeData. ReflectedTypeData contains all
-        // of the type information we have gathered for a given type.
-        //
-        private Hashtable? _typeData;
+        // ReflectedTypeData contains all of the type information we have gathered for a given type.
+        private readonly ConcurrentDictionary<Type, ReflectedTypeData> _typeData = new ConcurrentDictionary<Type, ReflectedTypeData>();
 
         // This is the signature we look for when creating types that are generic, but
         // want to know what type they are dealing with. Enums are a good example of this;
@@ -91,8 +90,6 @@ private static Type[] InitializeSkipInterfaceAttributeList()
 
         internal static Guid ExtenderProviderKey { get; } = Guid.NewGuid();
 
-
-        private static readonly object s_internalSyncObject = new object();
         /// <summary>
         /// Creates a new ReflectTypeDescriptionProvider. The type is the
         /// type we will obtain type information for.
@@ -243,7 +240,7 @@ internal static void AddEditorTable(Type editorBaseType, Hashtable table)
 
             Debug.Assert(table != null, "COMPAT: Editor table should not be null"); // don't throw; RTM didn't so we can't do it either.
 
-            lock (s_internalSyncObject)
+            lock (TypeDescriptor.s_commonSyncObject)
             {
                 Hashtable editorTables = EditorTables;
                 if (!editorTables.ContainsKey(editorBaseType))
@@ -298,7 +295,6 @@ internal static void AddEditorTable(Type editorBaseType, Hashtable table)
             return obj ?? Activator.CreateInstance(objectType, args);
         }
 
-
         /// <summary>
         /// Helper method to create editors and type converters. This checks to see if the
         /// type implements a Type constructor, and if it does it invokes that ctor.
@@ -429,7 +425,7 @@ internal TypeConverter GetConverter([DynamicallyAccessedMembers(DynamicallyAcces
                 //
                 if (table == null)
                 {
-                    lock (s_internalSyncObject)
+                    lock (TypeDescriptor.s_commonSyncObject)
                     {
                         table = editorTables[editorBaseType];
                         if (table == null)
@@ -837,22 +833,11 @@ internal Type[] GetPopulatedTypes(Module module)
         {
             List<Type> typeList = new List<Type>();
 
-            lock (s_internalSyncObject)
+            foreach (KeyValuePair<Type, ReflectedTypeData> kvp in _typeData)
             {
-                Hashtable? typeData = _typeData;
-                if (typeData != null)
+                if (kvp.Key.Module == module && kvp.Value!.IsPopulated)
                 {
-                    // Manual use of IDictionaryEnumerator instead of foreach to avoid DictionaryEntry box allocations.
-                    IDictionaryEnumerator e = typeData.GetEnumerator();
-                    while (e.MoveNext())
-                    {
-                        DictionaryEntry de = e.Entry;
-                        Type type = (Type)de.Key;
-                        if (type.Module == module && ((ReflectedTypeData)de.Value!).IsPopulated)
-                        {
-                            typeList.Add(type);
-                        }
-                    }
+                    typeList.Add(kvp.Key);
                 }
             }
 
@@ -897,28 +882,23 @@ public override Type GetReflectionType(
         /// </summary>
         private ReflectedTypeData? GetTypeData([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] Type type, bool createIfNeeded)
         {
-            ReflectedTypeData? td = null;
-
-            if (_typeData != null)
+            if (_typeData.TryGetValue(type, out ReflectedTypeData? td))
             {
-                td = (ReflectedTypeData?)_typeData[type];
-                if (td != null)
-                {
-                    return td;
-                }
+                Debug.Assert(td != null);
+                return td;
             }
 
-            lock (s_internalSyncObject)
+            lock (TypeDescriptor.s_commonSyncObject)
             {
-                if (_typeData != null)
+                if (_typeData.TryGetValue(type, out td))
                 {
-                    td = (ReflectedTypeData?)_typeData[type];
+                    Debug.Assert(td != null);
+                    return td;
                 }
 
-                if (td == null && createIfNeeded)
+                if (createIfNeeded)
                 {
                     td = new ReflectedTypeData(type);
-                    _typeData ??= new Hashtable();
                     _typeData[type] = td;
                 }
             }
@@ -1006,7 +986,7 @@ internal static Attribute[] ReflectGetAttributes(Type type)
                 return attrs;
             }
 
-            lock (s_internalSyncObject)
+            lock (TypeDescriptor.s_commonSyncObject)
             {
                 attrs = (Attribute[]?)attributeCache[type];
                 if (attrs == null)
@@ -1034,7 +1014,7 @@ internal static Attribute[] ReflectGetAttributes(MemberInfo member)
                 return attrs;
             }
 
-            lock (s_internalSyncObject)
+            lock (TypeDescriptor.s_commonSyncObject)
             {
                 attrs = (Attribute[]?)attributeCache[member];
                 if (attrs == null)
@@ -1063,7 +1043,7 @@ private static EventDescriptor[] ReflectGetEvents(
                 return events;
             }
 
-            lock (s_internalSyncObject)
+            lock (TypeDescriptor.s_commonSyncObject)
             {
                 events = (EventDescriptor[]?)eventCache[type];
                 if (events == null)
@@ -1160,7 +1140,7 @@ private static PropertyDescriptor[] ReflectGetExtendedProperties(IExtenderProvid
             ReflectPropertyDescriptor[]? extendedProperties = (ReflectPropertyDescriptor[]?)extendedPropertyCache[providerType];
             if (extendedProperties == null)
             {
-                lock (s_internalSyncObject)
+                lock (TypeDescriptor.s_commonSyncObject)
                 {
                     extendedProperties = (ReflectPropertyDescriptor[]?)extendedPropertyCache[providerType];
 
@@ -1240,7 +1220,7 @@ private static PropertyDescriptor[] ReflectGetProperties(
                 return properties;
             }
 
-            lock (s_internalSyncObject)
+            lock (TypeDescriptor.s_commonSyncObject)
             {
                 properties = (PropertyDescriptor[]?)propertyCache[type];