Skip to content

Bump to xamarin/Java.Interop/master@007b35b #5022

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 18, 2020

Conversation

jonpryor
Copy link
Contributor

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.

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 grendello merged commit d4e0738 into dotnet:master Aug 18, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Jan 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change in behavior around Java.Lang.Object.Dispose leads to a crash when calling Dispose recursively
4 participants