[main] Fix LoaderAllocator computation for a generic type instance

src/coreclr/vm/clsload.cpp

@@ -78,14 +78,14 @@ NameHandle::NameHandle(Module* pModule, mdToken token) :
 // This method determines the "loader module" for an instantiated type
 // or method. The rule must ensure that any types involved in the
 // instantiated type or method do not outlive the loader module itself
-// with respect to app-domain unloading (e.g. MyList<MyType> can't be
+// with respect to module unloading (e.g. MyList<MyType> can't be
 // put in the module of MyList if MyList's assembly is
-// app-domain-neutral but MyType's assembly is app-domain-specific).
+// non-collectible but MyType's assembly is collectible).
 // The rule we use is:
 //
 // * Pick the first type in the class instantiation, followed by
-//   method instantiation, whose loader module is non-shared (app-domain-bound)
-// * If no type is app-domain-bound, return the module containing the generic type itself
+//   method instantiation, whose loader allocator is collectible and has the highest creation number.
+// * If no type is in collectible assembly, return the module containing the generic type itself.
 //
 // Some useful effects of this rule (for ngen purposes) are:
 //
@@ -113,18 +113,15 @@ PTR_Module ClassLoader::ComputeLoaderModuleWorker(
     }
     CONTRACT_END
 
+    // No generic instantiation, return the definition module
     if (classInst.IsEmpty() && methodInst.IsEmpty())
         RETURN PTR_Module(pDefinitionModule);
 
-    Module *pLoaderModule = NULL;
-
-    if (pDefinitionModule)
-    {
-        if (pDefinitionModule->IsCollectible())
-            goto ComputeCollectibleLoaderModule;
-        pLoaderModule = pDefinitionModule;
-    }
+    // Use the definition module as the loader module by default
+    Module *pLoaderModule = pDefinitionModule;
 
+    // If any of generic type arguments are in collectible module,
+    // we use a generic procedure.
     for (DWORD i = 0; i < classInst.GetNumArgs(); i++)
     {
         TypeHandle classArg = classInst[i];
@@ -135,6 +132,8 @@ PTR_Module ClassLoader::ComputeLoaderModuleWorker(
             pLoaderModule = pModule;
     }
 
+    // If any of generic method arguments are in collectible module,
+    // we also use a generic procedure.
     for (DWORD i = 0; i < methodInst.GetNumArgs(); i++)
     {
         TypeHandle methodArg = methodInst[i];
@@ -155,14 +154,18 @@ PTR_Module ClassLoader::ComputeLoaderModuleWorker(
     if (FALSE)
     {
 ComputeCollectibleLoaderModule:
-        LoaderAllocator *pLoaderAllocatorOfDefiningType = NULL;
-        LoaderAllocator *pOldestLoaderAllocator = NULL;
-        Module *pOldestLoaderModule = NULL;
-        UINT64 oldestFoundAge = 0;
+        Module *pLatestLoaderModule = NULL;
+        UINT64 latestFoundNumber = 0;
         DWORD classArgsCount = classInst.GetNumArgs();
         DWORD totalArgsCount = classArgsCount + methodInst.GetNumArgs();
 
-        if (pDefinitionModule != NULL) pLoaderAllocatorOfDefiningType = pDefinitionModule->GetLoaderAllocator();
+        // If loader allocator of the defining type is collectible, we use it
+        // and its creation number as the starting age.
+        if (pDefinitionModule != NULL && pDefinitionModule->IsCollectible())
+        {
+            pLatestLoaderModule = pDefinitionModule;
+            latestFoundNumber = pDefinitionModule->GetLoaderAllocator()->GetCreationNumber();
+        }
 
         for (DWORD i = 0; i < totalArgsCount; i++) {
 
@@ -176,22 +179,18 @@ PTR_Module ClassLoader::ComputeLoaderModuleWorker(
             Module *pModuleCheck = arg.GetLoaderModule();
             LoaderAllocator *pLoaderAllocatorCheck = pModuleCheck->GetLoaderAllocator();
 
-            if (pLoaderAllocatorCheck != pLoaderAllocatorOfDefiningType &&
-                pLoaderAllocatorCheck->IsCollectible() &&
-                pLoaderAllocatorCheck->GetCreationNumber() > oldestFoundAge)
+            if (pLoaderAllocatorCheck->IsCollectible() &&
+                pLoaderAllocatorCheck->GetCreationNumber() > latestFoundNumber)
             {
-                pOldestLoaderModule = pModuleCheck;
-                pOldestLoaderAllocator = pLoaderAllocatorCheck;
-                oldestFoundAge = pLoaderAllocatorCheck->GetCreationNumber();
+                pLatestLoaderModule = pModuleCheck;
+                latestFoundNumber = pLoaderAllocatorCheck->GetCreationNumber();
             }
         }
 
-        // Only if we didn't find a different loader allocator than the defining loader allocator do we
-        // use the defining loader allocator
-        if (pOldestLoaderModule != NULL)
-            pLoaderModule = pOldestLoaderModule;
-        else
-            pLoaderModule = pDefinitionModule;
+        // Use the module of the latest found collectible loader allocator.
+        // If nothing was found, then by default we use the defining module.
+        if (pLatestLoaderModule != NULL)
+            pLoaderModule = pLatestLoaderModule;
     }
     RETURN PTR_Module(pLoaderModule);
 }

src/libraries/System.Runtime.Loader/tests/AssemblyLoadContextTest.cs

@@ -1,14 +1,10 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
-using Xunit;
-using System;
-using System.Collections.Generic;
 using System.IO;
-using System.Linq;
 using System.Reflection;
 using System.Reflection.Emit;
-using System.Threading.Tasks;
+using Xunit;
 
 namespace System.Runtime.Loader.Tests
 {
@@ -189,7 +185,7 @@ public static void DefaultAssemblyLoadContext_Properties()
             Assert.Contains("\"Default\"", alc.ToString());
             Assert.Contains("System.Runtime.Loader.DefaultAssemblyLoadContext", alc.ToString());
             Assert.Contains(alc, AssemblyLoadContext.All);
-            Assert.Contains(Assembly.GetCallingAssembly(), alc.Assemblies);
+            Assert.Contains(typeof(int).Assembly, alc.Assemblies);
         }
 
         [Fact]

src/libraries/System.Runtime.Loader/tests/CollectibleAssemblyLoadContextTest.cs

@@ -32,7 +32,7 @@ static void CreateAndLoadContext(CollectibleChecker checker, bool unloadTwice =
         }
 
         [Fact]
-        [ActiveIssue("https://github.com/mono/mono/issues/15142", TestRuntimes.Mono)]
+        [ActiveIssue("https://github.com/dotnet/runtime/issues/34072", TestRuntimes.Mono)]
         public static void Unload_CollectibleWithNoAssemblyLoaded()
         {
             // Use a collectible ALC + Unload
@@ -44,7 +44,7 @@ public static void Unload_CollectibleWithNoAssemblyLoaded()
         }
 
         [Fact]
-        [ActiveIssue("https://github.com/mono/mono/issues/15142", TestRuntimes.Mono)]
+        [ActiveIssue("https://github.com/dotnet/runtime/issues/34072", TestRuntimes.Mono)]
         public static void DoubleUnload_CollectibleWithNoAssemblyLoaded()
         {
             // Use a collectible ALC + Unload
@@ -91,15 +91,15 @@ public TestBase(int numContexts)
             }
 
             [MethodImpl(MethodImplOptions.NoInlining)]
-            public void CreateContextAndLoadAssembly(int contextIndex = 0)
+            public void CreateContextAndLoadAssembly(int contextIndex = 0, string testClassName = "TestClass")
             {
                 var asmName = new AssemblyName(TestAssembly);
                 _contexts[contextIndex] = new ResourceAssemblyLoadContext(true) { LoadBy = LoadBy.Path };
 
                 Assembly asm = _contexts[contextIndex].LoadFromAssemblyName(asmName);
 
                 Assert.NotNull(asm);
-                _testClassTypes[contextIndex] = asm.DefinedTypes.FirstOrDefault(t => t.Name == "TestClass");
+                _testClassTypes[contextIndex] = asm.DefinedTypes.FirstOrDefault(t => t.Name == testClassName);
                 Assert.NotNull(_testClassTypes[contextIndex]);
 
                 _checker.SetAssemblyLoadContext(contextIndex, _contexts[contextIndex]);
@@ -126,7 +126,7 @@ class CollectibleWithOneAssemblyLoadedTest : TestBase
         }
 
         [Fact]
-        [ActiveIssue("https://github.com/mono/mono/issues/15142", TestRuntimes.Mono)]
+        [ActiveIssue("https://github.com/dotnet/runtime/issues/34072", TestRuntimes.Mono)]
         public static void Unload_CollectibleWithOneAssemblyLoaded()
         {
             // Use a collectible ALC + Load an assembly by path + Unload
@@ -153,7 +153,7 @@ public void Execute()
         }
 
         [Fact]
-        [ActiveIssue("https://github.com/mono/mono/issues/15142", TestRuntimes.Mono)]
+        [ActiveIssue("https://github.com/dotnet/runtime/issues/34072", TestRuntimes.Mono)]
         public static void Unload_CollectibleWithOneAssemblyLoadedWithStatic()
         {
             // Use a collectible ALC + Load an assembly by path + New Instance + Static reference + Unload
@@ -185,7 +185,7 @@ public void CheckInstanceUnloaded()
         }
 
         [Fact]
-        [ActiveIssue("https://github.com/mono/mono/issues/15142", TestRuntimes.Mono)]
+        [ActiveIssue("https://github.com/dotnet/runtime/issues/34072", TestRuntimes.Mono)]
         public static void Unload_CollectibleWithOneAssemblyLoadedWithWeakReferenceToType()
         {
             // Use a collectible ALC + Load an assembly by path + WeakReference on the Type + Unload
@@ -219,7 +219,7 @@ public void CheckInstanceUnloaded()
         }
 
         [Fact]
-        [ActiveIssue("https://github.com/mono/mono/issues/15142", TestRuntimes.Mono)]
+        [ActiveIssue("https://github.com/dotnet/runtime/issues/34072", TestRuntimes.Mono)]
         public static void Unload_CollectibleWithOneAssemblyLoadedWithWeakReferenceToInstance()
         {
             // Use a collectible ALC + Load an assembly by path + WeakReference on an instance of a Type + Unload
@@ -271,7 +271,7 @@ public void CheckTypeUnloaded()
         }
 
         [Fact]
-        [ActiveIssue("https://github.com/mono/mono/issues/15142", TestRuntimes.Mono)]
+        [ActiveIssue("https://github.com/dotnet/runtime/issues/34072", TestRuntimes.Mono)]
         public static void Unload_CollectibleWithOneAssemblyLoadedWithStrongReferenceToType()
         {
             // Use a collectible ALC + Load an assembly by path + Strong reference on the Type + Unload
@@ -332,7 +332,7 @@ public void CheckInstanceUnloaded()
         }
 
         [Fact]
-        [ActiveIssue("https://github.com/mono/mono/issues/15142", TestRuntimes.Mono)]
+        [ActiveIssue("https://github.com/dotnet/runtime/issues/34072", TestRuntimes.Mono)]
         public static void Unload_CollectibleWithOneAssemblyLoadedWithStrongReferenceToInstance()
         {
             // Use a collectible ALC + Load an assembly by path + Strong reference on an instance of a Type + Unload
@@ -366,7 +366,7 @@ public void Execute()
         }
 
         [Fact]
-        [ActiveIssue("https://github.com/mono/mono/issues/15142", TestRuntimes.Mono)]
+        [ActiveIssue("https://github.com/dotnet/runtime/issues/34072", TestRuntimes.Mono)]
         public static void Unload_CollectibleWithTwoAssemblies()
         {
             // Use a collectible ALC + Load two assemblies (path + stream) + Unload
@@ -427,7 +427,7 @@ public void CheckContextUnloaded2()
         }
 
         [Fact]
-        [ActiveIssue("https://github.com/mono/mono/issues/15142", TestRuntimes.Mono)]
+        [ActiveIssue("https://github.com/dotnet/runtime/issues/34072", TestRuntimes.Mono)]
         public static void Unload_TwoCollectibleWithOneAssemblyAndOneInstanceReferencingAnother()
         {
             // We create 2 collectible ALC, load one assembly in each, create one instance in each, reference one instance from ALC1 to ALC2
@@ -453,6 +453,70 @@ public static void Unload_TwoCollectibleWithOneAssemblyAndOneInstanceReferencing
             test.CheckContextUnloaded1();
         }
 
+        class TwoCollectibleWithOneAssemblyAndOneInstanceReferencingAnotherThroughGenericStaticTest : TestBase
+        {
+            public TwoCollectibleWithOneAssemblyAndOneInstanceReferencingAnotherThroughGenericStaticTest() : base(2)
+            {
+            }
+
+            [MethodImpl(MethodImplOptions.NoInlining)]
+            public void Execute()
+            {
+                // Make an instance in ALC2 and assign it to a static field in a generic type declared in ALC1,
+                // but with a genric parameter in ALC2 and thus instantiated in ALC2.
+                Type type = _testClassTypes[0].MakeGenericType(new [] {_testClassTypes[1]});
+                FieldInfo field = type.GetField("StaticObjectRef");
+                Assert.NotNull(field);
+
+                object instance = Activator.CreateInstance(_testClassTypes[1]);
+                field.SetValue(null, instance);
+            }
+
+            [MethodImpl(MethodImplOptions.NoInlining)]
+            public void CheckNotUnloaded()
+            {
+                // None of the AssemblyLoadContexts should be unloaded
+                _checker.GcAndCheck(0);
+            }
+
+            [MethodImpl(MethodImplOptions.NoInlining)]
+            public void CheckContextUnloaded1()
+            {
+                // The AssemblyLoadContext should now be unloaded
+                _checker.GcAndCheck();
+            }
+
+            [MethodImpl(MethodImplOptions.NoInlining)]
+            public void CheckContextUnloaded2()
+            {
+                // The AssemblyLoadContext should now be unloaded
+                _checker.GcAndCheck(1);
+            }
+        }
+
+        // Test may fail when running on a different runtime
+        [Fact]
+        [ActiveIssue("https://github.com/dotnet/runtime/issues/34072", TestRuntimes.Mono)]
+        public static void Unload_TwoCollectibleWithOneAssemblyAndOneInstanceReferencingAnotherThroughGenericStatic()
+        {
+            // We create 2 collectible ALC, load one assembly in each, create one instance in the ALC2,
+            // reference it from ALC1 to ALC2 using a generic static variable.
+            // unload ALC2 -> check that instance is not there and we receive one unload
+            // unload ALC1 -> we should receive 1 unload
+
+            var test = new TwoCollectibleWithOneAssemblyAndOneInstanceReferencingAnotherThroughGenericStaticTest();
+            test.CreateContextAndLoadAssembly(0, "GenericTestClass`1");
+            test.CreateContextAndLoadAssembly(1);
+
+            test.Execute();
+
+            test.UnloadAndClearContext(1);
+            test.CheckContextUnloaded2();
+
+            test.UnloadAndClearContext(0);
+            test.CheckContextUnloaded1();
+        }
+
         private class CollectibleChecker
         {
             private readonly int _expectedCount;

src/libraries/System.Runtime.Loader/tests/System.Runtime.Loader.Test.Assembly/TestClass.cs

@@ -36,4 +36,11 @@ public class COMClass
         {
         }
     }
+
+    // Used to test LoaderAllocator used for a generic type instance allocation
+    public class GenericTestClass<T>
+    {
+        // Allow to store a static reference (either an instance of the ALC or an instance outside of it)
+        public static T StaticObjectRef;
+    }
 }