Bugfix FXIOS-8973 [Multi-window] Inject tab data stores (#20161)

* [8973] Inject tab data stores instead of using a shared ubiquitous instance, this fixes some problems that could occur when multiple windows would attempt to save their tabs at the same time, because the tab data store is not fundamentally thread safe for this scenario * [8973] Remove tab data store from shared app container * [8973] Minor cleanup, refactor * [8973] Minor cleanup * [8973] Minor cleanup * [8973] Fix window manager unit tests * [8973] Unit test fix * [8973] Remove tab data store mock from dependency helper in unit tests also
mozilla-mobile · May 10, 2024 · 5422599 · 5422599
1 parent fe6a1d0
commit 5422599
Show file tree

Hide file tree

Showing 8 changed files with 19 additions and 40 deletions.
diff --git a/BrowserKit/Sources/TabDataStore/TabDataStore.swift b/BrowserKit/Sources/TabDataStore/TabDataStore.swift
@@ -86,19 +86,7 @@ public actor DefaultTabDataStore: TabDataStore {
     }
 
     private func parseWindowDataFile(fromURL url: URL) -> WindowData? {
-        return parseWindowDataFiles(fromURLs: [url]).first
-    }
-
-    private func parseWindowDataFiles(fromURLs urlList: [URL]) -> [WindowData] {
-        var windowsData: [WindowData] = []
-        for fileURL in urlList {
-            do {
-                if let windowData = try? fileManager.getWindowDataFromPath(path: fileURL) {
-                    windowsData.append(windowData)
-                }
-            }
-        }
-        return windowsData
+        return try? fileManager.getWindowDataFromPath(path: url)
     }
 
     // MARK: - Saving Data

diff --git a/firefox-ios/Client/Application/DependencyHelper.swift b/firefox-ios/Client/Application/DependencyHelper.swift
@@ -32,9 +32,6 @@ class DependencyHelper {
         let downloadQueue: DownloadQueue = appDelegate.appSessionManager.downloadQueue
         AppContainer.shared.register(service: downloadQueue)
 
-        let tabDataStore: TabDataStore = appDelegate.tabDataStore
-        AppContainer.shared.register(service: tabDataStore)
-
         let windowManager: WindowManager = appDelegate.windowManager
         AppContainer.shared.register(service: windowManager)
 

diff --git a/firefox-ios/Client/Application/WindowManager+DebugUtilities.swift b/firefox-ios/Client/Application/WindowManager+DebugUtilities.swift
@@ -15,8 +15,10 @@ extension WindowManagerImplementation {
         var result = "----------- Window Debug Info ------------\n"
         result.append("Open windows (\(windows.count)) & normal tabs (via TabManager):\n")
         for (idx, (uuid, _)) in windows.enumerated() {
-            result.append("    \(idx + 1): \(short(uuid))\n")
             let tabMgr = tabManager(for: uuid)
+            let window = windows[uuid]?.sceneCoordinator?.window
+            let frame = window?.frame ?? .zero
+            result.append("    \(idx + 1): \(short(uuid)) (\(tabMgr.normalTabs.count) tabs) (frame: \(frame.debugDescription))\n")
             for (tabIdx, tab) in tabMgr.normalTabs.enumerated() {
                 let memAddr = Unmanaged.passUnretained(tab).toOpaque()
                 result.append("        \(tabIdx + 1) (\(memAddr)): \(tab.url?.absoluteString ?? "<nil url>")\n")
@@ -35,12 +37,15 @@ extension WindowManagerImplementation {
 
         // Note: this is provided as a convenience for internal debugging. See `DefaultTabDataStore.swift`.
         for (idx, uuid) in tabDataStore.fetchWindowDataUUIDs().enumerated() {
-            result.append("    \(idx + 1): Window \(short(uuid))\n")
             let baseURL = fileManager.windowDataDirectory(isBackup: false)!
             let dataURL = baseURL.appendingPathComponent("window-" + uuid.uuidString)
+            if idx == 0 {
+                result.append("    Data dir: \(baseURL.absoluteString)\n")
+            }
+            result.append("    \(idx + 1): Window \(short(uuid))\n")
             guard let data = try? fileManager.getWindowDataFromPath(path: dataURL) else { continue }
             for (tabIdx, tabData) in data.tabData.enumerated() {
-                result.append("        \(tabIdx + 1): \(tabData.siteUrl)\n")
+                result.append("        \(tabIdx + 1): \(tabData.siteUrl) (Window: \(short(data.id))\n")
             }
         }
         return result

diff --git a/firefox-ios/Client/Application/WindowManager.swift b/firefox-ios/Client/Application/WindowManager.swift
@@ -110,10 +110,10 @@ final class WindowManagerImplementation: WindowManager, WindowTabsSyncCoordinato
     // MARK: - Initializer
 
     init(logger: Logger = DefaultLogger.shared,
-         tabDataStore: TabDataStore = AppContainer.shared.resolve(),
+         tabDataStore: TabDataStore? = nil,
          userDefaults: UserDefaultsInterface = UserDefaults.standard) {
+        self.tabDataStore = tabDataStore ?? DefaultTabDataStore(logger: logger, fileManager: DefaultTabFileManager())
         self.logger = logger
-        self.tabDataStore = tabDataStore
         self.defaults = userDefaults
         tabSyncCoordinator.delegate = self
     }

diff --git a/firefox-ios/Client/TabManagement/TabManagerImplementation.swift b/firefox-ios/Client/TabManagement/TabManagerImplementation.swift
@@ -30,16 +30,17 @@ class TabManagerImplementation: LegacyTabManager, Notifiable {
          imageStore: DiskImageStore = AppContainer.shared.resolve(),
          logger: Logger = DefaultLogger.shared,
          uuid: WindowUUID,
-         tabDataStore: TabDataStore = AppContainer.shared.resolve(),
+         tabDataStore: TabDataStore? = nil,
          tabSessionStore: TabSessionStore = DefaultTabSessionStore(),
-         tabMigration: TabMigrationUtility = DefaultTabMigrationUtility(),
+         tabMigration: TabMigrationUtility? = nil,
          notificationCenter: NotificationProtocol = NotificationCenter.default,
          inactiveTabsManager: InactiveTabsManagerProtocol = InactiveTabsManager(),
          windowManager: WindowManager = AppContainer.shared.resolve()) {
-        self.tabDataStore = tabDataStore
+        let dataStore =  tabDataStore ?? DefaultTabDataStore(logger: logger, fileManager: DefaultTabFileManager())
+        self.tabDataStore = dataStore
         self.tabSessionStore = tabSessionStore
         self.imageStore = imageStore
-        self.tabMigration = tabMigration
+        self.tabMigration = tabMigration ?? DefaultTabMigrationUtility(tabDataStore: dataStore)
         self.notificationCenter = notificationCenter
         self.inactiveTabsManager = inactiveTabsManager
         self.windowManager = windowManager

diff --git a/firefox-ios/Client/TabManagement/TabMigrationUtility.swift b/firefox-ios/Client/TabManagement/TabMigrationUtility.swift
@@ -22,7 +22,7 @@ class DefaultTabMigrationUtility: TabMigrationUtility {
     var legacyTabs = [LegacySavedTab]()
 
     init(profile: Profile = AppContainer.shared.resolve(),
-         tabDataStore: TabDataStore = AppContainer.shared.resolve(),
+         tabDataStore: TabDataStore,
          logger: Logger = DefaultLogger.shared,
          legacyTabDataRetriever: LegacyTabDataRetriever = LegacyTabDataRetrieverImplementation()) {
         self.prefs = profile.prefs

diff --git a/...ox-ios/firefox-ios-tests/Tests/ClientTests/DependencyInjection/DependencyHelperMock.swift b/...ox-ios/firefox-ios-tests/Tests/ClientTests/DependencyInjection/DependencyHelperMock.swift
@@ -17,9 +17,6 @@ class DependencyHelperMock {
         )
         AppContainer.shared.register(service: profile)
 
-        let tabDataStore: TabDataStore = MockTabDataStore()
-        AppContainer.shared.register(service: tabDataStore)
-
         let diskImageStore: DiskImageStore = DefaultDiskImageStore(
             files: profile.files,
             namespace: TabManagerConstants.tabScreenshotNamespace,

diff --git a/firefox-ios/firefox-ios-tests/Tests/ClientTests/WindowManagerTests.swift b/firefox-ios/firefox-ios-tests/Tests/ClientTests/WindowManagerTests.swift
@@ -13,6 +13,7 @@ import TabDataStore
 class WindowManagerTests: XCTestCase {
     let tabManager = MockTabManager(windowUUID: WindowUUID())
     let secondTabManager = MockTabManager(windowUUID: WindowUUID())
+    let mockTabDataStore = MockTabDataStore()
 
     override func setUp() {
         super.setUp()
@@ -104,8 +105,6 @@ class WindowManagerTests: XCTestCase {
 
     func testNextAvailableUUIDWhenNoTabDataIsSaved() {
         let subject = createSubject()
-        let tabDataStore: TabDataStore = AppContainer.shared.resolve()
-        let mockTabDataStore = tabDataStore as! MockTabDataStore
         mockTabDataStore.resetMockTabWindowUUIDs()
 
         // Check that asking for two UUIDs results in two unique/random UUIDs
@@ -118,8 +117,6 @@ class WindowManagerTests: XCTestCase {
 
     func testNextAvailableUUIDWhenOnlyOneWindowSaved() {
         let subject = createSubject()
-        let tabDataStore: TabDataStore = AppContainer.shared.resolve()
-        let mockTabDataStore = tabDataStore as! MockTabDataStore
         mockTabDataStore.resetMockTabWindowUUIDs()
 
         let savedUUID = UUID()
@@ -135,8 +132,6 @@ class WindowManagerTests: XCTestCase {
 
     func testNextAvailableUUIDWhenMultipleWindowsSaved() {
         let subject = createSubject()
-        let tabDataStore: TabDataStore = AppContainer.shared.resolve()
-        let mockTabDataStore = tabDataStore as! MockTabDataStore
         mockTabDataStore.resetMockTabWindowUUIDs()
 
         let uuid1 = UUID()
@@ -191,8 +186,6 @@ class WindowManagerTests: XCTestCase {
 
     func testReservedUUIDsAreUnavailableInSuccessiveCalls() {
         let subject = createSubject()
-        let tabDataStore: TabDataStore = AppContainer.shared.resolve()
-        let mockTabDataStore = tabDataStore as! MockTabDataStore
         mockTabDataStore.resetMockTabWindowUUIDs()
 
         let savedUUID = UUID()
@@ -210,8 +203,6 @@ class WindowManagerTests: XCTestCase {
 
     func testClosingTwoWindowsInDifferentOrdersResultsInSensibleExpectedOrderWhenOpening() {
         let subject = createSubject()
-        let tabDataStore: TabDataStore = AppContainer.shared.resolve()
-        let mockTabDataStore = tabDataStore as! MockTabDataStore
         mockTabDataStore.resetMockTabWindowUUIDs()
 
         let uuid1 = UUID()
@@ -255,6 +246,6 @@ class WindowManagerTests: XCTestCase {
         // For this test case, we create a new WindowManager that we can
         // modify and reset between each test case as needed, without
         // impacting other tests that may use the shared AppContainer.
-        return WindowManagerImplementation()
+        return WindowManagerImplementation(tabDataStore: mockTabDataStore)
     }
 }