Avoid rare deadlocks when using TypeDescriptor (#103835)

steveharter · web-flow · commit c241cc99ac0c · 2024-06-26T12:42:02.000-05:00
diff --git a/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.cs b/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.cs
@@ -82,7 +82,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.
@@ -224,7 +223,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))
@@ -436,7 +435,7 @@ internal TypeConverter GetConverterFromRegisteredType(Type type, object? instanc
                 //
                 if (table == null)
                 {
-                    lock (s_internalSyncObject)
+                    lock (TypeDescriptor.s_commonSyncObject)
                     {
                         table = editorTables[editorBaseType];
                         if (table == null)
@@ -863,7 +862,7 @@ internal Type[] GetPopulatedTypes(Module module)
         {
             List<Type> typeList = new List<Type>();
 
-            lock (s_internalSyncObject)
+            lock (TypeDescriptor.s_commonSyncObject)
             {
                 Dictionary<Type, ReflectedTypeData>? typeData = _typeData;
                 if (typeData != null)
@@ -936,7 +935,7 @@ public override Type GetReflectionType(
                 return td;
             }
 
-            lock (s_internalSyncObject)
+            lock (TypeDescriptor.s_commonSyncObject)
             {
                 if (_typeData != null && _typeData.TryGetValue(type, out td))
                 {
@@ -999,7 +998,7 @@ private ReflectedTypeData GetTypeDataFromRegisteredType(Type type)
                 return;
             }
 
-            lock (s_internalSyncObject)
+            lock (TypeDescriptor.s_commonSyncObject)
             {
                 if (_typeData != null && _typeData.ContainsKey(componentType))
                 {
@@ -1024,7 +1023,7 @@ private ReflectedTypeData GetOrRegisterType(Type type)
                 return td;
             }
 
-            lock (s_internalSyncObject)
+            lock (TypeDescriptor.s_commonSyncObject)
             {
                 if (_typeData != null && _typeData.TryGetValue(type, out td))
                 {
@@ -1122,7 +1121,7 @@ internal static Attribute[] ReflectGetAttributes(Type type)
                 return attrs;
             }
 
-            lock (s_internalSyncObject)
+            lock (TypeDescriptor.s_commonSyncObject)
             {
                 attrs = (Attribute[]?)attributeCache[type];
                 if (attrs == null)
@@ -1150,7 +1149,7 @@ internal static Attribute[] ReflectGetAttributes(MemberInfo member)
                 return attrs;
             }
 
-            lock (s_internalSyncObject)
+            lock (TypeDescriptor.s_commonSyncObject)
             {
                 attrs = (Attribute[]?)attributeCache[member];
                 if (attrs == null)
@@ -1178,7 +1177,7 @@ private static EventDescriptor[] ReflectGetEvents(Type type)
                 return events;
             }
 
-            lock (s_internalSyncObject)
+            lock (TypeDescriptor.s_commonSyncObject)
             {
                 events = (EventDescriptor[]?)eventCache[type];
                 if (events == null)
@@ -1275,7 +1274,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];
 
@@ -1363,7 +1362,7 @@ private static PropertyDescriptor[] ReflectGetPropertiesImpl(Type type)
                 return properties;
             }
 
-            lock (s_internalSyncObject)
+            lock (TypeDescriptor.s_commonSyncObject)
             {
                 properties = (PropertyDescriptor[]?)propertyCache[type];
 
diff --git a/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs b/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs
@@ -48,6 +48,13 @@ public sealed class TypeDescriptor
         // class load anyway.
         private static readonly WeakHashtable s_providerTable = new WeakHashtable();
 
+        // This lock object protects access to several thread-unsafe areas below, and is a single lock object to prevent deadlocks.
+        // - During s_providerTypeTable access.
+        // - To act as a mutex for CheckDefaultProvider() when it needs to create the default provider, which may re-enter the above case.
+        // - For cache access in the ReflectTypeDescriptionProvider class which may re-enter the above case.
+        // - For logic added by consumers, such as custom provider, constructor and property logic, which may re-enter the above cases in unexpected ways.
+        internal static readonly object s_commonSyncObject = new object();
+
         // A direct mapping from type to provider.
         private static readonly Dictionary<Type, TypeDescriptionNode> s_providerTypeTable = new Dictionary<Type, TypeDescriptionNode>();
 
@@ -240,7 +247,7 @@ public static void AddProvider(TypeDescriptionProvider provider, Type type)
             ArgumentNullException.ThrowIfNull(provider);
             ArgumentNullException.ThrowIfNull(type);
 
-            lock (s_providerTable)
+            lock (s_commonSyncObject)
             {
                 // Get the root node, hook it up, and stuff it back into
                 // the provider cache.
@@ -270,7 +277,7 @@ public static void AddProvider(TypeDescriptionProvider provider, object instance
 
             // Get the root node, hook it up, and stuff it back into
             // the provider cache.
-            lock (s_providerTable)
+            lock (s_commonSyncObject)
             {
                 refreshNeeded = s_providerTable.ContainsKey(instance);
                 TypeDescriptionNode node = NodeFor(instance, true);
@@ -331,18 +338,15 @@ private static void CheckDefaultProvider(Type type)
                 return;
             }
 
-            // Lock on s_providerTable even though s_providerTable is not modified here.
-            // Using a single lock prevents deadlocks since other methods that call into or are called
-            // by this method also lock on s_providerTable and the ordering of the locks may be different.
-            lock (s_providerTable)
+            lock (s_commonSyncObject)
             {
                 AddDefaultProvider(type);
             }
         }
 
         /// <summary>
         /// Add the default provider, if it exists.
-        /// For threading, this is always called under a 'lock (s_providerTable)'.
+        /// For threading, this is always called under a 'lock (s_commonSyncObject)'.
         /// </summary>
         private static void AddDefaultProvider(Type type)
         {
@@ -1666,7 +1670,7 @@ private static TypeDescriptionNode NodeFor(Type type, bool createDelegator)
 
                     if (searchType == typeof(object) || baseType == null)
                     {
-                        lock (s_providerTable)
+                        lock (s_commonSyncObject)
                         {
                             node = (TypeDescriptionNode?)s_providerTable[searchType];
 
@@ -1682,7 +1686,7 @@ private static TypeDescriptionNode NodeFor(Type type, bool createDelegator)
                     else if (createDelegator)
                     {
                         node = new TypeDescriptionNode(new DelegatingTypeDescriptionProvider(baseType));
-                        lock (s_providerTable)
+                        lock (s_commonSyncObject)
                         {
                             s_providerTypeTable.TryAdd(searchType, node);
                         }
@@ -1793,7 +1797,7 @@ private static TypeDescriptionNode NodeFor(object instance, bool createDelegator
         /// </summary>
         private static void NodeRemove(object key, TypeDescriptionProvider provider)
         {
-            lock (s_providerTable)
+            lock (s_commonSyncObject)
             {
                 TypeDescriptionNode? head = (TypeDescriptionNode?)s_providerTable[key];
                 TypeDescriptionNode? target = head;
@@ -2314,7 +2318,7 @@ private static void Refresh(object component, bool refreshReflectionProvider)
             {
                 Type type = component.GetType();
 
-                lock (s_providerTable)
+                lock (s_commonSyncObject)
                 {
                     // ReflectTypeDescritionProvider is only bound to object, but we
                     // need go to through the entire table to try to find custom
@@ -2398,7 +2402,7 @@ public static void Refresh(Type type)
 
             bool found = false;
 
-            lock (s_providerTable)
+            lock (s_commonSyncObject)
             {
                 // ReflectTypeDescritionProvider is only bound to object, but we
                 // need go to through the entire table to try to find custom
@@ -2463,7 +2467,7 @@ public static void Refresh(Module module)
             // each of these levels.
             Hashtable? refreshedTypes = null;
 
-            lock (s_providerTable)
+            lock (s_commonSyncObject)
             {
                 // Manual use of IDictionaryEnumerator instead of foreach to avoid DictionaryEntry box allocations.
                 IDictionaryEnumerator e = s_providerTable.GetEnumerator();

Original file line number	Diff line number	Diff line change
`@@ -82,7 +82,6 @@ private static Type[] InitializeSkipInterfaceAttributeList()`
`82`	`82`
`83`	`83`	`internal static Guid ExtenderProviderKey { get; } = Guid.NewGuid();`
`84`	`84`
`85`		`- private static readonly object s_internalSyncObject = new object();`
`86`	`85`	`/// <summary>`
`87`	`86`	`/// Creates a new ReflectTypeDescriptionProvider. The type is the`
`88`	`87`	`/// type we will obtain type information for.`
`@@ -224,7 +223,7 @@ internal static void AddEditorTable(Type editorBaseType, Hashtable table)`
`224`	`223`
`225`	`224`	`Debug.Assert(table != null, "COMPAT: Editor table should not be null"); // don't throw; RTM didn't so we can't do it either.`
`226`	`225`
`227`		`- lock (s_internalSyncObject)`
	`226`	`+ lock (TypeDescriptor.s_commonSyncObject)`
`228`	`227`	`{`
`229`	`228`	`Hashtable editorTables = EditorTables;`
`230`	`229`	`if (!editorTables.ContainsKey(editorBaseType))`
`@@ -436,7 +435,7 @@ internal TypeConverter GetConverterFromRegisteredType(Type type, object? instanc`
`436`	`435`	`//`
`437`	`436`	`if (table == null)`
`438`	`437`	`{`
`439`		`- lock (s_internalSyncObject)`
	`438`	`+ lock (TypeDescriptor.s_commonSyncObject)`
`440`	`439`	`{`
`441`	`440`	`table = editorTables[editorBaseType];`
`442`	`441`	`if (table == null)`
`@@ -863,7 +862,7 @@ internal Type[] GetPopulatedTypes(Module module)`
`863`	`862`	`{`
`864`	`863`	`List<Type> typeList = new List<Type>();`
`865`	`864`
`866`		`- lock (s_internalSyncObject)`
	`865`	`+ lock (TypeDescriptor.s_commonSyncObject)`
`867`	`866`	`{`
`868`	`867`	`Dictionary<Type, ReflectedTypeData>? typeData = _typeData;`
`869`	`868`	`if (typeData != null)`
`@@ -936,7 +935,7 @@ public override Type GetReflectionType(`
`936`	`935`	`return td;`
`937`	`936`	`}`
`938`	`937`
`939`		`- lock (s_internalSyncObject)`
	`938`	`+ lock (TypeDescriptor.s_commonSyncObject)`
`940`	`939`	`{`
`941`	`940`	`if (_typeData != null && _typeData.TryGetValue(type, out td))`
`942`	`941`	`{`
`@@ -999,7 +998,7 @@ private ReflectedTypeData GetTypeDataFromRegisteredType(Type type)`
`999`	`998`	`return;`
`1000`	`999`	`}`
`1001`	`1000`
`1002`		`- lock (s_internalSyncObject)`
	`1001`	`+ lock (TypeDescriptor.s_commonSyncObject)`
`1003`	`1002`	`{`
`1004`	`1003`	`if (_typeData != null && _typeData.ContainsKey(componentType))`
`1005`	`1004`	`{`
`@@ -1024,7 +1023,7 @@ private ReflectedTypeData GetOrRegisterType(Type type)`
`1024`	`1023`	`return td;`
`1025`	`1024`	`}`
`1026`	`1025`
`1027`		`- lock (s_internalSyncObject)`
	`1026`	`+ lock (TypeDescriptor.s_commonSyncObject)`
`1028`	`1027`	`{`
`1029`	`1028`	`if (_typeData != null && _typeData.TryGetValue(type, out td))`
`1030`	`1029`	`{`
`@@ -1122,7 +1121,7 @@ internal static Attribute[] ReflectGetAttributes(Type type)`
`1122`	`1121`	`return attrs;`
`1123`	`1122`	`}`
`1124`	`1123`
`1125`		`- lock (s_internalSyncObject)`
	`1124`	`+ lock (TypeDescriptor.s_commonSyncObject)`
`1126`	`1125`	`{`
`1127`	`1126`	`attrs = (Attribute[]?)attributeCache[type];`
`1128`	`1127`	`if (attrs == null)`
`@@ -1150,7 +1149,7 @@ internal static Attribute[] ReflectGetAttributes(MemberInfo member)`
`1150`	`1149`	`return attrs;`
`1151`	`1150`	`}`
`1152`	`1151`
`1153`		`- lock (s_internalSyncObject)`
	`1152`	`+ lock (TypeDescriptor.s_commonSyncObject)`
`1154`	`1153`	`{`
`1155`	`1154`	`attrs = (Attribute[]?)attributeCache[member];`
`1156`	`1155`	`if (attrs == null)`
`@@ -1178,7 +1177,7 @@ private static EventDescriptor[] ReflectGetEvents(Type type)`
`1178`	`1177`	`return events;`
`1179`	`1178`	`}`
`1180`	`1179`
`1181`		`- lock (s_internalSyncObject)`
	`1180`	`+ lock (TypeDescriptor.s_commonSyncObject)`
`1182`	`1181`	`{`
`1183`	`1182`	`events = (EventDescriptor[]?)eventCache[type];`
`1184`	`1183`	`if (events == null)`
`@@ -1275,7 +1274,7 @@ private static PropertyDescriptor[] ReflectGetExtendedProperties(IExtenderProvid`
`1275`	`1274`	`ReflectPropertyDescriptor[]? extendedProperties = (ReflectPropertyDescriptor[]?)extendedPropertyCache[providerType];`
`1276`	`1275`	`if (extendedProperties == null)`
`1277`	`1276`	`{`
`1278`		`- lock (s_internalSyncObject)`
	`1277`	`+ lock (TypeDescriptor.s_commonSyncObject)`
`1279`	`1278`	`{`
`1280`	`1279`	`extendedProperties = (ReflectPropertyDescriptor[]?)extendedPropertyCache[providerType];`
`1281`	`1280`
`@@ -1363,7 +1362,7 @@ private static PropertyDescriptor[] ReflectGetPropertiesImpl(Type type)`
`1363`	`1362`	`return properties;`
`1364`	`1363`	`}`
`1365`	`1364`
`1366`		`- lock (s_internalSyncObject)`
	`1365`	`+ lock (TypeDescriptor.s_commonSyncObject)`
`1367`	`1366`	`{`
`1368`	`1367`	`properties = (PropertyDescriptor[]?)propertyCache[type];`
`1369`	`1368`
Original file line number	Diff line number	Diff line change
`@@ -48,6 +48,13 @@ public sealed class TypeDescriptor`
`48`	`48`	`// class load anyway.`
`49`	`49`	`private static readonly WeakHashtable s_providerTable = new WeakHashtable();`
`50`	`50`
	`51`	`+ // This lock object protects access to several thread-unsafe areas below, and is a single lock object to prevent deadlocks.`
	`52`	`+ // - During s_providerTypeTable access.`
	`53`	`+ // - To act as a mutex for CheckDefaultProvider() when it needs to create the default provider, which may re-enter the above case.`
	`54`	`+ // - For cache access in the ReflectTypeDescriptionProvider class which may re-enter the above case.`
	`55`	`+ // - For logic added by consumers, such as custom provider, constructor and property logic, which may re-enter the above cases in unexpected ways.`
	`56`	`+ internal static readonly object s_commonSyncObject = new object();`
	`57`	`+`
`51`	`58`	`// A direct mapping from type to provider.`
`52`	`59`	`private static readonly Dictionary<Type, TypeDescriptionNode> s_providerTypeTable = new Dictionary<Type, TypeDescriptionNode>();`
`53`	`60`
`@@ -240,7 +247,7 @@ public static void AddProvider(TypeDescriptionProvider provider, Type type)`
`240`	`247`	`ArgumentNullException.ThrowIfNull(provider);`
`241`	`248`	`ArgumentNullException.ThrowIfNull(type);`
`242`	`249`
`243`		`- lock (s_providerTable)`
	`250`	`+ lock (s_commonSyncObject)`
`244`	`251`	`{`
`245`	`252`	`// Get the root node, hook it up, and stuff it back into`
`246`	`253`	`// the provider cache.`
`@@ -270,7 +277,7 @@ public static void AddProvider(TypeDescriptionProvider provider, object instance`
`270`	`277`
`271`	`278`	`// Get the root node, hook it up, and stuff it back into`
`272`	`279`	`// the provider cache.`
`273`		`- lock (s_providerTable)`
	`280`	`+ lock (s_commonSyncObject)`
`274`	`281`	`{`
`275`	`282`	`refreshNeeded = s_providerTable.ContainsKey(instance);`
`276`	`283`	`TypeDescriptionNode node = NodeFor(instance, true);`
`@@ -331,18 +338,15 @@ private static void CheckDefaultProvider(Type type)`
`331`	`338`	`return;`
`332`	`339`	`}`
`333`	`340`
`334`		`- // Lock on s_providerTable even though s_providerTable is not modified here.`
`335`		`- // Using a single lock prevents deadlocks since other methods that call into or are called`
`336`		`- // by this method also lock on s_providerTable and the ordering of the locks may be different.`
`337`		`- lock (s_providerTable)`
	`341`	`+ lock (s_commonSyncObject)`
`338`	`342`	`{`
`339`	`343`	`AddDefaultProvider(type);`
`340`	`344`	`}`
`341`	`345`	`}`
`342`	`346`
`343`	`347`	`/// <summary>`
`344`	`348`	`/// Add the default provider, if it exists.`
`345`		`- /// For threading, this is always called under a 'lock (s_providerTable)'.`
	`349`	`+ /// For threading, this is always called under a 'lock (s_commonSyncObject)'.`
`346`	`350`	`/// </summary>`
`347`	`351`	`private static void AddDefaultProvider(Type type)`
`348`	`352`	`{`
`@@ -1666,7 +1670,7 @@ private static TypeDescriptionNode NodeFor(Type type, bool createDelegator)`
`1666`	`1670`
`1667`	`1671`	`if (searchType == typeof(object) \|\| baseType == null)`
`1668`	`1672`	`{`
`1669`		`- lock (s_providerTable)`
	`1673`	`+ lock (s_commonSyncObject)`
`1670`	`1674`	`{`
`1671`	`1675`	`node = (TypeDescriptionNode?)s_providerTable[searchType];`
`1672`	`1676`
`@@ -1682,7 +1686,7 @@ private static TypeDescriptionNode NodeFor(Type type, bool createDelegator)`
`1682`	`1686`	`else if (createDelegator)`
`1683`	`1687`	`{`
`1684`	`1688`	`node = new TypeDescriptionNode(new DelegatingTypeDescriptionProvider(baseType));`
`1685`		`- lock (s_providerTable)`
	`1689`	`+ lock (s_commonSyncObject)`
`1686`	`1690`	`{`
`1687`	`1691`	`s_providerTypeTable.TryAdd(searchType, node);`
`1688`	`1692`	`}`
`@@ -1793,7 +1797,7 @@ private static TypeDescriptionNode NodeFor(object instance, bool createDelegator`
`1793`	`1797`	`/// </summary>`
`1794`	`1798`	`private static void NodeRemove(object key, TypeDescriptionProvider provider)`
`1795`	`1799`	`{`
`1796`		`- lock (s_providerTable)`
	`1800`	`+ lock (s_commonSyncObject)`
`1797`	`1801`	`{`
`1798`	`1802`	`TypeDescriptionNode? head = (TypeDescriptionNode?)s_providerTable[key];`
`1799`	`1803`	`TypeDescriptionNode? target = head;`
`@@ -2314,7 +2318,7 @@ private static void Refresh(object component, bool refreshReflectionProvider)`
`2314`	`2318`	`{`
`2315`	`2319`	`Type type = component.GetType();`
`2316`	`2320`
`2317`		`- lock (s_providerTable)`
	`2321`	`+ lock (s_commonSyncObject)`
`2318`	`2322`	`{`
`2319`	`2323`	`// ReflectTypeDescritionProvider is only bound to object, but we`
`2320`	`2324`	`// need go to through the entire table to try to find custom`
`@@ -2398,7 +2402,7 @@ public static void Refresh(Type type)`
`2398`	`2402`
`2399`	`2403`	`bool found = false;`
`2400`	`2404`
`2401`		`- lock (s_providerTable)`
	`2405`	`+ lock (s_commonSyncObject)`
`2402`	`2406`	`{`
`2403`	`2407`	`// ReflectTypeDescritionProvider is only bound to object, but we`
`2404`	`2408`	`// need go to through the entire table to try to find custom`
`@@ -2463,7 +2467,7 @@ public static void Refresh(Module module)`
`2463`	`2467`	`// each of these levels.`
`2464`	`2468`	`Hashtable? refreshedTypes = null;`
`2465`	`2469`
`2466`		`- lock (s_providerTable)`
	`2470`	`+ lock (s_commonSyncObject)`
`2467`	`2471`	`{`
`2468`	`2472`	`// Manual use of IDictionaryEnumerator instead of foreach to avoid DictionaryEntry box allocations.`
`2469`	`2473`	`IDictionaryEnumerator e = s_providerTable.GetEnumerator();`