[generator] Remove JNINativeWrapper.CreateDelegate from bindings (#1275)

Fixes: #1258

Context: c8f3e51
Context: 176240d
Context: dotnet/runtime#108211
Context: dotnet/android#9306
Context: dotnet/android#9309
Context: xamarin/monodroid@3e9de5a
Context: dotnet/android@8bc7a3e

The [Java Native Interface][0] allows native code to be associated
with a [Java `native` method declaration][1], either by way of
[`Java_`-prefixed native functions][2], or via function pointers
provided to [`JNIEnv::RegisterNatives()`][3].

Both `Java_`-prefixed native functions and function pointers must
refer to C-callable functions with appropriate
[native method arguments][4].

A *Marshal Method* is a:

 1. Method or delegate which is C-callable,
 2. Accepting the appropriate Java Native Method arguments,
 3. Is responsible for marshaling parameter and return types, and
 3. *Delegates* the call to an appropriate managed method override.

We have multiple different Marshal Method implementations running
around, including:

  * XamarinAndroid1 and XAJavaInterop1 Marshal Methods, in which the
    Marshal Method is an `n_`-prefixed method in (roughly-ish) the
    same scope as the method that would be delegated to.
  * `jnimarshalmethod-gen`: see 176240d
  * LLVM Marshal Methods, which use LLVM-IR to emit `Java_`-prefixed
    native functions; see dotnet/android@8bc7a3e8.

Which brings us to the current XAJavaInterop1 Marshal Methods
implementation.  Consider the [`java.util.function.IntConsumer`][5]
interface:

	// Java
	public /* partial */ interface IntConsumer {
	  void accept(int value);
	}

With `generator --codegen-target=XAJavaInterop1` -- used by
.NET for Android -- `IntConsumer` is bound as `IIntConsumer`:

	namespace Java.Util.Functions {

	  // Metadata.xml XPath interface reference: path="/api/package[@name='java.util.function']/interface[@name='IntConsumer']"
	  [Register ("java/util/function/IntConsumer", "", "Java.Util.Functions.IIntConsumerInvoker", ApiSince = 24)]
	  public partial interface IIntConsumer : IJavaObject, IJavaPeerable {
	    [Register ("accept", "(I)V", "GetAccept_IHandler:Java.Util.Functions.IIntConsumerInvoker, Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null", ApiSince = 24)]
	    void Accept (int value);
	  }

	  [Register ("java/util/function/IntConsumer", DoNotGenerateAcw=true, ApiSince = 24)]
	  internal partial class IIntConsumerInvoker : global::Java.Lang.Object, IIntConsumer {
	    static Delegate? cb_accept_Accept_I_V;
	    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;
	    }

	    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);
	    }
	  }
	}

The Marshal Method is `IIntConsumerInvoker.n_Accept_I()`.

We also have a *Connector Method*.  A Connector Method is a `static`
method matching the signature of `Func<Delegate>`.  The name of the
connector method is mentioned in the 3rd `connector` parameter of
`RegisterAttribute` on the interface method.

During [Java Type Registration][6], all Connector methods for a type
are looked up and invoked, and the `Delegate` instances returned from
all those connector method invocations are provided to
`JNIEnv::RegisterNatives()`.

There are static and runtime issues with connector method and marshal
method implementations until now:

 1. Java Native Methods, and thus Marshal Methods, *must* conform to
    the C ABI.  C does not support exceptions.  C# *does*.

    What happens when `__this.Accept(value)` throws?

 2. The answer to (1) is in the connector method, via the
    `JNINativeWrapper.CreateDelegate()` invocation.
    [`JNINativeWrapper.CreateDelegate()`][7] uses
    System.Reflection.Emit to *wrap* the Marshal Method with a
    try/catch block.

At runtime, the intermixing of (1) and (2) will result in registering
a method similar to the following with `JNIEnv::RegisterNatives()`:

	static void n_Accept_I (IntPtr jnienv, IntPtr native__this, int value)
	{
	  JNIEnv.WaitForBridgeProcessing ();
	  try {
	    var __this = ava.Lang.Object.GetObject<IIntConsumer> (jnienv, native__this, JniHandleOwnership.DoNotTransfer)!;
	    __this.Accept (value);
	  }
	  catch (Exception e) when (!Debugger.IsAttached) {
	    AndroidEnvironment.UnhandledException (e);
	  }
	}

which presents a further two problems:

 1. System.Reflection.Emit is used, which possibly slows down type
    registration and won't work with NativeAOT.
 2. The `catch` block only executes when you're *not* debugging!

Which means that if you're debugging the app, and an exception is
thrown, you are now potentially unwinding the stack frame through a
JNI boundary, which can *corrupt JVM state*, possibly resulting in an
[app abort or crash][8].  ([***Why?!***][9])

This has been how things work since the beginning.

.NET 9 introduces some features that allow us to rethink all this:

  * [`DebuggerDisableUserUnhandledExceptionsAttribute`][10]
  * [`Debugger.BreakForUserUnhandledException(Exception)`][11]

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

Embrace .NET 9, remove the possible need for System.Reflection.Emit,
and fully prevent possible JVM corruption by updating connector
methods and marshal methods to instead be:

	namespace Java.Util.Functions {

	  internal partial class IIntConsumerInvoker {
	    static Delegate? cb_accept_Accept_I_V;
	    static Delegate GetAccept_IHandler ()
	    {
	      return cb_accept_Accept_I_V ??= new _JniMarshal_PPI_V (n_Accept_I);
	    }

	    [DebuggerDisableUserUnhandledExceptions]
	    static void n_Accept_I (IntPtr jnienv, IntPtr native__this, int value)
	    {
	      if (!JniEnvironment.BeginMarshalMethod (jnienv, out var __envp, out var __r))
	        return;

	      try {
	        var __this = Java.Lang.Object.GetObject<IIntConsumer> (jnienv, native__this, JniHandleOwnership.DoNotTransfer)!;
	        __this.Accept (value);
	      } catch (global::System.Exception __e) {
	        __r?.OnUserUnhandledException (ref __envp, __e);
	      } finally {
	        JniEnvironment.EndMarshalMethod (ref __envp);
	      }
	    }
	  }
	}

This removes the call to `JNINativeWrapper.CreateDelegate()` and it's
implicit use of System.Reflection.Emit, properly wraps *everything*
in a `try`/`catch` block so that exceptions are properly caught and
marshaled back to Java if necessary, and integrates properly with
expected "first chance exception" semantics.

The *downside* is that this requires the "new debugger backend" to
work, which at the time of this writing is only used by VSCode.
As this code will only be used for .NET 10+ (2025-Nov), this is fine.

[0]: https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/jniTOC.html
[1]: http://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/design.html#compiling_loading_and_linking_native_methods
[2]: https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/design.html#resolving_native_method_names
[3]: http://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/functions.html#RegisterNatives
[4]: https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/design.html#native_method_arguments
[5]: https://developer.android.com/reference/java/util/function/IntConsumer
[6]: https://github.com/dotnet/android/wiki/Blueprint#java-type-registration
[7]: https://github.com/dotnet/android/blob/65906e0b7b2f471fcfbd07e7e01b68169c25d9da/src/Mono.Android/Android.Runtime/JNINativeWrapper.cs#L29-L105
[8]: dotnet/android#8608 (comment)
[9]: dotnet/android#4877
[10]: https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.debuggerdisableuserunhandledexceptionsattribute?view=net-9.0
[11]: https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.debugger.breakforuserunhandledexception?view=net-9.0#system-diagnostics-debugger-breakforuserunhandledexception(system-exception)

Author: jpobst

Diff for: src/Java.Interop/Java.Interop/JniEnvironment.cs

@@ -3,6 +3,7 @@
 using System;
 using System.Collections.Generic;
 using System.Diagnostics;
+using System.Diagnostics.CodeAnalysis;
 using System.Linq;
 using System.Runtime.CompilerServices;
 using System.Runtime.InteropServices;
@@ -50,6 +51,50 @@ public      static  bool                    WithinNewObjectScope {
 			internal set {CurrentInfo.WithinNewObjectScope = value;}
 		}
 
+		[global::System.Diagnostics.CodeAnalysis.SuppressMessage (
+			"Design",
+			"CA1031:Do not catch general exception types",
+			Justification = "Exceptions cannot cross a JNI boundary.")]
+		public static bool BeginMarshalMethod (IntPtr jnienv, out JniTransition transition, [NotNullWhen (true)] out JniRuntime? runtime)
+		{
+			runtime = null;
+			Exception?          ex  = null;
+			try {
+				runtime = Info.Value?.Runtime;
+			}
+			catch (Exception e) {
+				ex  = e;
+			}
+			if (runtime == null || ex != null) {
+				transition  = default;
+				runtime     = null;
+				Console.Error.WriteLine ("JNI Environment Information is not available on this thread.");
+				if (ex != null) {
+					Console.Error.WriteLine (ex);
+				}
+				return false;
+			}
+
+			try {
+				runtime.OnEnterMarshalMethod ();
+				transition  = new JniTransition (jnienv);
+			}
+			catch (Exception e) {
+				runtime     = null;
+				transition  = default;
+
+				Console.Error.WriteLine ($"OnEnterMarshalMethod failed: {e}");
+				return false;
+			}
+
+			return true;
+		}
+
+		public static void EndMarshalMethod (ref JniTransition transition)
+		{
+			transition.Dispose ();
+		}
+
 		internal    static  void    SetEnvironmentPointer (IntPtr environmentPointer)
 		{
 			CurrentInfo.EnvironmentPointer  = environmentPointer;

Diff for: src/Java.Interop/Java.Interop/JniRuntime.cs

@@ -437,6 +437,19 @@ public virtual bool ExceptionShouldTransitionToJni (Exception e)
 
 	partial class JniRuntime {
 
+		public virtual void OnEnterMarshalMethod ()
+		{
+			ValueManager.WaitForGCBridgeProcessing ();
+		}
+
+		public virtual void OnUserUnhandledException (ref JniTransition transition, Exception e)
+		{
+			transition.SetPendingException (e);
+
+			// TODO: Enable when we move to 'net9.0'
+			//Debugger.BreakForUserUnhandledException (e);
+		}
+
 		public virtual void RaisePendingException (Exception pendingException)
 		{
 			JniEnvironment.Exceptions.Throw (pendingException);

Diff for: src/Java.Interop/PublicAPI.Unshipped.txt

@@ -1 +1,5 @@
 #nullable enable
+static Java.Interop.JniEnvironment.BeginMarshalMethod(nint jnienv, out Java.Interop.JniTransition transition, out Java.Interop.JniRuntime? runtime) -> bool
+static Java.Interop.JniEnvironment.EndMarshalMethod(ref Java.Interop.JniTransition transition) -> void
+virtual Java.Interop.JniRuntime.OnEnterMarshalMethod() -> void
+virtual Java.Interop.JniRuntime.OnUserUnhandledException(ref Java.Interop.JniTransition transition, System.Exception! e) -> void

Diff for: tests/generator-Tests/SupportFiles/DebuggerDisableUserUnhandledExceptionsAttribute.cs

@@ -0,0 +1,13 @@
+#if !NET9_0_OR_GREATER
+
+using System;
+
+namespace System.Diagnostics
+{
+	// This attribute was added in .NET 9, and we may not be targeting .NET 9 yet.
+	public class DebuggerDisableUserUnhandledExceptionsAttribute : Attribute
+	{
+	}
+}
+
+#endif  // !NET9_0_OR_GREATER

Diff for: tests/generator-Tests/Unit-Tests/CodeGeneratorExpectedResults/Common/WriteDuplicateInterfaceEventArgs.txt

@@ -70,15 +70,24 @@ internal partial class AnimatorListenerInvoker : global::Java.Lang.Object, Anima
 #pragma warning disable 0169
 	static Delegate GetOnAnimationEnd_IHandler ()
 	{
-		if (cb_OnAnimationEnd_OnAnimationEnd_I_Z == null)
-			cb_OnAnimationEnd_OnAnimationEnd_I_Z = JNINativeWrapper.CreateDelegate (new _JniMarshal_PPI_Z (n_OnAnimationEnd_I));
-		return cb_OnAnimationEnd_OnAnimationEnd_I_Z;
+		return cb_OnAnimationEnd_OnAnimationEnd_I_Z ??= new _JniMarshal_PPI_Z (n_OnAnimationEnd_I);
 	}
 
+	[global::System.Diagnostics.DebuggerDisableUserUnhandledExceptions]
 	static bool n_OnAnimationEnd_I (IntPtr jnienv, IntPtr native__this, int param1)
 	{
-		var __this = global::Java.Lang.Object.GetObject<java.code.AnimatorListener> (jnienv, native__this, JniHandleOwnership.DoNotTransfer);
-		return __this.OnAnimationEnd (param1);
+		if (!global::Java.Interop.JniEnvironment.BeginMarshalMethod (jnienv, out var __envp, out var __r))
+			return default;
+
+		try {
+			var __this = global::Java.Lang.Object.GetObject<java.code.AnimatorListener> (jnienv, native__this, JniHandleOwnership.DoNotTransfer);
+			return __this.OnAnimationEnd (param1);
+		} catch (global::System.Exception __e) {
+			__r.OnUserUnhandledException (ref __envp, __e);
+			return default;
+		} finally {
+			global::Java.Interop.JniEnvironment.EndMarshalMethod (ref __envp);
+		}
 	}
 #pragma warning restore 0169
 
@@ -96,15 +105,24 @@ internal partial class AnimatorListenerInvoker : global::Java.Lang.Object, Anima
 #pragma warning disable 0169
 	static Delegate GetOnAnimationEnd_IIHandler ()
 	{
-		if (cb_OnAnimationEnd_OnAnimationEnd_II_Z == null)
-			cb_OnAnimationEnd_OnAnimationEnd_II_Z = JNINativeWrapper.CreateDelegate (new _JniMarshal_PPII_Z (n_OnAnimationEnd_II));
-		return cb_OnAnimationEnd_OnAnimationEnd_II_Z;
+		return cb_OnAnimationEnd_OnAnimationEnd_II_Z ??= new _JniMarshal_PPII_Z (n_OnAnimationEnd_II);
 	}
 
+	[global::System.Diagnostics.DebuggerDisableUserUnhandledExceptions]
 	static bool n_OnAnimationEnd_II (IntPtr jnienv, IntPtr native__this, int param1, int param2)
 	{
-		var __this = global::Java.Lang.Object.GetObject<java.code.AnimatorListener> (jnienv, native__this, JniHandleOwnership.DoNotTransfer);
-		return __this.OnAnimationEnd (param1, param2);
+		if (!global::Java.Interop.JniEnvironment.BeginMarshalMethod (jnienv, out var __envp, out var __r))
+			return default;
+
+		try {
+			var __this = global::Java.Lang.Object.GetObject<java.code.AnimatorListener> (jnienv, native__this, JniHandleOwnership.DoNotTransfer);
+			return __this.OnAnimationEnd (param1, param2);
+		} catch (global::System.Exception __e) {
+			__r.OnUserUnhandledException (ref __envp, __e);
+			return default;
+		} finally {
+			global::Java.Interop.JniEnvironment.EndMarshalMethod (ref __envp);
+		}
 	}
 #pragma warning restore 0169
 

Diff for: tests/generator-Tests/Unit-Tests/CodeGeneratorExpectedResults/Common/WriteInterfaceRedeclaredDefaultMethod.txt

@@ -66,15 +66,23 @@ internal partial class IMyInterface2Invoker : global::Java.Lang.Object, IMyInter
 #pragma warning disable 0169
 	static Delegate GetDoSomethingHandler ()
 	{
-		if (cb_DoSomething_DoSomething_V == null)
-			cb_DoSomething_DoSomething_V = JNINativeWrapper.CreateDelegate (new _JniMarshal_PP_V (n_DoSomething));
-		return cb_DoSomething_DoSomething_V;
+		return cb_DoSomething_DoSomething_V ??= new _JniMarshal_PP_V (n_DoSomething);
 	}
 
+	[global::System.Diagnostics.DebuggerDisableUserUnhandledExceptions]
 	static void n_DoSomething (IntPtr jnienv, IntPtr native__this)
 	{
-		var __this = global::Java.Lang.Object.GetObject<java.code.IMyInterface2> (jnienv, native__this, JniHandleOwnership.DoNotTransfer);
-		__this.DoSomething ();
+		if (!global::Java.Interop.JniEnvironment.BeginMarshalMethod (jnienv, out var __envp, out var __r))
+			return;
+
+		try {
+			var __this = global::Java.Lang.Object.GetObject<java.code.IMyInterface2> (jnienv, native__this, JniHandleOwnership.DoNotTransfer);
+			__this.DoSomething ();
+		} catch (global::System.Exception __e) {
+			__r.OnUserUnhandledException (ref __envp, __e);
+		} finally {
+			global::Java.Interop.JniEnvironment.EndMarshalMethod (ref __envp);
+		}
 	}
 #pragma warning restore 0169