No Bug: Safeguard CoreData operations more. (#4479)

* No Bug: Safeguard CoreData operations more. * No Bug: Migrate CoreDatabase items after it is initialized.
brave · Nov 8, 2021 · 85d3b0c · 85d3b0c
1 parent 5f36651
commit 85d3b0c
Show file tree

Hide file tree

Showing 4 changed files with 35 additions and 22 deletions.
diff --git a/Client/Application/Delegates/SceneDelegate.swift b/Client/Application/Delegates/SceneDelegate.swift
@@ -33,6 +33,7 @@ class SceneDelegate: UIResponder, UIWindowSceneDelegate {
         // We have to wait until pre1.12 migration is done until we proceed with database
         // initialization. This is because Database container may change. See bugs #3416, #3377.
         DataController.shared.initializeOnce()
+        Migration.postCoreDataInitMigrations()
 
         Preferences.General.themeNormalMode.objectWillChange
             .merge(with: PrivateBrowsingManager.shared.objectWillChange)

diff --git a/Client/Application/Migration.swift b/Client/Application/Migration.swift
@@ -36,11 +36,6 @@ class Migration {
         if !Preferences.Migration.playlistV1FileSettingsLocationCompleted.value {
             movePlaylistV1Items()
         }
-
-        if !Preferences.Migration.removeLargeFaviconsMigrationCompleted.value {
-            FaviconMO.clearTooLargeFavicons()
-            Preferences.Migration.removeLargeFaviconsMigrationCompleted.value = true
-        }
 
         // Adding Observer to enable sync types
 
@@ -143,6 +138,28 @@ class Migration {
             log.error("Moving Playlist Files for \(errorPath) failed: \(error)")
         }
     }
+
+    static func postCoreDataInitMigrations() {
+        if !Preferences.Migration.removeLargeFaviconsMigrationCompleted.value {
+            FaviconMO.clearTooLargeFavicons()
+            Preferences.Migration.removeLargeFaviconsMigrationCompleted.value = true
+        }
+
+        if Preferences.Migration.coreDataCompleted.value { return }
+
+        // In 1.6.6 we included private tabs in CoreData (temporarely) until the user did one of the following:
+        //  - Cleared private data
+        //  - Exited Private Mode
+        //  - The app was terminated (bug)
+        // However due to a bug, some private tabs remain in the container. Since 1.7 removes `isPrivate` from TabMO,
+        // we must dismiss any records that are private tabs during migration from Model7
+        TabMO.deleteAllPrivateTabs()
+
+        Domain.migrateShieldOverrides()
+        LegacyBookmarksHelper.migrateBookmarkOrders()
+
+        Preferences.Migration.coreDataCompleted.value = true
+    }
 }
 
 fileprivate extension Preferences {
@@ -158,6 +175,8 @@ fileprivate extension Preferences {
             Option<Bool>(key: "migration.playlistv1-file-settings-location-completed", default: false)
         static let removeLargeFaviconsMigrationCompleted =
             Option<Bool>(key: "migration.remove-large-favicons", default: false)
+
+        static let coreDataCompleted = Option<Bool>(key: "migration.cd-completed", default: false)
     }
 
     /// Migrate the users preferences from prior versions of the app (<2.0)
@@ -233,19 +252,6 @@ fileprivate extension Preferences {
         // This needs to be translated to our new preference.
         Preferences.General.isFirstLaunch.value = Preferences.DAU.lastLaunchInfo.value == nil
 
-        // Core Data
-
-        // In 1.6.6 we included private tabs in CoreData (temporarely) until the user did one of the following:
-        //  - Cleared private data
-        //  - Exited Private Mode
-        //  - The app was terminated (bug)
-        // However due to a bug, some private tabs remain in the container. Since 1.7 removes `isPrivate` from TabMO,
-        // we must dismiss any records that are private tabs during migration from Model7
-        TabMO.deleteAllPrivateTabs()
-
-        Domain.migrateShieldOverrides()
-        LegacyBookmarksHelper.migrateBookmarkOrders()
-
         Preferences.Migration.completed.value = true
     }
 }
diff --git a/Data/models/CRUDProtocols.swift b/Data/models/CRUDProtocols.swift
@@ -25,7 +25,7 @@ protocol Readable where Self: NSManagedObject {
 }
 
 // MARK: - Implementations
-extension Deletable where Self: NSManagedObject {
+extension Deletable {
     func delete(context: WriteContext = .new(inMemory: false)) {
 
         DataController.perform(context: context) { context in
@@ -63,8 +63,10 @@ extension Deletable where Self: NSManagedObject {
 
                     // Batch delete writes directly to the persistent store.
                     // Therefore contexts and in-memory objects must be updated manually.
-                    let result = try context.execute(deleteRequest) as? NSBatchDeleteResult
-                    guard let objectIDArray = result?.result as? [NSManagedObjectID] else { return }
+
+                    guard let objectIDArray = try context.persistentStoreCoordinator?
+                        .execute(deleteRequest, with: context) as? [NSManagedObjectID] else { return }
+
                     let changes = [NSDeletedObjectsKey: objectIDArray]
 
                     // Merging changes to view context is important because fetch results controllers
@@ -80,7 +82,7 @@ extension Deletable where Self: NSManagedObject {
     }
 }
 
-extension Readable where Self: NSManagedObject { 
+extension Readable {
     static func count(predicate: NSPredicate? = nil,
                       context: NSManagedObjectContext = DataController.viewContext) -> Int? {
         let request = getFetchRequest()

diff --git a/Data/models/DataController.swift b/Data/models/DataController.swift
@@ -201,6 +201,10 @@ public class DataController {
 
     static func perform(context: WriteContext = .new(inMemory: false), save: Bool = true,
                         task: @escaping (NSManagedObjectContext) -> Void) {
+        if !DataController.shared.initializationCompleted && !AppConstants.isRunningTest {
+            assertionFailure("Performing on context before database initialized")
+            return
+        }
 
         switch context {
         case .existing(let existingContext):