diff --git a/CHANGELOG.md b/CHANGELOG.md index 9c7b249b4a..e891a66c51 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ - Detect development builds via provisioning profile and debugger attachment (#7702) - Keep replayType as `buffer` for Session Replay triggered by an error (#7804) +- Fix race condition in scope observer notifications causing EXC_BAD_ACCESS during cold launch (#7807) ## 9.10.0 diff --git a/Sources/Sentry/SentryScope.m b/Sources/Sentry/SentryScope.m index 5838befd89..4e16261668 100644 --- a/Sources/Sentry/SentryScope.m +++ b/Sources/Sentry/SentryScope.m @@ -244,7 +244,7 @@ - (void)setContextValue:(NSDictionary *)value forKey:(NSString * [_contextDictionary setValue:value forKey:key]; for (id observer in self.observers) { - [observer setContext:_contextDictionary]; + [observer setContext:_contextDictionary.copy]; } } } @@ -262,7 +262,7 @@ - (void)removeContextForKey:(NSString *)key [_contextDictionary removeObjectForKey:key]; for (id observer in self.observers) { - [observer setContext:_contextDictionary]; + [observer setContext:_contextDictionary.copy]; } } } @@ -280,7 +280,7 @@ - (void)setExtraValue:(id _Nullable)value forKey:(NSString *)key [_extraDictionary setValue:value forKey:key]; for (id observer in self.observers) { - [observer setExtras:_extraDictionary]; + [observer setExtras:_extraDictionary.copy]; } } } @@ -291,7 +291,7 @@ - (void)removeExtraForKey:(NSString *)key [_extraDictionary removeObjectForKey:key]; for (id observer in self.observers) { - [observer setExtras:_extraDictionary]; + [observer setExtras:_extraDictionary.copy]; } } } @@ -306,7 +306,7 @@ - (void)setExtras:(NSDictionary *_Nullable)extras addEntriesFromDictionary:SENTRY_UNWRAP_NULLABLE_DICT(NSString *, id, extras)]; for (id observer in self.observers) { - [observer setExtras:_extraDictionary]; + [observer setExtras:_extraDictionary.copy]; } } } @@ -324,7 +324,7 @@ - (void)setTagValue:(NSString *)value forKey:(NSString *)key _tagDictionary[key] = value; for (id observer in self.observers) { - [observer setTags:_tagDictionary]; + [observer setTags:_tagDictionary.copy]; } } } @@ -335,7 +335,7 @@ - (void)removeTagForKey:(NSString *)key [_tagDictionary removeObjectForKey:key]; for (id observer in self.observers) { - [observer setTags:_tagDictionary]; + [observer setTags:_tagDictionary.copy]; } } } @@ -350,7 +350,7 @@ - (void)setTags:(NSDictionary *_Nullable)tags [_tagDictionary addEntriesFromDictionary:tagsCopy]; for (id observer in self.observers) { - [observer setTags:_tagDictionary]; + [observer setTags:_tagDictionary.copy]; } } } @@ -401,7 +401,7 @@ - (void)setFingerprint:(NSArray *_Nullable)fingerprint } for (id observer in self.observers) { - [observer setFingerprint:_fingerprintArray]; + [observer setFingerprint:_fingerprintArray.copy]; } } } @@ -491,7 +491,7 @@ - (void)setAttributeValue:(id)value forKey:(NSString *)key _attributesDictionary[key] = value; for (id observer in self.observers) { - [observer setAttributes:_attributesDictionary]; + [observer setAttributes:_attributesDictionary.copy]; } } } @@ -502,7 +502,7 @@ - (void)removeAttributeForKey:(NSString *)key [_attributesDictionary removeObjectForKey:key]; for (id observer in self.observers) { - [observer setAttributes:_attributesDictionary]; + [observer setAttributes:_attributesDictionary.copy]; } } } diff --git a/Sources/Swift/Integrations/WatchdogTerminations/SentryWatchdogTerminationScopeObserver.swift b/Sources/Swift/Integrations/WatchdogTerminations/SentryWatchdogTerminationScopeObserver.swift index 8a97dd31f7..c460fb483a 100644 --- a/Sources/Swift/Integrations/WatchdogTerminations/SentryWatchdogTerminationScopeObserver.swift +++ b/Sources/Swift/Integrations/WatchdogTerminations/SentryWatchdogTerminationScopeObserver.swift @@ -3,8 +3,8 @@ /** * This scope observer is used by the Watchdog Termination integration to write breadcrumbs to disk. * The overhead is ~0.015 seconds for 1000 breadcrumbs. - * This class doesn't need to be thread safe as the scope already calls the scope observers in a - * thread safe manner. + * This class doesn't need additional synchronization because the scope copies mutable collections + * before passing them to observers, ensuring that the values received here are immutable snapshots. */ class SentryWatchdogTerminationScopeObserver: NSObject, SentryScopeObserver { diff --git a/Tests/SentryTests/SentryScopeSwiftTests.swift b/Tests/SentryTests/SentryScopeSwiftTests.swift index 2fba820d88..19ebc24f5f 100644 --- a/Tests/SentryTests/SentryScopeSwiftTests.swift +++ b/Tests/SentryTests/SentryScopeSwiftTests.swift @@ -443,7 +443,65 @@ class SentryScopeSwiftTests: XCTestCase { scope.serialize() }) } - + + func testModifyingFromMultipleThreads_withObserverAsyncDispatch() { + // Regression test for https://github.com/getsentry/sentry-react-native/issues/5995 + // Before the fix, the scope passed mutable collections directly to observers. + // Observers like SentryWatchdogTerminationScopeObserver dispatch async work that + // iterates the collection after the @synchronized lock is released, causing a + // concurrent read+write race (EXC_BAD_ACCESS). + let scope = Scope(maxBreadcrumbs: 10) + let observer = AsyncIteratingObserver() + scope.add(observer) + + testConcurrentModifications(asyncWorkItems: 3, writeLoopCount: 100, writeWork: { i in + let key = "key-\(i % 5)" + + scope.setContext(value: ["k": "v-\(i)"], key: key) + scope.removeContext(key: "key-\(i % 3)") + + scope.setTag(value: "v-\(i)", key: key) + scope.removeTag(key: "key-\(i % 3)") + scope.setTags(["a": "1", "b": "2"]) + + scope.setExtra(value: i, key: key) + scope.removeExtra(key: "key-\(i % 3)") + scope.setExtras(["x": 1, "y": 2]) + + scope.setFingerprint(["fp-\(i)"]) + }) + + // If the test completes without a crash or TSan error, the fix works. + } + + func testScopeObserver_passesDistinctCopyToObservers() { + // Verify that observers receive a different object (copy) than the internal + // mutable collection, preventing race conditions when observers dispatch async work. + let sut = Scope() + let observer = IdentityCapturingObserver() + sut.add(observer) + + sut.setContext(value: ["k": "v"], key: "key") + XCTAssertNotNil(observer.lastContext) + XCTAssertFalse(observer.lastContext === sut.contextDictionary, + "Observer should receive a copy, not the internal mutable contextDictionary") + + sut.setTag(value: "v", key: "key") + XCTAssertNotNil(observer.lastTags) + XCTAssertFalse(observer.lastTags === sut.tagDictionary, + "Observer should receive a copy, not the internal mutable tagDictionary") + + sut.setExtra(value: 1, key: "key") + XCTAssertNotNil(observer.lastExtras) + XCTAssertFalse(observer.lastExtras === sut.extraDictionary, + "Observer should receive a copy, not the internal mutable extraDictionary") + + sut.setFingerprint(["fp"]) + XCTAssertNotNil(observer.lastFingerprint) + XCTAssertFalse(observer.lastFingerprint === sut.fingerprintArray, + "Observer should receive a copy, not the internal mutable fingerprintArray") + } + func testScopeObserver_setUser() { let sut = Scope() let observer = fixture.observer @@ -1014,6 +1072,82 @@ class SentryScopeSwiftTests: XCTestCase { self.attributes = attributes } } + + /// A scope observer that simulates the behavior of SentryWatchdogTerminationScopeObserver: + /// it captures the received collection and iterates it asynchronously on a background queue. + /// Before the fix, the scope passed mutable collections directly, so this async iteration + /// would race with concurrent mutations — triggering EXC_BAD_ACCESS. + private class AsyncIteratingObserver: NSObject, SentryScopeObserver { + private let queue = DispatchQueue(label: "AsyncIteratingObserver", attributes: .concurrent) + + func setTags(_ tags: [String: String]?) { + guard let tags = tags else { return } + queue.async { _ = tags.map { "\($0.key)=\($0.value)" } } + } + + func setExtras(_ extras: [String: Any]?) { + guard let extras = extras else { return } + queue.async { _ = extras.map { "\($0.key)=\($0.value)" } } + } + + func setContext(_ context: [String: [String: Any]]?) { + guard let context = context else { return } + queue.async { _ = context.map { "\($0.key)=\($0.value)" } } + } + + func setFingerprint(_ fingerprint: [String]?) { + guard let fingerprint = fingerprint else { return } + queue.async { _ = fingerprint.joined(separator: ",") } + } + + func setAttributes(_ attributes: [String: Any]?) { + guard let attributes = attributes else { return } + queue.async { _ = attributes.map { "\($0.key)=\($0.value)" } } + } + + func setUser(_ user: User?) {} + func setDist(_ dist: String?) {} + func setEnvironment(_ environment: String?) {} + func setLevel(_ level: SentryLevel) {} + func setTraceContext(_ traceContext: [String: Any]?) {} + func addSerializedBreadcrumb(_ crumb: [String: Any]) {} + func clearBreadcrumbs() {} + func clear() {} + } + + /// Captures the raw NSDictionary/NSArray references passed to observer methods + /// so tests can verify identity (pointer) differs from the internal mutable collection. + private class IdentityCapturingObserver: NSObject, SentryScopeObserver { + var lastContext: NSDictionary? + func setContext(_ context: [String: [String: Any]]?) { + lastContext = context as NSDictionary? + } + + var lastTags: NSDictionary? + func setTags(_ tags: [String: String]?) { + lastTags = tags as NSDictionary? + } + + var lastExtras: NSDictionary? + func setExtras(_ extras: [String: Any]?) { + lastExtras = extras as NSDictionary? + } + + var lastFingerprint: NSArray? + func setFingerprint(_ fingerprint: [String]?) { + lastFingerprint = fingerprint as NSArray? + } + + func setUser(_ user: User?) {} + func setDist(_ dist: String?) {} + func setEnvironment(_ environment: String?) {} + func setLevel(_ level: SentryLevel) {} + func setTraceContext(_ traceContext: [String: Any]?) {} + func setAttributes(_ attributes: [String: Any]?) {} + func addSerializedBreadcrumb(_ crumb: [String: Any]) {} + func clearBreadcrumbs() {} + func clear() {} + } } // A minimal dummy Span implementation that is not SentrySpan.