From 6a389ded78578374632dd5ff9b6097c2a5eccb2b Mon Sep 17 00:00:00 2001 From: Jonathan Pryor Date: Fri, 14 Mar 2025 15:26:30 -0400 Subject: [PATCH 1/6] [Java.Interop-Tests] Add CreatePeer_ReplaceableDoesNotReplace test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Context: 3043d890239cf53eb42633430faa417c140c8952 Context: https://github.com/dotnet/android/issues/9862 Context: https://github.com/dotnet/android/issues/9862#issuecomment-2705027573 In dotnet/android#9862, there is an observed "race condition" around `Android.App.Application` subclass creation. *Two* instances of `AndroidApp` were created, one from the "normal" app startup: at crc647fae2f69c19dcd0d.AndroidApp.n_onCreate(Native Method) at crc647fae2f69c19dcd0d.AndroidApp.onCreate(AndroidApp.java:25) at android.app.Instrumentation.callApplicationOnCreate(Instrumentation.java:1316) and another from an `androidx.work.WorkerFactory`: at mono.android.TypeManager.n_activate(Native Method) at mono.android.TypeManager.Activate(TypeManager.java:7) at crc647fae2f69c19dcd0d.SyncWorker.(SyncWorker.java:23) at java.lang.reflect.Constructor.newInstance0(Native Method) at java.lang.reflect.Constructor.newInstance(Constructor.java:343) at androidx.work.WorkerFactory.createWorkerWithDefaultFallback(WorkerFactory.java:95) However, what was odd about this "race condition" was that the *second* instance created would reliably win! Further investigation suggested that this was less of a "race condition" and more a bug in `AndroidRuntime`, wherein when "Replaceable" instances were created, an existing instance would *always* be replaced. Aside: JniManagedPeerStates.Replaceable is from 3043d890: > `JniManagedPeerStates.Replaceable` … means > that the Peer instance was created through the activation constructor. > It additionally means that if two managed instances are created around > the same Java instance, the non-Replaceable instance will be the one > returned by JniRuntime.JniValueManager.PeekObject(). What we're observing in dotnet/android#9862 is that while the Replaceable instance is replaced, it's being replaced by *another* Replaceable instance! This feels bananas; yes, Replaceable should be replacable, but only by *non*-Replaceable instances. Update `JniRuntimeJniValueManagerContract` to add a new `CreatePeer_ReplaceableDoesNotReplace()` test to codify the desired semantic that Replaceable instances do not replace Replaceable instances. Surprisingly, this does not fail on java-interop! Apparently `ManagedValueManager.AddPeer()` bails early when `PeekPeer()` finds a value, while `AndroidRuntime.AddPeer()` does not bail early. --- .../JniRuntimeJniValueManagerContract.cs | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/Java.Interop-Tests/Java.Interop/JniRuntimeJniValueManagerContract.cs b/tests/Java.Interop-Tests/Java.Interop/JniRuntimeJniValueManagerContract.cs index 782b6e765..4cfa0497b 100644 --- a/tests/Java.Interop-Tests/Java.Interop/JniRuntimeJniValueManagerContract.cs +++ b/tests/Java.Interop-Tests/Java.Interop/JniRuntimeJniValueManagerContract.cs @@ -159,6 +159,26 @@ public unsafe void CreatePeer_UsesFallbackType () Assert.AreSame (typeof (IJavaInterfaceInvoker), p!.GetType ()); } + [Test] + public void CreatePeer_ReplaceableDoesNotReplace () + { + var v = new AnotherJavaInterfaceImpl (); + var lref = v.PeerReference.NewLocalRef (); + v.Dispose (); + + try { + var peer1 = valueManager.CreatePeer (ref lref, JniObjectReferenceOptions.Copy, typeof (AnotherJavaInterfaceImpl)); + Assert.IsTrue (peer1.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable)); + var peer2 = valueManager.CreatePeer (ref lref, JniObjectReferenceOptions.Copy, typeof (AnotherJavaInterfaceImpl)); + Assert.IsTrue (peer2.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable)); + Assert.AreNotSame (peer1, peer2); + var peeked = valueManager.PeekPeer (peer2.PeerReference); + Assert.AreSame (peer1, peeked); + } finally { + JniObjectReference.Dispose (ref lref); + } + } + [Test] public void CreateValue () { From 71c26e8e6068f64e6a7cbda8a309b2a03245776a Mon Sep 17 00:00:00 2001 From: Jonathan Pryor Date: Fri, 14 Mar 2025 20:32:19 -0400 Subject: [PATCH 2/6] Fix warnings. --- .../Java.Interop/JniRuntimeJniValueManagerContract.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/Java.Interop-Tests/Java.Interop/JniRuntimeJniValueManagerContract.cs b/tests/Java.Interop-Tests/Java.Interop/JniRuntimeJniValueManagerContract.cs index 4cfa0497b..8e7a3ca4e 100644 --- a/tests/Java.Interop-Tests/Java.Interop/JniRuntimeJniValueManagerContract.cs +++ b/tests/Java.Interop-Tests/Java.Interop/JniRuntimeJniValueManagerContract.cs @@ -168,9 +168,9 @@ public void CreatePeer_ReplaceableDoesNotReplace () try { var peer1 = valueManager.CreatePeer (ref lref, JniObjectReferenceOptions.Copy, typeof (AnotherJavaInterfaceImpl)); - Assert.IsTrue (peer1.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable)); + Assert.IsTrue (peer1!.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable)); var peer2 = valueManager.CreatePeer (ref lref, JniObjectReferenceOptions.Copy, typeof (AnotherJavaInterfaceImpl)); - Assert.IsTrue (peer2.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable)); + Assert.IsTrue (peer2!.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable)); Assert.AreNotSame (peer1, peer2); var peeked = valueManager.PeekPeer (peer2.PeerReference); Assert.AreSame (peer1, peeked); From 19ec1a997b328c65930e39916c4a04373dc38742 Mon Sep 17 00:00:00 2001 From: Jonathan Pryor Date: Fri, 14 Mar 2025 21:39:25 -0400 Subject: [PATCH 3/6] Provide assertion messages. --- .../JniRuntimeJniValueManagerContract.cs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/tests/Java.Interop-Tests/Java.Interop/JniRuntimeJniValueManagerContract.cs b/tests/Java.Interop-Tests/Java.Interop/JniRuntimeJniValueManagerContract.cs index 8e7a3ca4e..d95aa49c8 100644 --- a/tests/Java.Interop-Tests/Java.Interop/JniRuntimeJniValueManagerContract.cs +++ b/tests/Java.Interop-Tests/Java.Interop/JniRuntimeJniValueManagerContract.cs @@ -168,12 +168,16 @@ public void CreatePeer_ReplaceableDoesNotReplace () try { var peer1 = valueManager.CreatePeer (ref lref, JniObjectReferenceOptions.Copy, typeof (AnotherJavaInterfaceImpl)); - Assert.IsTrue (peer1!.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable)); + Assert.IsTrue ( + peer1!.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable), + $"Expected peer1.JniManagedPeerState to have .Replaceable, but was {peer1.JniManagedPeerState}."); var peer2 = valueManager.CreatePeer (ref lref, JniObjectReferenceOptions.Copy, typeof (AnotherJavaInterfaceImpl)); - Assert.IsTrue (peer2!.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable)); - Assert.AreNotSame (peer1, peer2); + Assert.IsTrue ( + peer2!.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable), + $"Expected peer2.JniManagedPeerState to have .Replaceable, but was {peer1.JniManagedPeerState}."); + Assert.AreNotSame (peer1, peer2, "Expected peer1 and peer2 to be different instances."); var peeked = valueManager.PeekPeer (peer2.PeerReference); - Assert.AreSame (peer1, peeked); + Assert.AreSame (peer1, peeked, "Expected peer1 and peeked to be the same instance."); } finally { JniObjectReference.Dispose (ref lref); } From f38a9462686f9e9544e536ba72792b9473782984 Mon Sep 17 00:00:00 2001 From: Jonathan Pryor Date: Wed, 9 Apr 2025 15:43:05 -0400 Subject: [PATCH 4/6] More tests, better error messages --- .../JniRuntimeJniValueManagerContract.cs | 42 +++++++++++++++++-- 1 file changed, 39 insertions(+), 3 deletions(-) diff --git a/tests/Java.Interop-Tests/Java.Interop/JniRuntimeJniValueManagerContract.cs b/tests/Java.Interop-Tests/Java.Interop/JniRuntimeJniValueManagerContract.cs index d95aa49c8..475403875 100644 --- a/tests/Java.Interop-Tests/Java.Interop/JniRuntimeJniValueManagerContract.cs +++ b/tests/Java.Interop-Tests/Java.Interop/JniRuntimeJniValueManagerContract.cs @@ -5,6 +5,7 @@ using System.Diagnostics.CodeAnalysis; using System.Linq; using System.Reflection; +using System.Runtime.CompilerServices; using System.Threading; using Java.Interop; @@ -159,6 +160,32 @@ public unsafe void CreatePeer_UsesFallbackType () Assert.AreSame (typeof (IJavaInterfaceInvoker), p!.GetType ()); } + [Test] + public void CreatePeer_CreatesNewValueUsingActivationConstructor () + { + using var v1 = new AnotherJavaInterfaceImpl (); + var lref = v1.PeerReference.NewLocalRef (); + try { + using var v2 = valueManager.CreatePeer (ref lref, JniObjectReferenceOptions.CopyAndDispose, typeof (AnotherJavaInterfaceImpl)); + Assert.AreNotSame (v1, v2, "CreatePeer() should create new values"); + } + finally { + JniObjectReference.Dispose (ref lref); + } + } + + [Test] + public void CreatePeer_ThrowsIfNoActivationConstructorPresent () + { + using var v1 = new GetThis (); + var lref = v1.PeerReference.NewLocalRef (); + var ex = Assert.Throws ( + () => valueManager.CreatePeer (ref lref, JniObjectReferenceOptions.CopyAndDispose, typeof (GetThis)), + $"`GetThis` has no activation constructor, so attempting to use it should throw NotSupportedException."); + Assert.IsTrue (lref.IsValid, "lref should still be valid"); + JniObjectReference.Dispose (ref lref); + } + [Test] public void CreatePeer_ReplaceableDoesNotReplace () { @@ -167,17 +194,26 @@ public void CreatePeer_ReplaceableDoesNotReplace () v.Dispose (); try { + Assert.IsNull (valueManager.PeekPeer (lref), "v.Dispose() should have unregistered the peer."); var peer1 = valueManager.CreatePeer (ref lref, JniObjectReferenceOptions.Copy, typeof (AnotherJavaInterfaceImpl)); Assert.IsTrue ( peer1!.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable), $"Expected peer1.JniManagedPeerState to have .Replaceable, but was {peer1.JniManagedPeerState}."); + Assert.AreSame (peer1, valueManager.PeekPeer (lref), + $"Expected peer1==PeekValue(peer1.PeerReference); it's the only one that should exist!"); + var peer2 = valueManager.CreatePeer (ref lref, JniObjectReferenceOptions.Copy, typeof (AnotherJavaInterfaceImpl)); Assert.IsTrue ( peer2!.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable), - $"Expected peer2.JniManagedPeerState to have .Replaceable, but was {peer1.JniManagedPeerState}."); + $"Expected peer2.JniManagedPeerState to have .Replaceable, but was {peer2.JniManagedPeerState}."); Assert.AreNotSame (peer1, peer2, "Expected peer1 and peer2 to be different instances."); - var peeked = valueManager.PeekPeer (peer2.PeerReference); - Assert.AreSame (peer1, peeked, "Expected peer1 and peeked to be the same instance."); + + var peeked = valueManager.PeekPeer (lref); + Assert.AreSame (peer1, peeked, + "Expected peer1 and peeked to be the same instance; " + + $"peeked={RuntimeHelpers.GetHashCode (peeked).ToString ("x")}, " + + $"peer1={RuntimeHelpers.GetHashCode (peer1).ToString ("x")}, " + + $"peer2={RuntimeHelpers.GetHashCode (peer2).ToString ("x")}"); } finally { JniObjectReference.Dispose (ref lref); } From 1f76a82a2ec3c7ee66d57a466a9f847ff1beb583 Mon Sep 17 00:00:00 2001 From: Jonathan Pryor Date: Wed, 9 Apr 2025 16:49:44 -0400 Subject: [PATCH 5/6] Update to mirror JnienvCreateInstance_RegistersMultipleInstances Context: https://github.com/dotnet/android/pull/10004 It looks like dotnet/android#10004 is closely tied to dotnet/android#9862. Might as well update tests to hit this behavior! --- .../CallVirtualFromConstructorDerived.cs | 16 +++- .../InvokeVirtualFromConstructorTests.cs | 85 ++++++++++++++++--- 2 files changed, 87 insertions(+), 14 deletions(-) diff --git a/tests/Java.Interop-Tests/Java.Interop/CallVirtualFromConstructorDerived.cs b/tests/Java.Interop-Tests/Java.Interop/CallVirtualFromConstructorDerived.cs index 78e09d469..a3a8b8e48 100644 --- a/tests/Java.Interop-Tests/Java.Interop/CallVirtualFromConstructorDerived.cs +++ b/tests/Java.Interop-Tests/Java.Interop/CallVirtualFromConstructorDerived.cs @@ -1,4 +1,7 @@ -using System; +#nullable enable + +using System; +using System.Runtime.CompilerServices; using Java.Interop; @@ -35,10 +38,15 @@ public CallVirtualFromConstructorDerived (int value) public bool InvokedActivationConstructor; + public static CallVirtualFromConstructorDerived? Intermediate_FromCalledFromConstructor; + public static CallVirtualFromConstructorDerived? Intermediate_FromActivationConstructor; + public CallVirtualFromConstructorDerived (ref JniObjectReference reference, JniObjectReferenceOptions options) : base (ref reference, options) { InvokedActivationConstructor = true; + + Intermediate_FromActivationConstructor = this; } public bool Called; @@ -47,6 +55,8 @@ public override void CalledFromConstructor (int value) { Called = true; calledValue = value; + + Intermediate_FromCalledFromConstructor = this; } public static unsafe CallVirtualFromConstructorDerived NewInstance (int value) @@ -54,7 +64,7 @@ public static unsafe CallVirtualFromConstructorDerived NewInstance (int value) JniArgumentValue* args = stackalloc JniArgumentValue [1]; args [0] = new JniArgumentValue (value); var o = _members.StaticMethods.InvokeObjectMethod ("newInstance.(I)Lnet/dot/jni/test/CallVirtualFromConstructorDerived;", args); - return JniEnvironment.Runtime.ValueManager.GetValue (ref o, JniObjectReferenceOptions.CopyAndDispose); + return JniEnvironment.Runtime.ValueManager.GetValue (ref o, JniObjectReferenceOptions.CopyAndDispose)!; } delegate void CalledFromConstructorMarshalMethod (IntPtr jnienv, IntPtr n_self, int value); @@ -63,7 +73,7 @@ static void CalledFromConstructorHandler (IntPtr jnienv, IntPtr n_self, int valu var envp = new JniTransition (jnienv); try { var r_self = new JniObjectReference (n_self); - var self = JniEnvironment.Runtime.ValueManager.GetValue(ref r_self, JniObjectReferenceOptions.Copy); + var self = JniEnvironment.Runtime.ValueManager.GetValue(ref r_self, JniObjectReferenceOptions.Copy)!; self.CalledFromConstructor (value); self.DisposeUnlessReferenced (); } diff --git a/tests/Java.Interop-Tests/Java.Interop/InvokeVirtualFromConstructorTests.cs b/tests/Java.Interop-Tests/Java.Interop/InvokeVirtualFromConstructorTests.cs index ed3e585e0..037d8f3c3 100644 --- a/tests/Java.Interop-Tests/Java.Interop/InvokeVirtualFromConstructorTests.cs +++ b/tests/Java.Interop-Tests/Java.Interop/InvokeVirtualFromConstructorTests.cs @@ -1,4 +1,5 @@ using System; +using System.Runtime.CompilerServices; using Java.Interop; @@ -7,25 +8,87 @@ namespace Java.InteropTests { [TestFixture] +#if !__ANDROID__ + // We want stability around the CallVirtualFromConstructorDerived static fields + [NonParallelizable] +#endif // !__ANDROID__ public class InvokeVirtualFromConstructorTests : JavaVMFixture { [Test] - public void InvokeVirtualFromConstructor () + public void CreateManagedInstanceFirst () { - using (var t = new CallVirtualFromConstructorDerived (42)) { - Assert.IsTrue (t.Called); - Assert.IsNotNull (JniRuntime.CurrentRuntime.ValueManager.PeekValue (t.PeerReference)); - } + CallVirtualFromConstructorDerived.Intermediate_FromActivationConstructor = null; + CallVirtualFromConstructorDerived.Intermediate_FromCalledFromConstructor = null; + + using var t = new CallVirtualFromConstructorDerived (42); + Assert.IsTrue ( + t.Called, + "CalledFromConstructor method override should have been called."); + Assert.IsFalse ( + t.InvokedActivationConstructor, + "Activation Constructor should have been called, as calledFromConstructor() is invoked before ManagedPeer.construct()."); + Assert.IsTrue ( + t.InvokedConstructor, + "(int) constructor should have been called, via ManagedPeer.construct()."); + + var registered = JniRuntime.CurrentRuntime.ValueManager.PeekValue (t.PeerReference); + var acIntermediate = CallVirtualFromConstructorDerived.Intermediate_FromActivationConstructor; + var cfIntermediate = CallVirtualFromConstructorDerived.Intermediate_FromCalledFromConstructor; + + Assert.AreSame (t, registered, + "Expected t and registered to be the same instance; " + + $"t={RuntimeHelpers.GetHashCode (t).ToString ("x")}, " + + $"registered={RuntimeHelpers.GetHashCode (registered).ToString ("x")}"); + Assert.AreNotSame (t, acIntermediate, + "Expected t and registered to be the same instance; " + + $"t={RuntimeHelpers.GetHashCode (t).ToString ("x")}, " + + $"registered={RuntimeHelpers.GetHashCode (registered).ToString ("x")}"); + Assert.AreSame (t, cfIntermediate, + "Expected t and cfIntermediate to be the same instance; " + + $"t={RuntimeHelpers.GetHashCode (t).ToString ("x")}, " + + $"cfIntermediate={RuntimeHelpers.GetHashCode (cfIntermediate).ToString ("x")}"); + + CallVirtualFromConstructorDerived.Intermediate_FromActivationConstructor = null; + CallVirtualFromConstructorDerived.Intermediate_FromCalledFromConstructor = null; } [Test] - public void ActivationConstructor () + public void CreateJavaInstanceFirst () { - var t = CallVirtualFromConstructorDerived.NewInstance (42); - using (t) { - Assert.IsTrue (t.InvokedActivationConstructor); - Assert.IsTrue (t.InvokedConstructor); - } + CallVirtualFromConstructorDerived.Intermediate_FromActivationConstructor = null; + CallVirtualFromConstructorDerived.Intermediate_FromCalledFromConstructor = null; + + using var t = CallVirtualFromConstructorDerived.NewInstance (42); + + Assert.IsTrue ( + t.Called, + "CalledFromConstructor method override should have been called."); + Assert.IsTrue ( + t.InvokedActivationConstructor, + "Activation Constructor should have been called, as calledFromConstructor() is invoked before ManagedPeer.construct()."); + Assert.IsTrue ( + t.InvokedConstructor, + "(int) constructor should have been called, via ManagedPeer.construct()."); + + var registered = JniRuntime.CurrentRuntime.ValueManager.PeekValue (t.PeerReference); + var acIntermediate = CallVirtualFromConstructorDerived.Intermediate_FromActivationConstructor; + var cfIntermediate = CallVirtualFromConstructorDerived.Intermediate_FromCalledFromConstructor; + + Assert.AreSame (t, registered, + "Expected t and registered to be the same instance; " + + $"t={RuntimeHelpers.GetHashCode (t).ToString ("x")}, " + + $"registered={RuntimeHelpers.GetHashCode (registered).ToString ("x")}"); + Assert.AreSame (t, acIntermediate, + "Expected t and registered to be the same instance; " + + $"t={RuntimeHelpers.GetHashCode (t).ToString ("x")}, " + + $"registered={RuntimeHelpers.GetHashCode (registered).ToString ("x")}"); + Assert.AreSame (t, cfIntermediate, + "Expected t and cfIntermediate to be the same instance; " + + $"t={RuntimeHelpers.GetHashCode (t).ToString ("x")}, " + + $"cfIntermediate={RuntimeHelpers.GetHashCode (cfIntermediate).ToString ("x")}"); + + CallVirtualFromConstructorDerived.Intermediate_FromActivationConstructor = null; + CallVirtualFromConstructorDerived.Intermediate_FromCalledFromConstructor = null; } } } From bf0266079648f91fa03f6944dc22a503475e504b Mon Sep 17 00:00:00 2001 From: Jonathan Pryor Date: Fri, 11 Apr 2025 08:29:27 -0400 Subject: [PATCH 6/6] Add support for JNIEnv::NewObject() TODO: proper explanation. See also: * 3043d890239cf53eb42633430faa417c140c8952 * https://github.com/xamarin/monodroid/commit/326509e56d4e582c53bbe5dfe6d5c741a27f1af5 * https://github.com/xamarin/monodroid/commit/940136ebf1318a7c57a855e2728ce2703c0240af * https://github.com/dotnet/android/pull/10004 --- .../JniRuntime.JniValueManager.cs | 31 ++++++-- src/Java.Interop/PublicAPI.Unshipped.txt | 2 +- .../Java.Interop/ManagedValueManager.cs | 10 +-- .../CallVirtualFromConstructorBase.cs | 22 +++++- .../CallVirtualFromConstructorDerived.cs | 16 +++- .../InvokeVirtualFromConstructorTests.cs | 74 +++++++++++++++++-- 6 files changed, 131 insertions(+), 24 deletions(-) diff --git a/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs b/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs index c33f4b9ae..1ce3e6543 100644 --- a/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs +++ b/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs @@ -401,7 +401,27 @@ static Type GetPeerType ([DynamicallyAccessedMembers (Constructors)] Type type) Type type) { type = Runtime.TypeManager.GetInvokerType (type) ?? type; - return TryCreatePeer (ref reference, options, type); + + var self = GetUninitializedObject (type); + var constructed = false; + try { + constructed = TryConstructPeer (self, ref reference, options, type); + } finally { + if (!constructed) { + GC.SuppressFinalize (self); + self = null; + } + } + return self; + + static IJavaPeerable GetUninitializedObject ( + [DynamicallyAccessedMembers (Constructors)] + Type type) + { + var v = (IJavaPeerable) System.Runtime.CompilerServices.RuntimeHelpers.GetUninitializedObject (type); + v.SetJniManagedPeerState (JniManagedPeerStates.Replaceable | JniManagedPeerStates.Activatable); + return v; + } } const BindingFlags ActivationConstructorBindingFlags = BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance; @@ -409,7 +429,8 @@ static Type GetPeerType ([DynamicallyAccessedMembers (Constructors)] Type type) static readonly Type[] JIConstructorSignature = new Type [] { ByRefJniObjectReference, typeof (JniObjectReferenceOptions) }; - protected virtual IJavaPeerable? TryCreatePeer ( + protected virtual bool TryConstructPeer ( + IJavaPeerable self, ref JniObjectReference reference, JniObjectReferenceOptions options, [DynamicallyAccessedMembers (Constructors)] @@ -421,11 +442,11 @@ static Type GetPeerType ([DynamicallyAccessedMembers (Constructors)] Type type) reference, options, }; - var p = (IJavaPeerable) c.Invoke (args); + c.Invoke (self, args); reference = (JniObjectReference) args [0]; - return p; + return true; } - return null; + return false; } public object? CreateValue ( diff --git a/src/Java.Interop/PublicAPI.Unshipped.txt b/src/Java.Interop/PublicAPI.Unshipped.txt index f25b9f6bb..90cde5ae6 100644 --- a/src/Java.Interop/PublicAPI.Unshipped.txt +++ b/src/Java.Interop/PublicAPI.Unshipped.txt @@ -4,7 +4,7 @@ static Java.Interop.JniEnvironment.EndMarshalMethod(ref Java.Interop.JniTransiti virtual Java.Interop.JniRuntime.OnEnterMarshalMethod() -> void virtual Java.Interop.JniRuntime.OnUserUnhandledException(ref Java.Interop.JniTransition transition, System.Exception! e) -> void virtual Java.Interop.JniRuntime.JniTypeManager.GetInvokerTypeCore(System.Type! type) -> System.Type? -virtual Java.Interop.JniRuntime.JniValueManager.TryCreatePeer(ref Java.Interop.JniObjectReference reference, Java.Interop.JniObjectReferenceOptions options, System.Type! type) -> Java.Interop.IJavaPeerable? +virtual Java.Interop.JniRuntime.JniValueManager.TryConstructPeer(Java.Interop.IJavaPeerable! self, ref Java.Interop.JniObjectReference reference, Java.Interop.JniObjectReferenceOptions options, System.Type! type) -> bool Java.Interop.JavaException.JavaException(ref Java.Interop.JniObjectReference reference, Java.Interop.JniObjectReferenceOptions transfer, Java.Interop.JniObjectReference throwableOverride) -> void Java.Interop.JavaException.SetJavaStackTrace(Java.Interop.JniObjectReference peerReferenceOverride = default(Java.Interop.JniObjectReference)) -> void Java.Interop.JniRuntime.JniTypeManager.GetInvokerType(System.Type! type) -> System.Type? diff --git a/src/Java.Runtime.Environment/Java.Interop/ManagedValueManager.cs b/src/Java.Runtime.Environment/Java.Interop/ManagedValueManager.cs index 46fd69206..aff5d4432 100644 --- a/src/Java.Runtime.Environment/Java.Interop/ManagedValueManager.cs +++ b/src/Java.Runtime.Environment/Java.Interop/ManagedValueManager.cs @@ -57,9 +57,6 @@ public override void AddPeer (IJavaPeerable value) var r = value.PeerReference; if (!r.IsValid) throw new ObjectDisposedException (value.GetType ().FullName); - var o = PeekPeer (value.PeerReference); - if (o != null) - return; if (r.Type != JniObjectReferenceType.Global) { value.SetPeerReference (r.NewGlobalRef ()); @@ -80,7 +77,7 @@ public override void AddPeer (IJavaPeerable value) var p = peers [i]; if (!JniEnvironment.Types.IsSameObject (p.PeerReference, value.PeerReference)) continue; - if (Replaceable (p)) { + if (Replaceable (p, value)) { peers [i] = value; } else { WarnNotReplacing (key, value, p); @@ -91,11 +88,12 @@ public override void AddPeer (IJavaPeerable value) } } - static bool Replaceable (IJavaPeerable peer) + static bool Replaceable (IJavaPeerable peer, IJavaPeerable value) { if (peer == null) return true; - return (peer.JniManagedPeerState & JniManagedPeerStates.Replaceable) == JniManagedPeerStates.Replaceable; + return peer.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable) && + !value.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable); } void WarnNotReplacing (int key, IJavaPeerable ignoreValue, IJavaPeerable keepValue) diff --git a/tests/Java.Interop-Tests/Java.Interop/CallVirtualFromConstructorBase.cs b/tests/Java.Interop-Tests/Java.Interop/CallVirtualFromConstructorBase.cs index 25e36762a..9fe6ebe8b 100644 --- a/tests/Java.Interop-Tests/Java.Interop/CallVirtualFromConstructorBase.cs +++ b/tests/Java.Interop-Tests/Java.Interop/CallVirtualFromConstructorBase.cs @@ -1,4 +1,5 @@ using System; +using System.Runtime.CompilerServices; using Java.Interop; using Java.Interop.GenericMarshaler; @@ -16,14 +17,31 @@ public override JniPeerMembers JniPeerMembers { } public unsafe CallVirtualFromConstructorBase (int value) + : this (value, useNewObject: false) + { + } + + public unsafe CallVirtualFromConstructorBase (int value, bool useNewObject) : this (ref *InvalidJniObjectReference, JniObjectReferenceOptions.None) { if (PeerReference.IsValid) return; - var peer = JniPeerMembers.InstanceMethods.StartGenericCreateInstance ("(I)V", GetType (), value); + const string __id = "(I)V"; + + if (useNewObject) { + var ctors = JniPeerMembers.InstanceMethods.GetConstructorsForType (GetType ()); + var init = ctors.GetConstructor (__id); + + JniArgumentValue* __args = stackalloc JniArgumentValue [1]; + __args [0] = new JniArgumentValue (value); + var lref = JniEnvironment.Object.NewObject (ctors.JniPeerType.PeerReference, init, __args); + Construct (ref lref, JniObjectReferenceOptions.CopyAndDispose); + return; + } + var peer = JniPeerMembers.InstanceMethods.StartGenericCreateInstance (__id, GetType (), value); Construct (ref peer, JniObjectReferenceOptions.CopyAndDispose); - JniPeerMembers.InstanceMethods.FinishGenericCreateInstance ("(I)V", this, value); + JniPeerMembers.InstanceMethods.FinishGenericCreateInstance (__id, this, value); } public CallVirtualFromConstructorBase (ref JniObjectReference reference, JniObjectReferenceOptions options) diff --git a/tests/Java.Interop-Tests/Java.Interop/CallVirtualFromConstructorDerived.cs b/tests/Java.Interop-Tests/Java.Interop/CallVirtualFromConstructorDerived.cs index a3a8b8e48..739b85e08 100644 --- a/tests/Java.Interop-Tests/Java.Interop/CallVirtualFromConstructorDerived.cs +++ b/tests/Java.Interop-Tests/Java.Interop/CallVirtualFromConstructorDerived.cs @@ -27,10 +27,22 @@ public override JniPeerMembers JniPeerMembers { public bool InvokedConstructor; public CallVirtualFromConstructorDerived (int value) - : base (value) + : this (value, useNewObject: false) + { + } + + public CallVirtualFromConstructorDerived (int value, bool useNewObject) + : base (value, useNewObject) { InvokedConstructor = true; - if (value != calledValue) + + if (useNewObject && calledValue != 0) { + // calledValue was set on a *different* instance! So it's 0 here. + throw new ArgumentException ( + string.Format ("value '{0}' doesn't match expected value '{1}'.", value, 0), + "value"); + } + if (!useNewObject && value != calledValue) throw new ArgumentException ( string.Format ("value '{0}' doesn't match expected value '{1}'.", value, calledValue), "value"); diff --git a/tests/Java.Interop-Tests/Java.Interop/InvokeVirtualFromConstructorTests.cs b/tests/Java.Interop-Tests/Java.Interop/InvokeVirtualFromConstructorTests.cs index 037d8f3c3..95d8e81d5 100644 --- a/tests/Java.Interop-Tests/Java.Interop/InvokeVirtualFromConstructorTests.cs +++ b/tests/Java.Interop-Tests/Java.Interop/InvokeVirtualFromConstructorTests.cs @@ -15,7 +15,7 @@ namespace Java.InteropTests { public class InvokeVirtualFromConstructorTests : JavaVMFixture { [Test] - public void CreateManagedInstanceFirst () + public void CreateManagedInstanceFirst_WithAllocObject () { CallVirtualFromConstructorDerived.Intermediate_FromActivationConstructor = null; CallVirtualFromConstructorDerived.Intermediate_FromCalledFromConstructor = null; @@ -39,17 +39,75 @@ public void CreateManagedInstanceFirst () "Expected t and registered to be the same instance; " + $"t={RuntimeHelpers.GetHashCode (t).ToString ("x")}, " + $"registered={RuntimeHelpers.GetHashCode (registered).ToString ("x")}"); - Assert.AreNotSame (t, acIntermediate, - "Expected t and registered to be the same instance; " + - $"t={RuntimeHelpers.GetHashCode (t).ToString ("x")}, " + - $"registered={RuntimeHelpers.GetHashCode (registered).ToString ("x")}"); + Assert.IsNull (acIntermediate, + "Activation Constructor should not have been called, because of AllocObject semantics"); Assert.AreSame (t, cfIntermediate, "Expected t and cfIntermediate to be the same instance; " + $"t={RuntimeHelpers.GetHashCode (t).ToString ("x")}, " + $"cfIntermediate={RuntimeHelpers.GetHashCode (cfIntermediate).ToString ("x")}"); + Dispose (ref CallVirtualFromConstructorDerived.Intermediate_FromActivationConstructor); + Dispose (ref CallVirtualFromConstructorDerived.Intermediate_FromCalledFromConstructor); + } + + static void Dispose (ref T peer) + where T : class, IJavaPeerable + { + if (peer == null) + return; + + peer.Dispose (); + peer = null; + } + + [Test] + public void CreateManagedInstanceFirst_WithNewObject () + { CallVirtualFromConstructorDerived.Intermediate_FromActivationConstructor = null; CallVirtualFromConstructorDerived.Intermediate_FromCalledFromConstructor = null; + + using var t = new CallVirtualFromConstructorDerived (42, useNewObject: true); + Assert.IsFalse ( + t.Called, + "CalledFromConstructor method override was called on a different instance."); + Assert.IsFalse ( + t.InvokedActivationConstructor, + "Activation Constructor should not have been called, as calledFromConstructor() is invoked before ManagedPeer.construct()."); + Assert.IsTrue ( + t.InvokedConstructor, + "(int) constructor should have been called, via ManagedPeer.construct()."); + + var registered = JniRuntime.CurrentRuntime.ValueManager.PeekValue (t.PeerReference); + var acIntermediate = CallVirtualFromConstructorDerived.Intermediate_FromActivationConstructor; + var cfIntermediate = CallVirtualFromConstructorDerived.Intermediate_FromCalledFromConstructor; + + Assert.AreSame (t, registered, + "Expected t and registered to be the same instance; " + + $"t={RuntimeHelpers.GetHashCode (t).ToString ("x")}, " + + $"registered={RuntimeHelpers.GetHashCode (registered).ToString ("x")}"); + Assert.IsNotNull (acIntermediate, + "Activation Constructor should have been called, because of NewObject"); + Assert.IsTrue ( + acIntermediate.Called, + "CalledFromConstructor method override should have been called on acIntermediate."); + Assert.IsTrue ( + acIntermediate.InvokedActivationConstructor, + "Activation Constructor should have been called on intermediate instance, as calledFromConstructor() is invoked before ManagedPeer.construct()."); + Assert.AreNotSame (t, acIntermediate, + "Expected t and registered to be different instances; " + + $"t={RuntimeHelpers.GetHashCode (t).ToString ("x")}, " + + $"acIntermediate={RuntimeHelpers.GetHashCode (acIntermediate).ToString ("x")}"); + Assert.AreNotSame (t, cfIntermediate, + "Expected t and cfIntermediate to be different instances; " + + $"t={RuntimeHelpers.GetHashCode (t).ToString ("x")}, " + + $"cfIntermediate={RuntimeHelpers.GetHashCode (cfIntermediate).ToString ("x")}"); + Assert.AreSame (acIntermediate, cfIntermediate, + "Expected acIntermediate and cfIntermediate to be the same instance; " + + $"acIntermediate={RuntimeHelpers.GetHashCode (acIntermediate).ToString ("x")}, " + + $"cfIntermediate={RuntimeHelpers.GetHashCode (cfIntermediate).ToString ("x")}"); + + Dispose (ref CallVirtualFromConstructorDerived.Intermediate_FromActivationConstructor); + Dispose (ref CallVirtualFromConstructorDerived.Intermediate_FromCalledFromConstructor); } [Test] @@ -81,14 +139,14 @@ public void CreateJavaInstanceFirst () Assert.AreSame (t, acIntermediate, "Expected t and registered to be the same instance; " + $"t={RuntimeHelpers.GetHashCode (t).ToString ("x")}, " + - $"registered={RuntimeHelpers.GetHashCode (registered).ToString ("x")}"); + $"acIntermediate={RuntimeHelpers.GetHashCode (acIntermediate).ToString ("x")}"); Assert.AreSame (t, cfIntermediate, "Expected t and cfIntermediate to be the same instance; " + $"t={RuntimeHelpers.GetHashCode (t).ToString ("x")}, " + $"cfIntermediate={RuntimeHelpers.GetHashCode (cfIntermediate).ToString ("x")}"); - CallVirtualFromConstructorDerived.Intermediate_FromActivationConstructor = null; - CallVirtualFromConstructorDerived.Intermediate_FromCalledFromConstructor = null; + Dispose (ref CallVirtualFromConstructorDerived.Intermediate_FromActivationConstructor); + Dispose (ref CallVirtualFromConstructorDerived.Intermediate_FromCalledFromConstructor); } } }