Skip to content
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.

Sign up for GitHub

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

[debugger] Removing usage of mono_unhandled_exception #4927

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

[debugger] Removing usage of mono_unhandled_exception #4927

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ff9223b
Trying to remove the usage of mono_unhandled_exception.
thaystg Jul 15, 2020
649f8cf
Removing unused variable.
thaystg Jul 16, 2020
1f51034
Totally remove this try catch generated when the debugger is attached.
thaystg Aug 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 0 additions & 12 deletions src/Mono.Android/Android.Runtime/JNIEnv.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,18 +237,10 @@ static void ManualJavaObjectDispose (Java.Lang.Object obj)
GC.SuppressFinalize (obj);
}

static Action<Exception> mono_unhandled_exception = null!;
static Action<AppDomain, UnhandledExceptionEventArgs> AppDomain_DoUnhandledException = null!;

static void Initialize ()
{
if (mono_unhandled_exception == null) {
var mono_UnhandledException = typeof (System.Diagnostics.Debugger)
.GetMethod ("Mono_UnhandledException", BindingFlags.NonPublic | BindingFlags.Static);
if (mono_UnhandledException != null)
mono_unhandled_exception = (Action<Exception>) Delegate.CreateDelegate (typeof(Action<Exception>), mono_UnhandledException);
}

if (AppDomain_DoUnhandledException == null) {
var ad_due = typeof (AppDomain)
.GetMethod ("DoUnhandledException",
Expand Down Expand Up @@ -276,10 +268,6 @@ internal static void PropagateUncaughtException (IntPtr env, IntPtr javaThreadPt

var javaException = JavaObject.GetObject<Java.Lang.Throwable> (env, javaExceptionPtr, JniHandleOwnership.DoNotTransfer)!;

// Disabled until Linker error surfaced in https://github.com/xamarin/xamarin-android/pull/4302#issuecomment-596400025 is resolved
//System.Diagnostics.Debugger.Mono_UnhandledException (javaException);
mono_unhandled_exception?.Invoke (javaException);

try {
var jltp = javaException as JavaProxyThrowable;
Exception? innerException = jltp?.InnerException;
Expand Down
48 changes: 20 additions & 28 deletions src/Mono.Android/Android.Runtime/JNINativeWrapper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,20 +7,13 @@
namespace Android.Runtime {
public static class JNINativeWrapper {

static MethodInfo? mono_unhandled_exception_method;
static MethodInfo? exception_handler_method;
static MethodInfo? wait_for_bridge_processing_method;

static void get_runtime_types ()
{
if (exception_handler_method != null)
return;
#if MONOANDROID1_0
mono_unhandled_exception_method = typeof (System.Diagnostics.Debugger).GetMethod (
"Mono_UnhandledException", BindingFlags.NonPublic | BindingFlags.Static);
if (mono_unhandled_exception_method == null)
AndroidEnvironment.FailFast ("Cannot find System.Diagnostics.Debugger.Mono_UnhandledException");
#endif
exception_handler_method = typeof (AndroidEnvironment).GetMethod (
"UnhandledException", BindingFlags.NonPublic | BindingFlags.Static);
if (exception_handler_method == null)
Expand Down Expand Up @@ -58,35 +51,34 @@ public static Delegate CreateDelegate (Delegate dlg)

ig.Emit (OpCodes.Call, wait_for_bridge_processing_method!);

var label = ig.BeginExceptionBlock ();

for (int i = 0; i < param_types.Length; i++)
ig.Emit (OpCodes.Ldarg, i);
ig.Emit (OpCodes.Call, dlg.Method);
bool filter = Debugger.IsAttached || !JNIEnv.PropagateExceptions;
if (!filter) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thaystg: This presents a question: is there any way to "mimic" the "normal" .NET Unhandled Exception behaviors while still catching the exception?

Related: #4548

The problem is that when the catch block doesn't exist, in a managed > Java > managed callstack, if the rightmost frame throws (and no catch block is present around it) and execution doesn't return to Java, and we unwind the rightmost Java > managed frames, then JVM state may be corrupted.

In an ideal world, every "marshal method" would have a try{…} catch (Exception) {…} handler, so that when a managed method throws an exception it can be properly caught & propagated back to Java. The problem with this ideal is it means every exception is by definition "handled", thus "mooting" the entire concept of "first chance unhandled exceptions". (There'd still eventually be an unhandled exception, but that would be on the leftmost managed frame, after the rightmost frame had been unwound.)

Is this possible?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you telling me that removing the try catch when the debugger is attached is a problem because when we have a case like:
managed -> Java -> managed (which throws the unhandled exception), debugger will stop in the exception, but when we continue the execution we may get an error because JVM state may be corrupted? Which kind of error? shouldn't the process be finished in this case?
If I test #4548 I will reproduce the case that you are saying?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you telling me that removing the try catch when the debugger is attached is a problem because when we have a case like:
managed -> Java -> managed (which throws the unhandled exception), debugger will stop in the exception, but when we continue the execution we may get an error because JVM state may be corrupted?

Yup. Because the debugger is running, there are no try/catch blocks at Java-to-managed boundaries, and because there are try/catch blocks, if there is an exception thrown from managed code, the JVM state is corrupted, and Android aborts the process.

Desktop repro:

$ git clone https://github.com/xamarin/java.interop.git
$ cd java.interop
$ make prepare
$ cat > d <<EOF
diff --git a/tests/Java.Interop-Tests/Java.Interop/TestType.cs b/tests/Java.Interop-Tests/Java.Interop/TestType.cs
index 4640d5f1..2cd108b1 100644
--- a/tests/Java.Interop-Tests/Java.Interop/TestType.cs
+++ b/tests/Java.Interop-Tests/Java.Interop/TestType.cs
@@ -172,7 +172,8 @@ namespace Java.InteropTests
 		static Delegate GetMethodThrowsHandler ()
 		{
 			Action<IntPtr, IntPtr> h = MethodThrowsHandler;
-			return JniEnvironment.Runtime.MarshalMemberBuilder.CreateMarshalToManagedDelegate (h);
+//			return JniEnvironment.Runtime.MarshalMemberBuilder.CreateMarshalToManagedDelegate (h);
+			return h;
 		}
 
 		static void MethodThrowsHandler (IntPtr jnienv, IntPtr n_self)

EOF
$ git apply < d
$ make
$ make run-tests TESTS=bin/TestDebug/Java.Interop-Tests.dll

Desired behavior: nothing bad happens. ;-)

Actual results: horrible flaming death:

$ make run-tests TESTS=bin/TestDebug/Java.Interop-Tests.dll
r=0; \
	 	msbuild /p:Configuration=Debug  build-tools/scripts/RunNUnitTests.targets /p:TestAssembly=bin/TestDebug/Java.Interop-Tests.dll || r=1; \
	exit $r;
…
  =================================================================
  	Native Crash Reporting
  =================================================================
  Got a segv while executing native code. This usually indicates
  a fatal error in the mono runtime or one of the native libraries 
  used by your application.
  =================================================================
  
  =================================================================
  	Native stacktrace:
  =================================================================
  	0x101da3779 - /Library/Frameworks/Mono.framework/Versions/Current/Commands/mono : mono_dump_native_crash_info
  	0x101d3b5be - /Library/Frameworks/Mono.framework/Versions/Current/Commands/mono : mono_handle_native_crash
  	0x101d9d8f8 - /Library/Frameworks/Mono.framework/Versions/Current/Commands/mono : altstack_handle_and_restore
  	0x10c849e4e - /Users/jon/android-toolchain/jdk-11/lib/server/libjvm.dylib : _ZN5Chunk9next_chopEv
  	0x10cb5ccec - /Users/jon/android-toolchain/jdk-11/lib/server/libjvm.dylib : checked_jni_GetBooleanField
  	0x10c299140 - /Users/jon/Dropbox/Developer/Java.Interop/bin/TestDebug/libjava-interop.dylib : java_interop_jnienv_get_boolean_field
  	0x1650e3841 - Unknown
  
  =================================================================
  	Telemetry Dumper:
  =================================================================
EXEC : warning : SIGSEGV handler expected:libjvm.dylib+0x5f61c3  found:0x0000000000000000 [/Users/jon/Dropbox/Developer/Java.Interop/build-tools/scripts/RunNUnitTests.targets]
  Signal Handlers:
  SIGSEGV: SIG_DFL, sa_mask[0]=00000000000000000000000000000000, sa_flags=none
…

Again, this crash is "expected", because by not catching all exceptions (which is what JniEnvironment.Runtime.MarshalMemberBuilder.CreateMarshalToManagedDelegate() does), we're propagating a Mono exception "through" a Java-to-managed boundary, and Java Does Not Like That. (Deservedly so!)

The Good And Proper solution is to catch all exceptions at all Java-to-managed boundaries, but in doing so we "kill" the normal/expected IDE unhandled exception behavior… because now all exceptions are handled.

Is there a way to have our cake and eat it, too?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thaystg what is the best path forward here?

Copy link
Member Author

@thaystg thaystg Jan 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how can we do this, but I would like to try to generate a try catch with some "mark" so we would know that the debugger should stop because it is not a really handled exception, it's only an exception that we catch in our forced catch.
I will come back soon to this issue to make more tests.

var label = ig.BeginExceptionBlock ();

if (retval != null)
ig.Emit (OpCodes.Stloc, retval);
for (int i = 0; i < param_types.Length; i++)
ig.Emit (OpCodes.Ldarg, i);
ig.Emit (OpCodes.Call, dlg.Method);

ig.Emit (OpCodes.Leave, label);
if (retval != null)
ig.Emit (OpCodes.Stloc, retval);

bool filter = Debugger.IsAttached || !JNIEnv.PropagateExceptions;
if (filter && mono_unhandled_exception_method != null) {
ig.BeginExceptFilterBlock ();
ig.Emit (OpCodes.Leave, label);

ig.Emit (OpCodes.Call, mono_unhandled_exception_method);
ig.Emit (OpCodes.Ldc_I4_1);
ig.BeginCatchBlock (null!);
} else {
ig.BeginCatchBlock (typeof (Exception));
}

ig.Emit (OpCodes.Dup);
ig.Emit (OpCodes.Call, exception_handler_method!);
ig.Emit (OpCodes.Dup);
ig.Emit (OpCodes.Call, exception_handler_method!);

if (filter)
ig.Emit (OpCodes.Throw);
ig.EndExceptionBlock ();
}
else { //let the debugger handle the exception
for (int i = 0; i < param_types.Length; i++)
ig.Emit (OpCodes.Ldarg, i);
ig.Emit (OpCodes.Call, dlg.Method);

ig.EndExceptionBlock ();
if (retval != null)
ig.Emit (OpCodes.Stloc, retval);
}

if (retval != null)
ig.Emit (OpCodes.Ldloc, retval);
Expand Down
8 changes: 0 additions & 8 deletions src/Mono.Android/Android.Runtime/UncaughtExceptionHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@ namespace Android.Runtime {

sealed class UncaughtExceptionHandler : Java.Lang.Object, Java.Lang.Thread.IUncaughtExceptionHandler {

Action<Exception>? mono_unhandled_exception;

Action<AppDomain, UnhandledExceptionEventArgs>? AppDomain_DoUnhandledException;

Java.Lang.Thread.IUncaughtExceptionHandler defaultHandler;
Expand All @@ -30,7 +28,6 @@ public void UncaughtException (Java.Lang.Thread thread, Java.Lang.Throwable ex)
Android.Runtime.AndroidEnvironment.FailFast ($"Unable to initialize UncaughtExceptionHandler. Nested exception caught: {e}");
}

mono_unhandled_exception! (ex);
if (AppDomain_DoUnhandledException != null) {
try {
var jltp = ex as JavaProxyThrowable;
Expand All @@ -48,11 +45,6 @@ public void UncaughtException (Java.Lang.Thread thread, Java.Lang.Throwable ex)

void Initialize ()
{
if (mono_unhandled_exception == null) {
var mono_UnhandledException = typeof (System.Diagnostics.Debugger)
.GetMethod ("Mono_UnhandledException", BindingFlags.NonPublic | BindingFlags.Static);
mono_unhandled_exception = (Action<Exception>) Delegate.CreateDelegate (typeof(Action<Exception>), mono_UnhandledException);
}

if (AppDomain_DoUnhandledException == null) {
var ad_due = typeof (AppDomain)
Expand Down