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

fix: prevent event duplication when JavaBackgroundThread crashes occur #3756

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

Conversation

bricefriha
Copy link
Collaborator

Reported on #3545 , this bug yielded two events when JavaBackgroundThread crashes occurred: one UnhandledException with no Logcat attachments and no scope attachments and one AppDomain.UnhandledException with all expected attachments.

This change prevents the UnhandledException event to popup.
The reason why we are choosing the event with UnhandledException mechanism over AppDomain.UnhandledException is for consistency purposes, as other Java crashes send a single AppDomain.UnhandledException event.

…avaBackgroundThread crashes from creating duplicated event: one UnhandledException one AppDomain.UnhandledException
…d when getting the value to avoid parsing errors
@bricefriha bricefriha marked this pull request as ready for review November 18, 2024 17:31
@bricefriha
Copy link
Collaborator Author

Since it's on java crashes, I couldn't find a way to add tests

else
{
// Prevent JavaBackgroundThread crashes from creating duplicated event: one UnhandledException one AppDomain.UnhandledException
o.BeforeSend = new BeforeSendCallback(((_, _) => null), options, o);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't this just tell the Android SDK to discard all exceptions? Are there no circumstances in which we want the Android SDK to catch exceptions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Are there no circumstances in which we want the Android SDK to catch exceptions?

In this case, they can set an options.Native.EnableBeforeSend to true. But I can't think of any use case in which they would since the event thrown by dotnet has a more detailed set of info

Copy link
Collaborator

Choose a reason for hiding this comment

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

But what about java exceptions that don't come from a JavaBackgroundThread? This would discard those too right? It's just discarding all and any events that get captured by the Java SDK.

And if the user has set any custom BeforeSend callback, this workaround wouldn't work right???

Copy link
Collaborator Author

@bricefriha bricefriha Nov 22, 2024

Choose a reason for hiding this comment

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

But what about java exceptions that don't come from a JavaBackgroundThread? This would discard those too right? It's just discarding all and any events that get captured by the Java SDK.

All Java exceptions event throws a dotnet event, as far as I can tell.

if the user has set any custom BeforeSend callback, this workaround wouldn't work right???

it would work as long as Native.EnableBeforeSend is not set to true. if it's set to true, it means they technically want the Java event to appear

@jamescrosswell
Copy link
Collaborator

Since it's on java crashes, I couldn't find a way to add tests

@bricefriha how did you test it manually?

@bricefriha
Copy link
Collaborator Author

how did you test it manually?

I added an attachment at scope configuration and generated a JavaBackgroundThread crash and checked Sentry if we have only one event and all the attachments

I was trying to make a device test, but that would crash the entire test, since it's an unhandled exception

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

Successfully merging this pull request may close these issues.

3 participants