[mono][android] Bring back api used by xamarin.android #55904
Conversation
xam.android catches all exceptions and then explicitly invokes all registered handlers via AppDomain.DoUnhandledException
Used to call the debugger when an unhandled exception is detected.
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
|
Tagging subscribers to this area: |
| #if TARGET_ANDROID | ||
| internal void DoUnhandledException (UnhandledExceptionEventArgs args) | ||
| { | ||
| AppContext.DoUnhandledException (this, args); | ||
| } | ||
| #endif |
There was a problem hiding this comment.
Where is this called and why is it Android specific?
There was a problem hiding this comment.
I don't fully understand how this stuff works, but xam-android is catching all exceptions in https://github.com/xamarin/xamarin-android/blob/main/src/Mono.Android/Android.Runtime/JNINativeWrapper.cs#L52. This wrapper might call into Mono_UnhandledException for the debugger stuff and later it calls into AndroidEnvironment.UnhandledException. Sometime later, not sure how, it will end up in https://github.com/xamarin/xamarin-android/blob/main/src/Mono.Android/Android.Runtime/JNIEnv.cs#L297 or in https://github.com/xamarin/xamarin-android/blob/main/src/Mono.Android/Android.Runtime/UncaughtExceptionHandler.cs#L39
There was a problem hiding this comment.
I don't see that code used anywhere and I don't think it should be Android-specific. Handling of unhandled exceptions should work everywhere.
There was a problem hiding this comment.
I don't see that code used anywhere
I don't know which code you're referring to specifically, given that @BrzVlad linked to relevant places of code. Thus, an example: suppose you have the following Xamarin.Android code:
using Android.App;
using Android.OS.;
using System;
[Activity (MainLauncher = true)
partial class MyActivity : Activity {
public override void OnCreate (Bundle savedInstanceState) => throw new System.Exception ("Yolo!");
}At runtime, the Java Callable Wrapper for MyActivity will be created, which will cause the C# MyActivity constructor to be executed. Later, Android will invoke Activity.onCreate() on that instance, which will invoke MyActivity.OnCreate() to be invoked.
On entry to MyActivity.OnCreate(), we have a stack frame of:
- Java [caller of
Activity.onCreate() JNINativeWrapper-generated delegate instance- A binding generated "marshal method",
Activity.n_OnCreate_Landroid_os_Bundle_() MyActivity.OnCreate()
(2) is effectively:
static void n_OnCreate(IntPtr env, IntPtr self, IntPtr bundle)
{
try {
Activity.n_OnCreate_Landroid_os_Bundle_ (env, self, bundle);
}
catch (Exception e) {
AndroidEnvironment.UnhandledException (e);
}
}(3) would be:
partial class Activity {
static void n_OnCreate_Landroid_os_Bundle_ (IntPtr jnienv, IntPtr native__this, IntPtr native_savedInstanceState)
{
var __this = global::Java.Lang.Object.GetObject<Android.App.Activity> (jnienv, native__this, JniHandleOwnership.DoNotTransfer)!;
var savedInstanceState = global::Java.Lang.Object.GetObject<Android.OS.Bundle> (native_savedInstanceState, JniHandleOwnership.DoNotTransfer);
__this.OnCreate (savedInstanceState);
}
}AndroidEnvironment.UnhandledException(e) will basically do:
JNIEnv.Throw(new JavaProxyThrowable(e));When MyActivity.OnCreate() throws the exception, we'll run the catch block in (2). JNIEnv.Throw() causes there to be a "pending exception"; on return to Java, Java will raise the exception, allowing Java-side catch and finally blocks to participate in the stack unwind.
This will cause frame (1) and callers of (1) to eventually unwind, potentially unwinding the entire Java-side stack. When that happens, Java will eventually execute:
java.lang.Thread.getUncaughtExceptionHandler()
.uncaughtException(currentThread, currentException);During startup, Xamarin.Android registers an uncaught exception handler with Java -- the UncaughtExceptionHandler type which @BrzVlad linked to -- and UncaughtExceptionHandler.UncaughtException() will raise the AppDomain.UnhandledException event, by calling AppDomain.DoUnhandledException(UnhandledExceptionEventArgs).
We don't want the JNINativeWrapper-emitted delegate to raise AppDomain.UnhandledException, because it might not be unhandled! If we're in a Java > Managed > Java > Managed stack, one of the calling stack frames might catch the exception.
We thus need a way for Xamarin.Android to cause the AppDomain.UnhandledException event to be raised.
There was a problem hiding this comment.
Apparently the implicit question was "how do we actually register the Java-side unhandled exception support"?
The mono.android.Runtime class is used everywhere in a Xamarin.Android app, including as part of process bootstrap. The Runtime static constructor registers a Thread.UncaughtExceptionHandler implementation, and implements Thread.UncaughtExceptionHandler.uncaughtException() by calling Runtime.propagateUncaughtException().
Runtime.propagateUncaughtException() calls JNIEnv.PropagateUncaughtException(), which calls AppDomain.DoUnhandledException().
There was a problem hiding this comment.
As per above conversation, I think we don't actually need AppDomain.DoUnhandledException(); xamarin-android could instead (presumably) be updated to call mono_unhandled_exception() instead.
There was a problem hiding this comment.
@grendello will work on a change to xamarin-android wherein we call mono_unhandled_exception() instead of AppDomain.DoUnhandledException(): https://discord.com/channels/732297728826277939/732297837953679412/866765321821618256
There was a problem hiding this comment.
Eventually, does it make sense to do something similar to XI?
Register:
And this is the runtime API it's using
There was a problem hiding this comment.
Let's try to use the existing embedding API first before considering this PR.
| </type> | ||
|
|
||
| <type fullname="System.Diagnostics.Debugger"> | ||
| <method name="Mono_UnhandledException"/> |
There was a problem hiding this comment.
If Xamarin Android is calling this, it should be designed, exposed and tested as proper public API.
There was a problem hiding this comment.
Android has been calling this method for over 10 years: https://github.com/xamarin/monodroid/commit/3e9de5a51b
Other related reading:
- https://github.com/xamarin/monodroid/commit/b0f85970102d43bab9cd860a8e8884d136d766b3
- https://github.com/xamarin/monodroid/commit/12a012e00b4533d586ef31ced33351b63c9de883
Yes, it should be designed, exposed, and tested. That said, this is what we had, and what worked.
There was a problem hiding this comment.
We already have mono_unhandled_exception which is part of the embedding API and should do the same thing. Android could also switch to using it instead.
There was a problem hiding this comment.
@BrzVlad : which raises a different question: mono_unhandled_exception() (added in 2002 via mono/mono@ce9497a) predates xamarin/monodroid@3e9de5a (2010), so why wasn't that used in the first place?
A difference between the two is debugger interaction: Debugger::Mono_UnhandledException, in legacy, hits debugger_agent_unhandled_exception():
- https://github.com/mono/mono/blob/78c2b8da29881eccda4c27ca7b2b7e9ed451c55f/mono/mini/mini-runtime.c#L4680-L4683
- https://github.com/mono/mono/blob/78c2b8da29881eccda4c27ca7b2b7e9ed451c55f/mono/mini/mini-runtime.c#L4307-L4318
- https://github.com/mono/mono/blob/78c2b8da29881eccda4c27ca7b2b7e9ed451c55f/mono/mini/debugger-agent.c#L10288
- https://github.com/mono/mono/blob/78c2b8da29881eccda4c27ca7b2b7e9ed451c55f/mono/mini/debugger-agent.c#L5125-L5143
Meanwhile, in mono_unhandled_exception(): https://github.com/mono/mono/blob/78c2b8da29881eccda4c27ca7b2b7e9ed451c55f/mono/metadata/object.c#L5105-L5211
I don't see any calls to mini_get_dbg_callbacks(), much less mini_get_dbg_callbacks()->unhandled_exception.
Calling mono_unhandled_exception() would allow re-working the AppDomain.DoUnhandledException() codepath (below), but is otherwise unrelated to Debugger.Mono_UnhandledException.
There was a problem hiding this comment.
Yes, it should be designed, exposed, and tested. That said, this is what we had, and what worked.
Right, so I would expect as part of this PR:
-
Code comments that explain that this is a legacy private reflection hack, with links where Xamarin Android calls it, so that the next person looking at this code won't delete it as dead code again.
-
API proposal issue should be opened to create proper public managed API for this that Xamarin Android can switch to.
There was a problem hiding this comment.
@jonpryor Yeah, I was actually referring to the DoUnhandledException callback. I think it makes sense for mono_unhandled_exception to also call into the debugger. @thaystg seems to have looked into removing the Debugger.Mono_UnhandledException API altogether, maybe she has some ideas.
Anyhow. This is a regression on android on the new netcore profile and I'm not sure it is worthwhile to bother refactoring it so late in the release cycle. We could keep the legacy code for this release, and remove these API's for next release.
There was a problem hiding this comment.
I think this is the best approach to fix this regression, as I was working on implementing other things I couldn't look before preview 7.
I was planning to look at this, during this week, but maybe you are right it's too late for this release, we could only remove it on next release.
There was a problem hiding this comment.
@jonpryor , you don't see mini_get_dbg_callbacks()->unhandled_exception because with componentize work this was renamed to: mono_component_debugger ()->unhandled_exception, and it's still in the code:
runtime/src/mono/mono/mini/mini-runtime.c
Line 4611 in c8f9a24
There was a problem hiding this comment.
@thaystg : the Debugger::Mono_UnhandledException_internal internal call is registered, but it doesn't exist: https://github.com/dotnet/runtime/blob/c8f9a24ca0ebf6d9593e00ccbc7353510a0e0213/src/mono/System.Private.CoreLib/src/System/Diagnostics/Debugger.cs
Hence the change below, which re-introduces the declaration for the Debugger::Mono_UnhandledException internal call to Debugger.cs.
marek-safar
added
the
NO-MERGE
|
I'll continue working in a solution to avoid calling |
Context: dotnet/runtime#55904 (comment) Context: dotnet#4927 (comment) Context: dotnet#4927 (comment) Xamarin.Android has been using the `AppDomain.DoUnhandledException` API since the dawn of time to propagate uncaught Java exceptions to the managed world. However, said API (and AppDomains) are gone from the NET6 MonoVM runtime and we need to switch to something else - the `mono_unhandled_exception` native API. This commit introduces an internal call, `JNIEnv.monodroid_unhandled_exception`, which is used instead of the older mechanism when targetting NET6 and which calls the native API mentioned above. Add a device integration test which makes sure the uncaught exceptions are propagated as required.
Context: dotnet/runtime#55904 (comment) Context: #4927 (comment) Context: #4927 (comment) Context: xamarin/monodroid@7d62dec Context: dotnet/runtime@15ab9f9 Xamarin.Android has been using the `AppDomain.DoUnhandledException()` internal API since 2015 (xamarin/monodroid@7d62dec1) to propagate uncaught Java exceptions to the managed world. However, `AppDomain.DoUnhandledException()` was an internal API which was *removed* from MonoVM as part of the .NET 6 effort in dotnet/runtime@15ab9f98. For .NET 6, instead of calling the no-longer-present `AppDomain.DoUnhandledException()` method, call the [`mono_unhandled_exception()` embedding API][0], which in turn raises the `AppDomain.UnhandledException` event. Add a new internal call, `JNIEnv.monodroid_unhandled_exception()`, which is used on .NET 6 to invoke `mono_unhandled_exception()`. Add an on-device integration test which ensures that the uncaught exceptions are propagated as required. [0]: http://docs.go-mono.com/index.aspx?link=xhtml%3adeploy%2fmono-api-exc.html
Context: dotnet/runtime#55904 (comment) Context: #4927 (comment) Context: #4927 (comment) Context: xamarin/monodroid@7d62dec Context: dotnet/runtime@15ab9f9 Xamarin.Android has been using the `AppDomain.DoUnhandledException()` internal API since 2015 (xamarin/monodroid@7d62dec1) to propagate uncaught Java exceptions to the managed world. However, `AppDomain.DoUnhandledException()` was an internal API which was *removed* from MonoVM as part of the .NET 6 effort in dotnet/runtime@15ab9f98. For .NET 6, instead of calling the no-longer-present `AppDomain.DoUnhandledException()` method, call the [`mono_unhandled_exception()` embedding API][0], which in turn raises the `AppDomain.UnhandledException` event. Add a new internal call, `JNIEnv.monodroid_unhandled_exception()`, which is used on .NET 6 to invoke `mono_unhandled_exception()`. Add an on-device integration test which ensures that the uncaught exceptions are propagated as required. [0]: http://docs.go-mono.com/index.aspx?link=xhtml%3adeploy%2fmono-api-exc.html
|
Since the android team was able to use another way, we can close this PR. |
ghost
locked as resolved and limited conversation to collaborators
Fixes #44526