From 9e798a7a466a2e0ec67ad03e704dc30affd3e902 Mon Sep 17 00:00:00 2001 From: Jonathan Pryor Date: Mon, 17 Aug 2020 12:12:17 -0400 Subject: [PATCH] Bump to xamarin/Java.Interop/master@007b35b MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes: https://github.com/xamarin/xamarin-android/issues/4989 What happens if you dispose an instance *while disposing the instance*? // C# public class MyDisposableObject : Java.Lang.Object { bool _isDisposed; protected override void Dispose (bool disposing) { if (_isDisposed) { return; } _isDisposed = true; if (this.Handle != IntPtr.Zero) this.Dispose (); base.Dispose (disposing); } } // … void MyTestMethod () { var value = new MyDisposableObject (); value.Dispose (); value.Dispose (); } Here, `MyDisposableObject.Dispose(bool)` calls `Object.Dispose()` when `Object.Handle` 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. Bump to xamarin/Java.Interop@007b35b, which contains fixes to `JniRuntime.JniValueManager.DisposePeer()`, and add appropriate tests. --- external/Java.Interop | 2 +- .../Android.Runtime/AndroidRuntime.cs | 6 +++-- src/Mono.Android/Test/Java.Lang/ObjectTest.cs | 27 +++++++++++++++++++ 3 files changed, 32 insertions(+), 3 deletions(-) diff --git a/external/Java.Interop b/external/Java.Interop index 6bbb00aa3fd..007b35b4891 160000 --- a/external/Java.Interop +++ b/external/Java.Interop @@ -1 +1 @@ -Subproject commit 6bbb00aa3fdfdb61f3b5167b6bb025f62a54ccdf +Subproject commit 007b35b489173010de038c95cb2a0d3ec514f30d diff --git a/src/Mono.Android/Android.Runtime/AndroidRuntime.cs b/src/Mono.Android/Android.Runtime/AndroidRuntime.cs index 6c3f738c244..d989036b95e 100644 --- a/src/Mono.Android/Android.Runtime/AndroidRuntime.cs +++ b/src/Mono.Android/Android.Runtime/AndroidRuntime.cs @@ -558,10 +558,12 @@ public override void RemovePeer (IJavaPeerable value) { if (value == null) throw new ArgumentNullException (nameof (value)); - if (!value.PeerReference.IsValid) - throw new ArgumentException ("Must have a valid JNI object reference!", nameof (value)); var reference = value.PeerReference; + if (!reference.IsValid) { + // Likely an idempotent DIspose(); ignore. + return; + } var hash = JNIEnv.IdentityHash! (reference.Handle); RemovePeer (value, hash); diff --git a/src/Mono.Android/Test/Java.Lang/ObjectTest.cs b/src/Mono.Android/Test/Java.Lang/ObjectTest.cs index 9eb8421d2f1..302ef9b2ac5 100644 --- a/src/Mono.Android/Test/Java.Lang/ObjectTest.cs +++ b/src/Mono.Android/Test/Java.Lang/ObjectTest.cs @@ -67,6 +67,14 @@ public void JnienvCreateInstance_RegistersMultipleInstances () Assert.AreSame (adapter, registered); } } + + [Test] + public void NestedDisposeInvocations () + { + var value = new MyDisposableObject (); + value.Dispose (); + value.Dispose (); + } } /* @@ -157,4 +165,23 @@ public override void SetSelection (int position) throw new NotImplementedException(); } } + + public class MyDisposableObject : Java.Lang.Object + { + bool _isDisposed; + public MyDisposableObject () + { + } + + protected override void Dispose (bool disposing) + { + if (_isDisposed) { + return; + } + _isDisposed = true; + if (this.Handle != IntPtr.Zero) + this.Dispose (); + base.Dispose (disposing); + } + } }