fix: Capture instance and class method [NSApplication _crashOnException] exceptions#7510
Conversation
Semver Impact of This PR🟢 Patch (bug fixes) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨
Bug Fixes 🐛
Internal Changes 🔧Deps
Samples
Other
🤖 This preview updates automatically when you update the PR. |
|
If calling the class function |
There was a problem hiding this comment.
Pull request overview
This PR fixes macOS NSException crash reports that were showing _crashOnException: in the stacktrace instead of the actual exception callstack. The root cause was twofold: (1) the async capture path never completed before abort() terminated the process, and (2) newer macOS versions call the class method instead of the instance method, so the original swizzle/override didn't work.
Changes:
- Replace async
captureCrashOnException:path with synchronousuncaughtExceptionHandlerto capture crash reports before process termination - Swizzle both class and instance methods of
_crashOnException:to support different macOS versions - Add instance method override in
SentryCrashExceptionApplicationfor the subclass approach
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Tests/SentryTests/SentryCrashExceptionApplicationTests.swift | Removed obsolete test for deleted async crash method |
| Tests/SentryTests/Integrations/SentryCrash/SentryUncaughtNSExceptionsTests.swift | Added tests for both class and instance method swizzling |
| Sources/Swift/Integrations/SentryCrash/SentryCrashIntegration.swift | Enabled swizzling of _crashOnException: method |
| Sources/Sentry/include/SentryUncaughtNSExceptions.h | Added public method declaration for new swizzling |
| Sources/Sentry/include/SentrySDK+Private.h | Removed obsolete async capture method declaration |
| Sources/Sentry/include/SentryCrashExceptionApplicationHelper.h | Removed obsolete _crashOnException: method declaration |
| Sources/Sentry/SentryUncaughtNSExceptions.m | Implemented dual swizzling for class and instance methods |
| Sources/Sentry/SentrySDKInternal.m | Removed implementation of obsolete async capture method |
| Sources/Sentry/SentryCrashExceptionApplicationHelper.m | Removed implementation of obsolete method and unused imports |
| Sources/Sentry/SentryCrashExceptionApplication.m | Added instance method override calling synchronous handler |
| Samples/SentrySampleShared/SentrySampleShared/SentrySDKWrapper.swift | Enabled uncaught exception reporting for testing |
| CHANGELOG.md | Added entry for the fix |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7510 +/- ##
========================================
Coverage ? 85.350%
========================================
Files ? 483
Lines ? 28785
Branches ? 12505
========================================
Hits ? 24568
Misses ? 4170
Partials ? 47
Continue to review full report in Codecov by Sentry.
|
itaybre
left a comment
There was a problem hiding this comment.
Thanks for fixing this, LGTM
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Class method swizzle stacks on repeated SDK starts
- Made
swizzleNSApplicationCrashOnExceptionidempotent withdispatch_onceso repeated SDK starts no longer stack class-method swizzles, and added a regression test covering repeated calls.
- Made
Or push these changes by commenting:
@cursor push ca91921bf6
Preview (ca91921bf6)
diff --git a/Sources/Sentry/SentryUncaughtNSExceptions.m b/Sources/Sentry/SentryUncaughtNSExceptions.m
--- a/Sources/Sentry/SentryUncaughtNSExceptions.m
+++ b/Sources/Sentry/SentryUncaughtNSExceptions.m
@@ -40,34 +40,37 @@
# if SENTRY_TEST || SENTRY_TEST_CI
# pragma clang diagnostic ignored "-Wunused-variable"
# endif
- // AppKit calls _crashOnException: when an exception is caught during CATransaction flush
- // or view layout, bypassing reportException: entirely. Depending on macOS version, AppKit
- // may call the class method +[NSApplication _crashOnException:] or the instance method
- // -[NSApp _crashOnException:], so we swizzle both.
- SEL selector = NSSelectorFromString(@"_crashOnException:");
+ static dispatch_once_t onceToken;
+ dispatch_once(&onceToken, ^{
+ // AppKit calls _crashOnException: when an exception is caught during CATransaction flush
+ // or view layout, bypassing reportException: entirely. Depending on macOS version, AppKit
+ // may call the class method +[NSApplication _crashOnException:] or the instance method
+ // -[NSApp _crashOnException:], so we swizzle both.
+ SEL selector = NSSelectorFromString(@"_crashOnException:");
- SentrySwizzleClassMethod(NSApplication, selector, SentrySWReturnType(void),
- SentrySWArguments(NSException * exception), SentrySWReplacement({
- [SentryUncaughtNSExceptions capture:exception];
+ SentrySwizzleClassMethod(NSApplication, selector, SentrySWReturnType(void),
+ SentrySWArguments(NSException * exception), SentrySWReplacement({
+ [SentryUncaughtNSExceptions capture:exception];
# if SENTRY_TEST || SENTRY_TEST_CI
- // Don't call the original in tests as it would abort() the process.
- swizzleInfo.originalCalled = YES;
+ // Don't call the original in tests as it would abort() the process.
+ swizzleInfo.originalCalled = YES;
# else
- return SentrySWCallOriginal(exception);
+ return SentrySWCallOriginal(exception);
# endif
- }));
+ }));
- SentrySwizzleInstanceMethod(NSApplication, selector, SentrySWReturnType(void),
- SentrySWArguments(NSException * exception), SentrySWReplacement({
- [SentryUncaughtNSExceptions capture:exception];
+ SentrySwizzleInstanceMethod(NSApplication, selector, SentrySWReturnType(void),
+ SentrySWArguments(NSException * exception), SentrySWReplacement({
+ [SentryUncaughtNSExceptions capture:exception];
# if SENTRY_TEST || SENTRY_TEST_CI
- // Don't call the original in tests as it would abort() the process.
- swizzleInfo.originalCalled = YES;
+ // Don't call the original in tests as it would abort() the process.
+ swizzleInfo.originalCalled = YES;
# else
- return SentrySWCallOriginal(exception);
+ return SentrySWCallOriginal(exception);
# endif
- }),
- SentrySwizzleModeOncePerClassAndSuperclasses, (void *)selector);
+ }),
+ SentrySwizzleModeOncePerClassAndSuperclasses, (void *)selector);
+ });
# pragma clang diagnostic pop
}
diff --git a/Tests/SentryTests/Integrations/SentryCrash/SentryUncaughtNSExceptionsTests.swift b/Tests/SentryTests/Integrations/SentryCrash/SentryUncaughtNSExceptionsTests.swift
--- a/Tests/SentryTests/Integrations/SentryCrash/SentryUncaughtNSExceptionsTests.swift
+++ b/Tests/SentryTests/Integrations/SentryCrash/SentryUncaughtNSExceptionsTests.swift
@@ -51,7 +51,31 @@
XCTAssertTrue(wasUncaughtExceptionHandlerCalled)
}
+
+ func testSwizzleNSApplicationCrashOnException_whenCalledMultipleTimes_shouldCaptureClassMethodOnce() throws {
+ let crashReporter = SentryDependencyContainer.sharedInstance().crashReporter
+ defer {
+ crashReporter.uncaughtExceptionHandler = nil
+ wasUncaughtExceptionHandlerCalled = false
+ uncaughtExceptionHandlerCallCount = 0
+ }
+
+ wasUncaughtExceptionHandlerCalled = false
+ uncaughtExceptionHandlerCallCount = 0
+ crashReporter.uncaughtExceptionHandler = uncaughtExceptionHandler
+
+ SentryUncaughtNSExceptions.swizzleNSApplicationCrashOnException()
+ SentryUncaughtNSExceptions.swizzleNSApplicationCrashOnException()
+
+ // Call the class method +[NSApplication _crashOnException:].
+ // In tests, SentrySWCallOriginal is skipped so this won't abort().
+ NSApplication.perform(NSSelectorFromString("_crashOnException:"), with: uncaughtInternalInconsistencyException)
+
+ XCTAssertTrue(wasUncaughtExceptionHandlerCalled)
+ XCTAssertEqual(1, uncaughtExceptionHandlerCallCount)
+ }
+
func testSwizzleNSApplicationCrashOnException_instanceMethod() throws {
let crashReporter = SentryDependencyContainer.sharedInstance().crashReporter
@@ -108,10 +132,12 @@
// We need to declare this on the file level because otherwise we get the error:
// A C function pointer cannot be formed from a closure that captures context.
var wasUncaughtExceptionHandlerCalled = false
+var uncaughtExceptionHandlerCallCount = 0
func uncaughtExceptionHandler(exception: NSException) {
XCTAssertEqual(uncaughtInternalInconsistencyException.name, exception.name)
XCTAssertEqual(uncaughtInternalInconsistencyException.reason, exception.reason)
wasUncaughtExceptionHandlerCalled = true
+ uncaughtExceptionHandlerCallCount += 1
}
let uncaughtInternalInconsistencyException = NSException(name: .internalInconsistencyException, reason: "reason", userInfo: nil)
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 795e128 | 1216.68 ms | 1245.83 ms | 29.15 ms |
| 0e4a9dc | 1222.92 ms | 1255.78 ms | 32.86 ms |
| 7e30a5e | 1214.57 ms | 1247.78 ms | 33.21 ms |
| 79e2bb8 | 1216.37 ms | 1242.42 ms | 26.05 ms |
| 64e79c5 | 1217.59 ms | 1243.98 ms | 26.39 ms |
| 6108e4c | 1212.31 ms | 1251.80 ms | 39.49 ms |
| 84bc307 | 1211.52 ms | 1237.09 ms | 25.56 ms |
| fee8669 | 1220.50 ms | 1231.84 ms | 11.34 ms |
| 80963bd | 1194.76 ms | 1212.13 ms | 17.38 ms |
| e03f459 | 1222.56 ms | 1255.94 ms | 33.37 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 795e128 | 24.14 KiB | 1.10 MiB | 1.08 MiB |
| 0e4a9dc | 24.14 KiB | 1.11 MiB | 1.09 MiB |
| 7e30a5e | 24.14 KiB | 1.10 MiB | 1.08 MiB |
| 79e2bb8 | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| 64e79c5 | 24.14 KiB | 1.11 MiB | 1.09 MiB |
| 6108e4c | 24.14 KiB | 1.11 MiB | 1.09 MiB |
| 84bc307 | 24.14 KiB | 1.08 MiB | 1.06 MiB |
| fee8669 | 24.14 KiB | 1.10 MiB | 1.08 MiB |
| 80963bd | 24.14 KiB | 1.11 MiB | 1.09 MiB |
| e03f459 | 24.14 KiB | 1.11 MiB | 1.09 MiB |
Previous results on branch: repro/cocoa-881-crashonexception-instead-of-st
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| fc934bc | 1197.49 ms | 1213.40 ms | 15.91 ms |
| ac652a2 | 1222.86 ms | 1245.84 ms | 22.98 ms |
| 181ff60 | 1232.25 ms | 1256.15 ms | 23.90 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| fc934bc | 24.14 KiB | 1.12 MiB | 1.10 MiB |
| ac652a2 | 24.14 KiB | 1.12 MiB | 1.10 MiB |
| 181ff60 | 24.14 KiB | 1.12 MiB | 1.10 MiB |
|
@itaybre I have added your feedback as well as the ones from the agents (de-duplication). Would you care to re-review this? I checked again with our sample app and AFAIK we do the correct thing now, you can double-check here: https://denrase.sentry.io/issues/7287964162/?project=4508007036551168&query=is%3Aunresolved&referrer=issue-stream thank you! 🙇 |
# Conflicts: # Tests/SentryTests/SentryClientTests.swift
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Unused import of
SentryCrashExceptionApplication.h- Removed the unused
SentryCrashExceptionApplication.himport fromSentryNSExceptionCaptureHelper.mto eliminate the unnecessary dependency.
- Removed the unused
Or push these changes by commenting:
@cursor push bc30c717f6
Preview (bc30c717f6)
diff --git a/Sources/Sentry/SentryNSExceptionCaptureHelper.m b/Sources/Sentry/SentryNSExceptionCaptureHelper.m
--- a/Sources/Sentry/SentryNSExceptionCaptureHelper.m
+++ b/Sources/Sentry/SentryNSExceptionCaptureHelper.m
@@ -3,7 +3,6 @@
#if TARGET_OS_OSX && !SENTRY_NO_UI_FRAMEWORK
# import "SentryCrash.h"
-# import "SentryCrashExceptionApplication.h"
# import "SentryNSExceptionCaptureHelper.h"
# import "SentrySwift.h"
Sentry Build Distribution
|
Sentry Build Distribution
|
itaybre
left a comment
There was a problem hiding this comment.
Almost LGTM, just a small comment + changelog entry
Sentry Build Distribution
|

📜 Description
Fix macOS NSException crash reports showing _crashOnException: in the stacktrace instead of the actual exception callstack.
Fixes both the subclass approach (SentryCrashExceptionApplication) and the swizzle approach (enableUncaughtNSExceptionReporting).
Closes #6620
💚 How did you test it?
📝 Checklist
You have to check all boxes before merging:
sendDefaultPIIis enabled.