ASSERT(ThreadStore::IsTrapThreadsRequested()) in Thread::InlineTryFastReversePInvoke

[MichalStrehovsky]

Hitting this assert in the repro from #110607 sometimes.

The stack is (line numbers are for release/9.0)

 	WinUIAppTest.exe!Assert(const char * expr, const char * file, unsigned int line_num, const char * message) Line 35	C++
	[Inline Frame] WinUIAppTest.exe!Thread::InlineTryFastReversePInvoke(ReversePInvokeFrame *) Line 130	C++
 	WinUIAppTest.exe!RhpReversePInvoke(ReversePInvokeFrame * pFrame) Line 1317	C++
 	WinUIAppTest.exe!S_P_CoreLib_System_Runtime_InteropServices_TrackerObjectManager__GCStopCollection() Line 173	Unknown
 	WinUIAppTest.exe!RestrictedCallouts::InvokeGcCallouts(GcRestrictedCalloutKind eKind, unsigned int uiCondemnedGeneration) Line 195	C++
 	WinUIAppTest.exe!GCToEEInterface::GcDone(int condemned) Line 161	C++
 	WinUIAppTest.exe!WKS::gc_heap::do_post_gc() Line 50870	C++
 	WinUIAppTest.exe!WKS::gc_heap::bgc_thread_function() Line 39957	C++
 	WinUIAppTest.exe!WKS::gc_heap::bgc_thread_stub(void * arg) Line 37744	C++

Hopefully this is not a product issue, just an assert that needs to be tweaked.

We set DoNotTriggerGC here:

runtime/src/coreclr/nativeaot/Runtime/RestrictedCallouts.cpp

Lines 180 to 182 in 1622f51

	// It is illegal for any of the callouts to trigger a GC.
	Thread * pThread = ThreadStore::GetCurrentThread();
	pThread->SetDoNotTriggerGc();

And then we go into the assert path here:

runtime/src/coreclr/nativeaot/Runtime/thread.inl

Lines 194 to 205 in 1622f51

	// If the thread is already in cooperative mode, this is a bad transition that will be a fail fast unless we are in
	// a do not trigger mode. The exception to the rule allows us to have [UnmanagedCallersOnly] methods that are called via
	// the "restricted GC callouts" as well as from native, which is necessary because the methods are CCW vtable
	// methods on interfaces passed to native.
	// We will allow threads in DoNotTriggerGc mode to do reverse PInvoke regardless of their coop state.
	if (IsDoNotTriggerGcSet())
	{
	// We expect this scenario only when EE is stopped.
	ASSERT(ThreadStore::IsTrapThreadsRequested());
	// no need to do anything
	return true;
	}

Cc @VSadov for opinion

[dotnet-policy-service]

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

[VSadov]

GC callouts are not supposed to happen on background threads. This does not seem right.

[VSadov]

GC callouts are not supposed to happen on background threads.

It looks like now they can. (as of #90283).

Before that IsDoNotTriggerGcSet was used in these cases:

• Transient use by exception handling to go through code that is not GC-safe, but that scenario is relatively trivial and does not involve PInvokes or reverse PInvokes.
• Another use was by GC threads while EE is stopped. In this case we could do reverse pinvokes and returns from them. It was special-cased and transitions are basically noops - ignoring the coop mode of the thread and not checking if suspension is requested. All that is meaningless anyways - when EE is stopped and blocking GC is already running.

Now we have a scenario when background threads call [UnmanagedCallersOnly] methods from GC callouts.
Since the scenario is limited to BGC threads and specific callouts, it may still be ok to do, depending on what those callouts do. - All the rules of what GC callouts cannot do would still apply, but otherwise we could ignore the coop state of a IsDoNotTriggerGcSet thread when doing a reverse PInvoke.

The GC callouts is a relatively fragile pattern, and it is hard to think of all the consequences, but I think we may be able to get away with it in this case.
Assuming we do not see any other troubles besides this assert, I think relaxing the assert to ASSERT(IsGcSpecial()); , just to be sure this is not happening with some random thread, could be a sufficient "fix".