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: Do not delete the app state #2382

Merged
merged 5 commits into from
Nov 14, 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 @@ -10,6 +10,7 @@
### Fixes

- Too long flush duration (#2370)
- Do not delete the app state when OOM tracking is disabled. The app state is needed to determine the app start type on the next app start. (#2382)

## 7.30.2

Expand Down
4 changes: 0 additions & 4 deletions Sources/Sentry/SentryAppStartTracker.m
Original file line number Diff line number Diff line change
Expand Up @@ -285,10 +285,6 @@ - (void)stop
[NSNotificationCenter.defaultCenter removeObserver:self
name:UIApplicationDidEnterBackgroundNotification
object:nil];

# if SENTRY_HAS_UIKIT
[self.appStateManager stop];
# endif
}

- (void)dealloc
Expand Down
8 changes: 0 additions & 8 deletions Sources/Sentry/SentryAppStateManager.m
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,6 @@ - (void)stop
removeObserver:self
name:SentryNSNotificationCenterWrapper.willTerminateNotificationName
object:nil];

[self deleteAppState];
}
}

Expand All @@ -124,7 +122,6 @@ - (void)dealloc
// In dealloc it's safe to unsubscribe for all, see
// https://developer.apple.com/documentation/foundation/nsnotificationcenter/1413994-removeobserver
[NSNotificationCenter.defaultCenter removeObserver:self];
[self deleteAppState];
}

/**
Expand Down Expand Up @@ -199,11 +196,6 @@ - (void)storeCurrentAppState
[self.fileManager storeAppState:[self buildCurrentAppState]];
}

- (void)deleteAppState
{
[self.fileManager deleteAppState];
}

#endif

@end
2 changes: 0 additions & 2 deletions Sources/Sentry/include/SentryAppStateManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,6 @@ SENTRY_NO_INIT

- (void)storeCurrentAppState;

- (void)deleteAppState;

- (void)updateAppState:(void (^)(SentryAppState *))block;

#endif
Expand Down
27 changes: 1 addition & 26 deletions Tests/SentryTests/Helper/SentryAppStateManagerTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -65,39 +65,14 @@ class SentryAppStateManagerTests: XCTestCase {
XCTAssertNil(fixture.fileManager.readAppState())
}

func testStopDeletesAppState() {
func testStopDoesNotDeleteAppState() {
XCTAssertNil(fixture.fileManager.readAppState())

sut.start()
XCTAssertNotNil(fixture.fileManager.readAppState())

sut.stop()
XCTAssertNil(fixture.fileManager.readAppState())
}

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

sut.start()
XCTAssertNotNil(fixture.fileManager.readAppState())

sut.start()

sut.stop()
XCTAssertNotNil(fixture.fileManager.readAppState())

sut.stop()
XCTAssertNil(fixture.fileManager.readAppState())
}

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

sut.storeCurrentAppState()
XCTAssertNotNil(fixture.fileManager.readAppState())

sut.deleteAppState()
XCTAssertNil(fixture.fileManager.readAppState())
}

func testUpdateAppState() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ class SentryAppStartTrackerTests: NotificationCenterTestCase {
func testSecondStart_AfterSystemReboot_IsColdStart() {
let previousBootTime = fixture.currentDate.date().addingTimeInterval(-1)
let appState = SentryAppState(releaseName: TestData.appState.releaseName, osVersion: UIDevice.current.systemVersion, vendorId: TestData.someUUID, isDebugging: false, systemBootTimestamp: previousBootTime)
givenPreviousAppState(appState: appState)
store(appState: appState)

startApp()

Expand All @@ -84,18 +84,33 @@ class SentryAppStartTrackerTests: NotificationCenterTestCase {

assertValidStart(type: .warm)
}

// Test for situation described in https://github.com/getsentry/sentry-cocoa/issues/2376
func testSecondStart_SystemNotRebooted_OOM_disabled_IsWarmStart() {
givenSystemNotRebooted()

fixture.options.enableOutOfMemoryTracking = false

fixture.fileManager.moveAppStateToPreviousAppState()
startApp()
assertValidStart(type: .warm)

fixture.fileManager.moveAppStateToPreviousAppState()
startApp()
assertValidStart(type: .warm)
}

func testAppUpgrade_IsColdStart() {
let appState = SentryAppState(releaseName: "0.9.0", osVersion: UIDevice.current.systemVersion, vendorId: TestData.someUUID, isDebugging: false, systemBootTimestamp: fixture.currentDate.date())
givenPreviousAppState(appState: appState)
store(appState: appState)

startApp()

assertValidStart(type: .cold)
}

func testAppWasInBackground_NoAppStartUp() {
givenPreviousAppState(appState: TestData.appState)
store(appState: TestData.appState)

startApp()

Expand All @@ -113,7 +128,7 @@ class SentryAppStartTrackerTests: NotificationCenterTestCase {
terminateApp()

let appState = SentryAppState(releaseName: "1.0.0", osVersion: "14.4.1", vendorId: TestData.someUUID, isDebugging: false, systemBootTimestamp: self.fixture.currentDate.date())
givenPreviousAppState(appState: appState)
store(appState: appState)

fixture.fileManager.moveAppStateToPreviousAppState()
startApp()
Expand All @@ -126,7 +141,7 @@ class SentryAppStartTrackerTests: NotificationCenterTestCase {
*/
func testAppLaunches_PreviousBootTimeInFuture_NoAppStartUp() {
let appState = SentryAppState(releaseName: TestData.appState.releaseName, osVersion: UIDevice.current.systemVersion, vendorId: TestData.someUUID, isDebugging: false, systemBootTimestamp: fixture.currentDate.date().addingTimeInterval(1))
givenPreviousAppState(appState: appState)
store(appState: appState)

fixture.fileManager.moveAppStateToPreviousAppState()
startApp()
Expand Down Expand Up @@ -271,15 +286,15 @@ class SentryAppStartTrackerTests: NotificationCenterTestCase {
assertValidHybridStart(type: .warm)
}

private func givenPreviousAppState(appState: SentryAppState) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this is not the previous but the current app state, this name really didn't make sense.

private func store(appState: SentryAppState) {
fixture.fileManager.store(appState)
}

private func givenSystemNotRebooted() {
let systemBootTimestamp = fixture.currentDate.date()
fixture.sysctl.setProcessStartTimestamp(value: fixture.currentDate.date())
let appState = SentryAppState(releaseName: TestData.appState.releaseName, osVersion: UIDevice.current.systemVersion, vendorId: TestData.someUUID, isDebugging: false, systemBootTimestamp: systemBootTimestamp)
givenPreviousAppState(appState: appState)
store(appState: appState)
}

private func givenProcessStartTimestamp(processStartTimestamp: Date? = nil) {
Expand Down