Skip to content

fix: prevent redundant native exceptions on Android/CoreCLR #5127

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.

Sign up for GitHub

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

Draft
jpnurmi wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix: prevent redundant native exceptions on Android/CoreCLR #5127

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions integration-test/Directory.Build.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
<Project>
<!-- Don't include parent Directory.Build.targets -->

<!-- Needed for SetAndroidNdkPreload (buildTransitive doesn't apply via ProjectReference) -->
<Import Project="../src/Sentry/buildTransitive/Sentry.props" Condition="Exists('../src/Sentry/buildTransitive/Sentry.props')" />
<Import Project="../src/Sentry/buildTransitive/Sentry.targets" Condition="Exists('../src/Sentry/buildTransitive/Sentry.targets')" />
</Project>
12 changes: 8 additions & 4 deletions integration-test/android.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,12 @@ BeforeDiscovery {
}

$cases = @(
@{ configuration = 'Release' }
@{ configuration = 'Debug' }
@{ configuration = 'Release'; runtime = 'mono' }
@{ configuration = 'Release'; runtime = 'coreclr' }
@{ configuration = 'Debug'; runtime = 'mono' }
@{ configuration = 'Debug'; runtime = 'coreclr' }
)
Describe 'MAUI app (<dotnet_version>, <configuration>)' -ForEach $cases -Skip:(-not $script:emulator) {
Describe 'MAUI app (<dotnet_version>, <configuration>, <runtime>)' -ForEach $cases -Skip:(-not $script:emulator) {
BeforeAll {
$tfm = "$dotnet_version-android$(GetAndroidTpv $dotnet_version)"

Expand All @@ -38,10 +40,12 @@ Describe 'MAUI app (<dotnet_version>, <configuration>)' -ForEach $cases -Skip:(-
$rid = "android-$arch"

Write-Host "::group::Build Sentry.Maui.Device.IntegrationTestApp.csproj"
$useMonoRuntime = if ($runtime -eq 'mono') { 'true' } else { 'false' }
dotnet build Sentry.Maui.Device.IntegrationTestApp.csproj `
--configuration $configuration `
--framework $tfm `
--runtime $rid
--runtime $rid `
-p:UseMonoRuntime=$useMonoRuntime
| ForEach-Object { Write-Host $_ }
Write-Host '::endgroup::'
$LASTEXITCODE | Should -Be 0
Expand Down
8 changes: 8 additions & 0 deletions src/Sentry/Platforms/Android/SentrySdk.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,14 @@
o.ShutdownTimeoutMillis = (long)options.ShutdownTimeout.TotalMilliseconds;

var signalHandlerStrategy = options.Native.ExperimentalOptions.SignalHandlerStrategy;
if (signalHandlerStrategy == SignalHandlerStrategy.ChainAtStart
&& Type.GetType("Mono.RuntimeStructs") == null)
{
options.LogDebug(
"SignalHandlerStrategy.ChainAtStart is not compatible with .NET CoreCLR runtime. " +
"Falling back to SignalHandlerStrategy.Default.");
signalHandlerStrategy = SignalHandlerStrategy.Default;
}

Check failure on line 76 in src/Sentry/Platforms/Android/SentrySdk.cs

View check run for this annotation

@sentry/warden / warden: code-review

Inverted runtime detection logic prevents ChainAtStart from working on CoreCLR 

The condition checks for `Type.GetType("Mono.RuntimeStructs") == null` and falls back to Default, but this detects the CoreCLR runtime (not Mono). According to the PR description, `SignalHandlerStrategy.ChainAtStart` is supposed to work with CoreCLR and NOT with Mono. The condition is inverted - it should fall back when `Mono.RuntimeStructs` is found (i.e., `!= null`), not when it's absent. As written, the feature will never activate on CoreCLR where it's designed to work, and would activate on Mono where it's not compatible.

Check failure on line 76 in src/Sentry/Platforms/Android/SentrySdk.cs

View check run for this annotation

@sentry/warden / warden: find-bugs

Inverted runtime detection causes ChainAtStart to be disabled on CoreCLR where it should work 

The condition `Type.GetType("Mono.RuntimeStructs") == null` evaluates to true when running on CoreCLR (not Mono), causing the code to fall back to `SignalHandlerStrategy.Default` on the exact runtime where `ChainAtStart` is supposed to work. According to the PR description, ChainAtStart 'only works with CoreCLR' and 'with Mono, one will have to opt in'. The log message also incorrectly states ChainAtStart is 'not compatible with .NET CoreCLR runtime' when it should say 'Mono runtime'. This bug defeats the purpose of the fix and prevents the duplicate exception prevention from working on CoreCLR.
if (signalHandlerStrategy == SignalHandlerStrategy.ChainAtStart
&& System.Environment.Version is { Major: 10, Minor: 0, Build: < 4 })
{
Expand Down
1 change: 1 addition & 0 deletions src/Sentry/buildTransitive/Sentry.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,6 @@
<_SentryTargetFrameworkVersion>$([MSBuild]::GetTargetFrameworkVersion($(TargetFramework)))</_SentryTargetFrameworkVersion>
<_SentryIsNet8OrGreater>$([MSBuild]::VersionGreaterThanOrEquals($(_SentryTargetFrameworkVersion), 8.0))</_SentryIsNet8OrGreater>
<_SentryIsNet9OrGreater>$([MSBuild]::VersionGreaterThanOrEquals($(_SentryTargetFrameworkVersion), 9.0))</_SentryIsNet9OrGreater>
<_SentryIsNet10OrGreater>$([MSBuild]::VersionGreaterThanOrEquals($(_SentryTargetFrameworkVersion), 10.0))</_SentryIsNet10OrGreater>
</PropertyGroup>
</Project>
16 changes: 16 additions & 0 deletions src/Sentry/buildTransitive/Sentry.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,22 @@
</ItemGroup>
</Target>

<!-- Preload NDK to ensure sentry-native's signal handlers are installed before .NET runtime's.
Disabled on Mono until: https://github.com/dotnet/runtime/pull/125835 -->
<Target Name="SetAndroidNdkPreload"
BeforeTargets="GetAssemblyAttributes"
Condition="'$(AndroidApplication)' == 'True'
and $([MSBuild]::GetTargetPlatformIdentifier('$(TargetFramework)')) == 'android'
and '$(_SentryIsNet10OrGreater)' == 'true'
and '$(UseMonoRuntime)' != 'true'">
<ItemGroup>
<AssemblyAttribute Include="Android.App.MetaData">
<_Parameter1>io.sentry.ndk.preload</_Parameter1>
<Value>true</Value>
</AssemblyAttribute>
</ItemGroup>
</Target>

<!-- Upload Android ProGuard mapping file to Sentry after the build. -->
<Target Name="UploadAndroidProGuardMappingFileToSentry" AfterTargets="Build" DependsOnTargets="PrepareSentryCLI"
Condition="'$(SentryCLI)' != '' and '$(SentryUploadAndroidProGuardMapping)' == 'true' And '$(AndroidProguardMappingFile)' != ''">
Expand Down
Loading