Skip to content

fix: Safe Execute on JVM#2215

Merged
bruno-garcia merged 8 commits intomainfrom
fix/android-jni-shenanigans
Jun 25, 2025
Merged

fix: Safe Execute on JVM#2215
bruno-garcia merged 8 commits intomainfrom
fix/android-jni-shenanigans

Conversation

@bitsandfoxes
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

@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
Copy Markdown
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