[Java.Interop] Avoid double-disposing PeerReferences

[jonpryor]

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.