[tests] enable SubscribeToAppDomainUnhandledException in .NET 6 #6119
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
Unexpectedly, the SubscribeToAppDomainUnhandledException test is failing: Recall that the SubscribeToAppDomainUnhandledException test explicitly write out yet the logcat we see does not contain @grendello: could you please investigate? Does |
Contributor
Contributor
|
I looked at the test enabled in this PR, and c1a2ee7 cannot make it work because it deals with Java-to-Managed exception propagation, not with exceptions thrown from the managed code, like in the failing test. The dotnet issue is still there and I don't think this is something that Xamarin.Android should fix (e.g. by intercepting an "uncaught" exception somehow and calling |
Member
Author
|
Do we need to reopen dotnet/runtime#44526? |
Contributor
Yep |
Contributor
|
With @jonpryor's change we now see the native After that the runtime calls The fatal error is reported by Mono, perhaps that's what eventually prevents the event from being raised? Perhaps the runtime is aborted before it even gets to the stage where it would raise the event? |
Contributor
|
This should fix the Thanks @thaystg! |
Context: c1a2ee7 Context: dotnet/runtime#44526 If c1a2ee7 is working now, we should be able to enable this test.
Context: dotnet/runtime#44526 (comment) Context: dotnet/runtime#44526 (comment) Context: start: https://discord.com/channels/732297728826277939/732297837953679412/869330822262587392 Context: end? https://discord.com/channels/732297728826277939/732297837953679412/869343082552893440 On .NET 6, `JNIEnv.mono_unhandled_exception` is `monodroid_debugger_unhandled_exception()`, which calls `mono_debugger_agent_unhandled_exception()`; see also e4debf7. The problem is that in our current world order of "Mono components" (0f7a0cd), if the debugger isn't used, then we get "debugger stubs" for the mono debugger agent, which turns `mono_debugger_agent_unhandled_exception()` into an [*assertion*][0]: static void stub_debugger_unhandled_exception (MonoException *exc) { g_assert_not_reached (); } The result is that when an exception is thrown, *before* the `AppDomain.UnhandledException` event can be raised, the runtime dies in a horrible flaming assertion death: E andledexceptio: * Assertion: should not be reached at /__w/1/s/src/mono/mono/component/debugger-stub.c:175 Avoid this issue by checking `Debugger.IsAttached` *before* calling `monodroid_debugger_unhandled_exception()`. Additionally, remove some obsolete comments: .NET 6 couldn't resolve `Debugger.Mono_UnhandledException()` because .NET 6 never had it, so the linker was right to warn about its absence. [0]: https://github.com/dotnet/runtime/blob/16b456426dfb5212a24bfb78bfd5d9adfcc95185/src/mono/mono/component/debugger-stub.c#L172-L176
jonathanpeppers
force-pushed
the
subscribetoappdomainunhandledexception
branch
from
51f1b8d to
87849be
Compare
Member
Author
|
@grendello we should have dotnet/runtime#56380 now. But it's still failing:
Do we need 87849be? Maybe we don't actually need to check |
Contributor
I think we need both. The runtime fix was for the earlier error, 87849be was to fix an issue that was masked by the dotnet/runtime fix. |
Contributor
|
Back on the unit test review front…
However, what's odd is that startup-logcat.log kinda contains that message! Apparently @thaystg : is this expected behavior in .NET 6+? |
Member
|
I really don't know, I tried to run on android sample on dotnet/runtime repo, something like this, without using And I got this on logcat: So, I think you cannot believe that the behavior would be print in one line. |
Contributor
|
What might be happening is
Every time Thus, if the This is "less than ideal behavior", shall we say? Regardless, to fix it, we'd have to "fix" dotnet/runtime…whatever that means/looks like. Which means we instead need to look into fixing the test itself? |
Context: dotnet#6119 (comment) Looks like under .NET 6, `Console.WriteLine(s)` output isn't guaranteed to be on a single (assuming `s` doesn't contain a newline). This causes the existing `SubscribeToAppDomainUnhandledException()` check to fail, because while it *wants* # Unhandled Exception: sender=RootDomain; e.IsTerminating=True; e.ExceptionObject=System.Exception: CRASH! it's *actually* getting: # Unhandled Exception: sender=RootDomain; e.IsTerminating=True; e.ExceptionObject=System.Exception : CRASH! Try to "add magic" around the line check so that expected output can span multiple lines. Let's see if that fixes things.
…nhandledexception
Contributor
|
The run MSBuildDeviceIntegration On Device - macOS - One .NET stage is consistently failing, but in a manner that means the Tests tab doesn't show any failures: I saw this both before and after merging origin/main to this branch. I can only assume it's caused by c37f58a, as it didn't happen before that commit, but I don't see how c37f58a would be responsible. :-( |
Contributor
|
I should learn to read better: This is within |
Contributor
|
…and once again, it would be nice if I could read… Expected text: Actual text: In particular, note the |
Contributor
|
Finally, it works!!!! |
Contributor
[tests] enable SubscribeToAppDomainUnhandledException in .NET 6 (#6119)
Context: c1a2ee70214e86757541b5759c9ed54941bd4680
Context: https://github.com/dotnet/runtime/issues/44526
Context: https://github.com/dotnet/runtime/issues/44526#issuecomment-886998881
Context: https://github.com/dotnet/runtime/issues/44526#issuecomment-887052471
Context: start: https://discord.com/channels/732297728826277939/732297837953679412/869330822262587392
Context: end? https://discord.com/channels/732297728826277939/732297837953679412/869343082552893440
Context: https://github.com/dotnet/runtime/issues/57304
Now that we are calling `mono_unhandled_exception()` when an unhandled
exception is reported (c1a2ee70), we should be able re-enable the
`InstallAndRunTests.SubscribeToAppDomainUnhandledException()` unit
test on .NET 6, which was disabled in 6e3e3831af.
What seemed like it should have been a 1-line removal ballooned a bit
due to a confluence of factors:
1. Debugger component stubs, and
2. .NET 6 `Console.WriteLine()` behavior.
On .NET 6, `JNIEnv.mono_unhandled_exception()` is
`monodroid_debugger_unhandled_exception()`, which calls
`mono_debugger_agent_unhandled_exception()`; see also e4debf72.
The problem is that in our current world order of "Mono components"
(0f7a0cde), if the debugger isn't used, then we get "debugger stubs"
for the mono debugger agent, which turns
`mono_debugger_agent_unhandled_exception()` into an [*assertion*][0]:
static void
stub_debugger_unhandled_exception (MonoException *exc)
{
g_assert_not_reached ();
}
The result is that when an exception is thrown, *before* the
`AppDomain.UnhandledException` event can be raised, the runtime dies
in a horrible flaming assertion death:
E andledexceptio: * Assertion: should not be reached at /__w/1/s/src/mono/mono/component/debugger-stub.c:175
Avoid this issue by checking `Debugger.IsAttached` *before* calling
`monodroid_debugger_unhandled_exception()`.
Additionally, remove some obsolete comments: .NET 6 couldn't resolve
`Debugger.Mono_UnhandledException()` because .NET 6 never had it, so
the linker was right to warn about its absence.
The problem with .NET 6 & `Console.WriteLine()` is that when format
strings are used, the output may be line-wrapped in unexpected ways;
see also dotnet/runtime@#57304. Additionally, the `sender` parameter
value differs under .NET 6. These two issues broke our unit test;
we *expected* `adb logcat` to contain:
# Unhandled Exception: sender=RootDomain; e.IsTerminating=True; e.ExceptionObject=System.Exception: CRASH
Under .NET 6, `adb logcat` *instead* contained:
# Unhandled Exception: sender=System.Object; e.IsTerminating=True; e.ExceptionObject=System.Exception
: CRASH
Yes, `: CRASH` was on a separate `adb logcat` line.
Fix the `SubscribeToAppDomainUnhandledException()` unit test so that
`adb logcat` messages can now span multiple lines (which is sure to
cause all sorts of GC garbage!), and update the expected message so
that we look for the right message under legacy & .NET 6.
Co-authored-by: Jonathan Pryor <jonpryor@vt.edu>
[0]: https://github.com/dotnet/runtime/blob/16b456426dfb5212a24bfb78bfd5d9adfcc95185/src/mono/mono/component/debugger-stub.c#L172-L176 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context: c1a2ee7
Context: dotnet/runtime#44526
If c1a2ee7 is working now, we should be able to enable this test.