[generator] Eliminate usage of JNINativeWrapper.CreateDelegate in bindings.
#1275
Conversation
|
This currently fails the test ExceptionTests.ManagedJavaManaged_JavaCatches in a
For reference, here is the logcat from running the test on .NET 9 RC2:
The generated [global::System.Diagnostics.DebuggerDisableUserUnhandledExceptionsAttribute]
static void n_RunCatchBlock_Ljava_lang_Runnable_ (IntPtr jnienv, IntPtr native__this, IntPtr native_p0)
{
var __envp = new global::Java.Interop.JniTransition (jnienv);
try {
var __this = global::Java.Lang.Object.GetObject<global::Com.Xamarin.Android.Bxc7634> (jnienv, native__this, JniHandleOwnership.DoNotTransfer)!;
var p0 = (global::Java.Lang.IRunnable?)global::Java.Lang.Object.GetObject<global::Java.Lang.IRunnable> (native_p0, JniHandleOwnership.DoNotTransfer);
__this.RunCatchBlock (p0);
} catch (Exception __e) {
__envp.SetPendingException (__e);
global::System.Diagnostics.Debugger.BreakForUserUnhandledException (__e);
} finally {
__envp.Dispose ();
}
}It seems interesting that the crash occurs after the tests finish running. I suspect the culprit is some sort of difference between |
|
I don't think that the body of Of particular interest is the body of What I think should be happening is:
The question is, where does reality not match expectations? The call stack in the first image partially confirms the above: We made it up through (6)! The implication is thus that Why isn't it? I can imagine two possibilities:
I can't locally repro (2), in that this code in a .NET 9 Android project: static void TestEx ()
{
try {
Console.WriteLine("# jonp: TestEx");
Inner();
} catch (Exception e) {
Console.WriteLine ($"# jonp: caught {e}");
System.Diagnostics.Debugger.BreakForUserUnhandledException(e);
}
}
static void Inner()
{
throw new Exception ("Inner");
}doesn't "leak" the exception out of Something is going wrong, and I'm not at all sure what. |
There is a difference: they're entirely unrelated. (Which might need to be addressed, and is something I had forgotten about.)
public override void RaisePendingException (Exception pendingException)
{
var je = pendingException as JavaProxyThrowable;
if (je == null) {
je = JavaProxyThrowable.Create (pendingException);
}
var r = new JniObjectReference (je.Handle);
JniEnvironment.Exceptions.Throw (r);
}(Aside: this will improperly wrap a type which is a Java.Lang.Throwable subclasses. That shouldn't be a concern now, as we're dealing with an This doesn't explain to me why a C# exception would be unhandled when thrown from within a This does raise a good question: how do we support |
|
Sometimes the simplest mistakes are the hardest to find. This: } catch (Exception __e) {was resolving to |
|
|
||
| writer.WriteLine ("} catch (global::System.Exception __e) {"); | ||
| writer.Indent (); | ||
| writer.WriteLine ("__envp.SetPendingException (__e);"); |
There was a problem hiding this comment.
@jpobst: as per this comment, the generated body should instead be:
global::Java.Interop.JniEnvironment.Runtime.OnUnhandledException (ref __envp, __e);The name OnUnhandledException() can (should) be debated, in part because the exception is not, in fact, "unhandled". Perhaps MarshalExceptionToJni()? (Suggestions welcome.)
|
|
||
| writer.WriteLine ("} catch (global::System.Exception __e) {"); | ||
| writer.Indent (); | ||
| writer.WriteLine ("JniEnvironment.Runtime.OnUserUnhandledException (ref __envp, __e);"); |
There was a problem hiding this comment.
I suspect that this should probably use global::Java.Interop.JniEnvironment.Runtime.OnUserUnhandledException (ref __envp, __e).
| namespace System.Diagnostics | ||
| { | ||
| // This attribute was added in .NET 9, and we may not be targeting .NET 9 yet. | ||
| public class DebuggerDisableUserUnhandledExceptionsAttributeAttribute : Attribute |
There was a problem hiding this comment.
Not sure if deliberate or accidental, but the .NET type has only one "Attribute" suffix, not two: DebuggerDisableUserUnhandledExceptionsAttribute
There was a problem hiding this comment.
Doh, surprisingly this works because we were generating [DebuggerDisableUserUnhandledExceptionsAttribute] and it removes one "Attribute".
Updated the generated code to [DebuggerDisableUserUnhandledExceptions] to match (most) of the other attributes we generate, and fixed this support file.
|
@jpobst: doh! I forgot something, and only realized while writing up the commit message: The new marshal methods also need to invoke Consider: It should instead be: [global::System.Diagnostics.DebuggerDisableUserUnhandledExceptions]
static void n_DoSomething (IntPtr jnienv, IntPtr native__this)
{
var __envp = new global::Java.Interop.JniTransition (jnienv);
var __r = global::Java.Interop.JniEnvironment.Runtime;
try {
__r.ValueManager.WaitForGCBridgeProcessing ();
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 {
__envp.Dispose ();
}
}Apologies for not noticing this earlier. Additionally, it makes me wonder if we should add an additional "pre-invoke" extension point, another |
|
Draft commit message: Fixes: https://github.com/dotnet/java-interop/issues/1258
Context: c8f3e51a6cfd78bdce89e2429efae4495481f57b
Context: 176240d2b1da51de03f01b61aa113e0e82ef17ed
Context: https://github.com/dotnet/runtime/issues/108211
Context: https://github.com/dotnet/android/issues/9306
Context: https://github.com/dotnet/android/pull/9309
Context: https://github.com/xamarin/monodroid/commit/3e9de5a51bd46263b08365ef18bed1ae472122d8
Context: https://github.com/dotnet/android/commit/8bc7a3e84f95e70fe12790ac31ecd97957771cb2
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 176240d2
* 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]: https://github.com/dotnet/android/issues/8608#issuecomment-1882658628
[9]: https://github.com/dotnet/android/pull/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) |
|
@jpobst: the "union" of #1275 (comment) and #1275 (comment) is that we add: namespace Java.Interop;
partial class JniRuntime {
public virtual void OnEnterMarshalMethod ()
{
ValueManager.WaitForGCBridgeProcessing ();
}
}and update the marshal methods to invoke |
| var __this = global::Java.Lang.Object.GetObject<java.code.AnimatorListener> (jnienv, native__this, JniHandleOwnership.DoNotTransfer); | ||
| return __this.OnAnimationEnd (param1); | ||
| var __envp = new global::Java.Interop.JniTransition (jnienv); | ||
| var __r = global::Java.Interop.JniEnvironment.Runtime; |
There was a problem hiding this comment.
I should have called this out more than just in the draft commit message, but the problem with this is that JniEnvironment.Runtime can throw:
It shouldn't throw, ideally, but it could, if e.g. the runtime hasn't been initialized yet, or if the runtime has been shutdown.
If it throws an exception outside of the try/catch, we can unwind past the Java frames, and corrupt the JVM.
We could ignore this possibility, and keep this as-is, or we can go with what the draft message suggested:
var __envp = new global::Java.Interop.JniTransition (jnienv);
global::Java.Interop.JniRuntime? __r = null;
try {
__r = global::Java.Interop.JniEnvironment.Runtime;
} …and alternate approach would be to introduce more public methods:
namespace Java.Interop;
partial class JniEnvironment {
public static bool BeginMarshalMethod (IntPtr jnienv, out JniTransition envp, [NotNullWhen(true)] out JniRuntime? runtime);
public static void EndMarshalMethod (ref JniTransition envp);
}allowing:
if (!JniEnvironment.BeginMarshalMethod (jnienv, out var __envp, out var __r))
return /* default */;
try {
__r.OnEnterMarshalMethod ();
// …
} catch (Exception e) {
__r.OnUserUnandledException (ref __e, e);
} finally {
JniEnvironment.EndMarshalMethod (ref __envp);
}A question that comes to mind is IL size: which is smaller, between:
- "just call
JniEnvironment.Runtime" - Call
JniEnvironment.Runtimewithin the try block - JniEnvironment.BeginMarshalMethod()
Enter my sample app (see below).
- A(): "just call
JniEnvironment.Runtime"
Code size 44 - B(): Call
JniEnvironment.Runtimewithin the try block
Code size: 49 - D(): JniEnvironment.BeginMarshalMethod()
Code size: 43
which makes it look like JniEnvironment.BeginMarshalMethod() + JniEnvironment.EndMarshalMethod() has the smallest impact on code size, while still permitting correctness.
// See https://aka.ms/new-console-template for more information
Console.WriteLine("Hello, World!");
void A ()
{
var __e = new JniTransition (0);
var r = JniEnvironment.Runtime;
try {
r.OnEnterMarshalMethod();
} catch (Exception e) {
r.OnUserUnandledException (ref __e, e);
} finally {
__e.Dispose();
}
}
void B ()
{
var __e = new JniTransition (0);
JniRuntime? r = null;
try {
r = JniEnvironment.Runtime;
r.OnEnterMarshalMethod();
} catch (Exception e) {
r?.OnUserUnandledException (ref __e, e);
} finally {
__e.Dispose();
}
}
void C ()
{
if (!JniEnvironment.BeginMarshalMethod (0, out var __e, out var r))
return;
try {
r.OnEnterMarshalMethod();
} catch (Exception e) {
r.OnUserUnandledException (ref __e, e);
} finally {
__e.Dispose();
}
}
void D ()
{
if (!JniEnvironment.BeginMarshalMethod (0, out var __e, out var r))
return;
try {
r.OnEnterMarshalMethod();
} catch (Exception e) {
r.OnUserUnandledException (ref __e, e);
} finally {
JniEnvironment.EndMarshalMethod (ref __e);
}
}
struct JniTransition {
public JniTransition (IntPtr env) {}
public void Dispose() {}
}
class JniEnvironment {
public static JniRuntime Runtime => throw null;
public static bool BeginMarshalMethod (int env, out JniTransition __e, out JniRuntime r) => throw null;
public static void EndMarshalMethod (ref JniTransition __e) => throw null;
}
class JniRuntime {
public class JniValueManager {
}
public virtual void OnUserUnandledException (ref JniTransition env, Exception e)
{
}
public virtual void OnEnterMarshalMethod ()
{
}
public JniValueManager ValueManager => throw null!;
}
| @@ -56,6 +58,15 @@ public MethodCallback (GenBase type, Method method, CodeGenerationOptions option | |||
|
|
|||
| protected override void WriteBody (CodeWriter writer) | |||
| { | |||
| writer.WriteLine ("if (!global::Java.Interop.JniEnvironment.BeginMarshalMethod (jnienv, out var __envp, out var __r)) "); | |||
There was a problem hiding this comment.
is there a reason this line ends with a space? It causes git show/git diff to "light up red" for the space before newline:
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Fixes: dotnet/java-interop#1258 Changes: dotnet/java-interop@2440416...2d48efe * dotnet/java-interop@2d48efe7: [build] `main` *conceptually* targets .NET 10 (dotnet/java-interop#1283) * dotnet/java-interop@356485ee: [generator] Remove `JNINativeWrapper.CreateDelegate` from bindings (dotnet/java-interop#1275) * dotnet/java-interop@14f94a57: [Java.Interop.BootstrapTasks] Filter out invalid JDKs (dotnet/java-interop#1278) Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Fixes: #1258
Change generated marshal methods to use an explicit
try/catchblock withDebugger.BreakForUserUnhandledException (Exception).This entirely removes
JNINativeWrapper.CreateDelegate ()and in turnSystem.Reflection.Emitfrom the marshal method codepath, which should improve app startup.XA test PR: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=10644574&view=results