Skip to content

Commit a6e2b83

Browse files
committed
fix(crash-reporting): keep backtrace reporter registration open when not opting out
Registering the policy-only `BacktraceReportingFeature` unconditionally claimed the single registration slot, so the `get(feature:) == nil` guard in `register(backtraceReporter:)` silently dropped any reporter registered afterwards — losing `binary_images` from logs and RUM view events for apps using a custom plugin with no backtrace reporter. Record the policy only when App Hang backtraces are actually turned off, leaving the default path behaving exactly as it did before. Restore `register(backtraceReporter:)` as its own symbol and add the `appHangBacktraceEnabled` variant as an overload, so the existing compound name and mangled symbol stay unchanged for XCFramework consumers. `DatadogInternal` is not part of `DATADOG_MODULES`, so `make api-surface-verify` does not catch this class of change. Tests: the App Hang integration test picked its RUM / Crash Reporting enablement order with `oneOf`, so the lazy per-hang read of the opt-out was only exercised on about half the runs; split it into two deterministic tests. Add coverage for the reporter-less default path, for the public plugin+configuration overload forwarding its argument, and for the Objective-C configuration default and setter.
1 parent 76dca49 commit a6e2b83

5 files changed

Lines changed: 84 additions & 22 deletions

File tree

Datadog/IntegrationUnitTests/RUM/AppHangsMonitoringTests.swift

Lines changed: 21 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -116,19 +116,28 @@ class AppHangsMonitoringTests: XCTestCase {
116116
#endif
117117
}
118118

119-
func testGivenAppHangBacktracesDisabledInCrashReporting_whenMainThreadHangs_itTracksAppHangWithNoStackTrace() throws {
119+
func testGivenAppHangBacktracesDisabledInCrashReporting_whenRUMIsEnabledFirst_itTracksAppHangWithNoStackTrace() throws {
120+
try assertAppHangIsTrackedWithNoStackTrace { crashReportingConfig in
121+
RUM.enable(with: self.rumConfig, in: self.core)
122+
CrashReporting.enable(with: crashReportingConfig, in: self.core)
123+
}
124+
}
125+
126+
func testGivenAppHangBacktracesDisabledInCrashReporting_whenCrashReportingIsEnabledFirst_itTracksAppHangWithNoStackTrace() throws {
127+
try assertAppHangIsTrackedWithNoStackTrace { crashReportingConfig in
128+
CrashReporting.enable(with: crashReportingConfig, in: self.core)
129+
RUM.enable(with: self.rumConfig, in: self.core)
130+
}
131+
}
132+
133+
/// Asserts that a hang is tracked with no stack trace, with the SDK enabled by `enableSDK`.
134+
///
135+
/// Both enablement orders get their own test rather than being picked at random: only the RUM-first order
136+
/// proves that the opt-out is read per hang instead of being captured when RUM is enabled, so randomizing
137+
/// would let that regression pass half of the runs.
138+
private func assertAppHangIsTrackedWithNoStackTrace(enableSDK: (CrashReporting.Configuration) -> Void) throws {
120139
// Given (initialize SDK on the main thread)
121-
let crashReportingConfig = CrashReporting.Configuration(appHangBacktraceEnabled: false)
122-
oneOf([ // no matter of RUM or CR initialization order
123-
{
124-
RUM.enable(with: self.rumConfig, in: self.core)
125-
CrashReporting.enable(with: crashReportingConfig, in: self.core)
126-
},
127-
{
128-
CrashReporting.enable(with: crashReportingConfig, in: self.core)
129-
RUM.enable(with: self.rumConfig, in: self.core)
130-
},
131-
])
140+
enableSDK(CrashReporting.Configuration(appHangBacktraceEnabled: false))
132141

133142
// When
134143
mainQueue.sync {

DatadogCore/Tests/Objc/ObjcAPITests/DDConfiguration+apiTests.m

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,13 @@ - (void)testDDConfigurationBuilderAPI {
7373

7474
- (void)testDatadogCrashReporterAPI {
7575
[DDCrashReporter enable];
76+
77+
DDCrashReporterConfiguration *configuration = [DDCrashReporterConfiguration new];
78+
XCTAssertTrue(configuration.appHangBacktraceEnabled, @"App Hang backtraces are enabled by default");
79+
configuration.appHangBacktraceEnabled = NO;
80+
XCTAssertFalse(configuration.appHangBacktraceEnabled, @"The setter must write through to the wrapped configuration");
81+
82+
[DDCrashReporter enableWith:configuration];
7683
}
7784

7885
#pragma clang diagnostic pop

DatadogCrashReporting/Sources/CrashReporting.swift

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -122,10 +122,14 @@ public final class CrashReporting {
122122
backtraceReporter: backtraceReporter,
123123
appHangBacktraceEnabled: configuration.appHangBacktraceEnabled
124124
)
125-
} else {
126-
// A custom plugin may provide no backtrace reporter. Register the policy anyway, so that opting out of
127-
// App Hang backtraces stays reportable as such rather than as "Crash Reporting was never enabled".
128-
try core.register(appHangBacktraceEnabled: configuration.appHangBacktraceEnabled)
125+
} else if !configuration.appHangBacktraceEnabled {
126+
// A custom plugin may provide no backtrace reporter. Record the opt-out anyway, so that it stays
127+
// reportable as such rather than as "Crash Reporting was never enabled".
128+
//
129+
// Only when opting out: registering unconditionally would claim the single `BacktraceReportingFeature`
130+
// slot with a reporter-less Feature, and the `get(feature:) == nil` guard in `register(backtraceReporter:)`
131+
// would then silently drop any reporter registered later.
132+
try core.register(appHangBacktraceEnabled: false)
129133
}
130134

131135
reporter.sendCrashReportIfFound()

DatadogCrashReporting/Tests/CrashReportingFeatureTests.swift

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,42 @@ class CrashReportingFeatureTests: XCTestCase {
143143
)
144144
}
145145

146+
func testGivenPluginWithNoBacktraceReporter_whenAppHangBacktracesAreEnabled_itLeavesRegistrationOpenForALaterReporter() throws {
147+
// Given
148+
let core = FeatureRegistrationCoreMock()
149+
let plugin = CrashReportingPluginMock()
150+
plugin.injectedBacktraceReporter = nil
151+
152+
// When
153+
try CrashReporting.enableOrThrow(with: plugin, in: core)
154+
155+
// Then
156+
XCTAssertTrue(core.isAppHangBacktraceEnabled)
157+
158+
try core.register(backtraceReporter: BacktraceReporterMock())
159+
XCTAssertNotNil(
160+
try core.backtraceReporter.generateBacktrace(),
161+
"Nothing must claim the backtrace reporter registration when there is no opt-out to record, otherwise "
162+
+ "the `already registered` guard silently drops a reporter registered later"
163+
)
164+
}
165+
166+
func testWhenEnablingWithPluginThroughThePublicAPI_itForwardsTheConfiguration() throws {
167+
// Given
168+
let core = FeatureRegistrationCoreMock()
169+
let plugin = CrashReportingPluginMock()
170+
plugin.injectedBacktraceReporter = BacktraceReporterMock()
171+
172+
// When (through the public API, so that the overload's own argument forwarding is covered)
173+
CrashReporting.enable(with: plugin, configuration: .init(appHangBacktraceEnabled: false), in: core)
174+
175+
// Then
176+
XCTAssertFalse(
177+
core.isAppHangBacktraceEnabled,
178+
"The public overload must forward its `configuration`, not a default-constructed one"
179+
)
180+
}
181+
146182
// MARK: - Crash Report Reading Tests
147183

148184
func testItSendsLaunchReportWhenNoPendingCrash() {

DatadogInternal/Sources/BacktraceReporting/BacktraceReporter.swift

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -85,11 +85,17 @@ internal struct CoreBacktraceReporter: BacktraceReporting, @unchecked Sendable {
8585

8686
/// Adds capability of reporting backtraces.
8787
extension DatadogCoreProtocol {
88+
/// Registers backtrace reporter in Core.
89+
/// - Parameter backtraceReporter: the implementation of backtrace reporter.
90+
public func register(backtraceReporter: BacktraceReporting) throws {
91+
try register(backtraceReporter: backtraceReporter, appHangBacktraceEnabled: true)
92+
}
93+
8894
/// Registers backtrace reporter in Core.
8995
/// - Parameters:
9096
/// - backtraceReporter: the implementation of backtrace reporter.
91-
/// - appHangBacktraceEnabled: whether backtraces may be generated for App Hangs detected by RUM. Default: `true`.
92-
public func register(backtraceReporter: BacktraceReporting, appHangBacktraceEnabled: Bool = true) throws {
97+
/// - appHangBacktraceEnabled: whether backtraces may be generated for App Hangs detected by RUM.
98+
public func register(backtraceReporter: BacktraceReporting, appHangBacktraceEnabled: Bool) throws {
9399
guard get(feature: BacktraceReportingFeature.self) == nil else {
94100
DD.logger.debug("Backtrace reporter is already registered to this core. Skipping registration of next one.")
95101
return
@@ -123,10 +129,10 @@ extension DatadogCoreProtocol {
123129

124130
/// Whether backtraces may be generated for App Hangs detected by RUM.
125131
///
126-
/// It is `false` only when a backtrace reporter was registered with App Hang backtraces turned off. Before any
127-
/// reporter is registered it is `true`: in that state backtrace generation is *unavailable* rather than
128-
/// *disabled*, and callers must keep distinguishing the two. Read it at the moment a backtrace is needed, as
129-
/// the reporter may be registered after the reading Feature was enabled.
132+
/// It is `false` only when Crash Reporting was enabled with App Hang backtraces turned off — whether or not a
133+
/// backtrace reporter came with it. Until then it is `true`: in that state backtrace generation may still be
134+
/// *unavailable* rather than *disabled*, and callers must keep distinguishing the two. Read it at the moment a
135+
/// backtrace is needed, as the reporter may be registered after the reading Feature was enabled.
130136
public var isAppHangBacktraceEnabled: Bool {
131137
// `self.` is required: a bare `get(...)` here parses as a `get` accessor.
132138
self.get(feature: BacktraceReportingFeature.self)?.appHangBacktraceEnabled ?? true

0 commit comments

Comments
 (0)