[release/6.0] 1388 xml serializer assembly load context awareness (#6…

…1266) * Generate dynamic serialization assembly in the appropriate ALC, and don't keep any hard refs to types that could prevent unloading. * Add small test for ALC unloading. * PR feedback; Move into TempAssembly; Strong/Weak lookup tables; Test refinement * Refining TempAssembly cache; Reflection-based fix; Test corrections * Mono/wasm test fixup * Ensure that serializers from XmlMappings don't run afoul of collectible ALCs. * PR feedback * Unloadable contexts not yet supported in Mono. * Fix trimming of types for XmlSerializer tests that got moved out of main test assembly. * Encapsulating the duality of dealing with unloadable ALC instead of rewriting similar code everywhere. * Missed new file in last commit. * Compilation bug not seen locally. * Perf improvement. * Remove extraneous line. Test feedback. * Fix unloadability (#58948) There is a bug in AppDomain::FindAssembly code path that iterates over the domain assemblies. The iteration loop leaves the refcount of the LoaderAllocator related to the returned DomainAssembly bumped, but nothing ever decrements it. So when a code that needs to be unloaded ends up in that code path, all the managed things like managed LoaderAllocator, LoaderAllocatorScout are destroyed, but the unloading doesn't complete due to the refcount. We have never found it before as this code path is never executed in any of the coreclr tests even with unloadability testing option. (cherry picked from commit 8b38c19) Co-authored-by: Steve Molloy <[email protected]> Co-authored-by: Jan Vorlicek <[email protected]>
dotnet · Jan 10, 2022 · 04a5836 · 04a5836
1 parent 3f6d8fa
commit 04a5836
Show file tree

Hide file tree

Showing 13 changed files with 418 additions and 195 deletions.
diff --git a/src/coreclr/vm/appdomain.cpp b/src/coreclr/vm/appdomain.cpp
@@ -3533,8 +3533,8 @@ DomainAssembly * AppDomain::FindAssembly(PEAssembly * pFile, FindAssemblyOptions
             !pManifestFile->IsResource() &&
             pManifestFile->Equals(pFile))
         {
-            // Caller already has PEAssembly, so we can give DomainAssembly away freely without AddRef
-            return pDomainAssembly.Extract();
+            // Caller already has PEAssembly, so we can give DomainAssembly away freely without added reference
+            return pDomainAssembly.GetValue();
         }
     }
     return NULL;

diff --git a/src/libraries/Common/tests/System/Runtime/Serialization/Utils.cs b/src/libraries/Common/tests/System/Runtime/Serialization/Utils.cs
@@ -9,6 +9,8 @@
 using System.Threading.Tasks;
 using System.Xml.Linq;
 using System.Linq;
+using System.Reflection;
+using System.Runtime.Loader;
 using Xunit;
 
 internal static class Utils
@@ -351,3 +353,30 @@ private static bool IsPrefixedAttributeValue(string atrValue, out string localPr
         return false;
     }
 }
+
+internal class TestAssemblyLoadContext : AssemblyLoadContext
+{
+    private AssemblyDependencyResolver _resolver;
+
+    public TestAssemblyLoadContext(string name, bool isCollectible, string mainAssemblyToLoadPath = null) : base(name, isCollectible)
+    {
+        if (!PlatformDetection.IsBrowser)
+            _resolver = new AssemblyDependencyResolver(mainAssemblyToLoadPath ?? Assembly.GetExecutingAssembly().Location);
+    }
+
+    protected override Assembly Load(AssemblyName name)
+    {
+        if (PlatformDetection.IsBrowser)
+        {
+            return base.Load(name);
+        }
+
+        string assemblyPath = _resolver.ResolveAssemblyToPath(name);
+        if (assemblyPath != null)
+        {
+            return LoadFromAssemblyPath(assemblyPath);
+        }
+
+        return null;
+    }
+}
diff --git a/src/libraries/System.Private.Xml/src/Resources/Strings.resx b/src/libraries/System.Private.Xml/src/Resources/Strings.resx
@@ -2787,6 +2787,9 @@
   <data name="XmlNotSerializable" xml:space="preserve">
     <value>Type '{0}' is not serializable.</value>
   </data>
+  <data name="XmlTypeInBadLoadContext" xml:space="preserve">
+    <value>Type '{0}' is from an AssemblyLoadContext which is incompatible with that which contains this XmlSerializer.</value>
+  </data>
   <data name="XmlPregenInvalidXmlSerializerAssemblyAttribute" xml:space="preserve">
     <value>Invalid XmlSerializerAssemblyAttribute usage. Please use {0} property or {1} property.</value>
   </data>

diff --git a/src/libraries/System.Private.Xml/src/System.Private.Xml.csproj b/src/libraries/System.Private.Xml/src/System.Private.Xml.csproj
@@ -446,6 +446,7 @@
     <Compile Include="System\Xml\Serialization\CodeIdentifiers.cs" />
     <Compile Include="System\Xml\Serialization\Compilation.cs" />
     <Compile Include="System\Xml\Serialization\Compiler.cs" />
+    <Compile Include="System\Xml\Serialization\ContextAwareTables.cs" />
     <Compile Include="System\Xml\Serialization\ImportContext.cs" />
     <Compile Include="System\Xml\Serialization\indentedWriter.cs" />
     <Compile Include="System\Xml\Serialization\IXmlSerializable.cs" />
@@ -565,6 +566,7 @@
     <Reference Include="System.Runtime.CompilerServices.Unsafe" />
     <Reference Include="System.Runtime.Extensions" />
     <Reference Include="System.Runtime.InteropServices" />
+    <Reference Include="System.Runtime.Loader" />
     <Reference Include="System.Security.Cryptography.Algorithms" />
     <Reference Include="System.Security.Cryptography.Primitives" />
     <Reference Include="System.Text.Encoding.Extensions" />

diff --git a/src/libraries/System.Private.Xml/src/System/Xml/Serialization/Compilation.cs b/src/libraries/System.Private.Xml/src/System/Xml/Serialization/Compilation.cs
diff --git a/src/libraries/System.Private.Xml/src/System/Xml/Serialization/ContextAwareTables.cs b/src/libraries/System.Private.Xml/src/System/Xml/Serialization/ContextAwareTables.cs
@@ -0,0 +1,66 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+namespace System.Xml.Serialization
+{
+    using System;
+    using System.Collections;
+    using System.Diagnostics.CodeAnalysis;
+    using System.Runtime.CompilerServices;
+    using System.Runtime.Loader;
+
+    internal class ContextAwareTables<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)]T> where T : class?
+    {
+        private Hashtable _defaultTable;
+        private ConditionalWeakTable<Type, T> _collectibleTable;
+
+        public ContextAwareTables()
+        {
+            _defaultTable = new Hashtable();
+            _collectibleTable = new ConditionalWeakTable<Type, T>();
+        }
+
+        internal T GetOrCreateValue(Type t, Func<T> f)
+        {
+            // The fast and most common default case
+            T? ret = (T?)_defaultTable[t];
+            if (ret != null)
+                return ret;
+
+            // Common case for collectible contexts
+            if (_collectibleTable.TryGetValue(t, out ret))
+                return ret;
+
+            // Not found. Do the slower work of creating the value in the correct collection.
+            AssemblyLoadContext? alc = AssemblyLoadContext.GetLoadContext(t.Assembly);
+
+            // Null and non-collectible load contexts use the default table
+            if (alc == null || !alc.IsCollectible)
+            {
+                lock (_defaultTable)
+                {
+                    if ((ret = (T?)_defaultTable[t]) == null)
+                    {
+                        ret = f();
+                        _defaultTable[t] = ret;
+                    }
+                }
+            }
+
+            // Collectible load contexts should use the ConditionalWeakTable so they can be unloaded
+            else
+            {
+                lock (_collectibleTable)
+                {
+                    if (!_collectibleTable.TryGetValue(t, out ret))
+                    {
+                        ret = f();
+                        _collectibleTable.AddOrUpdate(t, ret);
+                    }
+                }
+            }
+
+            return ret;
+        }
+    }
+}
diff --git a/...aries/System.Private.Xml/src/System/Xml/Serialization/ReflectionXmlSerializationReader.cs b/...aries/System.Private.Xml/src/System/Xml/Serialization/ReflectionXmlSerializationReader.cs
@@ -627,40 +627,48 @@ private static void AddObjectsIntoTargetCollection(object targetCollection, List
             }
         }
 
-        private static readonly ConcurrentDictionary<(Type, string), ReflectionXmlSerializationReaderHelper.SetMemberValueDelegate> s_setMemberValueDelegateCache = new ConcurrentDictionary<(Type, string), ReflectionXmlSerializationReaderHelper.SetMemberValueDelegate>();
+        private static readonly ContextAwareTables<Hashtable> s_setMemberValueDelegateCache = new ContextAwareTables<Hashtable>();
 
         [RequiresUnreferencedCode(XmlSerializer.TrimSerializationWarning)]
         private static ReflectionXmlSerializationReaderHelper.SetMemberValueDelegate GetSetMemberValueDelegate(object o, string memberName)
         {
             Debug.Assert(o != null, "Object o should not be null");
             Debug.Assert(!string.IsNullOrEmpty(memberName), "memberName must have a value");
-            (Type, string) typeMemberNameTuple = (o.GetType(), memberName);
-            if (!s_setMemberValueDelegateCache.TryGetValue(typeMemberNameTuple, out ReflectionXmlSerializationReaderHelper.SetMemberValueDelegate? result))
+            Type type = o.GetType();
+            var delegateCacheForType = s_setMemberValueDelegateCache.GetOrCreateValue(type, () => new Hashtable());
+            var result = delegateCacheForType[memberName];
+            if (result == null)
             {
-                MemberInfo memberInfo = ReflectionXmlSerializationHelper.GetEffectiveSetInfo(o.GetType(), memberName);
-                Debug.Assert(memberInfo != null, "memberInfo could not be retrieved");
-                Type memberType;
-                if (memberInfo is PropertyInfo propInfo)
-                {
-                    memberType = propInfo.PropertyType;
-                }
-                else if (memberInfo is FieldInfo fieldInfo)
-                {
-                    memberType = fieldInfo.FieldType;
-                }
-                else
+                lock (delegateCacheForType)
                 {
-                    throw new InvalidOperationException(SR.XmlInternalError);
-                }
+                    if ((result = delegateCacheForType[memberName]) == null)
+                    {
+                        MemberInfo memberInfo = ReflectionXmlSerializationHelper.GetEffectiveSetInfo(o.GetType(), memberName);
+                        Debug.Assert(memberInfo != null, "memberInfo could not be retrieved");
+                        Type memberType;
+                        if (memberInfo is PropertyInfo propInfo)
+                        {
+                            memberType = propInfo.PropertyType;
+                        }
+                        else if (memberInfo is FieldInfo fieldInfo)
+                        {
+                            memberType = fieldInfo.FieldType;
+                        }
+                        else
+                        {
+                            throw new InvalidOperationException(SR.XmlInternalError);
+                        }
 
-                MethodInfo getSetMemberValueDelegateWithTypeGenericMi = typeof(ReflectionXmlSerializationReaderHelper).GetMethod("GetSetMemberValueDelegateWithType", BindingFlags.Static | BindingFlags.Public)!;
-                MethodInfo getSetMemberValueDelegateWithTypeMi = getSetMemberValueDelegateWithTypeGenericMi.MakeGenericMethod(o.GetType(), memberType);
-                var getSetMemberValueDelegateWithType = (Func<MemberInfo, ReflectionXmlSerializationReaderHelper.SetMemberValueDelegate>)getSetMemberValueDelegateWithTypeMi.CreateDelegate(typeof(Func<MemberInfo, ReflectionXmlSerializationReaderHelper.SetMemberValueDelegate>));
-                result = getSetMemberValueDelegateWithType(memberInfo);
-                s_setMemberValueDelegateCache.TryAdd(typeMemberNameTuple, result);
+                        MethodInfo getSetMemberValueDelegateWithTypeGenericMi = typeof(ReflectionXmlSerializationReaderHelper).GetMethod("GetSetMemberValueDelegateWithType", BindingFlags.Static | BindingFlags.Public)!;
+                        MethodInfo getSetMemberValueDelegateWithTypeMi = getSetMemberValueDelegateWithTypeGenericMi.MakeGenericMethod(o.GetType(), memberType);
+                        var getSetMemberValueDelegateWithType = (Func<MemberInfo, ReflectionXmlSerializationReaderHelper.SetMemberValueDelegate>)getSetMemberValueDelegateWithTypeMi.CreateDelegate(typeof(Func<MemberInfo, ReflectionXmlSerializationReaderHelper.SetMemberValueDelegate>));
+                        result = getSetMemberValueDelegateWithType(memberInfo);
+                        delegateCacheForType[memberName] = result;
+                    }
+                }
             }
 
-            return result;
+            return (ReflectionXmlSerializationReaderHelper.SetMemberValueDelegate)result;
         }
 
         private object? GetMemberValue(object o, MemberInfo memberInfo)

diff --git a/src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializationWriter.cs b/src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializationWriter.cs
@@ -19,6 +19,7 @@ namespace System.Xml.Serialization
     using System.Xml.Serialization;
     using System.Xml;
     using System.Diagnostics.CodeAnalysis;
+    using System.Runtime.CompilerServices;
 
     ///<internalonly/>
     public abstract class XmlSerializationWriter : XmlSerializationGeneratedCode
@@ -1465,14 +1466,13 @@ internal static class DynamicAssemblies
     {
         private static readonly Hashtable s_nameToAssemblyMap = new Hashtable();
         private static readonly Hashtable s_assemblyToNameMap = new Hashtable();
-        private static readonly Hashtable s_tableIsTypeDynamic = Hashtable.Synchronized(new Hashtable());
+        private static readonly ContextAwareTables<object> s_tableIsTypeDynamic = new ContextAwareTables<object>();
 
         // SxS: This method does not take any resource name and does not expose any resources to the caller.
         // It's OK to suppress the SxS warning.
         internal static bool IsTypeDynamic(Type type)
         {
-            object? oIsTypeDynamic = s_tableIsTypeDynamic[type];
-            if (oIsTypeDynamic == null)
+            object oIsTypeDynamic = s_tableIsTypeDynamic.GetOrCreateValue(type, () =>
             {
                 Assembly assembly = type.Assembly;
                 bool isTypeDynamic = assembly.IsDynamic /*|| string.IsNullOrEmpty(assembly.Location)*/;
@@ -1500,8 +1500,8 @@ internal static bool IsTypeDynamic(Type type)
                         }
                     }
                 }
-                s_tableIsTypeDynamic[type] = oIsTypeDynamic = isTypeDynamic;
-            }
+                return isTypeDynamic;
+            });
             return (bool)oIsTypeDynamic;
         }
 

diff --git a/src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializer.cs b/src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializer.cs
@@ -10,6 +10,7 @@
 using System.IO;
 using System.Reflection;
 using System.Runtime.CompilerServices;
+using System.Runtime.Loader;
 using System.Runtime.Versioning;
 using System.Security;
 using System.Text;
@@ -161,8 +162,7 @@ private static XmlSerializerNamespaces DefaultNamespaces
         internal const string TrimSerializationWarning = "Members from serialized types may be trimmed if not referenced directly";
         private const string TrimDeserializationWarning = "Members from deserialized types may be trimmed if not referenced directly";
 
-        private static readonly Dictionary<Type, Dictionary<XmlSerializerMappingKey, XmlSerializer>> s_xmlSerializerTable = new Dictionary<Type, Dictionary<XmlSerializerMappingKey, XmlSerializer>>();
-
+        private static readonly ContextAwareTables<Dictionary<XmlSerializerMappingKey, XmlSerializer>> s_xmlSerializerTable = new ContextAwareTables<Dictionary<XmlSerializerMappingKey, XmlSerializer>>();
         protected XmlSerializer()
         {
         }
@@ -235,30 +235,28 @@ public XmlSerializer(Type type, string? defaultNamespace)
                     _tempAssembly = s_cache[defaultNamespace, type];
                     if (_tempAssembly == null)
                     {
+                        XmlSerializerImplementation? contract = null;
+                        Assembly? assembly = TempAssembly.LoadGeneratedAssembly(type, defaultNamespace, out contract);
+                        if (assembly == null)
                         {
-                            XmlSerializerImplementation? contract = null;
-                            Assembly? assembly = TempAssembly.LoadGeneratedAssembly(type, defaultNamespace, out contract);
-                            if (assembly == null)
-                            {
-                                if (Mode == SerializationMode.PreGenOnly)
-                                {
-                                    AssemblyName name = type.Assembly.GetName();
-                                    var serializerName = Compiler.GetTempAssemblyName(name, defaultNamespace);
-                                    throw new FileLoadException(SR.Format(SR.FailLoadAssemblyUnderPregenMode, serializerName));
-                                }
-
-                                // need to reflect and generate new serialization assembly
-                                XmlReflectionImporter importer = new XmlReflectionImporter(defaultNamespace);
-                                _mapping = importer.ImportTypeMapping(type, null, defaultNamespace);
-                                _tempAssembly = GenerateTempAssembly(_mapping, type, defaultNamespace)!;
-                            }
-                            else
+                            if (Mode == SerializationMode.PreGenOnly)
                             {
-                                // we found the pre-generated assembly, now make sure that the assembly has the right serializer
-                                // try to avoid the reflection step, need to get ElementName, namespace and the Key form the type
-                                _mapping = XmlReflectionImporter.GetTopLevelMapping(type, defaultNamespace);
-                                _tempAssembly = new TempAssembly(new XmlMapping[] { _mapping }, assembly, contract);
+                                AssemblyName name = type.Assembly.GetName();
+                                var serializerName = Compiler.GetTempAssemblyName(name, defaultNamespace);
+                                throw new FileLoadException(SR.Format(SR.FailLoadAssemblyUnderPregenMode, serializerName));
                             }
+
+                            // need to reflect and generate new serialization assembly
+                            XmlReflectionImporter importer = new XmlReflectionImporter(defaultNamespace);
+                            _mapping = importer.ImportTypeMapping(type, null, defaultNamespace);
+                            _tempAssembly = GenerateTempAssembly(_mapping, type, defaultNamespace)!;
+                        }
+                        else
+                        {
+                            // we found the pre-generated assembly, now make sure that the assembly has the right serializer
+                            // try to avoid the reflection step, need to get ElementName, namespace and the Key form the type
+                            _mapping = XmlReflectionImporter.GetTopLevelMapping(type, defaultNamespace);
+                            _tempAssembly = new TempAssembly(new XmlMapping[] { _mapping }, assembly, contract);
                         }
                     }
                     s_cache.Add(defaultNamespace, type, _tempAssembly);
@@ -403,7 +401,9 @@ public void Serialize(XmlWriter xmlWriter, object? o, XmlSerializerNamespaces? n
                     }
                 }
                 else
+                {
                     _tempAssembly.InvokeWriter(_mapping, xmlWriter, o, namespaces == null || namespaces.Count == 0 ? DefaultNamespaces : namespaces, encodingStyle, id);
+                }
             }
             catch (Exception? e)
             {
@@ -629,7 +629,10 @@ public static XmlSerializer[] FromMappings(XmlMapping[]? mappings, Type? type)
             {
                 XmlSerializer[] serializers = new XmlSerializer[mappings.Length];
                 for (int i = 0; i < serializers.Length; i++)
+                {
                     serializers[i] = (XmlSerializer)contract!.TypedSerializers[mappings[i].Key!]!;
+                    TempAssembly.VerifyLoadContext(serializers[i]._rootType, type!.Assembly);
+                }
                 return serializers;
             }
         }
@@ -696,16 +699,9 @@ internal static bool GenerateSerializer(Type[]? types, XmlMapping[] mappings, St
         private static XmlSerializer[] GetSerializersFromCache(XmlMapping[] mappings, Type type)
         {
             XmlSerializer?[] serializers = new XmlSerializer?[mappings.Length];
-
             Dictionary<XmlSerializerMappingKey, XmlSerializer>? typedMappingTable = null;
-            lock (s_xmlSerializerTable)
-            {
-                if (!s_xmlSerializerTable.TryGetValue(type, out typedMappingTable))
-                {
-                    typedMappingTable = new Dictionary<XmlSerializerMappingKey, XmlSerializer>();
-                    s_xmlSerializerTable[type] = typedMappingTable;
-                }
-            }
+
+            typedMappingTable = s_xmlSerializerTable.GetOrCreateValue(type, () => new Dictionary<XmlSerializerMappingKey, XmlSerializer>());
 
             lock (typedMappingTable)
             {

diff --git a/...l/tests/XmlSerializer/ReflectionOnly/System.Xml.XmlSerializer.ReflectionOnly.Tests.csproj b/...l/tests/XmlSerializer/ReflectionOnly/System.Xml.XmlSerializer.ReflectionOnly.Tests.csproj
@@ -3,9 +3,13 @@
     <DefineConstants>$(DefineConstants);ReflectionOnly</DefineConstants>
     <TargetFrameworks>$(NetCoreAppCurrent)</TargetFrameworks>
   </PropertyGroup>
+  <ItemGroup>
+    <ProjectReference Include="..\..\..\..\Microsoft.XmlSerializer.Generator\tests\SerializableAssembly.csproj" />
+    <TrimmerRootAssembly Include="SerializableAssembly" />
+  </ItemGroup>
   <ItemGroup>
     <Compile Include="$(CommonTestPath)System\Runtime\Serialization\Utils.cs" />
-    <Compile Include="$(TestSourceFolder)..\..\..\..\System.Runtime.Serialization.Xml\tests\SerializationTypes.cs" />
+    <None Include="$(TestSourceFolder)..\..\..\..\System.Runtime.Serialization.Xml\tests\SerializationTypes.cs" />
     <Compile Include="$(TestSourceFolder)..\..\..\..\System.Runtime.Serialization.Xml\tests\SerializationTypes.RuntimeOnly.cs" />
     <Compile Include="$(TestSourceFolder)..\XmlSerializerTests.cs" />
     <Compile Include="$(TestSourceFolder)..\XmlSerializerTests.RuntimeOnly.cs" />