Skip to content

[release/6.0] Unloadability bugs fixes #68674

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
carlossanlop merged 2 commits into from
May 5, 2022
Merged

[release/6.0] Unloadability bugs fixes #68674

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e23a69e
[Release/6.0] Port unloadability fixes
janvorli Apr 28, 2022
f981a07
Remove breaking change part
janvorli Apr 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/coreclr/binder/assemblybinder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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;
Expand All @@ -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);
Expand Down
16 changes: 11 additions & 5 deletions src/coreclr/binder/clrprivbinderassemblyloadcontext.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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));
Expand All @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/binder/clrprivbindercoreclr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/binder/inc/assemblybinder.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
class CLRPrivBinderCoreCLR;
class PEAssembly;
class PEImage;
class AssemblyLoadContext;

namespace BINDER_SPACE
{
Expand Down Expand Up @@ -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,
Expand Down
17 changes: 16 additions & 1 deletion src/coreclr/vm/appdomain.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down Expand Up @@ -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();
}

Expand Down
13 changes: 4 additions & 9 deletions src/coreclr/vm/loaderallocator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)(
Expand All @@ -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()))
{
Expand Down