Core Data: Prevent crash on the Blaze Dashboard when saving fails #15546

itsmeichigo · 2025-04-24T05:48:56Z

Description

In #14961 I added a workaround to drop the database and force crash the app to help users recover from their corrupted database. This caused an unexpected crash on the Blaze dashboard upon resetting the database, causing the app crash earlier without details about the saving failure.

This PR attempts to prevent this by:

Triggering a different notification that is listented to only by the SessionManager to clear the timestamps for cached data.
~~- Adding a future-proof update to prevent crash on the Blaze dashboard when the database is reset.~~

Steps to reproduce

Smoke test the app with a store that's eligible for Blaze and confirm that the Blaze dashboard is still loaded without issues.

Testing information

Smoke tested the Blaze dashboard card using simulator iPhone 16 Pro iOS 18.2.
Added a unit test to confirm the reset of timestamps upon dropping database.

Screenshots

N/A

I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Reviewer (or Author, in the case of optional code reviews):

Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement:

The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on all devices (phone/tablet) and no regressions are added.

…n there's database reset

wpmobilebot · 2025-04-24T05:58:56Z

📲 You can test the changes from this Pull Request in WooCommerce iOS Prototype by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce iOS Prototype
	Build Number	`29510`
	Version	`PR #15546`
	Bundle ID	`com.automattic.alpha.woocommerce`
	Commit	`a8bd777`
	Installation URL	63iad8mvfq240

Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

jaclync

If the change is to post a new notification that only one class subscribes, I think the changes are pretty confined and we can just smoke test the app to ensure the Blaze dashboard card works as before without crashes. Left some comments.

jaclync · 2025-04-25T08:25:28Z

Storage/Storage/CoreData/CoreDataManager.swift

        do {
            try persistentContainer.persistentStoreCoordinator.destroyPersistentStore(at: storeURL, type: .sqlite, options: nil)
-            NotificationCenter.default.post(name: .StorageManagerDidResetStorage, object: self)
+            NotificationCenter.default.post(name: .StorageManagerDidInvalidateCachedData, object: self)


nit: what's the part that invalidates cached data? maybe it could more specific to what this function does like StorageManagerDidDropDatabase/StorageManagerDidDropStorage?

Done in e7ce752.

jaclync · 2025-04-25T08:30:16Z

WooCommerce/Classes/System/SessionManager.swift

-        // Listens when the core data stack is rest.
-        NotificationCenter.default.addObserver(self, selector: #selector(handleStorageDidReset), name: .StorageManagerDidResetStorage, object: nil)


❓ Maybe this observation of StorageManagerDidResetStorage can still be kept, since this notification is still posted after destroying the database in createPersistentContainer's recovery upon error. Or do we also want to replace StorageManagerDidResetStorage with the new notification there for consistency? I'm not sure if StorageManagerDidResetStorage notification has always been posted in createPersistentContainer from git as there was some refactoring on the file.

woocommerce-ios/Storage/Storage/CoreData/CoreDataManager.swift

Lines 187 to 190 in 68f756a

try container.persistentStoreCoordinator.destroyPersistentStore(at: storeURL,

ofType: storeDescription.type,

options: nil)

NotificationCenter.default.post(name: .StorageManagerDidResetStorage, object: self)

Good point! This has been restored in 7109840.

jaclync · 2025-04-25T08:38:51Z

WooCommerce/Classes/ViewRelated/Dashboard/Blaze/BlazeCampaignDashboardViewModel.swift

+        refetchAllResultsControllers()
+    }
+
+    /// Refetching all the results controllers is necessary after a storage reset in `onDidResetContent`
+    /// callback and before reloading UI that involves more than one results controller.
+    func refetchAllResultsControllers() {


💬 How about trying the new notification fix first, to isolate the issue? If the crash still happens with a different stack trace, we know it's a different issue and can consider adding the changes to refetch both results controllers.

Sounds good! I revert the change in 361f079.

…oard when there's database reset" This reverts commit 5821693.

…base

itsmeichigo · 2025-04-25T11:02:15Z

Thanks @jaclync for the reviews! I addressed the suggestions in the subsequent commits. This PR is ready for another look.

jaclync

LGTM! Thanks for making these changes and we can observe the crash after the release.

jaclync · 2025-04-28T03:44:35Z

WooCommerce/Classes/System/SessionManager.swift

-    @objc func handleStorageDidReset() {
+    @objc func handleStorageDidInvalidateData() {


nit: how about marking resetTimestampsValues as @objc, removing this function, and calling resetTimestampsValues from the two notifications instead? I wasn't very sure the "invalidate data" part in the function name.

Good idea! Done in 56c7182.

jaclync · 2025-04-28T03:45:45Z

WooCommerce/WooCommerceTests/System/SessionManagerTests.swift

        }
    }
+
+    func test_core_data_database_drop_clears_timestamps_stores() throws {


thanks for adding this test case!

…base

itsmeichigo added 2 commits April 24, 2025 11:37

Refetch all results controllers to avoid crash on Blaze dashboard whe…

5821693

…n there's database reset

Use new notification to notify invalidated cached data

07e2de1

itsmeichigo added type: crash The worst kind of bug. feature: Blaze Related to the integration of the Blaze ads platform feature: core Core work. See "category: tooling" and "category: architecture" labels Apr 24, 2025

itsmeichigo added this to the 22.3 milestone Apr 24, 2025

Fix swiftlint violation

e25aa55

jaclync reviewed Apr 25, 2025

View reviewed changes

itsmeichigo added 5 commits April 25, 2025 17:42

Rename new notification

e7ce752

Restore observation for storage reset in SessionManager

7109840

Revert "Refetch all results controllers to avoid crash on Blaze dashb…

361f079

…oard when there's database reset" This reverts commit 5821693.

Update release notes

8b43e7d

Merge branch 'trunk' into fix/woomob-316-prevent-crash-resetting-data…

f626d99

…base

itsmeichigo marked this pull request as ready for review April 25, 2025 11:01

itsmeichigo requested a review from jaclync April 25, 2025 11:02

jaclync approved these changes Apr 28, 2025

View reviewed changes

itsmeichigo added 2 commits April 28, 2025 11:07

Remove handleStorageDidInvalidateData method

56c7182

Merge branch 'trunk' into fix/woomob-316-prevent-crash-resetting-data…

a8bd777

…base

itsmeichigo enabled auto-merge April 28, 2025 04:08

itsmeichigo merged commit 9a04272 into trunk Apr 28, 2025
13 checks passed

itsmeichigo deleted the fix/woomob-316-prevent-crash-resetting-database branch April 28, 2025 04:27

		// Listens when the core data stack is rest.
		NotificationCenter.default.addObserver(self, selector: #selector(handleStorageDidReset), name: .StorageManagerDidResetStorage, object: nil)

	try container.persistentStoreCoordinator.destroyPersistentStore(at: storeURL,
	ofType: storeDescription.type,
	options: nil)
	NotificationCenter.default.post(name: .StorageManagerDidResetStorage, object: self)

		@objc func handleStorageDidReset() {
		@objc func handleStorageDidInvalidateData() {

Core Data: Prevent crash on the Blaze Dashboard when saving fails #15546

Core Data: Prevent crash on the Blaze Dashboard when saving fails #15546

Uh oh!

Conversation

itsmeichigo commented Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Steps to reproduce

Testing information

Screenshots

Reviewer (or Author, in the case of optional code reviews):

Uh oh!

wpmobilebot commented Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaclync left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itsmeichigo commented Apr 25, 2025

Uh oh!

jaclync left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

itsmeichigo commented Apr 24, 2025 •

edited

Loading

wpmobilebot commented Apr 24, 2025 •

edited

Loading