-
Notifications
You must be signed in to change notification settings - Fork 534
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
[Mono.Android] Java.Interop Unification! #9640
base: main
Are you sure you want to change the base?
Conversation
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Context: dotnet/android#9640 Context: 25de1f3 dotnet/android#9640 completes "unification" with dotnet/java-interop, updating `Java.Lang.Object` to inherit `Java.Interop.JavaObject`, just like `src/Java.Base`. The biggest impediment with this unification step is that the semantics for referring to a Java instance are quite different: * .NET for Android née Xamarin.Android née Mono for Android uses an `(IntPtr, JniHandleOwnership)` pair to specify the JNI object reference pointer value and what should be done with it. The *type* of the JNI object reference is *inferred* by the `JniHandleOwnership` value, e.g. `JniHandleOwnership.TransferLocalRef` means that the `IntPtr` value is a JNI local reference. * dotnet/java-interop uses a `(ref JniObjectReference, JniObjectReferenceOptions)` pair to specify the JNI object reference and what can be done with it. The `JniObjectReference` value contains the type, but -- due to "pointer trickery"; see 25de1f3 -- it might be *so* invalid that it cannot be touched at all, and `JniObjectReferenceOptions` will let us know. Which brings us to the conundrum: how do we implement the `Java.Lang.Throwable(IntPtr, JniHandleOwnership)` constructor? namespace Java.Lang { public class Throwable { public Throwable(IntPtr handle, JniHandleOwnership transfer); } } The "obvious and wrong" solution would be to use `ref` with a temporary… public Throwable(IntPtr handle, JniHandleOwnership transfer) : base (ref new JniObjectReference(handle), …) { } …but that won't compile. Next, we want to be able to provide `message` and `innerException` parameters to `System.Exception`, but the `JavaException(ref JniObjectReference, JniObjectReferenceOptions)` constructor requires a valid JNI Object Reference to do that. If `Java.Lang.Throwable` tries to "work around" this by using the `JavaException(string, Exception)` constructor: public Throwable(IntPtr handle, JniHandleOwnership transfer) : base (_GetMessage (handle), _GetInnerException (handle)) { … } then the `JavaException(string, Exception)` constructor will try to invoke the `E(String)` constructor on the Java side, which: 1. Is semantically wrong, because we may already have a Java instance in `handle`, so why are we creating a new instance? But- 2. This fails for constructor subclasses which don't provide a `E(String)` constructor! Specifically, the [`JnienvTest.ActivatedDirectThrowableSubclassesShouldBeRegistered`][0] unit test starts crashing for "bizarre" reasons. Fix these issues by adding the new exception: namespace Java.Interop { partial class JavaException { protected JavaException ( ref JniObjectReference reference, JniObjectReferenceOptions transfer, JniObjectReference throwableOverride); } } The new `throwableOverride` parameter is used to obtain the `message` and `innerException` values to provide to the `System.Exception(string, Exception)` constructor, allowing `reference` to be a "do not touch" value. Additionally, add a `protected SetJavaStackTrace()` method which is used to set the `JavaException.JavaStackTrace` property based on a `JniObjectReference` value. [0]: https://github.com/dotnet/android/blob/main/tests/Mono.Android-Tests/Java.Interop/JnienvTest.cs#L285-L305
4926f0c
to
80177c7
Compare
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
80177c7
to
7ae8702
Compare
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Context: xamarin/monodroid@e318861 Context: 130905e Context: de04316 Context: #9636 Context: dotnet/java-interop#1293 In the beginning there was Mono for Android, which had a set of `Mono.Android.dll` assemblies (one per supported API level), each of which contained "duplicated" binding logic: each API level had its own `Java.Lang.Object`, `Android.Runtime.JNIEnv`, etc. dotnet/java-interop started, in part, as a way to "split out" the core integration logic, so that it *wouldn't* need to be duplicated across every assembly. As part of this, it introduced its own core abstractions, notably `Java.Interop.IJavaPeerable` and `Java.Interop.JavaObject`. When dotnet/java-interop was first introduced into Xamarin.Android, with xamarin/monodroid@e318861e, the integration was incomplete. Integration continued with 130905e, allowing unit tests within `Java.Interop-Tests.dll` to run within Xamarin.Android and construction of instances of e.g. `JavaInt32Array`, but one large piece of integration remained: Moving GC bridge code *out* of `Java.Lang.Object`, and instead relying on `Java.Interop.JavaObject`, turning this: namespace Java.Lang { public partial class Object : System.Object, IJavaPeerable /* … */ { } } into this: namespace Java.Lang { public partial class Object : Java.Interop.JavaObject, IJavaPeerable /* … */ { } } *Why*? In part because @jonpryor has wanted to do this for literal years at this point, but also in part because of #9636 and related efforts to use Native AOT, which involves avoiding / bypassing `DllImportAttribute` invocations (for now, everything touched by Native AOT becomes a single `.so` binary, which we don't know the name of). Avoiding P/Invoke means *embracing* and extending existing Java.Interop constructs (e.g. de04316). In addition to altering the base types of `Java.Lang.Object` and `Java.Lang.Throwable`: * Remove `handle` and related fields from `Java.Lang.Object` and `Java.Lang.Throwable`. * Update `PreserveLists/Mono.Android.xml` so that the removed fields are note preserved. * Rename `JNIenvInit.AndroidValueManager` to `JNIEnvInit.ValueManager`, and change its type to `JniRuntime.JniValueManager`. This is to help "force" usage of `JnIRuntime.JniValueManager` in more places, as we can't currently use `AndroidValueManager` in Native AOT (P/Invokes!). * Cleanup: Remove `JNIEnv.Exit()` and related code. These were used by the Android Designer, which is no longer supported. * Update (`internal`) interface `IJavaObjectEx` to remove constructs present on `IJavaPeerable.` * Update `ExceptionTest.CompareStackTraces()` to use `System.Diagnostics.StackTrace(ex, fNeedFileInfo:true)` so that when the `debug.mono.debug` system property is set, the `ExceptionTest.InnerExceptionIsSet()` unit test doesn't fail. Also, increase assertion message verbosity.
7ae8702
to
881291c
Compare
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
…since we removed that earlier. Fixes warning: D:\a\_work\1\s\bin\Release\lib\packs\Microsoft.Android.Sdk.Windows\35.99.0\targets\..\PreserveLists\Mono.Android.xml(12,5): warning IL2009: Could not find method 'Exit' on type 'Android.Runtime.JNIEnv'. Update `MonodroidRuntime::shutdown_android_runtime()` to abort when called. The Android Designer is not supported.
Fixes `JsonDeserializationCreatesJavaHandle()` test: E AndroidRuntime: android.runtime.JavaProxyThrowable: [System.Runtime.Serialization.InvalidDataContractException]: AttributedTypesCannotInheritFromNonAttributedSerializableTypes, Java.Lang.Object, Java.Interop.JavaObject
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
This is now down to "1" test failure (and a bunch of network-related test failures which I currently assume will disappear if I re-run). The failing test: JsonDeserializationCreatesJavaHandle(True), in: android/tests/MSBuildDeviceIntegration/Tests/InstallAndRunTests.cs Lines 345 to 435 in 7aed4e3
From
|
Of interest is the exception message:
which is not immediately helpful, so if we look up what <data name="SerializationInfo_ConstructorNotFound" xml:space="preserve">
<value>The constructor with parameters (SerializationInfo, StreamingContext) is not found in ISerializable type '{0}'.</value>
</data> Given that the parameter Alas, % monodis --method obj/Release/android-arm64/linked/shrunk/System.Private.CoreLib.dll
…
########## System.IntPtr
4070: instance default void '.ctor' (int32 A_1) (param: 3662 impl_flags: cil managed )
4071: instance default void '.ctor' (int64 A_1) (param: 3662 impl_flags: cil managed )
4072: instance default void '.ctor' (void* A_1) (param: 3662 impl_flags: cil managed )
… Compare to unlinked: % monodis --method bin/Debug/dotnet/packs/Microsoft.NETCore.App.Runtime.Mono.android-arm64/10.0.0-alpha.1.25061.3/runtimes/android-arm64/native/System.Private.CoreLib.dll
…
########## System.IntPtr
5530: instance default void '.ctor' (int32 'value') (param: 9168 impl_flags: cil managed )
5531: instance default void '.ctor' (int64 'value') (param: 9169 impl_flags: cil managed )
5532: instance default void '.ctor' (void* 'value') (param: 9170 impl_flags: cil managed )
5533: instance default void '.ctor' (class System.Runtime.Serialization.SerializationInfo info, class System.Runtime.Serialization.StreamingContext context) (param: 9171 impl_flags: cil managed ) Hypothesis "confirmed," but what happens with .NET 9? If I build the failing project with Of interest is the difference in This PR:
.NET 9:
Notice that this PR adds the keys Upon review, what I'm missing in |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
[NonSerialized] IntPtr key_handle; | ||
#pragma warning disable CS0649, CS0169, CS0414 // Suppress fields are never used warnings, these fields are used directly by monodroid-glue.cc | ||
[NonSerialized] int refs_added; | ||
#pragma warning restore CS0649, CS0169, CS0414 | ||
[NonSerialized] JObjectRefType handle_type; | ||
[NonSerialized] internal IntPtr handle; | ||
[NonSerialized] bool needsActivation; | ||
[NonSerialized] bool isProxy; |
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.
Regarding the one failure related to JSON serialization, do we need [NonSerialized]
on the fields here?
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.
Yup, done here: dotnet/java-interop@9d1cae1
<type fullname="Java.Lang.Object"> | ||
<field name="handle" /> | ||
<field name="handle_type" /> | ||
<field name="refs_added" /> | ||
<method name="SetHandleOnDeserialized" /> | ||
</type> | ||
<type fullname="Java.Lang.Throwable"> | ||
<field name="handle" /> | ||
<field name="handle_type" /> | ||
<field name="refs_added" /> | ||
</type> |
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.
It looks ok to remove these, we have this:
<assembly fullname="Java.Interop"> | |
<type fullname="Java.Interop.JavaObject"> | |
<field name="handle" /> | |
<field name="handle_type" /> | |
<field name="refs_added" /> | |
</type> | |
<type fullname="Java.Interop.JavaException"> | |
<field name="handle" /> | |
<field name="handle_type" /> | |
<field name="refs_added" /> | |
</type> | |
</assembly> |
return new JniObjectReference (handle, (JniObjectReferenceType) handle_type); | ||
} | ||
} | ||
public JniObjectReference PeerReference => base.PeerReference; |
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.
Does this need the new
keyword to avoid a C# warning?
return new JniObjectReference (handle, (JniObjectReferenceType) handle_type); | ||
} | ||
} | ||
public JniObjectReference PeerReference => base.PeerReference; |
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.
Same here, might need new
.
public IntPtr Handle { | ||
get { | ||
var peerRef = PeerReference; | ||
if (!peerRef.IsValid) |
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.
Missing curly braces around if
body
@@ -24,6 +24,13 @@ reinitialize_android_runtime_type_manager (JNIEnv *env) | |||
inline void | |||
MonodroidRuntime::shutdown_android_runtime (MonoDomain *domain) | |||
{ | |||
Helpers::abort_application (LOG_DEFAULT, "") |
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.
We should probably just remove this entire file
Context: https://github.com/xamarin/monodroid/commit/e318861ed8eb20a71852378ddd558409d6b1c234
Context: 130905e
Context: de04316
Context: #9636
In the beginning there was Mono for Android, which had a set of
Mono.Android.dll
assemblies (one per supported API level), each of which contained "duplicated" binding logic: each API level had its ownJava.Lang.Object
,Android.Runtime.JNIEnv
, etc.dotnet/java-interop started, in part, as a way to "split out" the core integration logic, so that it wouldn't need to be duplicated across every assembly. As part of this, it introduced its own core abstractions, notably
Java.Interop.IJavaPeerable
andJava.Interop.JavaObject
.When dotnet/java-interop was first introduced into Xamarin.Android, with xamarin/monodroid@e318861e, the integration was incomplete. Integration continued with 130905e, allowing unit tests within
Java.Interop-Tests.dll
to run within Xamarin.Android and construction of instances of e.g.JavaInt32Array
, but one large piece of integration remained:Moving GC bridge code out of
Java.Lang.Object
, and instead relying onJava.Interop.JavaObject
, turning this:into this:
Why? In part because @jonpryor has wanted to do this for literal years at this point, but also in part because of #9636 and related efforts to use Native AOT, which involves avoiding / bypassing
DllImportAttribute
invocations (for now, everything touched by Native AOT becomes a single.so
binary, which we don't know the name of). Avoiding P/Invoke means embracing and extending existing Java.Interop constructs (e.g. de04316).In addition to altering the base types of
Java.Lang.Object
andJava.Lang.Throwable
:Remove
handle
and related fields fromJava.Lang.Object
andJava.Lang.Throwable
.Update
PreserveLists/Mono.Android.xml
so that the removed fields are note preserved.Rename
JNIenvInit.AndroidValueManager
toJNIEnvInit.ValueManager
, and change its type toJniRuntime.JniValueManager
. This is to help "force" usage ofJnIRuntime.JniValueManager
in more places, as we can't currently useAndroidValueManager
in Native AOT (P/Invokes!).Cleanup: Remove
JNIEnv.Exit()
and related code. These were used by the Android Designer, which is no longer supported.Update (
internal
) interfaceIJavaObjectEx
to remove constructs present onIJavaPeerable.
Known issues:
Java.Lang.Throwable(IntPtr, JniHandleOwnership)
invocation will result in construction of an "extra"java.lang.Throwable
instance on the Java side, which will be immediately discarded. This is because it uses theJavaException(string, Exception)
constructor, which implicitly creates a Java peer.We may need dotnet/java-interop changes to better address this.