fix: SentryOptions.Native.SuppressSignalAborts and SuppressExcBadAccess on iOS

[jpnurmi]

This PR fixes a regression in SentryOptions.Native.SuppressExcBadAccess and SentryOptions.Native.SuppressSignalAborts. Let ProcessOnBeforeSend filter out SIGABRT and EXC_BAD_ACCESS sent early on startup during the Cocoa SDK init, even before the .NET Hub instance has been created.

Fixes: #4520

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.42%. Comparing base (3f12094) to head (256361c).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4521   +/-   ##
=======================================
  Coverage   73.42%   73.42%           
=======================================
  Files         479      479           
  Lines       17506    17506           
  Branches     3480     3480           
=======================================
  Hits        12854    12854           
  Misses       3773     3773           
  Partials      879      879           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jamescrosswell]

When would this be a problem? If the SDK is disabled, Sentry shouldn't be capturing any events at all right (either in the .NET or the Cocoa SDK)?

[jpnurmi]

It's a problem on startup when the previous run crashed with a duplicate SIGABRT or EXC_BAD_ACCESS that needs to be suppressed. Even if the .NET SDK is not (yet) in a fully initialized state, ProcessOnBeforeSend needs to return null for the suppression mechanism to work:

static partial class SentrySdk
{
    internal static IHub InitHub(SentryOptions options)
    {
        // ...

        if (options.InitNativeSdks)
        {
            InitSentryCocoaSdk(options); // <== ProcessOnBeforeSend for SIGABRT/EXC_BAD_ACCESS from the previous run
        }

        // We init the hub after native SDK in case the native init needs to adapt some options.
        var hub = new Hub(options);
        // ...
        return hub;
    }
}

[jamescrosswell]

I see... subtle. So the Cocoa SDK is already sending events before the .NET SDK has even been initialised!

// We init the hub after native SDK in case the native init needs to adapt some options.

I wonder what that's all about. @vaind do you recall (git blames you for that line 😜)

[vaind]

I don't recall specifically but what this is trying to get along, IMO, is that the InitSentryCocoaSdk() (in case of iOS) must be called before new Hub(options); because the former changes options which the latter uses.

[jamescrosswell]

Hm, thanks @vaind. The only managed options that get set by the Cocoa SDK are these:

sentry-dotnet/src/Sentry/Platforms/Cocoa/SentrySdk.cs

Lines 15 to 17 in cc613c9

	// Set default release and distribution
	options.Release ??= GetDefaultReleaseString();
	options.Distribution ??= GetDefaultDistributionString();

sentry-dotnet/src/Sentry/Platforms/Cocoa/SentrySdk.cs

Lines 154 to 158 in cc613c9

	// Set options for the managed SDK that depend on the Cocoa SDK. (The user will not be able to modify these.)
	options.AddEventProcessor(new CocoaEventProcessor());
	options.CrashedLastRun = () => SentryCocoaSdk.CrashedLastRun;
	options.EnableScopeSync = true;
	options.ScopeObserver = new CocoaScopeObserver(options);

I guess we'd want to detect the release and distribution before we initialise the .NET SDK Hub... and possibly wire up the CrashedLastRun handler (although I'm not sure why we delegate to the Cocoa SDK for this - Matt implemented that in #1849).

Ultimately the BeforeX handlers for whatever SDK we initialise 2nd may not be executed for the first few events that get sent... unless we add some concurrency mechanism to delay sending events until both SDKs are initialised. Maybe in the future we could consider that, if this ever becomes a problem.

As an aside, we're overwriting CrashedLastRun, EnableScopeSync and ScopeObserver here, so any user configured values for these will get overwritten. If that's genuinely what we want, those options should probably be internal (not public). I'll create a separate issue for that.

[vaind]

these being public may be related to Unity

[jamescrosswell]

LGTM - thanks again @jpnurmi !

[jurganson]

@jamescrosswell Do we know when this will be released? 🙂

[jamescrosswell]

@jurganson I'll start a 5.15.1 release now.

Additionally, it looks like this whole workaround might be unnecessary - I think @jpnurmi might have created a more reliable fix for this in the Sentry SDK for Cocoa:

• [WIP] fix: interop with managed Mono/CoreCLR runtimes sentry-cocoa#6193

That one will take a little bit longer as it would need to be reviewed, merged and released in the Cocoa SDK, then we need to bump the version of the Cocoa SDK that we're using in .NET. I'm guessing all of that will take at least a week or two.