-
Notifications
You must be signed in to change notification settings - Fork 53
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.
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
[Java.Interop] Add JniRuntime.JniValueManager.TryCreatePeer()
#1301
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Context: 78d5937 Context: dotnet/android@7a772f0 Context: dotnet/android#9716 Context: dotnet/android@694e975 dotnet/android@7a772f03 added the beginnings of a NativeAOT sample to dotnet/android which built a ".NET for Android" app using NativeAOT, which "rhymed with" the `Hello-NativeAOTFromAndroid` sample in 78d5937. Further work on the sample showed that it was lacking support for `Android.App.Application` subclasses. dotnet/android#9716 began fixing that oversight, but in the process was triggering a stack overflow because when it needed to create a "proxy" peer around the `my.MainApplication` Java type, which subclassed `android.app.Application`, instead of creating an instance of the expected `MainApplication` C# type, it instead created an instance of `Android.App.Application`. This was visible from the logs: Created PeerReference=0x2d06/G IdentityHashCode=0x8edcb07 Instance=0x957d2a Instance.Type=Android.App.Application, Java.Type=my/MainApplication Note that `Instance.Type` is `Android.App.Application`, not the in-sample `MainApplication` C# type. Because the runtime type was `Android.App.Application`, when we later attempted to dispatch the `Application.OnCreate()` override, this resulted in a *virtual* invocation of the Java `Application.onCreate()` method instead of a *non-virtual* invocation of `Application.onCreate()`. This virtual invocation was the root of a recursive loop which eventually resulted in a stack overflow. The fix in dotnet/android@694e975e was to fix `NativeAotTypeManager.CreatePeer()` so that it properly checked for a binding of the *runtime type* of the Java instance *before* using the "fallback" type provided to `Object.GetObject<T>()` in the `Application.n_OnCreate()` method: partial class Application { static void n_OnCreate (IntPtr jnienv, IntPtr native__this) { // … var __this = global::Java.Lang.Object.GetObject< Android.App.Application // This is the "fallback" NativeAotTypeManager > (jnienv, native__this, JniHandleOwnership.DoNotTransfer)!; __this.OnCreate (); // … } } All well and good. The problem is that `NativeAotTypeManager` in dotnet/android needs to support *both* dotnet/java-interop "activation constructors" with a signature of `(ref JniObjectReference, JniObjectReferenceOptions)`, *and* the .NET for Android signature of `(IntPtr, JniHandleOwnership)`. Trying to support both constructors resulted in the need to copy *all* of `JniRuntime.JniValueManager.CreatePeer()` *and dependencies*, which felt a bit excessive. Add a new `JniRuntime.JniValueManager.TryCreatePeer()` method, which will invoke the activation constructor to create an `IJavaPeerable`: partial class JniRuntime { partial class JniValueManager { protected virtual IJavaPeerable? TryCreatePeer ( ref JniObjectReference reference, JniObjectReferenceOptions options, Type targetType); } } If the activation constructor is not found, then `TryCreatePeer()` shall return `null`, allowing `CreatePeerInstance()` to try for a base type or, ultimately, the fallback type. This will allow a future dotnet/android PR to *remove* `NativeAotTypeManager.CreatePeer()` and its dependencies entirely, and instead do: partial class NativeAotTypeManager { const BindingFlags ActivationConstructorBindingFlags = BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance; static readonly Type[] XAConstructorSignature = new Type [] { typeof (IntPtr), typeof (JniHandleOwnership) }; protected override IJavaPeerable TryCreatePeer (ref JniObjectReference reference, JniObjectReferenceOptions options, Type type) { var c = type.GetConstructor (ActivationConstructorBindingFlags, null, XAConstructorSignature, null); if (c != null) { var args = new object[] { reference.Handle, JniHandleOwnership.DoNotTransfer, }; var p = (IJavaPeerable) c.Invoke (args); JniObjectReference.Dispose (ref reference, options); return p; } return base.TryCreatePeer (ref reference, options, type); } } vastly reducing the code it needs to care about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 1 out of 2 changed files in this pull request and generated no comments.
Files not reviewed (1)
- src/Java.Interop/PublicAPI.Unshipped.txt: Language not supported
Comments suppressed due to low confidence (1)
src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs:393
- Ensure that the TryCreatePeer method correctly handles the case where the constructor is not found and returns null.
protected virtual IJavaPeerable? TryCreatePeer (
Fixes unit test failures.
Context: be6cc8f This isn't strictly required *yet*, but it *should* be required as soon as we start trying to run dotnet/android unit tests within a NativeAOT environment, because dotnet/android still uses string-based Invoker lookup, as in a pre-be6cc8fb dotnet/java-interop world. Adding `GetInvokerType()` would allow e.g. `NativeAotTypeManager.GetInvokerType()` to "stringly" lookup and resolve types. (which might fail because of the trimmer, but at least this fallback path is *expressible*.)
…because why not? Related question about semantics: should it return null if there isn't an Invoker type? Or should it instead return `type` if it doesn't *need* an Invoker type?
I'm still not sure if `GetInvokerType()` should *always* return a `Type`, or only return a type that is an invoker type. Leaving that aside, there is a question around parameter validation: there wasn't any before. A common (if inconsistently applied) pattern in java-interop is to have a public non-virtual method that does parameter validation, and then calls a protected virtual method that does the actual work. Apply that pattern here, calling the protected method `GetInvokerTypeCore()`.
jonathanpeppers
approved these changes
Feb 3, 2025
jonathanpeppers
approved these changes
Feb 3, 2025
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
jonpryor
added a commit
to dotnet/android
that referenced
this pull request
Feb 3, 2025
Changes: dotnet/java-interop@e288589...dd3c1d0 * dotnet/java-interop@dd3c1d05: [Java.Interop] Add `JniRuntime.JniValueManager.TryCreatePeer()` (dotnet/java-interop#1301) * dotnet/java-interop@fb642c94: [ci] Move build pipeline to dnceng-public (dotnet/java-interop#1299)
jonpryor
added a commit
to dotnet/android
that referenced
this pull request
Feb 3, 2025
Changes: dotnet/java-interop@e288589...dd3c1d0 * dotnet/java-interop@dd3c1d05: [Java.Interop] Add `JniRuntime.JniValueManager.TryCreatePeer()` (dotnet/java-interop#1301) * dotnet/java-interop@fb642c94: [ci] Move build pipeline to dnceng-public (dotnet/java-interop#1299)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context: 78d5937
Context: dotnet/android@7a772f0
Context: dotnet/android#9716
Context: dotnet/android@694e975
dotnet/android@7a772f03 added the beginnings of a NativeAOT sample to dotnet/android which built a ".NET for Android" app using NativeAOT, which "rhymed with" the
Hello-NativeAOTFromAndroid
sample in 78d5937.Further work on the sample showed that it was lacking support for
Android.App.Application
subclasses. dotnet/android#9716 began fixing that oversight, but in the process was triggering a stack overflow because when it needed to create a "proxy" peer around themy.MainApplication
Java type, which subclassedandroid.app.Application
, instead of creating an instance of the expectedMainApplication
C# type, it instead created an instance ofAndroid.App.Application
. This was visible from the logs:Note that
Instance.Type
isAndroid.App.Application
, not the in-sampleMainApplication
C# type.Because the runtime type was
Android.App.Application
, when we later attempted to dispatch theApplication.OnCreate()
override, this resulted in a virtual invocation of the JavaApplication.onCreate()
method instead of a non-virtual invocation ofApplication.onCreate()
. This virtual invocation was the root of a recursive loop which eventually resulted in a stack overflow.The fix in dotnet/android@694e975e was to fix
NativeAotTypeManager.CreatePeer()
so that it properly checked for a binding of the runtime type of the Java instance before using the "fallback" type provided toObject.GetObject<T>()
in theApplication.n_OnCreate()
method:All well and good.
The problem is that
NativeAotTypeManager
in dotnet/android needs to support both dotnet/java-interop "activation constructors" with a signature of(ref JniObjectReference, JniObjectReferenceOptions)
, and the .NET for Android signature of(IntPtr, JniHandleOwnership)
. Trying to support both constructors resulted in the need to copy all ofJniRuntime.JniValueManager.CreatePeer()
and dependencies, which felt a bit excessive.Add a new
JniRuntime.JniValueManager.TryCreatePeer()
method, which will invoke the activation constructor to create anIJavaPeerable
:If the activation constructor is not found, then
TryCreatePeer()
shall returnnull
, allowingCreatePeerInstance()
to try for a base type or, ultimately, the fallback type.This will allow a future dotnet/android PR to remove
NativeAotTypeManager.CreatePeer()
and its dependencies entirely, and instead do:vastly reducing the code it needs to care about.