[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);
 
 				var h = value.PeerReference;
 				if (!h.IsValid)

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

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

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

@@ -0,0 +1,47 @@
+using System;
+
+using Java.Interop;
+
+namespace Java.InteropTests
+{
+	[JniTypeSignature (GetThis.JniTypeName)]
+	public class GetThis : JavaObject
+	{
+		internal const string JniTypeName = "com/xamarin/interop/GetThis";
+
+		bool _isDisposed;
+
+		readonly static JniPeerMembers _members = new JniPeerMembers (JniTypeName, typeof (GetThis));
+
+		public override JniPeerMembers JniPeerMembers {
+			get {return _members;}
+		}
+
+		public GetThis ()
+		{
+		}
+
+		public unsafe GetThis This {
+			get {
+				var o   = _members.InstanceMethods.InvokeNonvirtualObjectMethod ("getThis.()Lcom/xamarin/interop/GetThis;", this, null);
+				return JniEnvironment.Runtime.ValueManager.GetValue<GetThis> (ref o, JniObjectReferenceOptions.CopyAndDispose);
+			}
+		}
+
+		protected override void Dispose (bool disposing)
+		{
+			if (_isDisposed) {
+				return;
+			}
+			_isDisposed = true;
+			if (disposing) {
+				var t = This;
+				if (t != this) {
+					throw new InvalidOperationException ("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]
+		public void DisposeAccessesThis ()
+		{
+			var value = new GetThis ();
+			value.Dispose ();
+			value.Dispose ();
+		}
 	}
 
 	class JavaObjectWithNoJavaPeer : JavaObject {

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

@@ -0,0 +1,42 @@
+package com.xamarin.interop;
+
+import java.util.ArrayList;
+
+import com.xamarin.java_interop.GCUserPeerable;
+
+public class GetThis implements GCUserPeerable {
+
+	static  final   String  assemblyQualifiedName   = "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     = new ArrayList<Object>();
+
+	public GetThis () {
+		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",
+					""
+			);
+		}
+	}
+    
+	public final GetThis getThis() {
+		return this;
+	}
+
+	public void jiAddManagedReference (java.lang.Object obj)
+	{
+		managedReferences.add (obj);
+	}
+
+	public void jiClearManagedReferences ()
+	{
+		managedReferences.clear ();
+	}
+}