From 2fb55ddfa47aeeccb6df1e4357e5af5186c24d53 Mon Sep 17 00:00:00 2001 From: Salim Braksa Date: Tue, 13 Jun 2023 12:05:29 +0100 Subject: [PATCH 1/9] Ensure WPCrashLoggingDataProvider.currentUser is executed in the main context queue --- .../Utility/Logging/WPCrashLoggingProvider.swift | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/WordPress/Classes/Utility/Logging/WPCrashLoggingProvider.swift b/WordPress/Classes/Utility/Logging/WPCrashLoggingProvider.swift index bf8319dbd521..018dd629c19f 100644 --- a/WordPress/Classes/Utility/Logging/WPCrashLoggingProvider.swift +++ b/WordPress/Classes/Utility/Logging/WPCrashLoggingProvider.swift @@ -48,12 +48,11 @@ struct WPCrashLoggingDataProvider: CrashLoggingDataProvider { } var currentUser: TracksUser? { - let context = ContextManager.sharedInstance().mainContext - - guard let account = try? WPAccount.lookupDefaultWordPressComAccount(in: context) else { - return nil + return ContextManager.shared.performQuery { context -> TracksUser? in + guard let account = try? WPAccount.lookupDefaultWordPressComAccount(in: context) else { + return nil + } + return TracksUser(userID: account.userID.stringValue, email: account.email, username: account.username) } - - return TracksUser(userID: account.userID.stringValue, email: account.email, username: account.username) } } From c5c197cb3d88e43d1acd2a039fb4c033317ac205 Mon Sep 17 00:00:00 2001 From: Salim Braksa Date: Thu, 15 Jun 2023 17:11:21 +0100 Subject: [PATCH 2/9] Write unit test to check whether the crash logging track user is accessible from background and main threads --- .../Logging/WPCrashLoggingProvider.swift | 8 +- WordPress/WordPress.xcodeproj/project.pbxproj | 12 +++ .../WPCrashLoggingDataProviderTests.swift | 79 +++++++++++++++++++ 3 files changed, 98 insertions(+), 1 deletion(-) create mode 100644 WordPress/WordPressTest/WPCrashLoggingDataProviderTests.swift diff --git a/WordPress/Classes/Utility/Logging/WPCrashLoggingProvider.swift b/WordPress/Classes/Utility/Logging/WPCrashLoggingProvider.swift index 018dd629c19f..500bc712afe7 100644 --- a/WordPress/Classes/Utility/Logging/WPCrashLoggingProvider.swift +++ b/WordPress/Classes/Utility/Logging/WPCrashLoggingProvider.swift @@ -35,6 +35,12 @@ struct WPLoggingStack { } struct WPCrashLoggingDataProvider: CrashLoggingDataProvider { + private let contextManager: ContextManager + + init(coreDataStack: ContextManager = .shared) { + self.contextManager = coreDataStack + } + let sentryDSN: String = ApiCredentials.sentryDSN var userHasOptedOut: Bool { @@ -48,7 +54,7 @@ struct WPCrashLoggingDataProvider: CrashLoggingDataProvider { } var currentUser: TracksUser? { - return ContextManager.shared.performQuery { context -> TracksUser? in + return contextManager.performQuery { context -> TracksUser? in guard let account = try? WPAccount.lookupDefaultWordPressComAccount(in: context) else { return nil } diff --git a/WordPress/WordPress.xcodeproj/project.pbxproj b/WordPress/WordPress.xcodeproj/project.pbxproj index ac52fc10913a..6e5054fea032 100644 --- a/WordPress/WordPress.xcodeproj/project.pbxproj +++ b/WordPress/WordPress.xcodeproj/project.pbxproj @@ -3563,6 +3563,7 @@ F41E4EEF28F247D3001880C6 /* white-on-green-icon-app-60@2x.png in Resources */ = {isa = PBXBuildFile; fileRef = F41E4EEA28F247D3001880C6 /* white-on-green-icon-app-60@2x.png */; }; F41E4EF028F247D3001880C6 /* white-on-green-icon-app-76@2x.png in Resources */ = {isa = PBXBuildFile; fileRef = F41E4EEB28F247D3001880C6 /* white-on-green-icon-app-76@2x.png */; }; F42A1D9729928B360059CC70 /* BlockedAuthor.swift in Sources */ = {isa = PBXBuildFile; fileRef = F42A1D9629928B360059CC70 /* BlockedAuthor.swift */; }; + F4394D1F2A3AB06F003955C6 /* WPCrashLoggingDataProviderTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = F4394D1E2A3AB06F003955C6 /* WPCrashLoggingDataProviderTests.swift */; }; F4426FD3287E08C300218003 /* SuggestionServiceMock.swift in Sources */ = {isa = PBXBuildFile; fileRef = F4426FD2287E08C300218003 /* SuggestionServiceMock.swift */; }; F4426FD9287F02FD00218003 /* SiteSuggestionsServiceMock.swift in Sources */ = {isa = PBXBuildFile; fileRef = F4426FD8287F02FD00218003 /* SiteSuggestionsServiceMock.swift */; }; F4426FDB287F066400218003 /* site-suggestions.json in Resources */ = {isa = PBXBuildFile; fileRef = F4426FDA287F066400218003 /* site-suggestions.json */; }; @@ -8963,6 +8964,7 @@ F41E4EEB28F247D3001880C6 /* white-on-green-icon-app-76@2x.png */ = {isa = PBXFileReference; lastKnownFileType = image.png; path = "white-on-green-icon-app-76@2x.png"; sourceTree = ""; }; F42A1D9629928B360059CC70 /* BlockedAuthor.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BlockedAuthor.swift; sourceTree = ""; }; F432964A287752690089C4F7 /* WordPress 144.xcdatamodel */ = {isa = PBXFileReference; lastKnownFileType = wrapper.xcdatamodel; path = "WordPress 144.xcdatamodel"; sourceTree = ""; }; + F4394D1E2A3AB06F003955C6 /* WPCrashLoggingDataProviderTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = WPCrashLoggingDataProviderTests.swift; sourceTree = ""; }; F4426FD2287E08C300218003 /* SuggestionServiceMock.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SuggestionServiceMock.swift; sourceTree = ""; }; F4426FD8287F02FD00218003 /* SiteSuggestionsServiceMock.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SiteSuggestionsServiceMock.swift; sourceTree = ""; }; F4426FDA287F066400218003 /* site-suggestions.json */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.json; path = "site-suggestions.json"; sourceTree = ""; }; @@ -13386,6 +13388,7 @@ 852416D01A12ED2D0030700C /* Utility */ = { isa = PBXGroup; children = ( + F4394D202A3B6F43003955C6 /* Crash Logging */, 17AF92241C46634000A99CFB /* BlogSiteVisibilityHelperTest.m */, 93A379EB19FFBF7900415023 /* KeychainTest.m */, 5948AD101AB73D19006E8882 /* WPAppAnalyticsTests.m */, @@ -17144,6 +17147,14 @@ path = "white-on-green"; sourceTree = ""; }; + F4394D202A3B6F43003955C6 /* Crash Logging */ = { + isa = PBXGroup; + children = ( + F4394D1E2A3AB06F003955C6 /* WPCrashLoggingDataProviderTests.swift */, + ); + name = "Crash Logging"; + sourceTree = ""; + }; F44293D428E3B39400D340AF /* App Icons */ = { isa = PBXGroup; children = ( @@ -23497,6 +23508,7 @@ 08A4E12F289D2795001D9EC7 /* UserPersistentStoreTests.swift in Sources */, 436D55F5211632B700CEAA33 /* RegisterDomainDetailsViewModelTests.swift in Sources */, E180BD4C1FB462FF00D0D781 /* CookieJarTests.swift in Sources */, + F4394D1F2A3AB06F003955C6 /* WPCrashLoggingDataProviderTests.swift in Sources */, 9813512E22F0FC2700F7425D /* FileDownloadsStatsRecordValueTests.swift in Sources */, 9363113F19FA996700B0C739 /* AccountServiceTests.swift in Sources */, 17FC0032264D728E00FCBD37 /* SharingServiceTests.swift in Sources */, diff --git a/WordPress/WordPressTest/WPCrashLoggingDataProviderTests.swift b/WordPress/WordPressTest/WPCrashLoggingDataProviderTests.swift new file mode 100644 index 000000000000..189884bdd906 --- /dev/null +++ b/WordPress/WordPressTest/WPCrashLoggingDataProviderTests.swift @@ -0,0 +1,79 @@ +import XCTest +import AutomatticTracks + +@testable import WordPress + +final class WPCrashLoggingDataProviderTests: XCTestCase { + + // MARK: - Testing Log Error + + func testReadingTrackUserInMainThread() throws { + // Given + let dataProvider = self.makeCrashLoggingDataProvider() + + // When + let user = try XCTUnwrap(dataProvider.currentUser) + + // Then + XCTAssertEqual(user.userID, "\(Constants.defaultAccountID)") + XCTAssertEqual(user.username, Constants.defaultAccountUsername) + XCTAssertEqual(user.email, Constants.defaultAccountEmail) + } + + func testReadingTrackUserInBackgroundThread() { + // Given + let expectation = XCTestExpectation(description: "Should return current user") + let dataProvider = self.makeCrashLoggingDataProvider() + let dispatchGroup = DispatchGroup() + let numberOfOperations = 1000 + + // When + for _ in 0.. WPCrashLoggingDataProvider { + let provider = WPCrashLoggingDataProvider(coreDataStack: makeCoreDataStack()) + return provider + } + + private func makeCoreDataStack() -> ContextManager { + let contextManager = ContextManager.forTesting() + let account = AccountBuilder(contextManager) + .with(id: Constants.defaultAccountID) + .with(email: Constants.defaultAccountEmail) + .with(username: Constants.defaultAccountUsername) + .build() + UserSettings.defaultDotComUUID = account.uuid + return contextManager + } + + // MARK: - Constants + + private enum Constants { + static let defaultAccountID: Int64 = 123 + static let defaultAccountEmail: String = "foo@automattic.com" + static let defaultAccountUsername: String = "foobar" + } +} From 04a98bcb11368487158f1809af5adbe0b8d827ea Mon Sep 17 00:00:00 2001 From: Salim Braksa Date: Thu, 15 Jun 2023 17:24:03 +0100 Subject: [PATCH 3/9] Minor change --- WordPress/Classes/Utility/Logging/WPCrashLoggingProvider.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/WordPress/Classes/Utility/Logging/WPCrashLoggingProvider.swift b/WordPress/Classes/Utility/Logging/WPCrashLoggingProvider.swift index 500bc712afe7..460b71d0b58e 100644 --- a/WordPress/Classes/Utility/Logging/WPCrashLoggingProvider.swift +++ b/WordPress/Classes/Utility/Logging/WPCrashLoggingProvider.swift @@ -37,7 +37,7 @@ struct WPLoggingStack { struct WPCrashLoggingDataProvider: CrashLoggingDataProvider { private let contextManager: ContextManager - init(coreDataStack: ContextManager = .shared) { + init(contextManager: ContextManager = .shared) { self.contextManager = coreDataStack } From 1103f30d22a8bc346324b5eeee917c9c51c419cc Mon Sep 17 00:00:00 2001 From: Salim Braksa Date: Thu, 15 Jun 2023 17:24:07 +0100 Subject: [PATCH 4/9] Minor change --- WordPress/Classes/Utility/Logging/WPCrashLoggingProvider.swift | 2 +- WordPress/WordPressTest/WPCrashLoggingDataProviderTests.swift | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/WordPress/Classes/Utility/Logging/WPCrashLoggingProvider.swift b/WordPress/Classes/Utility/Logging/WPCrashLoggingProvider.swift index 460b71d0b58e..43770b7f486a 100644 --- a/WordPress/Classes/Utility/Logging/WPCrashLoggingProvider.swift +++ b/WordPress/Classes/Utility/Logging/WPCrashLoggingProvider.swift @@ -38,7 +38,7 @@ struct WPCrashLoggingDataProvider: CrashLoggingDataProvider { private let contextManager: ContextManager init(contextManager: ContextManager = .shared) { - self.contextManager = coreDataStack + self.contextManager = contextManager } let sentryDSN: String = ApiCredentials.sentryDSN diff --git a/WordPress/WordPressTest/WPCrashLoggingDataProviderTests.swift b/WordPress/WordPressTest/WPCrashLoggingDataProviderTests.swift index 189884bdd906..3a265b33fefd 100644 --- a/WordPress/WordPressTest/WPCrashLoggingDataProviderTests.swift +++ b/WordPress/WordPressTest/WPCrashLoggingDataProviderTests.swift @@ -54,7 +54,7 @@ final class WPCrashLoggingDataProviderTests: XCTestCase { // MARK: - Helpers private func makeCrashLoggingDataProvider() -> WPCrashLoggingDataProvider { - let provider = WPCrashLoggingDataProvider(coreDataStack: makeCoreDataStack()) + let provider = WPCrashLoggingDataProvider(contextManager: makeCoreDataStack()) return provider } From bc1d858fbb675b520513d10401bc0f5f99484cc8 Mon Sep 17 00:00:00 2001 From: Salim Braksa Date: Thu, 15 Jun 2023 17:43:57 +0100 Subject: [PATCH 5/9] Update release notes --- RELEASE-NOTES.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/RELEASE-NOTES.txt b/RELEASE-NOTES.txt index b9cde9cd71fd..727d64e38962 100644 --- a/RELEASE-NOTES.txt +++ b/RELEASE-NOTES.txt @@ -11,6 +11,7 @@ * [**] Block editor: [iOS] Fix dictation regression, in which typing/dictating at the same time caused content loss. [https://github.com/WordPress/gutenberg/pull/49452] * [*] Block editor: Display lock icon in disabled state of `Cell` component [https://github.com/wordpress-mobile/gutenberg-mobile/pull/5798] * [*] Block editor: Show "No title"/"No description" placeholder for not belonged videos in VideoPress block [https://github.com/wordpress-mobile/gutenberg-mobile/pull/5840] +* [*] Resolve an issue that was causing the app crash when `CrashLogging.logError` is called from a backgroung thread. [#20846] 22.5 ----- From 2334cb5a15bda6c887d8597c6bd74e9688e55942 Mon Sep 17 00:00:00 2001 From: Salim Braksa Date: Fri, 16 Jun 2023 17:07:50 +0100 Subject: [PATCH 6/9] Refactor testReadingTrackUserInBackgroundThread test to async await syntax --- .../Logging/WPCrashLoggingProvider.swift | 9 +++++ .../WPCrashLoggingDataProviderTests.swift | 33 +++++-------------- 2 files changed, 17 insertions(+), 25 deletions(-) diff --git a/WordPress/Classes/Utility/Logging/WPCrashLoggingProvider.swift b/WordPress/Classes/Utility/Logging/WPCrashLoggingProvider.swift index 43770b7f486a..cb1af04a94d6 100644 --- a/WordPress/Classes/Utility/Logging/WPCrashLoggingProvider.swift +++ b/WordPress/Classes/Utility/Logging/WPCrashLoggingProvider.swift @@ -62,3 +62,12 @@ struct WPCrashLoggingDataProvider: CrashLoggingDataProvider { } } } + +extension NSManagedObjectContext { + + func proxy_save() throws { + // 1. Log this method call + // 2. Check for concurrency violation, otherwise send an error to Sentry + try self.save() + } +} diff --git a/WordPress/WordPressTest/WPCrashLoggingDataProviderTests.swift b/WordPress/WordPressTest/WPCrashLoggingDataProviderTests.swift index 3a265b33fefd..8e82948a1e52 100644 --- a/WordPress/WordPressTest/WPCrashLoggingDataProviderTests.swift +++ b/WordPress/WordPressTest/WPCrashLoggingDataProviderTests.swift @@ -20,35 +20,18 @@ final class WPCrashLoggingDataProviderTests: XCTestCase { XCTAssertEqual(user.email, Constants.defaultAccountEmail) } - func testReadingTrackUserInBackgroundThread() { - // Given - let expectation = XCTestExpectation(description: "Should return current user") + func testReadingTrackUserInBackgroundThread() async { let dataProvider = self.makeCrashLoggingDataProvider() - let dispatchGroup = DispatchGroup() - let numberOfOperations = 1000 - - // When - for _ in 0.. Date: Mon, 19 Jun 2023 00:58:48 +0100 Subject: [PATCH 7/9] Update RELEASE-NOTES.txt Co-authored-by: Tanner Stokes --- RELEASE-NOTES.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/RELEASE-NOTES.txt b/RELEASE-NOTES.txt index 727d64e38962..2a69a285b225 100644 --- a/RELEASE-NOTES.txt +++ b/RELEASE-NOTES.txt @@ -11,7 +11,7 @@ * [**] Block editor: [iOS] Fix dictation regression, in which typing/dictating at the same time caused content loss. [https://github.com/WordPress/gutenberg/pull/49452] * [*] Block editor: Display lock icon in disabled state of `Cell` component [https://github.com/wordpress-mobile/gutenberg-mobile/pull/5798] * [*] Block editor: Show "No title"/"No description" placeholder for not belonged videos in VideoPress block [https://github.com/wordpress-mobile/gutenberg-mobile/pull/5840] -* [*] Resolve an issue that was causing the app crash when `CrashLogging.logError` is called from a backgroung thread. [#20846] +* [*] Resolve an issue that was causing the app crash when `CrashLogging.logError` is called from a background thread. [#20846] 22.5 ----- From 119b6d148cf3a2b9b7ed5cb4b8174bac4ed7ffab Mon Sep 17 00:00:00 2001 From: Salim Braksa Date: Mon, 19 Jun 2023 01:02:06 +0100 Subject: [PATCH 8/9] Remove code that was added by accident --- .../Utility/Logging/WPCrashLoggingProvider.swift | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/WordPress/Classes/Utility/Logging/WPCrashLoggingProvider.swift b/WordPress/Classes/Utility/Logging/WPCrashLoggingProvider.swift index cb1af04a94d6..20ee638b8911 100644 --- a/WordPress/Classes/Utility/Logging/WPCrashLoggingProvider.swift +++ b/WordPress/Classes/Utility/Logging/WPCrashLoggingProvider.swift @@ -61,13 +61,4 @@ struct WPCrashLoggingDataProvider: CrashLoggingDataProvider { return TracksUser(userID: account.userID.stringValue, email: account.email, username: account.username) } } -} - -extension NSManagedObjectContext { - - func proxy_save() throws { - // 1. Log this method call - // 2. Check for concurrency violation, otherwise send an error to Sentry - try self.save() - } -} +} \ No newline at end of file From 8ed96b608b4199ffc76245edacd23d69254f093f Mon Sep 17 00:00:00 2001 From: Salim Braksa Date: Mon, 19 Jun 2023 01:04:08 +0100 Subject: [PATCH 9/9] Fix Trailing Newline Violation --- WordPress/Classes/Utility/Logging/WPCrashLoggingProvider.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/WordPress/Classes/Utility/Logging/WPCrashLoggingProvider.swift b/WordPress/Classes/Utility/Logging/WPCrashLoggingProvider.swift index 20ee638b8911..43770b7f486a 100644 --- a/WordPress/Classes/Utility/Logging/WPCrashLoggingProvider.swift +++ b/WordPress/Classes/Utility/Logging/WPCrashLoggingProvider.swift @@ -61,4 +61,4 @@ struct WPCrashLoggingDataProvider: CrashLoggingDataProvider { return TracksUser(userID: account.userID.stringValue, email: account.email, username: account.username) } } -} \ No newline at end of file +}