[release/6.0] Unloadability bugs fixes (#68674)

* [Release/6.0] Port unloadability fixes

This change ports two unloadability fixes for issues found by a
customer.
* #68550 that adds reference between native
`LoaderAllocator`s of two collectible `AssemblyLoadContexts` when one of
them is used to resolve assembly using the other one. This reference
ensures that the one that provides the resolved `Assembly` isn't
collected before the one that uses it.
* #68336 that adds a missing lock around `m_LoaderAllocatorReferences`
iteration during assembly load context destruction.

* Remove breaking change part

The change in main has added an exception in case a collectible
`Assembly` is resolved into a non-collectible `AssemblyLoadContext`.
Although that is incorrect, there are cases when it would work. So the
added exception is a breaking change that I think we likely don't want
to get into 6.0

Author: janvorli

src/coreclr/binder/assemblybinder.cpp

@@ -33,6 +33,7 @@
 extern HRESULT RuntimeInvokeHostAssemblyResolver(INT_PTR pManagedAssemblyLoadContextToBindWithin,
                                                  BINDER_SPACE::AssemblyName *pAssemblyName,
                                                  CLRPrivBinderCoreCLR *pTPABinder,
+                                                 AssemblyLoadContext *pBinder,
                                                  ICLRPrivAssembly **ppLoadedAssembly);
 
 #endif // !defined(DACCESS_COMPILE) && !defined(CROSSGEN_COMPILE)
@@ -1408,6 +1409,7 @@ namespace BINDER_SPACE
 HRESULT AssemblyBinder::BindUsingHostAssemblyResolver(/* in */ INT_PTR pManagedAssemblyLoadContextToBindWithin,
                                                       /* in */ AssemblyName       *pAssemblyName,
                                                       /* in */ CLRPrivBinderCoreCLR *pTPABinder,
+                                                      /* in */ AssemblyLoadContext *pBinder,
                                                       /* out */ Assembly           **ppAssembly)
 {
     HRESULT hr = E_FAIL;
@@ -1417,7 +1419,7 @@ HRESULT AssemblyBinder::BindUsingHostAssemblyResolver(/* in */ INT_PTR pManagedA
     // RuntimeInvokeHostAssemblyResolver will perform steps 2-4 of CLRPrivBinderAssemblyLoadContext::BindAssemblyByName.
     ICLRPrivAssembly *pLoadedAssembly = NULL;
     hr = RuntimeInvokeHostAssemblyResolver(pManagedAssemblyLoadContextToBindWithin,
-                                           pAssemblyName, pTPABinder, &pLoadedAssembly);
+                                           pAssemblyName, pTPABinder, pBinder, &pLoadedAssembly);
     if (SUCCEEDED(hr))
     {
         _ASSERTE(pLoadedAssembly != NULL);

src/coreclr/binder/clrprivbinderassemblyloadcontext.cpp

@@ -84,7 +84,7 @@ HRESULT CLRPrivBinderAssemblyLoadContext::BindAssemblyByName(AssemblyNameData *p
             // of what to do next. The host-overridden binder can either fail the bind or return reference to an existing assembly
             // that has been loaded.
             //
-            hr = AssemblyBinder::BindUsingHostAssemblyResolver(GetManagedAssemblyLoadContext(), pAssemblyName, m_pTPABinder, &pCoreCLRFoundAssembly);
+            hr = AssemblyBinder::BindUsingHostAssemblyResolver(GetManagedAssemblyLoadContext(), pAssemblyName, m_pTPABinder, this, &pCoreCLRFoundAssembly);
             if (SUCCEEDED(hr))
             {
                 // We maybe returned an assembly that was bound to a different AssemblyLoadContext instance.
@@ -258,11 +258,13 @@ void CLRPrivBinderAssemblyLoadContext::PrepareForLoadContextRelease(INT_PTR ptrM
     _ASSERTE(m_pAssemblyLoaderAllocator != NULL);
     _ASSERTE(m_loaderAllocatorHandle != NULL);
 
-    // We cannot delete the binder here as it is used indirectly when comparing assemblies with the same binder
-    // It will be deleted when the LoaderAllocator will be deleted
-    // But we can release the LoaderAllocator as we are no longer using it here
+    // We need to keep the LoaderAllocator pointer set as it still may be needed for creating references between the
+    // native LoaderAllocators of two collectible contexts in case the AssemblyLoadContext.Unload was called on the current
+    // context before returning from its AssemblyLoadContext.Load override or the context's Resolving event.
+    // But we need to release the LoaderAllocator so that it doesn't prevent completion of the final phase of unloading in
+    // some cases. It is safe to do as the AssemblyLoaderAllocator is guaranteed to be alive at least until the 
+    // CustomAssemblyBinder::ReleaseLoadContext is called, where we NULL this pointer.
     m_pAssemblyLoaderAllocator->Release();
-    m_pAssemblyLoaderAllocator = NULL;
 
     // Destroy the strong handle to the LoaderAllocator in order to let it reach its finalizer
     DestroyHandle(reinterpret_cast<OBJECTHANDLE>(m_loaderAllocatorHandle));
@@ -287,6 +289,10 @@ void CLRPrivBinderAssemblyLoadContext::ReleaseLoadContext()
     handle = reinterpret_cast<OBJECTHANDLE>(m_ptrManagedStrongAssemblyLoadContext);
     DestroyHandle(handle);
     m_ptrManagedAssemblyLoadContext = NULL;
+
+    // The AssemblyLoaderAllocator is in a process of shutdown and should not be used 
+    // after this point.
+    m_pAssemblyLoaderAllocator = NULL;
 }
 
 #endif // !defined(DACCESS_COMPILE) && !defined(CROSSGEN_COMPILE)

src/coreclr/binder/clrprivbindercoreclr.cpp

@@ -107,7 +107,7 @@ HRESULT CLRPrivBinderCoreCLR::BindUsingAssemblyName(BINDER_SPACE::AssemblyName *
         if (pManagedAssemblyLoadContext != NULL)
         {
             hr = AssemblyBinder::BindUsingHostAssemblyResolver(pManagedAssemblyLoadContext, pAssemblyName,
-                                                                NULL, &pCoreCLRFoundAssembly);
+                                                                NULL, this, &pCoreCLRFoundAssembly);
             if (SUCCEEDED(hr))
             {
                 // We maybe returned an assembly that was bound to a different AssemblyLoadContext instance.

src/coreclr/binder/inc/assemblybinder.hpp

@@ -21,6 +21,7 @@
 class CLRPrivBinderCoreCLR;
 class PEAssembly;
 class PEImage;
+class AssemblyLoadContext;
 
 namespace BINDER_SPACE
 {
@@ -59,6 +60,7 @@ namespace BINDER_SPACE
         static HRESULT BindUsingHostAssemblyResolver (/* in */ INT_PTR pManagedAssemblyLoadContextToBindWithin,
                                                       /* in */ AssemblyName       *pAssemblyName,
                                                       /* in */ CLRPrivBinderCoreCLR *pTPABinder,
+                                                      /* in */ AssemblyLoadContext *pBinder,
                                                       /* out */ Assembly           **ppAssembly);
 
         static HRESULT BindUsingPEImage(/* in */  ApplicationContext *pApplicationContext,

src/coreclr/vm/appdomain.cpp

@@ -5275,7 +5275,7 @@ AppDomain::AssemblyIterator::Next_Unlocked(
 #if !defined(DACCESS_COMPILE) && !defined(CROSSGEN_COMPILE)
 
 // Returns S_OK if the assembly was successfully loaded
-HRESULT RuntimeInvokeHostAssemblyResolver(INT_PTR pManagedAssemblyLoadContextToBindWithin, BINDER_SPACE::AssemblyName *pAssemblyName, CLRPrivBinderCoreCLR *pTPABinder, ICLRPrivAssembly **ppLoadedAssembly)
+HRESULT RuntimeInvokeHostAssemblyResolver(INT_PTR pManagedAssemblyLoadContextToBindWithin, BINDER_SPACE::AssemblyName *pAssemblyName, CLRPrivBinderCoreCLR *pTPABinder, AssemblyLoadContext *pBinder, ICLRPrivAssembly **ppLoadedAssembly)
 {
     CONTRACTL
     {
@@ -5453,6 +5453,21 @@ HRESULT RuntimeInvokeHostAssemblyResolver(INT_PTR pManagedAssemblyLoadContextToB
                 COMPlusThrowHR(COR_E_INVALIDOPERATION, IDS_HOST_ASSEMBLY_RESOLVER_DYNAMICALLY_EMITTED_ASSEMBLIES_UNSUPPORTED, name);
             }
 
+            // For collectible assemblies, ensure that the parent loader allocator keeps the assembly's loader allocator
+            // alive for all its lifetime.
+            if (pDomainAssembly->IsCollectible())
+            {
+                LoaderAllocator *pResultAssemblyLoaderAllocator = pDomainAssembly->GetLoaderAllocator();
+                LoaderAllocator *pParentLoaderAllocator = NULL;
+                hr = pBinder->GetLoaderAllocator((LPVOID*)&pParentLoaderAllocator);
+                if (SUCCEEDED(hr))
+                {
+                    _ASSERTE(pParentLoaderAllocator);
+                    _ASSERTE(pResultAssemblyLoaderAllocator);
+                    pParentLoaderAllocator->EnsureReference(pResultAssemblyLoaderAllocator);
+                }
+            }
+
             pResolvedAssembly = pLoadedPEAssembly->GetHostAssembly();
         }
 

src/coreclr/vm/loaderallocator.cpp

@@ -401,8 +401,9 @@ LoaderAllocator * LoaderAllocator::GCLoaderAllocators_RemoveAssemblies(AppDomain
 #endif //0
 
     AppDomain::AssemblyIterator i;
-    // Iterate through every loader allocator, marking as we go
     {
+        // Iterate through every loader allocator, marking as we go
+        CrstHolder chLoaderAllocatorReferencesLock(pAppDomain->GetLoaderAllocatorReferencesLock());
         CrstHolder chAssemblyListLock(pAppDomain->GetAssemblyListLock());
 
         i = pAppDomain->IterateAssembliesEx((AssemblyIterationFlags)(
@@ -424,17 +425,11 @@ LoaderAllocator * LoaderAllocator::GCLoaderAllocators_RemoveAssemblies(AppDomain
                 }
             }
         }
-    }
-
-    // Iterate through every loader allocator, unmarking marked loaderallocators, and
-    // build a free list of unmarked ones
-    {
-        CrstHolder chLoaderAllocatorReferencesLock(pAppDomain->GetLoaderAllocatorReferencesLock());
-        CrstHolder chAssemblyListLock(pAppDomain->GetAssemblyListLock());
 
+        // Iterate through every loader allocator, unmarking marked loaderallocators, and
+        // build a free list of unmarked ones
         i = pAppDomain->IterateAssembliesEx((AssemblyIterationFlags)(
             kIncludeExecution | kIncludeLoaded | kIncludeCollected));
-        CollectibleAssemblyHolder<DomainAssembly *> pDomainAssembly;
 
         while (i.Next_Unlocked(pDomainAssembly.This()))
         {