Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: SDK reports OOM when crashing after calling close #2468

Merged
merged 5 commits into from
Nov 30, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ This version adds a dependency on Swift.
- Use the same default environment for events and sessions (#2447)
- Increase `SentryCrashMAX_STRINGBUFFERSIZE` to reduce the instances where we're dropping a crash due to size limit (#2465)
- `SentryAppStateManager` correctly unsubscribes from `NSNotificationCenter` when closing the SDK (#2460)
- The SDK no longer reports an OOM when a crash happens after closing the SDK (#2468)

### Breaking Changes

Expand Down
11 changes: 11 additions & 0 deletions Sources/Sentry/SentryAppState.m
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ - (instancetype)initWithReleaseName:(NSString *)releaseName
_isActive = NO;
_wasTerminated = NO;
_isANROngoing = NO;
_isSDKRunning = YES;
}
return self;
}
Expand Down Expand Up @@ -89,6 +90,15 @@ - (nullable instancetype)initWithJSONObject:(NSDictionary *)jsonObject
} else {
_isANROngoing = [isANROngoing boolValue];
}

id isSDKRunning = [jsonObject valueForKey:@"is_sdk_running"];
if (isSDKRunning == nil || ![isSDKRunning isKindOfClass:[NSNumber class]]) {
// This property was added later so instead of returning nil,
// we're setting it to the default value.
_isSDKRunning = YES;
} else {
_isSDKRunning = [isSDKRunning boolValue];
}
}
return self;
}
Expand All @@ -106,6 +116,7 @@ - (nullable instancetype)initWithJSONObject:(NSDictionary *)jsonObject
[data setValue:@(self.isActive) forKey:@"is_active"];
[data setValue:@(self.wasTerminated) forKey:@"was_terminated"];
[data setValue:@(self.isANROngoing) forKey:@"is_anr_ongoing"];
[data setValue:@(self.isSDKRunning) forKey:@"is_sdk_running"];

return data;
}
Expand Down
4 changes: 4 additions & 0 deletions Sources/Sentry/SentryAppStateManager.m
Original file line number Diff line number Diff line change
Expand Up @@ -88,13 +88,17 @@ - (void)stop
[self stopWithForce:NO];
}

// forceStop is YES when the SDK gets closed
- (void)stopWithForce:(BOOL)forceStop
{
if (self.startCount <= 0) {
return;
}

if (forceStop) {
[self
updateAppStateInBackground:^(SentryAppState *appState) { appState.isSDKRunning = NO; }];

self.startCount = 0;
} else {
self.startCount -= 1;
Expand Down
7 changes: 6 additions & 1 deletion Sources/Sentry/SentryOutOfMemoryLogic.m
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ - (BOOL)isOOM
SentryAppState *currentAppState = [self.appStateManager buildCurrentAppState];

// If there is no previous app state, we can't do anything.
if (nil == previousAppState) {
if (previousAppState == nil) {
return NO;
}

Expand Down Expand Up @@ -84,6 +84,11 @@ - (BOOL)isOOM
return NO;
}

// The SDK wasn't running, so *any* crash after the SDK got closed would be seen as OOM.
if (!previousAppState.isSDKRunning) {
return NO;
}

// Was the app in foreground/active ?
// If the app was in background we can't reliably tell if it was an OOM or not.
if (!previousAppState.isActive) {
Expand Down
2 changes: 2 additions & 0 deletions Sources/Sentry/include/SentryAppState.h
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ SENTRY_NO_INIT

@property (nonatomic, assign) BOOL isANROngoing;

@property (nonatomic, assign) BOOL isSDKRunning;

@end

NS_ASSUME_NONNULL_END
12 changes: 12 additions & 0 deletions Tests/SentryTests/Helper/SentryAppStateManagerTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,18 @@ class SentryAppStateManagerTests: XCTestCase {
XCTAssertNotNil(fixture.fileManager.readAppState())
}

func testStopUpdatesAppState() {
sut.start()

let stateBeforeStop = fixture.fileManager.readAppState()
XCTAssertTrue(stateBeforeStop!.isSDKRunning)

sut.stop(withForce: true)

let stateAfterStop = fixture.fileManager.readAppState()
XCTAssertFalse(stateAfterStop!.isSDKRunning)
}

func testForcedStop() {
XCTAssertNil(fixture.fileManager.readAppState())

Expand Down
4 changes: 3 additions & 1 deletion Tests/SentryTests/Helper/SentryAppStateTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ class SentryAppStateTests: XCTestCase {
XCTAssertEqual(appState.isActive, actual["is_active"] as? Bool)
XCTAssertEqual(appState.wasTerminated, actual["was_terminated"] as? Bool)
XCTAssertEqual(appState.isANROngoing, actual["is_anr_ongoing"] as? Bool)
XCTAssertEqual(appState.isSDKRunning, actual["is_sdk_running"] as? Bool)
}

func testInitWithJSON_AllFields() {
Expand All @@ -26,7 +27,8 @@ class SentryAppStateTests: XCTestCase {
"system_boot_timestamp": (appState.systemBootTimestamp as NSDate).sentry_toIso8601String(),
"is_active": appState.isActive,
"was_terminated": appState.wasTerminated,
"is_anr_ongoing": appState.isANROngoing
"is_anr_ongoing": appState.isANROngoing,
"is_sdk_running": appState.isSDKRunning
] as [String: Any]

let actual = SentryAppState(jsonObject: dict)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,16 @@ class SentryOutOfMemoryTrackerTests: NotificationCenterTestCase {

assertNoOOMSent()
}


func testSDKWasClosed_NoOOM() {
let appState = SentryAppState(releaseName: TestData.appState.releaseName, osVersion: UIDevice.current.systemVersion, vendorId: TestData.someUUID, isDebugging: false, systemBootTimestamp: fixture.currentDate.date())
appState.isSDKRunning = false

givenPreviousAppState(appState: appState)
sut.start()
assertNoOOMSent()
}

func testAppWasInBackground_NoOOM() {
sut.start()
goToForeground()
Expand Down
3 changes: 3 additions & 0 deletions Tests/SentryTests/SentrySDKTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -513,6 +513,9 @@ class SentrySDKTests: XCTestCase {
SentrySDK.close()

XCTAssertEqual(appStateManager.startCount, 0)

let stateAfterStop = fixture.fileManager.readAppState()
XCTAssertFalse(stateAfterStop!.isSDKRunning)
}
#endif

Expand Down