Stop relying on UB in 'IContextCallback' dispatch logic

[Sergio0694]

See https://github.com/dotnet/runtime/blob/main/docs/design/specs/Memory-model.md#cross-thread-access-to-local-variables. This PR fixes accessing the callback state (which is a managed object) from the native callback invoked on the target thread.

[jkotas]

Fix a GC hole

Nit: The potential undefined behavior that this is fixing does not fit the definition of GC hole (https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/clr-code-guide.md#211-how-gc-holes-are-created).

[jkotas]

Are you sure that there cannot ever be recursion that would cause multiple recursive uses of CallbackData on the same thread?

Calls between STA apartments always come with a lot of surprises and corner-case bugs...

[Sergio0694]

Applied the suggested changes and also left some notes about reentrancy. I talked with @manodasanW as well and in this scenario this should be fine. We're only ever doing this context switch to release an object reference on its original context, and nothing else. There is no way that should/could then somehow switch back to this starting thread (it wouldn't even know which one that was) to then try to schedule another context callback recursively. So this should be fine. Also in ASTA scenarios this would also just be flat out impossible by construction (because ASTA blocks reentrant calls into it, so even if hypothetically we somehow ended up there, that second context callback would just throw before being scheduled). But then again either case should just never happen. Definitely a good point though and worth writing some additional comments about this, thank you! 😄

[jkotas]

Release can run arbitrary code, is that right? I do not see what guarantees that the arbitrary code cannot switch threads at will.

Also, this seems to be used for more than just releasing the object reference:

CsWinRT/src/WinRT.Runtime/ObjectReference.cs

Lines 751 to 760 in 93482af

	Context.CallInContext(
	_contextCallbackPtr,
	_contextToken,
	#if NET && CsWinRT_LANG_11_FEATURES
	&InitAgileReference,
	#else
	InitAgileReference,
	#endif
	null,
	this);

It would be more correct by construction to implement this as usual pool - clear the cached instance when it is rented, so that there is no way for one instance to be rented multiple times at the same time.

[hamarb123]

I would have assumed that IContextCallback::ContextCallback with ICallbackWithNoReentrancyToApplicationSTA means it shouldn't be able to run any arbitrary code on this thread as a result of that call (I'm not familiar with these APIs though, so I don't actually know), and nothing else from when we set the field to when we clear it seems problematic. If that thing doesn't guarantee this however, then something should be done to allow multiple current things at once potentially existing (e.g., using a List<object> or similar (probably manually via array for resizing purposes) would do the job).

[hamarb123]

Or do you mean the callback might call back into this thread & somehow end up back here? That would make sense I suppose. Is that possible @Sergio0694? Actually, I think in this case it would still be fine, as long as the callback is called before anything else silly happens, since it immediately reads the field & thus wouldn't get the new field value (assuming appropriate barrier or whatever - for which a volatile read would certainly be enough, but probably none is "needed"); then it would just be set to null twice, but the meaningful value of the field would already be read & that version would have already been read & had the correct value.

[Sergio0694]

"as long as the callback is called before anything else silly happens, since it immediately reads the field

That's actually a good point. Even if we somehow recursively ended up here (which I don't believe is possible), each callback would've already read the target state before invoking the user-provided callback anyway.

"Release can run arbitrary code"

Not really, I mean yes from the point of view of this API, but the context callback is only ever used internally to release IObjectReference objects, which are implemented in CsWinRT only. And we're only ever using these to pass the Release pointers which simply do a release on the tracker ref and the native object on the target context:

CsWinRT/src/WinRT.Runtime/ObjectReference.cs

Lines 955 to 967 in 93482af

	static void Release(object state)
	{
	ObjectReferenceWithContext<T> @this = Unsafe.As<ObjectReferenceWithContext<T>>(state);
	@this.ReleaseFromBase();
	}
	static void ReleaseWithoutContext(object state)
	{
	ObjectReferenceWithContext<T> @this = Unsafe.As<ObjectReferenceWithContext<T>>(state);
	@this.ReleaseWithoutContext();
	}

[hamarb123]

This looks fine now (assuming we don't need volatile read in the callback), assuming it's not possible to come back into ContextCallback while another one is executing on the same thread.