Avoid deadlocks in TypeDescriptor (dotnet#97236)

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

@@ -33,7 +33,6 @@ public sealed class TypeDescriptor
                                                                                           // A value of `null` indicates initialization is in progress.
                                                                                           // A value of s_initializedDefaultProvider indicates the provider is initialized.
         private static readonly object s_initializedDefaultProvider = new object();
-        private static readonly object s_defaultProviderSyncObject = new object();
 
         private static WeakHashtable? s_associationTable;
         private static int s_metadataVersion;                          // a version stamp for our metadata. Used by property descriptors to know when to rebuild attributes.
@@ -271,15 +270,18 @@ private static void CheckDefaultProvider(Type type)
                 return;
             }
 
-            lock (s_defaultProviderSyncObject)
+            // 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)
             {
                 AddDefaultProvider(type);
             }
         }
 
         /// <summary>
         /// Add the default provider, if it exists.
-        /// For threading, this is always called under a 'lock (s_defaultProviderSyncObject)'.
+        /// For threading, this is always called under a 'lock (s_providerTable)'.
         /// </summary>
         private static void AddDefaultProvider(Type type)
         {
@@ -326,11 +328,11 @@ private static void AddDefaultProvider(Type type)
         }
 
         /// <summary>
-        /// The CreateAssocation method creates an association between two objects.
+        /// The CreateAssociation method creates an association between two objects.
         /// Once an association is created, a designer or other filtering mechanism
         /// can add properties that route to either object into the primary object's
         /// property set. When a property invocation is made against the primary
-        /// object, GetAssocation will be called to resolve the actual object
+        /// object, GetAssociation will be called to resolve the actual object
         /// instance that is related to its type parameter.
         /// </summary>
         [EditorBrowsable(EditorBrowsableState.Advanced)]

src/libraries/System.ComponentModel.TypeConverter/tests/TypeDescriptorTests.cs

@@ -8,6 +8,7 @@
 using System.Linq;
 using System.Threading;
 using System.Threading.Tasks;
+using Microsoft.DotNet.RemoteExecutor;
 using Moq;
 using Xunit;
 
@@ -1289,6 +1290,62 @@ public void ConcurrentGetProperties_ReturnsExpected()
             }
         }
 
+        [SkipOnPlatform(TestPlatforms.Browser, "Thread.Start is not supported on browsers.")]
+        [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
+        public static void ConcurrentAddProviderAndGetProvider()
+        {
+            // Use a timeout value lower than RemoteExecutor in order to get a nice Fail message.
+            const int Timeout = 50000;
+
+            RemoteInvokeOptions options = new()
+            {
+                TimeOut = 60000
+            };
+
+            RemoteExecutor.Invoke(() =>
+            {
+                using var finished = new CountdownEvent(2);
+
+                Thread t1 = new Thread(() =>
+                {
+                    ConcurrentAddProvider();
+                    finished.Signal();
+                });
+
+                Thread t2 = new Thread(() =>
+                {
+                    ConcurrentGetProvider();
+                    finished.Signal();
+                });
+
+                t1.Start();
+                t2.Start();
+                finished.Wait(Timeout);
+
+                if (finished.CurrentCount != 0)
+                {
+                    Assert.Fail("Timeout. Possible deadlock.");
+                }
+            }, options).Dispose();
+
+            static void ConcurrentAddProvider()
+            {
+                var provider = new EmptyPropertiesTypeProvider();
+                TypeDescriptor.AddProvider(provider, typeof(MyClass));
+
+                // This test primarily verifies no deadlock, but verify the values anyway.
+                Assert.True(TypeDescriptor.GetProvider(typeof(MyClass)).IsSupportedType(typeof(MyClass)));
+            }
+
+            static void ConcurrentGetProvider()
+            {
+                TypeDescriptionProvider provider = TypeDescriptor.GetProvider(typeof(TypeWithProperty));
+
+                // This test primarily verifies no deadlock, but verify the values anyway.
+                Assert.True(provider.IsSupportedType(typeof(TypeWithProperty)));
+            }
+        }
+
         public sealed class EmptyPropertiesTypeProvider : TypeDescriptionProvider
         {
             private sealed class EmptyPropertyListDescriptor : ICustomTypeDescriptor
@@ -1343,6 +1400,7 @@ public async void GetConverter_ByMultithread_ReturnsExpected(Type typeForGetConv
             TypeConverter[] actualConverters = await Task.WhenAll(
                 Enumerable.Range(0, 100).Select(_ =>
                     Task.Run(() => TypeDescriptor.GetConverter(typeForGetConverter))));
+
             Assert.All(actualConverters,
                 currentConverter => Assert.IsType(expectedConverterType, currentConverter));
         }