Add check for unloaded types in GetAppDomainStaticAddress

[davmason]

Fixes #33367

When a module is unloaded, the managed objectref is collected during a GC before the profiler is notified that the module is unloading. That means if you call in to GetAppDomainStaticAddress between when the object is collected and when you are notified about the module unload (GarbageCollectionFinished is a place that will hit this every time) it will cause an AV from trying to use the null objectref.

This fix prevents this AV by checking to see if the object's loaderheap or managed loaderheap object ref are invalid first.

[Dotnet-GitSync-Bot]

I couldn't figure out the best area label to add to this PR. Please help me learn by adding exactly one area label.

[janvorli]

@davmason I assume the runtime is suspended when we run this code, correct?

[davmason]

We don't enforce that the runtime is suspended, but it would be dangerous even without this fix. This API returns a pointer to a managed static field, so with collectible contexts the object could disappear on you if you call it at arbitrary times. The expected code sequence is that this is called in the GC finished profiler callback, which happens before the runtime is resumed.

[janvorli]

Great, thanks.

[noahfalk]

LGTM!

[janvorli]

LGTM, thank you!

[janvorli]

Great, thanks.

[janvorli]

@davmason I am not sure I understand the purpose of this change. The commit title says "prevent ALC from being finalized in test". While this change requests unload and disables finalization, the unload will still happen at some point in the future when all references to the collectibleContext, the assemblies loaded into it and all instances of types from those assemblies are gone. So there is not much difference from what finalizer would do.

[davmason]

Before this change the ALC was being intermittently collected and finalized before the call to GetType, causing failures in the test. By adding the call to Unload at the end of the method there is something rooting it and preventing it from being collected and finalized prematurely. So this isn’t about disabling finalization altogether, but preventing it from happening before the test is done using the assemblies loaded in that context.

[janvorli]

This is a mere chance that its life span is extended this way. Why don't you just keep the reference to the collectibleContext e.g. in a static field if you want it to not to be collected over the period of the test?

[davmason]

I do want it to be collected in the test, I am testing profiler behavior with unloadable statics. The behavior that I want is that the test loads an assembly and unloads it, then verify that GetAppDomainStaticAddress always either returns a valid object or CORPROF_E_DATAINCOMPLETE.

The problem is that before this change the ALC wouldn't last for the whole LoadCollectibleAssembly method. It would work sometimes but if a GC happened at the wrong point the call to dynamicLibrary.GetType would crash the runtime because the ALC was collected and then finalized.

Having the call to unload at the end of the method forces the behavior I want, where the assembly is loaded and then unloaded.

[janvorli]

Ok, got it, then your change would work. You can also use GC.KeepAlive(collectibleContext) instead of the collectibleContext.Unload(); That would make it clearer that the purpose is to keep the collectibleContext alive.

[janvorli]

Ok, got it, then your change would work. You can also use GC.KeepAlive(collectibleContext) instead of the collectibleContext.Unload(); That would make it clearer that the purpose is to keep the collectibleContext alive.

[janvorli]

There is no guarantee that the collectible context is going to be collected here. If you want to be sure it was collected, you'd need to make the LoadCollectibleAssembly non-inlineable, return a weak reference to the dynamicLibrary from it and then add calling GC.Collect / GC.WaitForPendingFinalizers in a loop while the weak reference is alive. See https://docs.microsoft.com/en-us/dotnet/standard/assembly/unloadability.