Change in behavior around Java.Lang.Object.Dispose leads to a crash when calling Dispose recursively #4989
Comments
jonpryor
added a commit
to jonpryor/java.interop
that referenced
this issue
Aug 14, 2020
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`. …. "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.
jonpryor
added a commit
to jonpryor/java.interop
that referenced
this issue
Aug 14, 2020
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.
grendello
pushed a commit
to dotnet/java-interop
that referenced
this issue
Aug 15, 2020
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.
jonpryor
added a commit
to jonpryor/xamarin-android
that referenced
this issue
Aug 17, 2020
Fixes: dotnet#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 <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. Bump to dotnet/java-interop@007b35b, which contains fixes to `JniRuntime.JniValueManager.DisposePeer()`, and add appropriate tests.
grendello
pushed a commit
that referenced
this issue
Aug 18, 2020
Fixes: #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 <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. Bump to dotnet/java-interop@007b35b, which contains fixes to `JniRuntime.JniValueManager.DisposePeer()`, and add appropriate tests.
jonpryor
added a commit
to dotnet/java-interop
that referenced
this issue
Aug 31, 2020
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.
jonpryor
added a commit
that referenced
this issue
Aug 31, 2020
Fixes: dotnet/java-interop#694 Fixes: #3776 Fixes: #4989 Changes: dotnet/java-interop@a92fd18...b4c2a67 * dotnet/java-interop@b4c2a67c: [Xamarin.Android.Tools.Bytecode] Hide Kotlin synthetic constructors (#700) * dotnet/java-interop@4141d848: [Java.Interop] Avoid double-disposing PeerReferences (#690)
jonpryor
added a commit
to jonpryor/xamarin-android
that referenced
this issue
Sep 4, 2020
Fixes: dotnet#4989 Fixes: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1193891 Commit 62a5709 forgot to apply the `Mono.Android.dll` changes! (Doh!) Property cherry-pick d4e0738. ~~~ 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 <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. Bump to dotnet/java-interop@007b35b, which contains fixes to `JniRuntime.JniValueManager.DisposePeer()`, and add appropriate tests.
jonpryor
added a commit
that referenced
this issue
Sep 5, 2020
Fixes: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1193891 Fixes: #4989 Changes: dotnet/java-interop@b4c2a67...a807961 * dotnet/java-interop@a8079616: [Java.Interop] Dispose, *then* Remove (#708) ~~~ Also cherry-picks d4e0738, which *should* have been done as part of commit 62a5709, but was overlooked: 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 <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. Commit dotnet/java-interop@4141d848 contains fixes to `JniRuntime.JniValueManager.DisposePeer()`, and add appropriate tests.
|
Release status update A new Preview version of Xamarin.Android has now been published that includes the fix for this item. The fix is not yet included in a Release version. I will update this item again when a Release version is available that includes the fix. Fix included in Xamarin.Android SDK version 11.1.0.3. Fix included on Windows in Visual Studio 2019 version 16.8 Preview 3. To try the Preview version that includes the fix, check for the latest updates in Visual Studio Preview. Fix included on macOS in Visual Studio 2019 for Mac version 8.8 Preview 3. To try the Preview version that includes the fix, check for the latest updates on the Preview updater channel. |
|
Release status update A new Release version of Xamarin.Android has now been published that includes the fix for this item. Fix included in Xamarin.Android SDK version 11.1.0.17. Fix included on Windows in Visual Studio 2019 version 16.8. To get the new version that includes the fix, check for the latest updates or install the most recent release from https://visualstudio.microsoft.com/downloads/. Fix included on macOS in Visual Studio 2019 for Mac version 8.8. To get the new version that includes the fix, check for the latest updates on the Stable updater channel. |
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Steps to Reproduce
Related Xamarin.Forms issue here
xamarin/Xamarin.Forms#11029
User started seeing a crash that they weren't seeing before
App34.zip
I realize the code that causes this is kind of weird but I just wanted to highlight the change in behavior
Expected Behavior
Not crash
Actual Behavior
Crashes with
Version Information
The text was updated successfully, but these errors were encountered: