Bump to dotnet/java-interop/main@d3d3a1bf

[jonpryor]

Fixes: #9862

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

• dotnet/java-interop@d3d3a1bf: [Java.Interop] JNIEnv::NewObject and Replaceable instances ([Java.Interop] JNIEnv::NewObject and Replaceable instances 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).

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.

[jonpryor]

So I expected JniRuntimeJniValueManagerContract.CreatePeer_ReplaceableDoesNotReplace() to fail, but it failed for the wrong reason:

Expected peer1.JniManagedPeerState to have .Replaceable, but was None.

Further investigation was because we weren't hitting this line:

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

Line 363 in 542cf52

result.SetJniManagedPeerState (JniManagedPeerStates.Replaceable | JniManagedPeerStates.Activatable);

because Runtime.IsGCUserPeer() returned false, because AnotherJavaInterfaceImpl.java does not implement GCUserPeerable.

Okay, fair.

What I no longer remember -- and thus need to dig around for -- is why we're "protect" setting .Replacable with IsGCUserPeer(). Offhand, this feels like something we could always do?

[jonpryor]

The story so far…?

dotnet/java-interop@d3d3a1bf added several unit tests, one of which is CreatePeer_ReplaceableDoesNotReplace(), which was the unit test for #9862.

CI for this PR is observing crashes in tests/Mono.Android-Tests, which don't entirely make sense.

For testing, PR #10047 is a "similar" bump that also bumps to dotnet/java-interop@d3d3a1bf and fixes ManagedValueManager so that it builds, but does not contain fixes for CreatePeer_ReplaceableDoesNotReplace(). It shows two meaningful results:

1. CreatePeer_ReplaceableDoesNotReplace() fails, which is expected, and
2. It does not crash.

This suggests that the crash isn't caused by dotnet/java-interop@d3d3a1bf (yay), and may be "random" based on history.

Next step: copy over the remaining changes in this PR into #10047. Maybe it'll pass the tests?

[jonathanpeppers]

With this change, can we remove the categories here:

android/tests/Mono.Android-Tests/Mono.Android-Tests/Java.Lang/ObjectTest.cs

Lines 68 to 71 in b946ae1

	[Test]
	[Category ("CoreCLRIgnore")] //TODO: https://github.com/dotnet/android/issues/10069
	[Category ("NativeAOTIgnore")] //TODO: https://github.com/dotnet/android/issues/10079
	public void JnienvCreateInstance_RegistersMultipleInstances ()

And maybe the test passes on CoreCLR/NativeAOT?