Fix a couple of issues with StackTraceSymbols.TryGetReader (dotnet#67300

)

The implementation has a comment about how ConditionalWeakTable prevents multiple threads from racing to create readers, but CWT doesn't invole the delegate under its lock.  So multiple threads can actually race to create a reader, and if one loses, it won't Dispose the reader it created.  On top of this, every call to TryGetReader is allocating a closure, even if one of the fast paths is hit, because the cache callback captures all the parameters.

src/libraries/System.Diagnostics.StackTrace/src/System/Diagnostics/StackTraceSymbols.cs

@@ -123,14 +123,20 @@ internal void GetSourceLineInfo(Assembly assembly, string assemblyPath, IntPtr l
                 return null;
             }
 
-            // The ConditionalWeakTable's GetValue + callback will atomically create the cache entry for us
-            // so we are protected from multiple threads racing to get/create the same MetadataReaderProvider
-            MetadataReaderProvider? provider = _metadataCache.GetValue(assembly, (assembly) =>
+            MetadataReaderProvider? provider;
+            while (!_metadataCache.TryGetValue(assembly, out provider))
             {
-                return (inMemoryPdbAddress != IntPtr.Zero) ?
-                            TryOpenReaderForInMemoryPdb(inMemoryPdbAddress, inMemoryPdbSize) :
-                            TryOpenReaderFromAssemblyFile(assemblyPath!, loadedPeAddress, loadedPeSize, isFileLayout);
-            });
+                provider = inMemoryPdbAddress != IntPtr.Zero ?
+                    TryOpenReaderForInMemoryPdb(inMemoryPdbAddress, inMemoryPdbSize) :
+                    TryOpenReaderFromAssemblyFile(assemblyPath!, loadedPeAddress, loadedPeSize, isFileLayout);
+
+                if (_metadataCache.TryAdd(assembly, provider))
+                {
+                    break;
+                }
+
+                provider?.Dispose();
+            }
 
             // The reader has already been open, so this doesn't throw.
             return provider?.GetMetadataReader();

src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/ConditionalWeakTable.cs

@@ -89,6 +89,30 @@ public void Add(TKey key, TValue value)
             }
         }
 
+        /// <summary>Adds a key to the table if it doesn't already exist.</summary>
+        /// <param name="key">The key to add.</param>
+        /// <param name="value">The key's property value.</param>
+        /// <returns>true if the key/value pair was added; false if the table already contained the key.</returns>
+        public bool TryAdd(TKey key, TValue value) // TODO: Expose in ref assembly https://github.com/dotnet/runtime/issues/29368
+        {
+            if (key is null)
+            {
+                ThrowHelper.ThrowArgumentNullException(ExceptionArgument.key);
+            }
+
+            lock (_lock)
+            {
+                int entryIndex = _container.FindEntry(key, out _);
+                if (entryIndex != -1)
+                {
+                    return false;
+                }
+
+                CreateEntry(key, value);
+                return true;
+            }
+        }
+
         /// <summary>Adds the key and value if the key doesn't exist, or updates the existing key's value if it does exist.</summary>
         /// <param name="key">key to add or update. May not be null.</param>
         /// <param name="value">value to associate with key.</param>

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/NativeLibrary.cs

@@ -196,13 +196,8 @@ public static void SetDllImportResolver(Assembly assembly!!, DllImportResolver r
                     new ConditionalWeakTable<Assembly, DllImportResolver>(), null);
             }
 
-            try
+            if (!s_nativeDllResolveMap.TryAdd(assembly, resolver))
             {
-                s_nativeDllResolveMap.Add(assembly, resolver);
-            }
-            catch (ArgumentException)
-            {
-                // ConditionalWeakTable throws ArgumentException if the Key already exists
                 throw new InvalidOperationException(SR.InvalidOperation_CannotRegisterSecondResolver);
             }
         }

src/libraries/System.Runtime/src/MatchingRefApiCompatBaseline.txt

@@ -12,6 +12,7 @@ MembersMustExist : Member 'public System.Diagnostics.DebugProvider System.Diagno
 MembersMustExist : Member 'protected System.ModuleHandle System.Reflection.Module.GetModuleHandleImpl()' does not exist in the reference but it does exist in the implementation.
 MembersMustExist : Member 'protected System.String System.String System.Resources.ResourceManager.BaseNameField' does not exist in the reference but it does exist in the implementation.
 MembersMustExist : Member 'protected System.Resources.IResourceReader System.Resources.IResourceReader System.Resources.ResourceSet.Reader' does not exist in the reference but it does exist in the implementation.
+MembersMustExist : Member 'public System.Boolean System.Runtime.CompilerServices.ConditionalWeakTable<TKey, TValue>.TryAdd(TKey, TValue)' does not exist in the reference but it does exist in the implementation.
 MembersMustExist : Member 'public System.Boolean System.Runtime.Serialization.SerializationInfo.DeserializationInProgress.get()' does not exist in the reference but it does exist in the implementation.
 MembersMustExist : Member 'public System.Runtime.Serialization.DeserializationToken System.Runtime.Serialization.SerializationInfo.StartDeserialization()' does not exist in the reference but it does exist in the implementation.
 MembersMustExist : Member 'public void System.Runtime.Serialization.SerializationInfo.ThrowIfDeserializationInProgress()' does not exist in the reference but it does exist in the implementation.