Remove JNINativeWrapper.CreateDelegate() Usage From Marshal Methods

[jonpryor]

Context: dotnet/runtime#108211
Context: dotnet/android#9306
Context: dotnet/android#9309
Context: https://github.com/xamarin/monodroid/commit/3e9de5a51bd46263b08365ef18bed1ae472122d8

Consider this marshal method and related infrastructure::

namespace Java.Util.Functions {
	public partial interface IIntConsumerInvoker : global::Java.Lang.Object, IIntConsumer {
		static Delegate? cb_accept_Accept_I_V;
#pragma warning disable 0169
		[global::System.Runtime.Versioning.SupportedOSPlatformAttribute ("android24.0")]
		static Delegate GetAccept_IHandler ()
		{
			if (cb_accept_Accept_I_V == null)
				cb_accept_Accept_I_V = JNINativeWrapper.CreateDelegate (new _JniMarshal_PPI_V (n_Accept_I));
			return cb_accept_Accept_I_V;
		}

		[global::System.Runtime.Versioning.SupportedOSPlatformAttribute ("android24.0")]
		static void n_Accept_I (IntPtr jnienv, IntPtr native__this, int value)
		{
			var __this = global::Java.Lang.Object.GetObject<Java.Util.Functions.IIntConsumer> (jnienv, native__this, JniHandleOwnership.DoNotTransfer)!;
			__this.Accept (value);
		}
#pragma warning restore 0169
	}
}

Why do we have JNINativeWrapper.CreateDelegate()? (Closely related: why isn't there a try/catch block in n_Accept_I()? Though part of that is "we didn't think of it.")

The answer is around the "unhandled exception" experience when a debugger is attached: the debugger breaks at the "active" throw site. If the type will be caught by a catch block, then you won't get an "unhandled exception" notification, which made customers sad.

Which means that for a good debugger experience, we can't have catch blocks catching exceptions; if we did, then the exceptions would be handled!

Meanwhile, we must catch and marshal exceptions back to Java, otherwise we'll corrupt the JVM during stack unwind!

Where we wound up was a terrible middle:

• If a debugger is attached, then basically have no exception handling. (Wild oversimplification, but close enough for here.)
• If a debugger isn't attached, then all exceptions are caught.
• JNINativeWrapper.CreateDelegate() used System.Reflection.Emit to bridge these two worlds.

However, .NET 9 introduces System.Diagnostics.DebuggerDisableUserUnhandledExceptionsAttribute:

If a .NET Debugger is attached that supports the BreakForUserUnhandledException(Exception) API, the debugger won't break on user-unhandled exceptions when the exception is caught by a method with this attribute, unless BreakForUserUnhandledException(Exception) is called.

Thus, the proposal: update the above marshal method related infrastructure to instead be:

namespace Java.Util.Functions {
	public partial interface IIntConsumerInvoker : global::Java.Lang.Object, IIntConsumer {
		static Delegate? cb_accept_Accept_I_V;
#pragma warning disable 0169
		[global::System.Runtime.Versioning.SupportedOSPlatformAttribute ("android24.0")]
		static Delegate GetAccept_IHandler ()
		{
			return cb_accept_Accept_I_V ?? (cb_accept_Accept_I_V = new _JniMarshal_PPI_V (n_Accept_I));
		}

		[global::System.Runtime.Versioning.SupportedOSPlatformAttribute ("android24.0")]
		[global::System.Diagnostics.DebuggerDisableUserUnhandledExceptionsAttribute)]
		static void n_Accept_I (IntPtr jnienv, IntPtr native__this, int value)
		{
			var __envp = new global::Java.Interop.JniTransition (jnienv);
			try {
				var __this = global::Java.Lang.Object.GetObject<Java.Util.Functions.IIntConsumer> (jnienv, native__this, JniHandleOwnership.DoNotTransfer)!;
				__this.Accept (value);
			}
			catch (Exception e) {
				__envp.SetPendingException (e);
				Debugger.BreakForUserUnhandledException (e);
			}
			finally {
				__envp.Dispose ();
			}
		}
#pragma warning restore 0169
	}
}

This entirely removes JNINativeWrapper.CreateDelegate() and in turn System.Reflection.Emit from the marshal method codepath, which should improve app startup.

[jonpryor]

In order for this approach to be viable, MonoVM needs to implement support for Debugger.BreakForUserUnhandledException(); see also: dotnet/runtime#108211 (comment)

Hopefully we can get that support for .NET 10.

[jonpryor]

We should also make this "only use blittable types" change as part of this as well: #1027

[jonpryor]

Question: should there be any "extensibility" in here, to reduce the need to change (implicit) ABI after this? I'm thinking something along the lines of:

partial class JniRuntime {
    public virtual void OnUnhandledException (ref JniTransition transition, Exception e)
    {
        transition.SetPendingException (e);
        Debugger.BreakForUserUnhandledException (e);
    }
}

catch blocks would then call:

catch (Exception __e) {
    JniEnvironment.Runtime.OnUnhandledException(ref __envp, __e);
}

The downside is that catch blocks become slightly slower (JniEnvironment.Runtime looks up per-thread TLS data, plus virtual method invocation), but that's in an "exceptional" path, so I'm not sure it'll matter, performance-wise.

[jonathanpeppers]

The $(DebuggerSupport) MSBuild property toggles the System.Diagnostics.Debugger.IsSupported trimmer feature flag.

We already set this by default in Release mode:

• https://github.com/dotnet/android/blob/4fdc8014658dc37469fabdf433afd51af09ddc58/src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.DefaultProperties.targets#L105

One thing we could do, is solve this issue for Release mode by using this trimmer flag, MSBuild property, etc.? We could avoid System.Reflection.Emit for Release mode completely?

[jonpryor]

@jonathanpeppers: I don't immediately understand your previous comment. System.Reflection.Emit is used in two places:

• Within JNINativeWrapper.CreateDelegate()
• For Mono.Android.Export.dll: https://github.com/dotnet/android/blob/70948d5eaa72091389940fdd5a0eb828c85bdd60/src/Mono.Android.Export/CallbackCode.cs#L634-L650

This issue #1258 would remove the call to JNINativeWrapper.CreateDelegate(), and thus remove the need for System.Reflection.Emit entirely for new bindings in .NET 10.

This would not impact [ExportAttribute] usage / Mono.Android.Export; the only way I know of to attempt to fix that would be via jnimarshalmethod-gen…