Bump to dotnet/java-interop@d3d3a1bf (#9921)

Fixes: #9862

Changes: dotnet/java-interop@8221b7d...d3d3a1b

  * dotnet/java-interop@d3d3a1bf: [Java.Interop] JNIEnv::NewObject and Replaceable instances (dotnet/java-interop#1323)

Context: 5c23bcd

Issue #9862 is an observed "race condition" around
`Android.App.Application` subclass creation.  *Two* instances of
`AndroidApp` were created, one from the "normal" app startup:

	at MailClient.Mobile.Droid.AndroidApp..ctor(IntPtr handle, JniHandleOwnership ownership)
	…
	at Java.Lang.Object.GetObject(IntPtr , JniHandleOwnership , Type )
	at Java.Lang.Object._GetObject[Application](IntPtr , JniHandleOwnership )
	at Java.Lang.Object.GetObject[Application](IntPtr handle, JniHandleOwnership transfer)
	at Java.Lang.Object.GetObject[Application](IntPtr jnienv, IntPtr handle, JniHandleOwnership transfer)
	at Android.App.Application.n_OnCreate(IntPtr jnienv, IntPtr native__this)
	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`, via parameter:

	at MailClient.Mobile.Droid.AndroidApp..ctor(IntPtr handle, JniHandleOwnership ownership)
	…
	at Java.Lang.Object.GetObject(IntPtr , JniHandleOwnership , Type )
	at Android.Runtime.JNIEnv.<>c.<CreateNativeArrayElementToManaged>b__70_9(Type type, IntPtr source, Int32 index)
	at Android.Runtime.JNIEnv.GetObjectArray(IntPtr , Type[] )
	at Java.Interop.TypeManager.n_Activate(IntPtr jnienv, IntPtr jclass, IntPtr typename_ptr, IntPtr signature_ptr, IntPtr jobject, IntPtr parameters_ptr)
	at mono.android.TypeManager.n_activate(Native Method)
	at mono.android.TypeManager.Activate(TypeManager.java:7)
	at crc647fae2f69c19dcd0d.SyncWorker.<init>(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 `AndroidValueManager`, wherein
when "Replaceable" instances were created, an existing instance would
*always* be replaced, even if the new instance was also Replaceable!
This feels bananas; yes, Replaceable should be replaceable, but the
expectation was that it would be replaced by *non*-Replaceable
instances, not just any instance that came along later.

Aside: a "Replaceable" instance is an instance created via
`JniRuntime.JniValueManager.CreatePeer()` /
`TypeManager.CreateInstance()`.  "Replaceable" instances can be
replaced in the `JniRuntime.JniValueManager` by instances
created via the constructor, e.g. `new Integer(42)`.

	JniPeerReference h;
	using (var x = new Java.Lang.Integer(1))
	  h = x.PeerReference.NewLocalRef();
	// No mapping for `h` in JniValueManager, as x was disposed.

	var a = JniEnvironment.Runtime.ValueManager.CreatePeer (ref h, JniObjectReferenceOptions.Copy, null);
	  // a is "Replaceable"
	var b = new Java.Lang.Integer(x.Handle, JniHandleOwnership.DoNotTransfer);
	  // b is *not* Replaceable; created via constructor
	
	var p = JniEnvironment.Runtime.ValueManager.PeekPeer(h);
	// p should be the non-Replaceable value b, *not* a


dotnet/java-interop@d3d3a1bf updates
`JniRuntimeJniValueManagerContract` to add a new
`CreatePeer_ReplaceableDoesNotReplace()` test to codify the desired
semantic that Replaceable instances do not replace Replaceable
instances.

Update `AndroidValueManager` so that the new
`CreatePeer_ReplaceableDoesNotReplace()` test passes.  Supporting
this new semantic required "extending" Replaceable lifetime; it turns
out that the existing code within `TypeManager.CreateInstance()`:

	var result = CreateProxy (type, handle, transfer);
	if (Runtime.IsGCUserPeer (result.PeerReference.Handle)) {
	  result.SetJniManagedPeerState (JniManagedPeerStates.Replaceable | JniManagedPeerStates.Activatable);
	}
	return result;

sets `.Replaceable` *too late*, as during execution of the activation
constructor the instance thinks it *isn't* replaceable, and thus
creation of a new instance via the activation constructor will
replace an already existing replaceable instance; it's not until
*after* the constructor finished executing that we set `.Replaceable`.

Address this by updating `CreatePeer()` to split up instance creation:

 1. Create an *un-constructed* instance using
    [`RuntimeHelpers.GetUninitializedObject(Type)`][0].

 2. Set `.Replaceable` on the instance from (1)

 3. *Then* invoke the activation constructor.

This allows `JniRuntime.JniValueManager.AddPeer()` to reliably
determine that an instance is `.Replaceable`, and thus *not* replace
a `.Replaceable` instance with another `.Replaceable` instance.

dotnet/java-interop@d3d3a1bf does similar things with
`JniRuntime.JniValueManager`.  Update `ManagedValueManager` to
override the new `TryConstructPeer()` method, as `TryCreatePeer()`
has been removed.

Finally, an "interesting" crash happened when adding the
`RuntimeHelpers.GetUninitializedObject()` + invoke constructor fix:

	E droid.NET_Test: JNI ERROR (app bug): accessed deleted Global 0x3056
	F droid.NET_Test: java_vm_ext.cc:570] JNI DETECTED ERROR IN APPLICATION: use of deleted global reference 0x3056

or, when it didn't up and crash, we could see "bizarre" unit test
failures from `JnienvTest.MoarThreadingTests()`:

	I NUnit   : 1) Java.InteropTests.JnienvTest.MoarThreadingTests (Mono.Android.NET-Tests)
	I NUnit   :   No exception should be thrown [t2]! Got: System.ObjectDisposedException: Cannot access disposed object with JniIdentityHashCode=158880748.
	I NUnit   : Object name: 'Java.Lang.Integer'.
	I NUnit   :    at Java.Interop.JniPeerMembers.AssertSelf(IJavaPeerable self)
	I NUnit   :    at Java.Interop.JniPeerMembers.JniInstanceMethods.InvokeAbstractInt32Method(String encodedMember, IJavaPeerable self, JniArgumentValue* parameters)
	I NUnit   :    at Java.Lang.Integer.IntValue()
	I NUnit   :    at Java.Lang.Integer.System.IConvertible.ToInt32(IFormatProvider provider)
	I NUnit   :    at System.Convert.ChangeType(Object value, Type conversionType, IFormatProvider provider)
	I NUnit   :    at Android.Runtime.JNIEnv.GetObjectArray(IntPtr array_ptr, Type[] element_types)
	I NUnit   :    at Java.InteropTests.JnienvTest.<>c__DisplayClass26_0.<MoarThreadingTests>b__1()
	I NUnit   :   Expected: null
	I NUnit   :   But was:  <System.ObjectDisposedException: Cannot access disposed object with JniIdentityHashCode=158880748.

or:

	I NUnit   : 1) Java.InteropTests.JnienvTest.MoarThreadingTests (Mono.Android.NET-Tests)
	I NUnit   :   No exception should be thrown [t2]! Got: System.ArgumentException: Handle must be valid. (Parameter 'instance')
	I NUnit   :    at Java.Interop.JniEnvironment.InstanceMethods.CallIntMethod(JniObjectReference instance, JniMethodInfo method, JniArgumentValue* args)
	I NUnit   :    at Java.Interop.JniPeerMembers.JniInstanceMethods.InvokeAbstractInt32Method(String encodedMember, IJavaPeerable self, JniArgumentValue* parameters)
	I NUnit   :    at Java.Lang.Integer.IntValue()
	I NUnit   :    at Java.Lang.Integer.System.IConvertible.ToInt32(IFormatProvider provider)
	I NUnit   :    at System.Convert.ChangeType(Object value, Type conversionType, IFormatProvider provider)
	I NUnit   :    at Android.Runtime.JNIEnv.GetObjectArray(IntPtr array_ptr, Type[] element_types)
	I NUnit   :    at Java.InteropTests.JnienvTest.<>c__DisplayClass26_0.<MoarThreadingTests>b__1()

The thread using the invalid global reference is `t1` in
`MoarThreadingTests()`, which uses
`JNIEnv.CopyObjectArray<int>(IntPtr, int[])`, which in turn uses
`JavaConvert.FromJniHandle<T>()`.

Why would that be failing?

The answer: consider `JavaConvert.JniHandleConverters`:

	partial class JavaConvert {
	  static Dictionary<Type, Func<IntPtr, JniHandleOwnership, object>> JniHandleConverters = new() {
	    { typeof (int), (handle, transfer) => {
	      using (var value = new Java.Lang.Integer (handle, transfer | JniHandleOwnership.DoNotRegister))
	        return value.IntValue ();
	    } },
	  }
	}

Note that we're invoking the `Integer(IntPtr, JniHandleOwnership)`
constructor.  As per the above Aside about Replaceable instances,
this means that `value` should *replace* any existing mapping
within the `JniRuntime.JniValueManager`.  *However*, here was *also*
provide `JniHandleOwnership.DoNotRegister`, which *prevents* the
instance from being placed into the instance mapping, i.e.
`Object.PeekObject()` *should not find the instance*.

Next, turn to `Object.SetHandle()` from 5c23bcd:

	partial class /* Java.Lang. */ Object {
	  protected void SetHandle (IntPtr value, JniHandleOwnership transfer)
	  {
	    var reference = new JniObjectReference (value);
	    JNIEnvInit.ValueManager?.ConstructPeer (
	        this,
	        ref reference,
	        value == IntPtr.Zero ? JniObjectReferenceOptions.None : JniObjectReferenceOptions.Copy);
	    JNIEnv.DeleteRef (value, transfer);
	  }
	}

What's missing?  A check for `JniHandleOwnership.DoNotRegister`!

Which means that the `new Integer(…)` will be in the instance map,
*and replace any existing mappings*, only to be subsequently disposed.

This permits the following "total execution order" to happen between
threads `t1` and `t2` in `MoarThreadingTests()`:

  * Main thread:

        // JNI equivalent to Java `new Object[]{ new Integer (1) }`
        IntPtr lrefJliArray = JNIEnv.NewObjectArray<int>(new[]{1})

  * `t1` Thread:

        IntPtr lrefInteger  = JNIEnv.GetObjectArrayElement(lrefJliArray, 0);
        var value = new Integer(lrefInteger, .TransferLocalRef);
          // via JavaConvert.JniHandleConverters

  * `t2` Thread:

        IntPtr lrefInteger  = JNIEnv.GetObjectArrayElement(lrefJliArray, 0);
        var x = Java.Lang.Object.GetObject (lrefInteger, .TransferLocalRef);
          // via JNIEnv.NativeArrayElementToManaged[typeof(IJavaObject)]

    Note that `x` is the same instance as `value`.

  * `t1` Thread:

        int v = value.IntValue();
        value.Dispose();

  * `t2` Thread: does *anything* with `x`.  As `x` and `value` are
    the same instance, `x` has been disposed by thread `t1`.
    
    Depending on where thread scheduling, this would explain both the
    `ObjectDisposedException` as well as the `JNI ERROR (app bug)`.

The fix is to correct the oversight from 5c23bcd, and forward the
`JniHandleOwnership.DoNotRegister` flag to `JniObjectReferenceOptions`.

*On the way to investigating this…*

Update `tests/Mono.Android-Tests` to use `@(AndroidEnvironment)` in
Debug builds to set `debug.mono.debug=1`.  This allows for filenames
and line numbers in stack traces.

Update `src/native` so that the GREF log can be created; previously,
`adb logcat` would contain:

	W monodroid: Failed to create directory '/data/user/0/Mono.Android.NET_Tests/files/.__override__/arm64-v8a'. No such file or directory
	…
	E monodroid: fopen failed for file /data/user/0/Mono.Android.NET_Tests/files/.__override__/arm64-v8a/grefs.txt: No such file or directory

The apparent cause for this is that the ABI is now part of the path,
`…/.__override__/arm64-v8a/grefs.txt` and not `…/.__override__/grefs.txt`.
Additionally, `create_public_directory()` was a straight `mkdir()` call,
which *does not* create intermediate directories.  Update
`create_public_directory()` to use `create_directory()` instead,
which *does* create intermediate directories.  This allows
`grefs.txt` to be created.

Next, the contents of `grefs.txt` occasionally looked "wrong".
Turns out, `WriteGlobalReferenceLine()` and `WriteLocalReferenceLine()`
didn't consistently append a newline to the message, which impacts e.g.
the `Created …` message:

	Created PeerReference=0x3c06/G IdentityHashCode=0x2e29bd Instance=0xbe0aab36 Instance.Type=Android.OS.Bundle, Java.Type=android/os/Bundle+g+ grefc 19 gwrefc 0 obj-handle 0x706f711035/L -> new-handle 0x3d06/G from thread '<null>'(1)

Update `WriteGlobalReferenceLine()` and `WriteLocalReferenceLine()`
to always append a newline to the message.

[0]: https://learn.microsoft.com/dotnet/api/system.runtime.compilerservices.runtimehelpers.getuninitializedobject?view=net-9.0

Author: jonpryor

external/Java.Interop

@@ -173,11 +173,13 @@ public override IntPtr ReleaseLocalReference (ref JniObjectReference value, ref
 		public override void WriteLocalReferenceLine (string format, params object?[] args)
 		{
 			RuntimeNativeMethods._monodroid_gref_log ("[LREF] " + string.Format (CultureInfo.InvariantCulture, format, args));
+			RuntimeNativeMethods._monodroid_gref_log ("\n");
 		}
 
 		public override void WriteGlobalReferenceLine (string format, params object?[] args)
 		{
 			RuntimeNativeMethods._monodroid_gref_log (string.Format (CultureInfo.InvariantCulture, format, args));
+			RuntimeNativeMethods._monodroid_gref_log ("\n");
 		}
 
 		public override JniObjectReference CreateGlobalReference (JniObjectReference value)
@@ -689,7 +691,7 @@ internal void AddPeer (IJavaPeerable value, JniObjectReference reference, IntPtr
 				for (int i = 0; i < targets.Count; ++i) {
 					IJavaPeerable? target;
 					var wref = targets [i];
-					if (ShouldReplaceMapping (wref!, reference, out target)) {
+					if (ShouldReplaceMapping (wref!, reference, value, out target)) {
 						found = true;
 						targets [i] = IdentityHashTargets.CreateWeakReference (value);
 						break;
@@ -747,7 +749,7 @@ internal void AddPeer (IJavaPeerable value, IntPtr handle, JniHandleOwnership tr
 			}
 		}
 
-		bool ShouldReplaceMapping (WeakReference<IJavaPeerable> current, JniObjectReference reference, out IJavaPeerable? target)
+		bool ShouldReplaceMapping (WeakReference<IJavaPeerable> current, JniObjectReference reference, IJavaPeerable value, out IJavaPeerable? target)
 		{
 			target      = null;
 
@@ -771,12 +773,17 @@ bool ShouldReplaceMapping (WeakReference<IJavaPeerable> current, JniObjectRefere
 			// we want the 2nd MCW to replace the 1st, as the 2nd is
 			// the one the dev created; the 1st is an implicit intermediary.
 			//
+			// Meanwhile, a new "replaceable" instance should *not* replace an
+			// existing "replaceable" instance; see dotnet/android#9862.
+			//
 			// [0]: If Java ctor invokes overridden virtual method, we'll
 			// transition into managed code w/o a registered instance, and
 			// thus will create an "intermediary" via
 			// (IntPtr, JniHandleOwnership) .ctor.
-			if ((target.JniManagedPeerState & JniManagedPeerStates.Replaceable) == JniManagedPeerStates.Replaceable)
+			if (target.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable) &&
+					!value.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable)) {
 				return true;
+			}
 
 			return false;
 		}

src/Mono.Android/Android.Runtime/AndroidRuntime.cs

@@ -382,22 +382,34 @@ internal static object CreateProxy (
 		{
 			// Skip Activator.CreateInstance() as that requires public constructors,
 			// and we want to hide some constructors for sanity reasons.
+			var peer = GetUninitializedObject (type);
 			BindingFlags flags = BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance;
 			var c = type.GetConstructor (flags, null, XAConstructorSignature, null);
 			if (c != null) {
-				return c.Invoke (new object [] { handle, transfer });
+				c.Invoke (peer, new object[] { handle, transfer });
+				return peer;
 			}
 			c = type.GetConstructor (flags, null, JIConstructorSignature, null);
 			if (c != null) {
 				JniObjectReference          r = new JniObjectReference (handle);
 				JniObjectReferenceOptions   o = JniObjectReferenceOptions.Copy;
-				var peer = (IJavaPeerable) c.Invoke (new object [] { r, o });
+				c.Invoke (peer, new object [] { r, o });
 				JNIEnv.DeleteRef (handle, transfer);
 				return peer;
 			}
+			GC.SuppressFinalize (peer);
 			throw new MissingMethodException (
 					"No constructor found for " + type.FullName + "::.ctor(System.IntPtr, Android.Runtime.JniHandleOwnership)",
 					CreateJavaLocationException ());
+
+			static IJavaPeerable GetUninitializedObject (
+					[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors)]
+					Type type)
+			{
+				var v   = (IJavaPeerable) System.Runtime.CompilerServices.RuntimeHelpers.GetUninitializedObject (type);
+				v.SetJniManagedPeerState (JniManagedPeerStates.Replaceable | JniManagedPeerStates.Activatable);
+				return v;
+			}
 		}
 
 		public static void RegisterType (string java_class, Type t)

src/Mono.Android/Java.Interop/TypeManager.cs

@@ -110,13 +110,22 @@ protected override void Dispose (bool disposing)
 		protected void SetHandle (IntPtr value, JniHandleOwnership transfer)
 		{
 			var reference = new JniObjectReference (value);
+			var options   = FromJniHandleOwnership (transfer);
 			JNIEnvInit.ValueManager?.ConstructPeer (
 					this,
 					ref reference,
-					value == IntPtr.Zero ? JniObjectReferenceOptions.None : JniObjectReferenceOptions.Copy);
+					value == IntPtr.Zero ? JniObjectReferenceOptions.None : options);
 			JNIEnv.DeleteRef (value, transfer);
 		}
 
+		static JniObjectReferenceOptions FromJniHandleOwnership (JniHandleOwnership transfer)
+		{
+			var options = JniObjectReferenceOptions.Copy;
+			if (transfer.HasFlag (JniHandleOwnership.DoNotRegister))
+				options |= JniObjectReferenceOptions.CopyAndDoNotRegister;
+			return options;
+		}
+
 		internal static IJavaPeerable? PeekObject (IntPtr handle, Type? requiredType = null)
 		{
 			var peeked  = JNIEnvInit.ValueManager?.PeekPeer (new JniObjectReference (handle));

src/Mono.Android/Java.Lang/Object.cs

@@ -265,7 +265,8 @@ public override List<JniSurfacedPeerInfo> GetSurfacedPeers ()
 
 	static  readonly    Type[]  XAConstructorSignature  = new Type [] { typeof (IntPtr), typeof (JniHandleOwnership) };
 
-	protected override IJavaPeerable? TryCreatePeer (
+	protected override bool TryConstructPeer (
+			IJavaPeerable self,
 			ref JniObjectReference reference,
 			JniObjectReferenceOptions options,
 			[DynamicallyAccessedMembers (Constructors)]
@@ -277,10 +278,10 @@ public override List<JniSurfacedPeerInfo> GetSurfacedPeers ()
 				reference.Handle,
 				JniHandleOwnership.DoNotTransfer,
 			};
-			var p       = (IJavaPeerable) c.Invoke (args);
+			c.Invoke (self, args);
 			JniObjectReference.Dispose (ref reference, options);
-			return p;
+			return true;
 		}
-		return base.TryCreatePeer (ref reference, options, type);
+		return base.TryConstructPeer (self, ref reference, options, type);
 	}
 }

src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs

@@ -147,12 +147,10 @@ Util::path_combine (const char *path1, const char *path2)
 void
 Util::create_public_directory (const char *dir)
 {
-	mode_t m = umask (0);
-	int ret = mkdir (dir, 0777);
+	int ret = create_directory (dir, 0777);
 	if (ret < 0) {
 		log_warn (LOG_DEFAULT, "Failed to create directory '{}'. {}", dir, std::strerror (errno));
 	}
-	umask (m);
 }
 
 int

src/native/mono/runtime-base/util.cc

@@ -52,6 +52,13 @@
     <_DefaultValueAttributeSupport Condition="'$(TrimMode)' == 'full'">true</_DefaultValueAttributeSupport>
   </PropertyGroup>
 
+  <ItemGroup Condition=" '$(Configuration)' == 'Debug' ">
+    <!-- trying to track:
+      JNI ERROR (app bug): accessed deleted Global 0x3056
+    -->
+    <AndroidEnvironment Include="env.txt" />
+  </ItemGroup>
+
   <ItemGroup>
     <ProjectReference Include="..\..\TestRunner.Core\TestRunner.Core.NET.csproj" />
     <ProjectReference Include="..\..\TestRunner.NUnit\TestRunner.NUnit.NET.csproj" />

tests/Mono.Android-Tests/Mono.Android-Tests/Mono.Android.NET-Tests.csproj

@@ -0,0 +1,3 @@
+# Environment Variables and system properties
+# debug.mono.log=gref,default
+debug.mono.debug=1