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.

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

Document handling uncaught exceptions in WinUI 3 applications with AOT enabled #2778

Closed
jamescrosswell opened this issue Nov 2, 2023 · 19 comments
Assignees
Milestone

Comments

@jamescrosswell
Copy link
Collaborator

jamescrosswell commented Nov 2, 2023

To get trimming working we had to remove some code that used reflection to automatically register an unhandled exception handler in WinUI applications, if Trimming is enabled (typically when doing AOT compilation). Further details in #2732

This means we have to document how users can send unhandled exceptions to Sentry
- getsentry/sentry-docs#8557
- getsentry/sentry-docs#8602

@jamescrosswell jamescrosswell added this to the 4.0.0 milestone Nov 2, 2023
@tipa
Copy link

tipa commented Nov 16, 2023

Interested in the guidance. Currently I am using the snippet below, which seems to work. Is it the right way to do it?

UnhandledException += (s, e) =>
{
    e.Exception.SetSentryMechanism(
        "Microsoft.UI.Xaml.UnhandledException",
        "This exception was caught by the Windows UI global error handler. " +
        "The application likely crashed as a result of this exception.",
        e.Handled);
    Sentry.SentrySdk.CaptureException(e.Exception);
    Sentry.SentrySdk.Flush();
};

@jamescrosswell
Copy link
Collaborator Author

Interested in the guidance. Currently I am using the snippet below, which seems to work. Is it the right way to do it?

What we have in Sentry 3.x is the reflection based equivalient of this:

The exception handler it wires up is a bit more convoluted. In addition to what you're doing, there's a workaround for not sending through a stack trace, in some circumstances, setting a mechanism and also flushing events in the face of an irrecoverable error... but broad strokes, it looks like what you're doing is all good.

This issue (once completed) should provide a way for you to hook up the WinUIUnhandledExceptionHandler that we provide... currently this is private, so there's no easy way to do this. If you don't have any urgent issues with your current solution then, I'd say keep it how it is and, in a few weeks, we'll provide a way for you to point unhandled exceptions at Sentry's event handler (so one less thing for you to maintain).

@bruno-garcia
Copy link
Member

This WinUI stuff done with reflection was a hack originally done because stuff in .NET was broken. Did they fix this in the latest versions?

@jamescrosswell
Copy link
Collaborator Author

This WinUI stuff done with reflection was a hack originally done because stuff in .NET was broken. Did they fix this in the latest versions?

Is this related?

@bruno-garcia
Copy link
Member

bruno-garcia commented Nov 20, 2023

I assume so, not sure.

Could we get away without the hack and just document this?

Troubleshooting page:

WinUI customers on version of WinUI below XYZ: ...

@jamescrosswell jamescrosswell changed the title Allow SDK users to manually register WinUiUnhandledExceptionHandler Document handling uncaught exceptions in WinUI 3 applications with AOT enabled Nov 22, 2023
@tipa
Copy link

tipa commented Nov 24, 2023

I published my app with trimming enabled and just observed this potential bug:

  • My app crashes, Sentry catches the exception (using the code snipped I posted above) and sends it - I can see the crash report in the dashboard
  • I see another, unexpected exception showing up in the dashboard:
System.InvalidOperationException: JsonTypeInfo metadata for type 'WinRT.ExceptionHelpers+__RestrictedErrorObject' was not provided by TypeInfoResolver of type 'Sentry.Internal.Extensions.SentryJsonContext'. If using source generation, ensure that all root types passed to the serializer have been annotated with 'JsonSerializableAttribute', along with any types that might be serialized polymorphically.
  ?, in void ThrowHelper.ThrowInvalidOperationException_NoMetadataForType(Type, IJsonTypeInfoResolver)
  ?, in JsonTypeInfo JsonSerializer.GetTypeInfo(JsonSerializerContext, Type)
  ?, in byte[] JsonSerializer.SerializeToUtf8Bytes(object, Type, JsonSerializerContext)
  ?, in byte[] JsonExtensions.InternalSerializeToUtf8Bytes(object)+AotSerializeToUtf8Bytes()
  ?, in byte[] JsonExtensions.InternalSerializeToUtf8Bytes(object)
  ?, in void JsonExtensions.WriteDynamicValue(Utf8JsonWriter, object, IDiagnosticLogger)
  ?, in void JsonExtensions.WriteDynamic(Utf8JsonWriter, string, object, IDiagnosticLogger)

Is this something that could be prevented on the SDK side?
To give additional context, I use these build properties in my app:

  <PropertyGroup Condition="'$(Configuration)' == 'Release'">
    <SelfContained>true</SelfContained>
    <PublishTrimmed>true</PublishTrimmed>
    <TrimMode>full</TrimMode>
    <BuiltInComInteropSupport>true</BuiltInComInteropSupport>
  </PropertyGroup>

Additionally, I exclude some assemblies/types from trimming, using TrimmerRootDescriptor:

<linker>
  <assembly fullname="WinRT.Runtime" />
  <assembly fullname="Microsoft.WinUI">
    <type fullname="Microsoft.UI.Xaml.Markup.*" />
  </assembly>
</linker>

SDK version 4.0.0-beta.1

@vaind
Copy link
Collaborator

vaind commented Nov 26, 2023

Maybe we could also take a step back and look at this from a different angle:

Originally, the idea was to replace all code present in the main Sentry package that is not trimmable with a trimmable alternative. This has proven problematic in other pars, e.g. Ben.Demystifier integration, JSON serialization for old TFMs, etc.
Therefore, it may make sense to roll back the change to the WinUI exception handler, because WinUI doesn't support trimming itself, so there's no point in trying to do that here.

@tipa
Copy link

tipa commented Nov 26, 2023

because WinUI doesn't support trimming itself

Not sure if I understand that statement correctly, but I was able to enable full trimming in my WinUI/WinAppSDK apps (with the TrimmerRootDescriptor exclusions mentioned above) and they appear to run just fine.

@jamescrosswell
Copy link
Collaborator Author

System.InvalidOperationException: JsonTypeInfo metadata for type 'WinRT.ExceptionHelpers+__RestrictedErrorObject' was not provided by TypeInfoResolver of type 'Sentry.Internal.Extensions.SentryJsonContext'. If using source generation, ensure that all root types passed to the serializer have been annotated with 'JsonSerializableAttribute', along with any types that might be serialized polymorphically.

I think this is because Sentry is trying to serialize WinRT.ExceptionHelpers+__RestrictedErrorObject and send this through to Sentry.io as part of the exception context. However, with trimming enabled, that's only possible if some additional context is provided. This is the relevant bit of the changelog for the 4.0.0-alpha.0 release:

Reflection cannot be leveraged for JSON Serialization and you may need to use SentryOptions.AddJsonSerializerContext to supply a serialization context for types that you'd like to send to Sentry (e.g. in the Span.Context). (#2732, #2793)

We can register some of the commong/foreseeable types that will need to be serialized in the default context that comes with Sentry:

[JsonSerializable(typeof(GrowableArray<int>))]
[JsonSerializable(typeof(Dictionary<string, bool>))]
[JsonSerializable(typeof(Dictionary<string, object>))]
internal partial class SentryJsonContext : JsonSerializerContext
{
}

In the case of the particular type reported in your error message, we can't easily do that though I don't think, since it's sealed internal (and part of an assembly we can't take a dependency on in the Sentry Core package anyway):
https://github.com/microsoft/CsWinRT/blob/71169932920500310921b4c3fadba1e5a99c6138/src/WinRT.Runtime/ExceptionHelpers.cs#L396C1-L421C10

More generally, there are an unlimited number of system and user defined exceptions that could potentially be thrown and need to be serialized, and neither the Sentry SDK maintainers nor SDK users are able to know, in advance, which exceptions might come up. Implicitly, we can only discover these things at runtime with trimming enabled. We might need a more elegant fallback mechanism when serialization fails in these circumstances.

@tipa are you able to provide a minimal reproducable example of this in a demo app you could share? Or just a code snippet that will reliably reproduce the two exceptions you observed?

@tipa
Copy link

tipa commented Nov 27, 2023

In my particular case, I ran into this exception: microsoft/WindowsAppSDK#3002

new DisplayRequest().RequestActive(); crashed on a users machine, running an older Windows 11 installation. I did not see that crash locally as I have my Windows 11 fully updated.

The original crash was captured by Sentry correctly (from what I can see):

System.Runtime.InteropServices.COMException: null
  ?, in void ExceptionHelpers.ThrowExceptionForHR(int)+Throw()
  ?, in void ExceptionHelpers.ThrowExceptionForHR(int)
  ?, in void IDisplayRequestMethods.RequestActive(IObjectReference)
  ?, in void DisplayRequest.RequestActive()
  ?, in void MainWindow.PreventLockscreenToggle_Toggled(object sender, RoutedEventArgs e)
  ?, in Delegate EventState.GetEventInvoke()+(object sender, RoutedEventArgs e) => { }
  ?, in int RoutedEventHandler.Do_Abi_Invoke(IntPtr thisPtr, IntPtr sender, IntPtr e)
  ?, in void ExceptionHelpers.ThrowExceptionForHR(int)+Throw()
  ?, in void ExceptionHelpers.ThrowExceptionForHR(int)
  ?, in void IToggleSwitchMethods.set_IsOn(IObjectReference, bool)
  ?, in void ToggleSwitch.set_IsOn(bool)
  ?, in void MainWindow.InitSettingsView()
  ?, in new MainWindow()
  ?, in void App.OnLaunched(LaunchActivatedEventArgs)
  ?, in void Application.Microsoft.UI.Xaml.IApplicationOverrides.OnLaunched(LaunchActivatedEventArgs)
  ?, in int IApplicationOverrides.Do_Abi_OnLaunched_0(IntPtr thisPtr, IntPtr args)

image

And for every InteropServices.COMException crash report I also get this System.InvalidOperationException caused by failed serialization, so I think there must be a connection between the two.

This is an example app that also calls new DisplayRequest().RequestActive(); and should crash on older Windows 11 versions. You can also see what build properties I am using in the csproj file
test.zip

@jamescrosswell
Copy link
Collaborator Author

Thanks @tipa - I can see in the exception that you received in Sentry, there are a couple of blank properties (RestrictedCapabilitySid, RestrictedErrorReference and __HasRestrictedLanguageErrorObject) - those are probably the ones that are causing the failed serialization exceptions.

What would you expect Sentry to be doing in this scenario? Do you expect/want to be made aware of the serialization failures or would you like to be able to turn these off? If you'd like to know about them, how would you prefer to be made aware of these?

@tipa
Copy link

tipa commented Nov 27, 2023

@jamescrosswell I don't know what data these properties would normally contain, but at least for this particular crash report I was able to identify what's causing the exception with the stack trace alone. I don't have a problem with these properties being shown as blanks or not being shown at all. But it would be great if Sentry wouldn't log the System.InvalidOperationException as that's not an exception I can fix in my code.

@jamescrosswell
Copy link
Collaborator Author

So digging into this I think it might be something entirely different. I tried creating/throwing an exception with a property that I knew couldn't be serialized with AOT switched on and I only get one exception coming through to Sentry. The property that can't be serialized is ignored, but no other exceptions are created.

I think this might genuinely be a second exception in your case... perhaps related to this?

My windows box is also fully patched so I can't use the sample you provided. 🤔

@tipa
Copy link

tipa commented Nov 28, 2023

Thanks for investigating this problem.
What I have done now is try-catched the exception:

try { displayRequest.RequestActive(); } catch (Exception ex) { Sentry.SentrySdk.CaptureException(ex); }

The exception is logged with handled=true and no serialization exception is logged any more. If something similar happens again for another exception, I'll let you know.

I knew couldn't be serialized with AOT switched on

just to clarify: AOT is not switched on in my app - but full trimming is (with the entire WinRT.Runtime assembly excluded from it)

@jamescrosswell
Copy link
Collaborator Author

OK, I'll close this issue (which was related to documenting how to wire up an exception handler for WinUI applications).

If anything else comes up, let's create a new issue for it. Thanks for all your feedback @tipa !

PS: Yes, I should have said Trimming (not AOT... it's the trimming bit that trips up serialization).

@tipa
Copy link

tipa commented Nov 28, 2023

Ah - I just notice that the docs have been added - will adapt my code accordingly.
Just realized a small typo here, probably shouldn't be called "UWP"
https://github.com/getsentry/sentry-docs/blob/f20d64b3f4a278341e614acbca562ed749b393a6/src/platforms/dotnet/guides/winui/index.mdx#L102

Also, the docs docs make the differentiation between JIT and AOT compiled apps and say that manually registering for the UnhandledException is only needed for AOT apps. I think it would be correct if you make the differentiation between non-trimmed & trimmed app. It is mentioned that configuring the UnhandledException handler is also required when trimming is enabled, but this is easy to skip, especially when you stop reading at If you are using Just-In-Time (JIT) compilation then all you need to do is initialize the SDK with SentrySdk.Init

The docs also state Get a reference to the exception, because the Exception property is cleared when accessed. which I don't think is the case, at least in my tests.

Anyways, thanks so much for supporting WinUI/Trimming/AOT!

@tipa
Copy link

tipa commented Nov 28, 2023

Sorry if this doesn't belong in this thread (I can open a new issue if this is preferred), but is the manual exception setup really necessary even for trimmed apps? Isn't it possible for Sentry to just something like this in the initialization process?

Application.Current.UnhandledException += (s, e) =>
{
        if (e.Exception is not Exception exception) return;
        exception.Data[Sentry.Protocol.Mechanism.HandledKey] = false;
        exception.Data[Sentry.Protocol.Mechanism.MechanismKey] = "Application.UnhandledException";
        Sentry.SentrySdk.CaptureException(exception);
        Sentry.SentrySdk.Flush(TimeSpan.FromSeconds(2));
};

@jamescrosswell
Copy link
Collaborator Author

Isn't it possible for Sentry to just something like this in the initialization process?

I don't think so no. You might be able to do that from your application but not from our NuGet package.

What needs wiring up is the equivalent of this:

Microsoft.UI.Xaml.Application.Current.UnhandledException += WinUIUnhandledExceptionHandler;

In order to do that we need to:

  1. Either use reflection (which we can't do with trimming)
  2. Or take a dependency on Microsoft.UI.Xaml
    • We can't do in the core Sentry package
    • We could create a separate Sentry.WinUI package for this but it's a lot of effort to maintain for a pretty simple code snippet that users wire up once... thus the decision to write documentation rather than code.

@jamescrosswell
Copy link
Collaborator Author

Ah - I just notice that the docs have been added - will adapt my code accordingly. Just realized a small typo here, probably shouldn't be called "UWP" https://github.com/getsentry/sentry-docs/blob/f20d64b3f4a278341e614acbca562ed749b393a6/src/platforms/dotnet/guides/winui/index.mdx#L102

Also, the docs docs make the differentiation between JIT and AOT compiled apps and say that manually registering for the UnhandledException is only needed for AOT apps. I think it would be correct if you make the differentiation between non-trimmed & trimmed app.

Thanks @tipa - I've created a PR to update accordingly:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Archived in project
Development

No branches or pull requests

4 participants