Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[generator] Eliminate usage of JNINativeWrapper.CreateDelegate in bindings. #1275

Merged
merged 8 commits into from
Dec 4, 2024
Merged

[generator] Eliminate usage of JNINativeWrapper.CreateDelegate in bindings. #1275

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
98beeaa
[generator] Eliminate usage of `JNINativeWrapper.CreateDelegate` in b…
jpobst Nov 5, 2024
11145de
Use `JniEnvironment.Runtime.OnUserUnhandledException`.
jpobst Nov 21, 2024
369ae73
Update tests.
jpobst Dec 2, 2024
1d2fab9
Add `OnEnterMarshalMethod` code.
jpobst Dec 4, 2024
e4db2bb
Add JniEnvironment.BeginMarshalMethod(), EndMarshalMethod().
jonpryor Dec 4, 2024
d653a14
Update to latest API.
jpobst Dec 4, 2024
dee6eb6
Cleanup BeginMarshalMethod()
jonpryor Dec 4, 2024
b2850e1
Fix errant generated space.
jpobst Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/Java.Interop/Java.Interop/JniRuntime.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 2 additions & 0 deletions src/Java.Interop/PublicAPI.Unshipped.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,3 @@
#nullable enable
virtual Java.Interop.JniRuntime.OnEnterMarshalMethod() -> void
virtual Java.Interop.JniRuntime.OnUserUnhandledException(ref Java.Interop.JniTransition transition, System.Exception! e) -> void
13 changes: 13 additions & 0 deletions tests/generator-Tests/SupportFiles/DebuggerDisableUserUnhandledExceptionsAttribute.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
40 changes: 30 additions & 10 deletions ...Tests/Unit-Tests/CodeGeneratorExpectedResults/Common/WriteDuplicateInterfaceEventArgs.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,15 +70,25 @@ 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);
var __envp = new global::Java.Interop.JniTransition (jnienv);
var __r = global::Java.Interop.JniEnvironment.Runtime;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

java-interop/src/Java.Interop/Java.Interop/JniEnvironment.cs

Line 221 in 14f94a5

get => runtime ?? throw new NotSupportedException ();

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.Runtime within the try block
  • JniEnvironment.BeginMarshalMethod()

Enter my sample app (see below).

  • A(): "just call JniEnvironment.Runtime"
    Code size 44
  • B(): Call JniEnvironment.Runtime within 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!;
}


try {
__r.OnEnterMarshalMethod ();
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 {
__envp.Dispose ();
}
}
#pragma warning restore 0169

Expand All @@ -96,15 +106,25 @@ 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);
var __envp = new global::Java.Interop.JniTransition (jnienv);
var __r = global::Java.Interop.JniEnvironment.Runtime;

try {
__r.OnEnterMarshalMethod ();
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 {
__envp.Dispose ();
}
}
#pragma warning restore 0169

Expand Down
19 changes: 14 additions & 5 deletions .../Unit-Tests/CodeGeneratorExpectedResults/Common/WriteInterfaceRedeclaredDefaultMethod.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,15 +66,24 @@ 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 ();
var __envp = new global::Java.Interop.JniTransition (jnienv);
var __r = global::Java.Interop.JniEnvironment.Runtime;

try {
__r.OnEnterMarshalMethod ();
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 ();
}
}
#pragma warning restore 0169

Expand Down
Loading