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

Suppress Native SIGSEGV signal errors on Android #3903

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

Conversation

jamescrosswell
Copy link
Collaborator

@jamescrosswell jamescrosswell commented Jan 17, 2025

See #3902:

Resolves #3837
Resolves #3898
Resolves #3935

This PR uses the Native BeforeSend callback on Android to intercept and suppress SIGSEGV errors, as these duplicate managed NullReferenceExceptions.

There's a small risk we're suppressing native SIGSEGV errors that are not wrapped by managed exceptions, but there's no easy way around that for the time being. This is the short term workaround described in #3902

In case SDK users see this as a problem though, we've added an option to let them control whether or not to suppress SIGSEGV Segfault errors.

Usage

        var builder = MauiApp.CreateBuilder()
            .UseMauiApp<App>()
            .UseSentry(options =>
            {
                // All the standard options

#if ANDROID
                // Suppress native SIGSEGV Segfault errors
                options.Android.SuppressSegfaults = true;
#endif
            })

This should be documented. See:

@jamescrosswell jamescrosswell marked this pull request as ready for review January 19, 2025 20:42
Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

It's quite unfortunate since now users will be blind of native crashes.

CHANGELOG.md Outdated Show resolved Hide resolved
@bruno-garcia
Copy link
Member

@filipnavara any chance you can think of a better way?

@filipnavara
Copy link
Contributor

@filipnavara any chance you can think of a better way?

I'll think about it. That said, I observed it and never really considered it a bug... NRE in .NET is almost never intentional, so this essentially just caused double reporting. Depending on .NET runtime used (NativeAOT vs MonoVM) this may contain some useful information.

@jamescrosswell jamescrosswell changed the title Disable the NDK by default Suppress Native SIGSEGV signal errors Feb 3, 2025
@jamescrosswell jamescrosswell changed the title Suppress Native SIGSEGV signal errors Suppress Native SIGSEGV signal errors on Android Feb 3, 2025
@jamescrosswell jamescrosswell linked an issue Feb 3, 2025 that may be closed by this pull request
@jamescrosswell
Copy link
Collaborator Author

NRE in .NET is almost never intentional, so this essentially just caused double reporting. Depending on .NET runtime used (NativeAOT vs MonoVM) this may contain some useful information.

Thanks @filipnavara - I think that's a perfectly valid perspective.

Currently this PR simply suppresses all SIGSEGV exceptions... which runs the risk of suppressing some that come from native code (and aren't reported as NREs). We've got two sub-optimal options at this point:

  1. Let the SIGSEGV errors bubble up (possibly duplicating NREs)
  2. Suppress the SIGSEGV errors... prevents duplicates but possibly suppresses genuine native errors

I think we have to pick one of those as the default behaviour, at the very least. But potentially we could expose an option to let SDK users override the default choice we make, depending on whether they prefer to catch all/every error (but possibly some twice) or whether they're happy to let a few slip through the cracks but prevent duplicates.

@jamescrosswell jamescrosswell marked this pull request as ready for review February 5, 2025 02:06
@aritchie
Copy link
Collaborator

aritchie commented Feb 6, 2025

What about providing an option to control whether or not segfaults are suppressed. If they are annoying and not providing value to the user, disable them otherwise leave them on. In my experience, segfaults were a rare thing so I would still want to see them in most cases.

@jamescrosswell
Copy link
Collaborator Author

What about providing an option to control whether or not segfaults are suppressed. If they are annoying and not providing value to the user, disable them otherwise leave them on. In my experience, segfaults were a rare thing so I would still want to see them in most cases.

I'm of two minds about this... not so much surfacing an option (which I think is probably a good idea) but what the default should be for that option (to suppress or not to suppress).

Any managed NullReferenceException is resulting in a SIGSEGV Segfault being captured by our native SDK... so these are not all that uncommon (evidenced by the fact that we've had quite a few users reach out about this). Those users obviously find it irritating to get both a NullReferenceException and a SIGSEGV error reported/captured and I'm sure we'd get less issues in GitHub if the default behaviour was to suppress these.

On the other hand, I think it would be safer to capture these so that people have visibility, by default... and let them deliberately suppress if they want to. Then at least they'd be aware of the dangers/risks in doing so. So maybe a bit more work for us, but safer for SDK users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants