Assembly unload logic with COM interop asserts

[AaronRobinsonMSFT]

Originally posted by @janvorli in #77554 (comment)

I have just found that the freeing of the handle added in this PR causes assert in debug builds / crash in release ones during unloading. The problem is that at the time the DispatchMemberInfo destructor is called, the LoaderAllocator's handle table is already gone. It is stored in a managed array and a reference to it is stored in the managed LoaderAllocator.
We assert here in a callchain of the LoaderAllocator::FreeHandle:

runtime/src/coreclr/vm/loaderallocator.cpp

Lines 984 to 989 in b33aede

	_ASSERTE(!ObjectHandleIsNull(m_hLoaderAllocatorObjectHandle));
	UINT_PTR index = (((UINT_PTR)handle) >> 1) - 1;
	LOADERALLOCATORREF loaderAllocator = (LOADERALLOCATORREF)ObjectFromHandle(m_hLoaderAllocatorObjectHandle);
	PTRARRAYREF handleTable = loaderAllocator->GetHandleTable();
	handleTable->SetAt(index, value);

Here is the relevant part of the call stack:

 # Child-SP          RetAddr               Call Site
00 000000ee`a107b728 00007ffb`ff20902e     KERNELBASE!DebugBreak+0x2
01 000000ee`a107b730 00007ffb`fe9ddfc3     coreclr!DbgAssertDialog+0x34e [F:\git\runtime7\src\coreclr\utilcode\debug.cpp @ 464] 
02 000000ee`a107b970 00007ffb`fe9cfbc8     coreclr!LoaderAllocator::SetHandleValue+0x393 [F:\git\runtime7\src\coreclr\vm\loaderallocator.cpp @ 984] 
03 000000ee`a107bc70 00007ffb`fefd8be8     coreclr!LoaderAllocator::FreeHandle+0x2b8 [F:\git\runtime7\src\coreclr\vm\loaderallocator.cpp @ 880] 
04 000000ee`a107be40 00007ffb`fefd9488     coreclr!DispatchMemberInfo::~DispatchMemberInfo+0x328 [F:\git\runtime7\src\coreclr\vm\dispatchinfo.cpp @ 144] 
05 000000ee`a107bfa0 00007ffb`fefd883e     coreclr!DispatchMemberInfo::`scalar deleting destructor'+0x18
06 000000ee`a107bfd0 00007ffb`fefd9448     coreclr!DispatchInfo::~DispatchInfo+0x1ee [F:\git\runtime7\src\coreclr\vm\dispatchinfo.cpp @ 1065] 
07 000000ee`a107c110 00007ffb`fef8db8a     coreclr!DispatchInfo::`scalar deleting destructor'+0x18
08 000000ee`a107c140 00007ffb`fefaf523     coreclr!ComMethodTable::Cleanup+0x2da [F:\git\runtime7\src\coreclr\vm\comcallablewrapper.cpp @ 3105] 
09 000000ee`a107c290 00007ffb`fef8d84a     coreclr!ComMethodTable::Release+0x2e3 [F:\git\runtime7\src\coreclr\vm\comcallablewrapper.h @ 435] 
0a 000000ee`a107c450 00007ffb`fefaf1ee     coreclr!ComCallWrapperTemplate::Cleanup+0x4fa [F:\git\runtime7\src\coreclr\vm\comcallablewrapper.cpp @ 3907] 
0b 000000ee`a107c730 00007ffb`fe8171da     coreclr!ComCallWrapperTemplate::Release+0x2ae [F:\git\runtime7\src\coreclr\vm\comcallablewrapper.cpp @ 3938] 
0c 000000ee`a107c8c0 00007ffb`fe7e5ec9     coreclr!EEClass::Destruct+0x3aa [F:\git\runtime7\src\coreclr\vm\class.cpp @ 136] 
0d 000000ee`a107cf00 00007ffb`fe7df081     coreclr!Module::FreeClassTables+0x409 [F:\git\runtime7\src\coreclr\vm\ceeload.cpp @ 2016] 
0e 000000ee`a107d200 00007ffb`fe8c9c81     coreclr!Module::Destruct+0x441 [F:\git\runtime7\src\coreclr\vm\ceeload.cpp @ 669] 
0f 000000ee`a107d9d0 00007ffb`fe844581     coreclr!EditAndContinueModule::Destruct+0x41 [F:\git\runtime7\src\coreclr\vm\encee.cpp @ 83] 
10 000000ee`a107da00 00007ffb`fe8382ef     coreclr!ClassLoader::FreeModules+0x3c1 [F:\git\runtime7\src\coreclr\vm\clsload.cpp @ 1885] 
11 000000ee`a107dc60 00007ffb`fe77b6f8     coreclr!ClassLoader::~ClassLoader+0x29f [F:\git\runtime7\src\coreclr\vm\clsload.cpp @ 1947] 
12 000000ee`a107de10 00007ffb`fe78c6b3     coreclr!ClassLoader::`scalar deleting destructor'+0x18
13 000000ee`a107de40 00007ffb`fe77a341     coreclr!Assembly::Terminate+0x123 [F:\git\runtime7\src\coreclr\vm\assembly.cpp @ 310] 
14 000000ee`a107df30 00007ffb`fe77b638     coreclr!Assembly::~Assembly+0x191 [F:\git\runtime7\src\coreclr\vm\assembly.cpp @ 243] 
15 000000ee`a107e050 00007ffb`fe894666     coreclr!Assembly::`scalar deleting destructor'+0x18
16 000000ee`a107e080 00007ffb`fe726ed8     coreclr!DomainAssembly::~DomainAssembly+0x336 [F:\git\runtime7\src\coreclr\vm\domainassembly.cpp @ 91] 
17 000000ee`a107e250 00007ffb`fe9cffff     coreclr!DomainAssembly::`scalar deleting destructor'+0x18
18 000000ee`a107e280 00007ffb`fe9ce856     coreclr!LoaderAllocator::GCLoaderAllocators+0x39f [F:\git\runtime7\src\coreclr\vm\loaderallocator.cpp @ 565] 
19 000000ee`a107e4e0 00007ffb`fe9df922     coreclr!LoaderAllocator::Destroy+0x3b6 [F:\git\runtime7\src\coreclr\vm\loaderallocator.cpp @ 698] 
1a 000000ee`a107e6b0 00007ffb`5e288679     coreclr!LoaderAllocator_Destroy+0x212 [F:\git\runtime7\src\coreclr\vm\loaderallocator.cpp @ 714] 
1b 000000ee`a107e8a0 00007ffb`5e2886c7     System_Private_CoreLib!System.Reflection.LoaderAllocatorScout.Destroy+0x39 [F:\git\runtime7\src\coreclr\System.Private.CoreLib\Microsoft.Interop.LibraryImportGenerator\Microsoft.Interop.LibraryImportGenerator\LibraryImports.g.cs @ 1115] 
1c 000000ee`a107e970 00007ffb`ff191fe6     System_Private_CoreLib!System.Reflection.LoaderAllocatorScout.Finalize+0x17 [F:\git\runtime7\src\coreclr\System.Private.CoreLib\src\System\Reflection\LoaderAllocator.cs @ 39] 
1d 000000ee`a107e9a0 00007ffb`fea21f45     coreclr!FastCallFinalizeWorker+0x6

The LoaderAllocator_Destroy is called from the finalizer of the LoaderAllocatorScout, which in turn is invoked as a result of the managed LoaderAllocator being collected.

Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

---

I have just found that the freeing of the handle added in this PR causes assert in debug builds / crash in release ones during unloading. The problem is that at the time the DispatchMemberInfo destructor is called, the LoaderAllocator's handle table is already gone. It is stored in a managed array and a reference to it is stored in the managed LoaderAllocator.
We assert here in a callchain of the LoaderAllocator::FreeHandle:

runtime/src/coreclr/vm/loaderallocator.cpp

Lines 984 to 989 in b33aede

	_ASSERTE(!ObjectHandleIsNull(m_hLoaderAllocatorObjectHandle));
	UINT_PTR index = (((UINT_PTR)handle) >> 1) - 1;
	LOADERALLOCATORREF loaderAllocator = (LOADERALLOCATORREF)ObjectFromHandle(m_hLoaderAllocatorObjectHandle);
	PTRARRAYREF handleTable = loaderAllocator->GetHandleTable();
	handleTable->SetAt(index, value);

Here is the relevant part of the call stack:

 # Child-SP          RetAddr               Call Site
00 000000ee`a107b728 00007ffb`ff20902e     KERNELBASE!DebugBreak+0x2
01 000000ee`a107b730 00007ffb`fe9ddfc3     coreclr!DbgAssertDialog+0x34e [F:\git\runtime7\src\coreclr\utilcode\debug.cpp @ 464] 
02 000000ee`a107b970 00007ffb`fe9cfbc8     coreclr!LoaderAllocator::SetHandleValue+0x393 [F:\git\runtime7\src\coreclr\vm\loaderallocator.cpp @ 984] 
03 000000ee`a107bc70 00007ffb`fefd8be8     coreclr!LoaderAllocator::FreeHandle+0x2b8 [F:\git\runtime7\src\coreclr\vm\loaderallocator.cpp @ 880] 
04 000000ee`a107be40 00007ffb`fefd9488     coreclr!DispatchMemberInfo::~DispatchMemberInfo+0x328 [F:\git\runtime7\src\coreclr\vm\dispatchinfo.cpp @ 144] 
05 000000ee`a107bfa0 00007ffb`fefd883e     coreclr!DispatchMemberInfo::`scalar deleting destructor'+0x18
06 000000ee`a107bfd0 00007ffb`fefd9448     coreclr!DispatchInfo::~DispatchInfo+0x1ee [F:\git\runtime7\src\coreclr\vm\dispatchinfo.cpp @ 1065] 
07 000000ee`a107c110 00007ffb`fef8db8a     coreclr!DispatchInfo::`scalar deleting destructor'+0x18
08 000000ee`a107c140 00007ffb`fefaf523     coreclr!ComMethodTable::Cleanup+0x2da [F:\git\runtime7\src\coreclr\vm\comcallablewrapper.cpp @ 3105] 
09 000000ee`a107c290 00007ffb`fef8d84a     coreclr!ComMethodTable::Release+0x2e3 [F:\git\runtime7\src\coreclr\vm\comcallablewrapper.h @ 435] 
0a 000000ee`a107c450 00007ffb`fefaf1ee     coreclr!ComCallWrapperTemplate::Cleanup+0x4fa [F:\git\runtime7\src\coreclr\vm\comcallablewrapper.cpp @ 3907] 
0b 000000ee`a107c730 00007ffb`fe8171da     coreclr!ComCallWrapperTemplate::Release+0x2ae [F:\git\runtime7\src\coreclr\vm\comcallablewrapper.cpp @ 3938] 
0c 000000ee`a107c8c0 00007ffb`fe7e5ec9     coreclr!EEClass::Destruct+0x3aa [F:\git\runtime7\src\coreclr\vm\class.cpp @ 136] 
0d 000000ee`a107cf00 00007ffb`fe7df081     coreclr!Module::FreeClassTables+0x409 [F:\git\runtime7\src\coreclr\vm\ceeload.cpp @ 2016] 
0e 000000ee`a107d200 00007ffb`fe8c9c81     coreclr!Module::Destruct+0x441 [F:\git\runtime7\src\coreclr\vm\ceeload.cpp @ 669] 
0f 000000ee`a107d9d0 00007ffb`fe844581     coreclr!EditAndContinueModule::Destruct+0x41 [F:\git\runtime7\src\coreclr\vm\encee.cpp @ 83] 
10 000000ee`a107da00 00007ffb`fe8382ef     coreclr!ClassLoader::FreeModules+0x3c1 [F:\git\runtime7\src\coreclr\vm\clsload.cpp @ 1885] 
11 000000ee`a107dc60 00007ffb`fe77b6f8     coreclr!ClassLoader::~ClassLoader+0x29f [F:\git\runtime7\src\coreclr\vm\clsload.cpp @ 1947] 
12 000000ee`a107de10 00007ffb`fe78c6b3     coreclr!ClassLoader::`scalar deleting destructor'+0x18
13 000000ee`a107de40 00007ffb`fe77a341     coreclr!Assembly::Terminate+0x123 [F:\git\runtime7\src\coreclr\vm\assembly.cpp @ 310] 
14 000000ee`a107df30 00007ffb`fe77b638     coreclr!Assembly::~Assembly+0x191 [F:\git\runtime7\src\coreclr\vm\assembly.cpp @ 243] 
15 000000ee`a107e050 00007ffb`fe894666     coreclr!Assembly::`scalar deleting destructor'+0x18
16 000000ee`a107e080 00007ffb`fe726ed8     coreclr!DomainAssembly::~DomainAssembly+0x336 [F:\git\runtime7\src\coreclr\vm\domainassembly.cpp @ 91] 
17 000000ee`a107e250 00007ffb`fe9cffff     coreclr!DomainAssembly::`scalar deleting destructor'+0x18
18 000000ee`a107e280 00007ffb`fe9ce856     coreclr!LoaderAllocator::GCLoaderAllocators+0x39f [F:\git\runtime7\src\coreclr\vm\loaderallocator.cpp @ 565] 
19 000000ee`a107e4e0 00007ffb`fe9df922     coreclr!LoaderAllocator::Destroy+0x3b6 [F:\git\runtime7\src\coreclr\vm\loaderallocator.cpp @ 698] 
1a 000000ee`a107e6b0 00007ffb`5e288679     coreclr!LoaderAllocator_Destroy+0x212 [F:\git\runtime7\src\coreclr\vm\loaderallocator.cpp @ 714] 
1b 000000ee`a107e8a0 00007ffb`5e2886c7     System_Private_CoreLib!System.Reflection.LoaderAllocatorScout.Destroy+0x39 [F:\git\runtime7\src\coreclr\System.Private.CoreLib\Microsoft.Interop.LibraryImportGenerator\Microsoft.Interop.LibraryImportGenerator\LibraryImports.g.cs @ 1115] 
1c 000000ee`a107e970 00007ffb`ff191fe6     System_Private_CoreLib!System.Reflection.LoaderAllocatorScout.Finalize+0x17 [F:\git\runtime7\src\coreclr\System.Private.CoreLib\src\System\Reflection\LoaderAllocator.cs @ 39] 
1d 000000ee`a107e9a0 00007ffb`fea21f45     coreclr!FastCallFinalizeWorker+0x6

The LoaderAllocator_Destroy is called from the finalizer of the LoaderAllocatorScout, which in turn is invoked as a result of the managed LoaderAllocator being collected.

Originally posted by @janvorli in #77554 (comment)

Author:	AaronRobinsonMSFT
Assignees:	-
Labels:	area-AssemblyLoader-coreclr
Milestone:	8.0.0

[janvorli]

@AaronRobinsonMSFT I have discovered it when running coreclr tests with runincontext option (src\tests\run.cmd runincontext). The Interop\PInvoke\Delegate\DelegateTest and Interop\PInvoke\Variant\VariantTestComWrappers fail with this option (there are 14 tests that fail, I am working on a fix for another one that crashes, the rest just doesn't unload, may be just an incompatibility of the test with unloadability).
To run just single tests with runincontext, you can use a this (with paths replaced by your ones, the referencespath and the test assembly path need to be absolute) :

"F:\git\runtime7\artifacts\tests\coreclr\windows.x64.Debug\Tests\Core_Root\corerun.exe" "F:\git\runtime7\artifacts\tests\coreclr\windows.x64.Debug\Tests\Core_Root\runincontext.dll"  /referencespath:F:\git\runtime7\artifacts\tests\coreclr\windows.x64.Debug\Tests\Core_Root\ F:\git\runtime7\artifacts\tests\coreclr\windows.x64.Debug\Interop\PInvoke\Delegate\DelegateTest\DelegateTest.dll

[AaronRobinsonMSFT]

@janvorli I'm not sure the right fix here. The unload logic is rather dense and from some basic changes it is more fragile than I thought, at least with approaches I am considering. Do you have a suggestion as to the best way to fix this? I am considering the following options:

1. Extend the ALC lifetime to be associated with the ComMethodTable or DispatchInfo
2. Perform a type of "is alive" check prior to cleaning up the handle in the DispatchInfo
3. Similar to previous, but instead rely upon AddReferenceIfAlive() and if that fails, don't free the handle

Logically (1), trying to couple the lifetimes, seems the most correct, but it isn't clear how to make that correct because the design has few obvious patterns or invariants I can follow and be sure. Option (2) isn't something I am too confident of because this is entire system seems lockless and I am just introducing a future race. Option (3) seems like something I can do with some confidence but I don't know if I am missing something there. Any thoughts?

[janvorli]

@AaronRobinsonMSFT I wonder - shouldn't the object that the DispatchMemberInfo::m_hndMemberInfo references keep the ALC alive? Maybe we are allocating that object using a wrong (the default?) ALC. Or is it the case that when I hit the issue, the handle (if the ALC's handle table still existed) would already be NULL?

[AaronRobinsonMSFT]

@janvorli We are using the LoaderAllocator associated with the MethodTable, so I think it should be correct, no?

runtime/src/coreclr/vm/dispatchinfo.h

Lines 296 to 299 in ad4bdd1

	LoaderAllocator* GetLoaderAllocator()
	{
	return m_pMT->GetLoaderAllocator();
	}