From 007b35b489173010de038c95cb2a0d3ec514f30d Mon Sep 17 00:00:00 2001 From: Jonathan Pryor Date: Sat, 15 Aug 2020 12:36:10 -0400 Subject: [PATCH] [Java.Interop] Avoid double-disposing PeerReferences (#690) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Context: https://github.com/xamarin/xamarin-android/issues/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 <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 <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. --- .../JniRuntime.JniValueManager.cs | 9 ++++-- .../Java.Interop/JavaObjectTest.cs | 28 +++++++++++++++++++ 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs b/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs index d028c4fbd..1e8d4dbae 100644 --- a/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs +++ b/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}", diff --git a/tests/Java.Interop-Tests/Java.Interop/JavaObjectTest.cs b/tests/Java.Interop-Tests/Java.Interop/JavaObjectTest.cs index 92133e194..bf65b4b84 100644 --- a/tests/Java.Interop-Tests/Java.Interop/JavaObjectTest.cs +++ b/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); + } + } }