[Java.Interop] Dispose, *then* Remove (#708)

Fixes: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1193891
A bug was found in Xamarin.Android 11.1.0.2, candidate for
Visual Studio 16.8 Preview 3:
1. Create a new "Shell Forms App"
2. Build & Run the app
3. Tap the "Browse" button.
Expected results: it works!
Actual results: A rather abrupt crash:
Unhandled Exception:
System.NotSupportedException: Unable to activate instance of type Xamarin.Forms.Platform.Android.PageRenderer from native handle 0x69 (key_handle 0x33efaa1). ---> System.MissingMethodException: No constructor found for Xamarin.Forms.Platform.Android.PageRenderer::.ctor(System.IntPtr, Android.Runtime.JniHandleOwnership) ---> Java.Interop.JavaLocationException: Exception of type 'Java.Interop.JavaLocationException' was thrown.
--- End of inner exception stack trace ---
at Java.Interop.TypeManager.CreateProxy (System.Type type, System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer)
at Java.Interop.TypeManager.CreateInstance (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer, System.Type targetType)
--- End of inner exception stack trace ---
at (wrapper dynamic-method) Android.Runtime.DynamicMethodNameCounter.64(intptr,intptr)
at (wrapper native-to-managed) Android.Runtime.DynamicMethodNameCounter.64(intptr,intptr)
The crash was caused by commit 007b35b, which changed
`JniRuntime.JniValueManager.DisposePeer()` semantics from:
value.Disposed ();
RemovePeer (value);
and reversed this to:
RemovePeer (value);
value.Disposed ();
(Why this reversal? Unmentioned in 007b35b -- doh! -- but as
@jonpryor is the one who did this, "it seemed like a good idea
at the time!")
(Very clearly, this wasn't a good idea.)
The crash was caused by commit 007b35b interacting with Xamarin.Forms'
[`VisualElementPackager.Dispose(bool)`][0], which can call back into
Java code, potentially causing `this` to need to be returned.
When `RemovePeer()` is done *first*, as was done in 007b35b, then
there may be no available instance to return, causing the
`NotSupportedException` to be thrown.
The new `JavaObjectTest.DisposeAccessesThis()` test and associated
`Java.InteropTests.GetThis` type & Java peer trigger this scenario.
Without the fix, `DisposeAccessesThis()` fails with:
System.NotSupportedException : Could not find an appropriate constructable wrapper type for Java type 'com/xamarin/interop/GetThis', targetType='Java.InteropTests.GetThis'.
at Java.Interop.JniRuntime+JniValueManager.CreatePeer (Java.Interop.JniObjectReference& reference, Java.Interop.JniObjectReferenceOptions transfer, System.Type targetType)
at Java.Interop.JavaPeerableValueMarshaler.CreateGenericValue (Java.Interop.JniObjectReference& reference, Java.Interop.JniObjectReferenceOptions options, System.Type targetType)
at Java.Interop.JniRuntime+JniValueManager.GetValue[T] (Java.Interop.JniObjectReference& reference, Java.Interop.JniObjectReferenceOptions options, System.Type targetType)
at Java.InteropTests.GetThis.get_This ()
at Java.InteropTests.GetThis.Dispose (System.Boolean disposing)
at Java.Interop.JavaObject.Java.Interop.IJavaPeerable.Disposed ()
at Java.Interop.JniRuntime+JniValueManager.DisposePeer (Java.Interop.IJavaPeerable value)
at Java.Interop.JavaObject.Dispose ()
demonstrating how `RemovePeer(value)` *must not* happen before
`value.Disposed()`.
[0]: https://github.com/xamarin/Xamarin.Forms/blob/17881ec93d6b3fb0ee5e1a2be46d7eeadef23529/Xamarin.Forms.Platform.Android/VisualElementPackager.cs#L65-L108

Author: jonpryor

src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs

@@ -146,9 +146,8 @@ public virtual void DisposePeer (IJavaPeerable value)
 if(!value.PeerReference.IsValid)
 return;
 
-RemovePeer(value);
-
 value.Disposed();
+RemovePeer(value);
 
 varh=value.PeerReference;
 if(!h.IsValid)

tests/Java.Interop-Tests/Java.Interop-Tests.csproj

@@ -40,6 +40,7 @@
  <JavaInteropTestJarInclude="$(MSBuildThisFileDirectory)java\com\xamarin\interop\CallNonvirtualDerived2.java" />
  <JavaInteropTestJarInclude="$(MSBuildThisFileDirectory)java\com\xamarin\interop\CallVirtualFromConstructorBase.java" />
  <JavaInteropTestJarInclude="$(MSBuildThisFileDirectory)java\com\xamarin\interop\CallVirtualFromConstructorDerived.java" />
+ <JavaInteropTestJarInclude="$(MSBuildThisFileDirectory)java\com\xamarin\interop\GetThis.java" />
  <JavaInteropTestJarInclude="$(MSBuildThisFileDirectory)java\com\xamarin\interop\SelfRegistration.java" />
  <JavaInteropTestJarInclude="$(MSBuildThisFileDirectory)java\com\xamarin\interop\TestType.java" />
  </ItemGroup>

tests/Java.Interop-Tests/Java.Interop/GetThis.cs

@@ -0,0 +1,47 @@
+usingSystem;
+
+usingJava.Interop;
+
+namespaceJava.InteropTests
+{
+[JniTypeSignature(GetThis.JniTypeName)]
+publicclassGetThis:JavaObject
+{
+internalconststringJniTypeName="com/xamarin/interop/GetThis";
+
+bool_isDisposed;
+
+readonlystaticJniPeerMembers_members=newJniPeerMembers(JniTypeName,typeof(GetThis));
+
+publicoverrideJniPeerMembersJniPeerMembers{
+get{return_members;}
+}
+
+publicGetThis()
+{
+}
+
+publicunsafeGetThisThis{
+get{
+varo=_members.InstanceMethods.InvokeNonvirtualObjectMethod("getThis.()Lcom/xamarin/interop/GetThis;",this,null);
+returnJniEnvironment.Runtime.ValueManager.GetValue<GetThis>(refo,JniObjectReferenceOptions.CopyAndDispose);
+}
+}
+
+protectedoverridevoidDispose(booldisposing)
+{
+if(_isDisposed){
+return;
+}
+_isDisposed=true;
+if(disposing){
+vart=This;
+if(t!=this){
+thrownewInvalidOperationException("SHOULD NOT BE REACHED");
+}
+}
+base.Dispose(disposing);
+}
+}
+}
+

tests/Java.Interop-Tests/Java.Interop/JavaObjectTest.cs

@@ -193,6 +193,14 @@ public void NestedDisposeInvocations ()
 value.Dispose();
 value.Dispose();
 }
+
+[Test]
+publicvoidDisposeAccessesThis()
+{
+varvalue=newGetThis();
+value.Dispose();
+value.Dispose();
+}
 }
 
 classJavaObjectWithNoJavaPeer:JavaObject{

tests/Java.Interop-Tests/java/com/xamarin/interop/GetThis.java

@@ -0,0 +1,42 @@
+packagecom.xamarin.interop;
+
+importjava.util.ArrayList;
+
+importcom.xamarin.java_interop.GCUserPeerable;
+
+publicclassGetThisimplementsGCUserPeerable {
+
+staticfinalStringassemblyQualifiedName = "Java.InteropTests.GetThis, Java.Interop-Tests, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null";
+static {
+com.xamarin.java_interop.ManagedPeer.registerNativeMembers (
+GetThis.class,
+assemblyQualifiedName,
+"");
+	}
+
+ArrayList<Object> managedReferences = newArrayList<Object>();
+
+publicGetThis () {
+if (GetThis.class == getClass ()) {
+com.xamarin.java_interop.ManagedPeer.construct (
+this,
+"Java.InteropTests.GetThis, Java.Interop-Tests, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null",
+""
+ );
+ }
+	}
+
+publicfinalGetThisgetThis() {
+returnthis;
+	}
+
+publicvoidjiAddManagedReference (java.lang.Objectobj)
+	{
+managedReferences.add (obj);
+	}
+
+publicvoidjiClearManagedReferences ()
+	{
+managedReferences.clear ();
+	}
+}