[Java.Interop] Avoid double-disposing PeerReferences (#690)

Context: dotnet/android#4989

What happens if you dispose an instance *while disposing the instance*?

	// C#
	public class MyDisposableObject : JavaObject {
	    bool _isDisposed;

	    protected override void Dispose (bool disposing)
	    {
	        if (_isDisposed) {
	            return;
	        }
	        _isDisposed = true;
	        if (this.PeerReference.IsValid)
	            this.Dispose ();
	        base.Dispose (disposing);
	    }
	}

	// …
	void MyTestMethod ()
	{
	    var value = new MyDisposableObject ();
	    value.Dispose ();
	    value.Dispose ();
	}

Here, `MyDisposableObject.Dispose(bool)` calls `JavaObject.Dispose()`
when `PeerReference` is valid.  This "feels" admittedly unusual, but
`IDisposable.Dispose()` is *supposed to be* Idempotent: it can be
called multiple times with no ill effects.  Shouldn't this be the same?

Unfortunately, it *isn't* the same; it crashes, hard:

	  =================================================================
	  	Native stacktrace:
	  =================================================================
	  	0x10245bc49 - /Library/Frameworks/Mono.framework/Versions/Current/Commands/mono : mono_dump_native_crash_info
	  	0x1023f3d35 - /Library/Frameworks/Mono.framework/Versions/Current/Commands/mono : mono_handle_native_crash
	  	0x10245b20f - /Library/Frameworks/Mono.framework/Versions/Current/Commands/mono : sigabrt_signal_handler
	  	0x7fff6983d5fd - /usr/lib/system/libsystem_platform.dylib : _sigtramp
	  	0x700009b716b0 - Unknown
	  	0x7fff69713808 - /usr/lib/system/libsystem_c.dylib : abort
	  	0x10658377a - …/lib/server/libjvm.dylib : _ZN2os5abortEbPvPKv
	  	0x10637ea5f - …/lib/server/libjvm.dylib : _ZN8jniCheck15validate_handleEP10JavaThreadP8_jobject
	  	0x10637eb08 - …/lib/server/libjvm.dylib : _ZN8jniCheck15validate_objectEP10JavaThreadP8_jobject
	  	0x1063818a1 - …/lib/server/libjvm.dylib : checked_jni_GetObjectClass
	  	0x10594cfa8 - …/Java.Interop/bin/TestDebug/libjava-interop.dylib : java_interop_jnienv_get_object_class
	  	0x107a1c636 - Unknown
	  	0x107aa2c73 - Unknown

	…
	  =================================================================
	  	Managed Stacktrace:
	  =================================================================
	  	  at <unknown> <0xffffffff>
	  	  at Java.Interop.NativeMethods:java_interop_jnienv_get_object_class <0x000a5>
	  	  at Types:GetObjectClass <0x0010a>
	  	  at Types:GetJniTypeNameFromInstance <0x000a2>
	  	  at JniValueManager:DisposePeer <0x002c2>
	  	  at JniValueManager:DisposePeer <0x000f2>
	  	  at Java.Interop.JavaObject:Dispose <0x000ea>
	  	  at Java.InteropTests.JavaObjectTest:NestedDisposeInvocations <0x00072>
	  	  at System.Object:runtime_invoke_void__this__ <0x000b0>
	  	  at <unknown> <0xffffffff>
	  	  at System.Reflection.RuntimeMethodInfo:InternalInvoke <0x000b8>
	  	  at System.Reflection.RuntimeMethodInfo:Invoke <0x00152>
	  	  at System.Reflection.MethodBase:Invoke <0x00058>

Ouch.

The cause of the crash isn't the "successive" `.Dispose()` invocations
within `MyTestMethod()`, but rather the *nested* `.Dispose()`
invocation within `MyDisposableObject.Dispose()`.

Runtime execution is thus:

 1. `JavaObject.Dispose()`
 2. `JniRuntime.JniValueManager.DisposePeer(this)`
 3. `var h = this.PeerReference`
 4. `JniRuntime.JniValueManager.DisposePeer(h, this)`
 5. `JavaObject.Disposed()`
 6. `MyDisposableObject.Dispose(disposing:true)`
 7. `JavaObject.Dispose()`              // back to (1)?
 8. `JniRuntime.JniValueManager.DisposePeer(this)`
 9. `var h = this.PeerReference`        // *second* ref to `h`
10. `JniRuntime.JniValueManager.DisposePeer(h, this)`, which passes
    `h` to e.g. `JniEnvironment.Types.GetJniTypeNameFromInstance()`,
    thus requiring that `h` be a valid JNI reference, and also calls
    `JniObjectReference.Dispose()`, invalidating `h`.
11. "Unwinding" (4), call `Types.GetJniTypeNameFromInstance()` with
    the `h` from (3).

The problem *appears to be* the recursive `Dispose()` invocation on
(7), but the *actual* problem is step (3): by holding a cached/"old"
value of `this.PeerReference` -- and then later using that *same* value
in `JniRuntime.JniValueManager.DisposePeer()`-- when the nested
`JavaObject.Dispose()` invocation continues execution,
`this.PeerReference` will be *invalidated*, but the copy of the handle
from (3) will still be used!  This causes the JVM to very loudly abort.

The fix is to defer the "caching" present in (3): instead of storing
the `PeerReference` value "immediately" -- and disposing the same
value "later" -- don't store the value until *after*
`IJavaPeerable.Disposed()` is called.  This gives the
`Dispose(disposing:true)` method a chance to execute *before*
retaining any cached references to `PeerReference` -- which may in
turn invalidate `PeerReference`! -- thus ensuring that we only attempt
to dispose *valid* JNI handles.

Author: jonpryor

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

@@ -143,6 +143,13 @@ public virtual void DisposePeer (IJavaPeerable value)
 				if (value == null)
 					throw new ArgumentNullException (nameof (value));
 
+				if (!value.PeerReference.IsValid)
+					return;
+
+				RemovePeer (value);
+
+				value.Disposed ();
+
 				var h = value.PeerReference;
 				if (!h.IsValid)
 					return;
@@ -155,8 +162,6 @@ void DisposePeer (JniObjectReference h, IJavaPeerable value)
 				if (disposed)
 					throw new ObjectDisposedException (GetType ().Name);
 
-				value.Disposed ();
-				RemovePeer (value);
 				var o = Runtime.ObjectReferenceManager;
 				if (o.LogGlobalReferenceMessages) {
 					o.WriteGlobalReferenceLine ("Disposing PeerReference={0} IdentityHashCode=0x{1} Instance=0x{2} Instance.Type={3} Java.Type={4}",

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

@@ -185,6 +185,14 @@ public void CrossThreadSharingRequresRegistration ()
 			o.ToString ();
 			o.Dispose ();
 		}
+
+		[Test]
+		public void NestedDisposeInvocations ()
+		{
+			var value = new MyDisposableObject ();
+			value.Dispose ();
+			value.Dispose ();
+		}
 	}
 
 	class JavaObjectWithNoJavaPeer : JavaObject {
@@ -215,5 +223,25 @@ protected override void Dispose (bool disposing)
 				OnFinalized ();
 		}
 	}
+
+	[JniTypeSignature ("java/lang/Object")]
+	class MyDisposableObject : JavaObject {
+		bool _isDisposed;
+
+		public MyDisposableObject ()
+		{
+		}
+
+		protected override void Dispose (bool disposing)
+		{
+			if (_isDisposed) {
+				return;
+			}
+			_isDisposed = true;
+			if (this.PeerReference.IsValid)
+				this.Dispose ();
+			base.Dispose (disposing);
+		}
+	}
 }