Skip to content

[mono][android] Bring back api used by xamarin.android #55904

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

Closed
wants to merge 2 commits into from
Closed

[mono][android] Bring back api used by xamarin.android #55904

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3af9e98
[mono] Bring back api used by xamarin.android
BrzVlad Jul 19, 2021
7e284b9
[mono] Bring back api used by xamarin.android
BrzVlad Jul 19, 2021
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
2 changes: 2 additions & 0 deletions src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,8 @@
<ItemGroup>
<Compile Include="$(BclSourcesRoot)\Mono\RuntimeHandles.cs" />
<Compile Include="$(BclSourcesRoot)\System\ArgIterator.cs" />
<Compile Include="$(BclSourcesRoot)\System\AppContext.Mono.cs" />
<Compile Include="$(BclSourcesRoot)\System\AppDomain.Mono.cs" />
<Compile Include="$(BclSourcesRoot)\System\Array.Mono.cs" />
<Compile Include="$(BclSourcesRoot)\System\Attribute.Mono.cs" />
<Compile Include="$(BclSourcesRoot)\System\Buffer.Mono.cs" />
Expand Down
8 changes: 8 additions & 0 deletions src/mono/System.Private.CoreLib/src/ILLink/ILLink.Descriptors.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -633,6 +633,14 @@
<method name="get_BaseDirectory"/>
</type>

<type fullname="System.AppDomain">
<method name="DoUnhandledException"/>
</type>

<type fullname="System.Diagnostics.Debugger">
<method name="Mono_UnhandledException"/>
Copy link
Member

@jkotas jkotas Jul 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

If Xamarin Android is calling this, it should be designed, exposed and tested as proper public API.

Copy link
Contributor

Choose a reason for hiding this comment

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

Android has been calling this method for over 10 years: https://github.com/xamarin/monodroid/commit/3e9de5a51b

Other related reading:

Yes, it should be designed, exposed, and tested. That said, this is what we had, and what worked.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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():

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@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

mono_add_internal_call_internal ("System.Diagnostics.Debugger::Mono_UnhandledException_internal",

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

</type>

<!-- mono_method_has_unmanaged_callers_only_attribute () -->
<!-- aot-compiler.c -->
<type fullname="System.Runtime.InteropServices.UnmanagedCallersOnlyAttribute"/>
Expand Down
14 changes: 14 additions & 0 deletions src/mono/System.Private.CoreLib/src/System/AppContext.Mono.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

namespace System
{
public static partial class AppContext
{
internal static void DoUnhandledException (object sender, UnhandledExceptionEventArgs args)
{
if (UnhandledException != null)
UnhandledException (sender, args);
}
}
}
15 changes: 15 additions & 0 deletions src/mono/System.Private.CoreLib/src/System/AppDomain.Mono.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

namespace System
{
public partial class AppDomain
{
#if TARGET_ANDROID
internal void DoUnhandledException (UnhandledExceptionEventArgs args)
{
AppContext.DoUnhandledException (this, args);
}
#endif
Comment on lines +8 to +13
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this called and why is it Android specific?

Copy link
Member Author

@BrzVlad BrzVlad Jul 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@marek-safar wrote:

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:

  1. Java [caller of Activity.onCreate()
  2. JNINativeWrapper-generated delegate instance
  3. A binding generated "marshal method", Activity.n_OnCreate_Landroid_os_Bundle_()
  4. 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.

Related? https://github.com/xamarin/xamarin-android/pull/4877/files?short_path=87e7d90#diff-87e7d904d49ad72bcc40356d159cf46a7508f28adf19a8cf2707d109f223adb7

Copy link
Contributor

Choose a reason for hiding this comment

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

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().

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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

Copy link
Member

Choose a reason for hiding this comment

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

Eventually, does it make sense to do something similar to XI?

Register:

https://github.com/xamarin/xamarin-macios/blob/2bee92225cf1ec3695cfdd9e60618a5f9bb7dfdf/src/ObjCRuntime/Runtime.CoreCLR.cs#L102

And this is the runtime API it's using

runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ObjectiveC/ObjectiveCMarshal.cs

Line 60 in e0024e2

public static unsafe void Initialize(

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's try to use the existing embedding API first before considering this PR.

}
}
10 changes: 10 additions & 0 deletions src/mono/System.Private.CoreLib/src/System/Diagnostics/Debugger.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,5 +41,15 @@ public static void Log(int level, string? category, string? message)
public static void NotifyOfCrossThreadDependency()
{
}

#if TARGET_ANDROID
[MethodImplAttribute (MethodImplOptions.InternalCall)]
private static extern void Mono_UnhandledException_internal (Exception ex);

internal static void Mono_UnhandledException (Exception ex)
{
Mono_UnhandledException_internal (ex);
}
#endif
}
}