[Java.Interop] JNIEnv::NewObject and Replaceable instances

[jonpryor]

[Java.Interop] JNIEnv::NewObject and Replaceable instances (#1323)

Context: 3043d89
Context: dec35f5
Context: dotnet/android#9862
Context: dotnet/android#9862 (comment)
Context: dotnet/android#10004
Context: https://github.com/xamarin/monodroid/commit/326509e56d4e582c53bbe5dfe6d5c741a27f1af5
Context: https://github.com/xamarin/monodroid/commit/940136ebf1318a7c57a855e2728ce2703c0240af

Ever get the feeling that everything is inextricably related?

JNI has two pattens for create an instance of a Java type:

1. JNIEnv::NewObject(jclass clazz, jmethodID methodID, const jvalue* args)

2. JNIEnv::AllocObject(jclass clazz) +
   JNIEnv::CallNonvirtualVoidMethod(jobject obj, jclass clazz, jmethodID methodID, const jvalue* args)

In both patterns:

• clazz is the java.lang.Class of the type to create.
• methodID is the constructor to execute
• args are the constructor arguments.

In .NET terms:

• JNIEnv::NewObject() is equivalent to using
  System.Reflection.ConstructorInfo.Invoke(object?[]?), while

• JNIEnv::AllocObject() + JNIEnv::CallNonvirtualVoidMethod() is
  equivalent to using
  System.Runtime.CompilerServices.RuntimeHelpers.GetUninitializedObject(Type) +
  System.Reflection.MethodBase.Invoke(object?, object?[]?).

Why prefer one over the other?

When hand-writing your JNI code, JNIEnv::NewObject() is easier.
This is less of a concern when a code generator is used.

The real reason to avoid JNIEnv::NewObject() whenever possible
is the Java Activation scenario, summarized as the "are you sure
you want to do this?" ¹ scenario of invoking a virtual method from the
constructor:

class Base {
  public Base() {
    VirtualMethod();
  }
  public virtual void VirtualMethod() {}
}

class Derived : Base {
  public override void VirtualMethod() {}
}

Java and C# are identical here: when a constructor invokes a virtual
method, the most derived method implementation is used, which will
occur before the constructor of the derived type has started
execution. (With lots of quibbling about field initializers…)

Thus, assume you have a Java CallVirtualFromConstructorBase type,
which has its constructor Do The Wrong Thing™ and invoke a virtual
method from the constructor, and that method is overridden in C#?

// Java
public class CallVirtualFromConstructorBase {
  public CallVirtualFromConstructorBase(int value) {
    calledFromConstructor(value);
  }
  public void calledFromConstructor(int value) {
  }
}

// C#
public class CallVirtualFromConstructorDerived : CallVirtualFromConstructorBase {
  public CallVirtualFromConstructorDerived(int value)
    : base (value)
  {
  }

  public override void CalledFromConstructor(int value)
  {
  }
}

What happens with:

var p = new CallVirtualFromConstructorDerived(42);

The answer depends on whether or not JNIEnv::NewObject() is used.

If JNIEnv::NewObject() is not used (the default!)

1. CallVirtualFromConstructorDerived(int) constructor begins
   execution, immediately calls base(value).

2. CallVirtualFromConstructorBase(int) constructor runs, uses
   JNIEnv::AllocObject() to create (but not construct!) Java
   CallVirtualFromConstructorDerived instance.

3. JavaObject.Construct(ref JniObjectReference, JniObjectReferenceOptions)
   invoked, creating a mapping between the C# instance created in
   (1) and the Java instance created in (2).

4. CallVirtualFromConstructorBase(int) C# constructor calls
   JniPeerMembers.InstanceMethods.FinishGenericCreateInstance(),
   which eventually invokes JNIEnv::CallNonvirtualVoidMethod()
   with the Java CallVirtualFromConstructorDerived(int) ctor.

5. Java CallVirtualFromConstructorDerived(int) constructor invokes
   Java CallVirtualFromConstructorBase(int) constructor, which
   invokes CallVirtualFromConstructorDerived.calledFromConstructor().

6. Marshal method (356485e) for
   CallVirtualFromConstructorBase.CalledFromConstructor() invoked,
   immediately calls JniRuntime.JniValueManager.GetPeer()
   (e288589) to obtain an instance upon which to invoke
   .CalledFromConstructor(), finds the instance mapping from (3),
   invokes
   CallVirtualFromConstructorDerived.CalledFromConstructor()
   override.

7. Marshal Method for CalledFromConstructor() returns, Java
   CallVirtualFromConstructorBase(int) constructor finishes,
   Java CallVirtualFromConstructorDerived(int) constructor
   finishes, JNIEnv::CallNonvirtualVoidMethod() finishes.

8. CallVirtualFromConstructorDerived instance finishes construction.

If JNIEnv::NewObject() is used:

1. CallVirtualFromConstructorDerived(int) constructor begins
   execution, immediately calls base(value).

   Note that this is the first created CallVirtualFromConstructorDerived
   instance, but it hasn't been registered yet.

2. CallVirtualFromConstructorBase(int) constructor runs, uses
   JNIEnv::NewObject() to construct Java
   CallVirtualFromConstructorDerived instance.

3. JNIEnv::NewObject() invokes Java
   CallVirtualFromConstructorDerived(int) constructor, which invokes
   CallVirtualFromConstructorBase(int) constructor, which invokes
   CallVirtualFromConstructorDerived.calledFromConstructor().

4. Marshal method (356485e) for
   CallVirtualFromConstructorBase.CalledFromConstructor() invoked,
   immediately calls JniRuntime.JniValueManager.GetPeer()
   (e288589) to obtain an instance upon which to invoke
   .CalledFromConstructor().

   Here is where things go "off the rails" compared to the
   JNIEnv::AllocObject() code path:

   There is no such instance -- we're still in the middle of
   constructing it! -- so we look for an "activation constructor".

5. CallVirtualFromConstructorDerived(ref JniObjectReference, JniObjectReferenceOptions)
   activation constructor executed.

   This is the second CallVirtualFromConstructorDerived instance
   created, and registers a mapping from the Java instance that
   we started constructing in (3) to what we'll call the
   "activation intermediary".

   The activation intermediary instance is marked as "Replaceable".

6. CallVirtualFromConstructorDerived.CalledFromConstructor() method
   override invoked on the activation intermediary.

7. Marshal Method for CalledFromConstructor() returns, Java
   CallVirtualFromConstructorBase(int) constructor finishes,
   Java CallVirtualFromConstructorDerived(int) constructor
   finishes, JNIEnv::NewObject() returns instance.

8. C# CallVirtualFromConstructorBase(int) constructor calls
   JavaObject.Construct(ref JniObjectReference, JniObjectReferenceOptions),
   to create a mapping between (3) and (1).

   In .NET for Android, this causes the C# instance created in (1)
   to replace the C# instance created in (5), which allows
   "Replaceable" instance to be replaced.

   In dotnet/java-interop, this replacement didn't happen, which
   meant that ValueManager.PeekPeer(p.PeerReference) would return
   the activation intermediary, not p, which confuses everyone.

9. CallVirtualFromConstructorDerived instance finishes construction.

For awhile, dotnet/java-interop did not fully support this scenario
around JNIEnv::NewObject(). Additionally, support for using
JNIEnv::NewObject() as part of
JniPeerMembers.JniInstanceMethods.StartCreateInstance() was
removed in dec35f5.

Which brings us to dotnet/android#9862: where 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.<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.

Update JniRuntimeJniValueManagerContract to add a new
CreatePeer_ReplaceableDoesNotReplace() test to codify the desired
semantic that Replaceable instances do not replace Replaceable
instances.

Surprisingly, this new test did not fail on java-interop, as
ManagedValueManager.AddPeer() bails early when PeekPeer() finds
a value, while AndroidValueManager.AddPeer() does not bail early.

An obvious fix for CreatePeer_ReplaceableDoesNotReplace() within
dotnet/android would be to adopt the "AddPeer() calls PeekPeer()"
logic from java-interop. The problem is that doing so breaks
ObjectTest.JnienvCreateInstance_RegistersMultipleInstances(),
as seen in dotnet/android#10004!

JnienvCreateInstance_RegistersMultipleInstances() in turn fails
when PeekPeer() is used because follows the JNIEnv::NewObject()
construction codepath!

public CreateInstance_OverrideAbsListView_Adapter (Context context)
  : base (
      JNIEnv.CreateInstance (
        JcwType,
        "(Landroid/content/Context;)V",
        new JValue (context)),
      JniHandleOwnership.TransferLocalRef)
{
  AdapterValue = new ArrayAdapter (context, 0);
}

as JNIEnv.CreateInstance() uses JNIEnv.NewObject().

We thus have a conundrum: how do we fix both
CreatePeer_ReplaceableDoesNotReplace() and
JnienvCreateInstance_RegistersMultipleInstances()?

The answer is to add proper support for the JNIEnv::NewObject()
construction scenario to dotnet/java-interop, which in turn requires
"lowering" the setting of .Replaceable. Previously, we would set
.Replaceable after the activation constructor was invoked:

// dotnet/android TypeManager.CreateInstance(), paraphrasing
var result = CreateProxy (type, handle, transfer);
result.SetJniManagedPeerState (JniManagedPeerStates.Replaceable | JniManagedPeerStates.Activatable);
return result;

This is 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'd set .Replaceable.

To fix this, update JniRuntime.JniValueManager.TryCreatePeerInstance()
to first create an uninitialized instance, set .Replaceable, and
then invoke the activation constructor. This allows
JniRuntime.JniValueManager.AddPeer() to check to see if the new
value is also replaceable, and ignore the replacement if appropriate.

This in turn requires replacing:

partial class /* JniRuntime. */ JniValueManager {
  protected virtual IJavaPeerable? TryCreatePeer ()
      ref JniObjectReference reference,
      JniObjectReferenceOptions options,
      Type type);
}

with:

partial class /* JniRuntime. */ JniValueManager {
  protected virtual bool TryConstructPeer ()
      IJavaPeerable self,
      ref JniObjectReference reference,
      JniObjectReferenceOptions options,
      Type type);
}

This is fine because we haven't shipped TryCreatePeer() in a stable
release yet.

Footnotes

1. See also Framework Design Guidelines > Constructor Design:

   ❌ AVOID calling virtual members on an object inside its constructor.
   Calling a virtual member will cause the most derived override to be
   called, even if the constructor of the most derived type has not
   been fully run yet.

   ↩

[jonpryor]

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.

Turns Out™ that this difference is a cause of dotnet/android#10004, causing JnienvCreateInstance_RegistersMultipleInstances() to fail when using NativeAOT.

Further investigation shows that JnienvCreateInstance_RegistersMultipleInstances() and CreatePeer_ReplaceableDoesNotReplace() are unexpectedly closely related and near opposites. Doh.

[Copilot]

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.