Skip to content

fix: Safe Execute on JVM #2215

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

Merged
merged 8 commits into from
Jun 25, 2025
Merged

fix: Safe Execute on JVM #2215

merged 8 commits into from
Jun 25, 2025

Conversation

bitsandfoxes
Copy link
Contributor

@bitsandfoxes bitsandfoxes commented Jun 25, 2025

Basically a followup on the scope sync changes in #2107
Fixes #2164

Problem

The issue originates from the following scenario:

  1. User/plugin code runs on a background thread
  2. That thread attaches to the JVM to do some work
  3. It triggers a scope sync
  4. We end up in the SDK, on the same thread
  5. Since it's not on the main thread it attaches/detaches for the scope sync with the Android SDK
  6. The thread executing user/plugin code was detached prematurely, causing a crash with attempting to detach while still running code

The fix

Since we cannot verify whether the current thread is already attached or not we "have" to do the scope sync on a thread we actually control, so we can safely attach/detach. We only have to do that when not running on the main thread and we only do that on fire&forget scope sync calls. The rest of SentryJava is currently guaranteed - and now required - to run on the main thread.

Alternative attempts

I could not find a way to check whether the current thread is already attached to the JVM that works for all supported versions of Unity.

Check GetVersion

In versions 2022_and_newer it's possible to call AndroidJNI.GetVersion() which returns

  • When not attached: 0
  • When attached: the actual version

On older versions of Unity GetVersion always returns the actual version, creating a false positive.

Try-Catch AndroidException

I tried an approach by accessing AndroidObjects while not attached to capture the resulting AndroidException which again, worked on 2022_and_newer but straight up crashes the game on older versions.

Comment on lines +68 to +72
if (!MainThreadData.IsMainThread())
{
_logger?.LogError("Calling IsEnabled() on Android SDK requires running on MainThread");
return null;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is called by the SDK as part of the initialization process which is currently explicitly required to run on the main thread. I'm adding the check here now too so any future changes to the initialization flow (i.e. programmatic through user code) is not turning into a footgun.

Comment on lines +89 to +93
if (!MainThreadData.IsMainThread())
{
_logger?.LogError("Calling Init() on Android SDK requires running on MainThread");
return;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above.

Comment on lines +144 to +148
if (!MainThreadData.IsMainThread())
{
_logger?.LogError("Calling GetInstallationId() on Android SDK requires running on MainThread");
return null;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above.

Comment on lines +177 to +181
if (!MainThreadData.IsMainThread())
{
_logger?.LogError("Calling CrashedLastRun() on Android SDK requires running on MainThread");
return null;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above.

Comment on lines +199 to +203
if (!MainThreadData.IsMainThread())
{
_logger?.LogError("Calling Close() on Android SDK requires running on MainThread");
return;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above.

Copy link
Collaborator

@vaind vaind left a comment

Choose a reason for hiding this comment

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

LGTM & nice writeup. 🤞 this really fixes the issue.

@bruno-garcia
Copy link
Member

CI flaked because GitHub got unicorns

Invoke-WebRequest: /home/runner/work/sentry-unity/sentry-unity/scripts/download-sentry-cli.ps1:23
Line |
  23 |      Invoke-WebRequest -Uri "$baseUrl$name" -OutFile $targetFile
     |      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     |           Unicorn! · GitHub            body {        
     | background-color: #f1f1f1;         margin: 0;         font-family:
     | "Helvetica Neue", Helvetica, Arial, sans-serif;       }       
     | .container { margin: 50px auto 40px auto; width: 600px; text-align:
     | center; }        a { color: #4183c4; text-decoration: none; }      
     | a:hover { text-decoration: underline; }        h1 { letter-spacing:
     | -1px; line-height: 60px; font-size: 60px; font-weight: 100; margin: 0px;
     | text-shadow: 0 1px 0 #fff; }       p { color: rgba(0, 0, 0, 0.5);
     | margin: 10px 0 10px; font-size: 18px; font-weight: 200; line-height:
     | 1.6em;}        ul { list-style: none; margin: 25px 0; padding: 0; }  
     | li { display: table-cell; font-weight: bold; width: 1%; }        .logo
     | { display: inline-block; margin-top: 35px; }       .logo-img-2x {
     | display: none; }       @media       only screen and
     | (-webkit-min-device-pixel-ratio: 2),       only screen and (  
     | min--moz-device-pixel-ratio: 2),       only screen and (    
     | -o-min-device-pixel-ratio: 2/1),       only screen and (       
     | min-device-pixel-ratio: 2),       only screen and (               
     | min-resolution: 192dpi),       only screen and (               
     | min-resolution: 2dppx) {         .logo-img-1x { display: none; }      
     | .logo-img-2x { display: inline-block; }       }        #suggestions {
     | margin-top: 35px;         color: #ccc;       }       #suggestions a {
     | color: #666666;         font-weight: 200;         font-size: [14](https://github.com/getsentry/sentry-unity/actions/runs/15878142374/job/44770897275?pr=2215#step:11:15)px;   
     | margin: 0 10px;       }                                       
     | No server is currently available to service your request.       Sorry
     | about that. Please try refreshing and contact us if the problem
     | persists.                Contact Support —         GitHub
     | Status —         @githubstatus

@bruno-garcia bruno-garcia merged commit 5f52e04 into main Jun 25, 2025
141 of 142 checks passed
@bruno-garcia bruno-garcia deleted the fix/android-jni-shenanigans branch June 25, 2025 14:43
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.

SIGABORT in Android NDK in version 3.2.0
3 participants