Conversation
…e class - Add concrete facade class bridging SDK services to SentryCrash - Expose five required services: notificationCenterWrapper, dateProvider, crashReporter, uncaughtExceptionHandler, activeScreenSize - Implement platform-conditional activeScreenSize for iOS/tvOS only - Follow existing @objc bridging patterns from SentryNSNotificationCenterWrapper - Inherit from NSObject and mark as @_spi(Private) for SDK-internal access
- Create SUMMARY.md documenting SentryCrashBridge facade implementation - Update STATE.md with Phase 01 Plan 01 completion (50% progress) - Record execution metrics: 252s duration, 1 file created, 1 task completed - Add decisions: computed property pattern, platform-conditional compilation, @_spi(Private) access
…to SentryCrashIntegration - Add bridge property to store SentryCrashBridge instance - Create bridge before startCrashHandler to ensure services available - Wire notificationCenterWrapper, dateProvider, and crashReporter from dependencies - Update CrashIntegrationProvider to include DateProviderProvider and NotificationCenterProvider protocols
…ces to MockCrashDependencies - Add dateProvider property bridging to SentryDependencyContainer - Add notificationCenterWrapper property bridging to SentryDependencyContainer - Fixes compilation error after adding DateProviderProvider and NotificationCenterProvider to CrashIntegrationProvider This fix was required to unblock Task 2 (Deviation Rule 3 - blocking issue).
Plan 01-02 Summary: - SentryCrashBridge integrated into SentryCrashIntegration - Bridge created before crash handler installation with dependency injection - Extended CrashIntegrationProvider with required protocol conformances - Fixed MockCrashDependencies test compilation (deviation Rule 3) - All 27 tests pass, iOS/macOS/tvOS compilation verified - Duration: 9m 33s Phase 01 now complete (2/2 plans, 825s total).
…dge propagation patterns
- Add bridge: SentryCrashBridge parameter to both production initializers - Store bridge as private property for activeScreenSize access - Convert lazy var systemInfo to immutable let initialized from bridge.crashReporter.systemInfo - Replace SentryDependencyContainerSwiftHelper.activeScreenSize() with bridge.activeScreenSize() - Update Dependencies.crashWrapper to create and pass bridge - Preserve test-only initializer for backward compatibility - Keep crashedLastLaunch and activeDurationSinceLastCrash unchanged (Phase 3)
- Add convenience initializer to TestSentryCrashWrapper that creates bridge - Update SentryHubTests to pass bridge when using production initializer - Remove systemInfo override from TestSentryCrashWrapper (now immutable let) - All SentryCrashWrapperTests pass without modification
…ssionHandler - Add bridge parameter to SentryCrashIntegrationSessionHandler constructors - Replace container singleton dateProvider access with bridge.dateProvider - Update CrashIntegrationSessionHandlerBuilder protocol signature - Update SentryCrashIntegration to pass bridge to session builder - Update MockCrashDependencies test implementation - Remove SentryDependencyContainer reference from SessionHandler Phase 02, Plan 02: SessionHandler now receives all dependencies through bridge
- Replace crashedLastLaunch container access with bridge.crashReporter - Replace activeDurationSinceLastCrash container access with bridge.crashReporter - Only test-only initializer retains container reference (acceptable)
- Add SentryCrashBridge property to SentryCrash.h with forward declaration - Add NS_ASSUME_NONNULL_BEGIN/END to SentryCrash.h for nullability audit - Mark nullable properties explicitly (userInfo, sink, onCrash, etc.) - Update SentryCrash.m to use bridge.notificationCenterWrapper with fallback - Add setBridge method to SentryCrashSwift.swift using KVC - All 4 notificationCenterWrapper references now use bridge when available
…entryCrash - Call crashReporter.setBridge(bridge) in SentryCrashIntegration init - Bridge set before crash handler installation (before startCrashHandler) - Bridge propagation complete: Integration → SentryCrashSwift → SentryCrash.m
- Add sentrycrashcm_nsexception_setBridge function declaration in header - Add static g_bridge variable to store bridge reference - Update setEnabled to use g_bridge.crashReporter.uncaughtExceptionHandler - Add fallback to container when bridge not set - Call sentrycrashcm_nsexception_setBridge in SentryCrash.m install method - Wrap ObjC-specific code in __OBJC__ guards for C compatibility
- Bridge injection into SentryCrash and NSException monitor - 5 container references eliminated with fallback compatibility - 18.1 min execution time - All 3 tasks committed atomically
- Add SentryCrashBridge forward declaration to SentryCrashInstallation.h - Add bridge property to enable SDK services access without container - Update 4 crashReporter references in SentryCrashInstallation.m to use bridge with container fallback - Replace container singleton access in dealloc, install, uninstall, sendAllReportsWithCompletion
…ashIntegration - Add bridge injection to installation after creation using KVC - Bridge set before install() call to ensure crash handler has access - Complete bridge propagation path: Integration -> Installation
- Bridge property on base class enables crashReporter access via facade - 4 container references replaced with bridge-first fallback pattern - Complete bridge propagation: Integration -> Installation -> lifecycle - Duration: 26 min, 2 tasks, 3 files modified
Phase 3 (ObjC Isolation) verified and marked complete. All 11 production container references converted to bridge-first pattern. Verification passed 6/6 must-haves. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Single plan with 2 tasks: run SentryCrash test suite and confirm zero container references via grep + multi-platform builds. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ttern Single plan to add Dependency Isolation section to develop-docs/SENTRYCRASH.md covering facade rationale, five services, and guidance for adding new dependencies.
Semver Impact of This PR🟢 Patch (bug fixes) 📋 Changelog PreviewThis is how your changes will appear in the changelog. Internal Changes 🔧
🤖 This preview updates automatically when you update the PR. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7514 +/- ##
=============================================
+ Coverage 85.307% 85.349% +0.041%
=============================================
Files 480 486 +6
Lines 28620 28872 +252
Branches 12376 12521 +145
=============================================
+ Hits 24415 24642 +227
- Misses 4156 4183 +27
+ Partials 49 47 -2
... and 48 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
…r crashWrapper and extraContextProvider
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| b8fcca6 | 1228.85 ms | 1259.59 ms | 30.74 ms |
| 79e2bb8 | 1216.37 ms | 1242.42 ms | 26.05 ms |
| 60ea6f8 | 1209.42 ms | 1246.62 ms | 37.21 ms |
| 0286ae4 | 1230.42 ms | 1261.39 ms | 30.97 ms |
| 14bb180 | 1222.75 ms | 1252.47 ms | 29.72 ms |
| fee8669 | 1220.50 ms | 1231.84 ms | 11.34 ms |
| a8ec727 | 1225.29 ms | 1259.59 ms | 34.30 ms |
| eaeb9bc | 1223.18 ms | 1256.45 ms | 33.27 ms |
| 84bc307 | 1211.52 ms | 1237.09 ms | 25.56 ms |
| 7e93d85 | 1205.28 ms | 1243.71 ms | 38.44 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| b8fcca6 | 24.14 KiB | 1.08 MiB | 1.06 MiB |
| 79e2bb8 | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| 60ea6f8 | 24.14 KiB | 1.12 MiB | 1.10 MiB |
| 0286ae4 | 24.14 KiB | 1.12 MiB | 1.10 MiB |
| 14bb180 | 24.14 KiB | 1.12 MiB | 1.10 MiB |
| fee8669 | 24.14 KiB | 1.10 MiB | 1.08 MiB |
| a8ec727 | 24.14 KiB | 1.12 MiB | 1.10 MiB |
| eaeb9bc | 24.14 KiB | 1.12 MiB | 1.10 MiB |
| 84bc307 | 24.14 KiB | 1.08 MiB | 1.06 MiB |
| 7e93d85 | 24.14 KiB | 1.06 MiB | 1.04 MiB |
itaybre
requested review from
noahsmartin,
philipphofmann and
philprime
as code owners
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Bridge set after monitors already enabled during install
- Moved sentrycrashcm_nsexception_setBridge call before sentrycrash_install to ensure bridge is available when monitors are enabled.
- ✅ Fixed: Nullable bridge breaks synchronized dealloc cleanup
- Added nil handler checks before @synchronized blocks in dealloc, install, and uninstall methods to prevent synchronization failure.
Or push these changes by commenting:
@cursor push 2019a71872
Preview (2019a71872)
diff --git a/Sources/SentryCrash/Installations/SentryCrashInstallation.m b/Sources/SentryCrash/Installations/SentryCrashInstallation.m
--- a/Sources/SentryCrash/Installations/SentryCrashInstallation.m
+++ b/Sources/SentryCrash/Installations/SentryCrashInstallation.m
@@ -94,10 +94,12 @@
- (void)dealloc
{
SentryCrashSwift *handler = self.bridge.crashReporter;
- @synchronized(handler) {
- if (g_crashHandlerData == self.crashHandlerData) {
- g_crashHandlerData = NULL;
- [handler removeOnCrash];
+ if (handler != nil) {
+ @synchronized(handler) {
+ if (g_crashHandlerData == self.crashHandlerData) {
+ g_crashHandlerData = NULL;
+ [handler removeOnCrash];
+ }
}
}
}
@@ -163,23 +165,27 @@
- (void)install:(NSString *)customCacheDirectory
{
SentryCrashSwift *handler = self.bridge.crashReporter;
- @synchronized(handler) {
- handler.basePath = customCacheDirectory;
- g_crashHandlerData = self.crashHandlerData;
- [handler setupOnCrash];
- [handler install];
+ if (handler != nil) {
+ @synchronized(handler) {
+ handler.basePath = customCacheDirectory;
+ g_crashHandlerData = self.crashHandlerData;
+ [handler setupOnCrash];
+ [handler install];
+ }
}
}
- (void)uninstall
{
SentryCrashSwift *handler = self.bridge.crashReporter;
- @synchronized(handler) {
- if (g_crashHandlerData == self.crashHandlerData) {
- g_crashHandlerData = NULL;
- [handler removeOnCrash];
+ if (handler != nil) {
+ @synchronized(handler) {
+ if (g_crashHandlerData == self.crashHandlerData) {
+ g_crashHandlerData = NULL;
+ [handler removeOnCrash];
+ }
+ [handler uninstall];
}
- [handler uninstall];
}
}
diff --git a/Sources/SentryCrash/Recording/SentryCrash.m b/Sources/SentryCrash/Recording/SentryCrash.m
--- a/Sources/SentryCrash/Recording/SentryCrash.m
+++ b/Sources/SentryCrash/Recording/SentryCrash.m
@@ -231,14 +231,14 @@
NSString *pathEnd = [@"SentryCrash" stringByAppendingPathComponent:[self getBundleName]];
NSString *installPath = [self.basePath stringByAppendingPathComponent:pathEnd];
+ // Set bridge for NSException monitor before enabling monitors
+ sentrycrashcm_nsexception_setBridge(self.bridge);
+
_monitoring = sentrycrash_install(self.bundleName.UTF8String, installPath.UTF8String);
if (self.monitoring == 0) {
return false;
}
- // Set bridge for NSException monitor before enabling monitors
- sentrycrashcm_nsexception_setBridge(self.bridge);
-
#if SENTRY_HAS_UIKIT
id<SentryNSNotificationCenterWrapper> notificationCenter
= self.bridge.notificationCenterWrapper;This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
…nfo to prevent empty data
…e proper monitoring
| * SentryDependencyContainerSwiftHelper, providing a clean architectural boundary | ||
| * between layers. | ||
| */ | ||
| @objc @_spi(Private) public final class SentryCrashBridge: NSObject { |
There was a problem hiding this comment.
h: I am not convinced of this architectural approach. SentryCrash is a library which was vendored in, so from a user perspective the dependency graph is App > SentrySDK > SentryCrash. But this bridge is now a link from SentryCrash > SentrySDK instead. Instead I believe it would make more sense to use protocols which are defined in SentryCrash and need to be injected by our SDK, or we use a delegate/callback pattern. I am thinking about how the architecture would look like if SentryCrash was a separate target or even an external package, and the SentryNSNotificationCenterWrapper would not be in that package but in ours.
There was a problem hiding this comment.
I agree.
I would approach this by defining Swift protocols that describe the interaction between the Cocoa SDK and the crash reporting component / SentryCrash.
These protocols should provide a generic crash-reporting interface without exposing SentryCrash-specific concepts. The Cocoa SDK would depend only on these protocols.
We then introduce an adapter that implements the protocol and translates the generic crash reporting interface to SentryCrash. Ideally, this keeps changes inside SentryCrash minimal and avoids it needing to be aware that it runs inside Sentry Cocoa. Another significant reason for not making too many changes in SentryCrash is that we can port changes and fixes from KSCrash back to SentryCrash. The more we modify SentryCrash, the more it deviates from KSCrash, and the harder it will be to migrate changes back or to do a potential switch. Which then also applies to any other crash reporter that we might use in the future.
Conceptually:
Cocoa SDK → Crash Reporting Protocol → SentryCrashReportAdapter → SentryCrash
Ideally, when you then swap out SentryCrash, for example, with KSCrash, you would only need to write a new adapter, but you don't need to change the crash reporting protocol. But I think it's okay if we're not fully there yet, because making that actually work is quite some effort. I think the main goal should be to have clear boundaries and make an optional switch in the future easier. If we, at some point, actually switch out the crash report with another one, for example, also on Swift Server only for Linux, it's acceptable to maybe have to adapt the protocol slightly, but it should not be too much effort.
And coming back to what Phil said about SentryCrash being an external package, I think we should work towards that goal. When you could theoretically move SentryCrash into its own target or external package, we know we achieved this goal because the dependencies would be only from the Cocoa SDK to SentryCrash, not the other way around.
There was a problem hiding this comment.
I definitely agree with that, I wanted to make a slow approach towards the protocol approach.
This was the first step:
- Remove SentryDependencyContainer uses
- With this we have a better idea of what SentryCrash needs to use
- Create the interfaces / protocols (probably bigger than just the bridge here)
The PR title may have been misleading, this doesn't reach full isolation yet
philprime
left a comment
philprime
left a comment
There was a problem hiding this comment.
Approving this PR based on the discussion we had earlier in the Apple SDK team sync. Please rename the PR to explain that this does not properly isolate SentryCrash but it implements a bridge as a step towards it
itaybre
changed the title
SentryDependencyContainer uses in SentryCrash
Sentry Build Distribution
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| NSSetUncaughtExceptionHandler(&handleUncaughtException); | ||
| SentryDependencyContainer.sharedInstance.crashReporter.uncaughtExceptionHandler | ||
| = &handleUncaughtException; | ||
| g_bridge.crashReporter.uncaughtExceptionHandler = &handleUncaughtException; |
There was a problem hiding this comment.
Nil bridge silently skips uncaught exception handler registration
High Severity
If g_bridge is nil when the NSException monitor is enabled, g_bridge.crashReporter.uncaughtExceptionHandler = &handleUncaughtException silently becomes a no-op due to ObjC nil messaging. The handler is still installed globally via NSSetUncaughtExceptionHandler, but the crash reporter's uncaughtExceptionHandler property won't be set. The previous code via SentryDependencyContainer.sharedInstance guaranteed non-nil access. No warning or error is logged for this failure.
3 participants
📜 Description
Isolate SentryCrash from Sentry SDK.
💡 Motivation and Context
💚 How did you test it?
📝 Checklist
You have to check all boxes before merging:
sendDefaultPIIis enabled.Created with GSD
Closes #7668