Bugfix FXIOS-11014, FXIOS-11015 Remove deprecated Site GUID and fix I…

…nt 64 ID crashes in SQLite Database (#24064) * FXIOS-11014 - Remove deprecated Site GUID from SQL Lite Database * FXIOS-11015 - Bugfix - Sites inserted into the SQLite database with Swift Int ID's larger than Int32 will crash. * Add unit test for int64 size Site IDs to TestSQLitePinnedSites.swift. Improved existing tests and refactored force unwraps. Fixed misleading assert messages.
mozilla-mobile · Jan 9, 2025 · dcc1214 · dcc1214
1 parent 907817e
commit dcc1214
Show file tree

Hide file tree

Showing 4 changed files with 70 additions and 22 deletions.
diff --git a/firefox-ios/Storage/SQL/SQLiteHistoryFactories.swift b/firefox-ios/Storage/SQL/SQLiteHistoryFactories.swift
@@ -12,16 +12,15 @@ extension BrowserDBSQLite {
     class func basicHistoryColumnFactory(_ row: SDRow) -> Site {
         let id = row["historyID"] as? Int
         guard let url = row["url"] as? String, let title = row["title"] as? String else {
+            assertionFailure("None of these properties should be nil")
             return Site(url: "", title: "")
         }
-        let guid = row["guid"] as? String
 
         // Extract a boolean from the row if it's present.
         let iB = row["is_bookmarked"] as? Int
         let isBookmarked: Bool? = (iB == nil) ? nil : (iB! != 0)
 
         let site = Site(url: url, title: title, bookmarked: isBookmarked)
-        site.guid = guid
         site.id = id
 
         // Find the most recent visit, regardless of which column it might be in.
@@ -33,6 +32,7 @@ extension BrowserDBSQLite {
         if latest > 0 {
             site.latestVisit = Visit(date: latest, type: .link)
         }
+
         return site
     }
 

diff --git a/firefox-ios/Storage/SQL/SQLitePinnedSites.swift b/firefox-ios/Storage/SQL/SQLitePinnedSites.swift
@@ -76,17 +76,10 @@ extension BrowserDBSQLite: PinnedSites {
             return deferMaybe(DatabaseError(description: "Invalid site \(site.url)"))
         }
 
-        // We insert a dummy guid for backward compatibility.
-        // in the past, the guid was required, but we removed that requirement.
-        // if we do not insert a guid, users who downgrade their version of firefox will
-        // crash when loading their pinned tabs.
-        //
-        // We have since allowed the guid to be optional, and should remove this guid
-        // once we stop supporting downgrading to any versions less than 110.
-        let args: Args = [site.url, now, site.title, site.id, "dummy-guid", host]
+        let args: Args = [site.url, now, site.title, site.id, host]
         let arglist = BrowserDB.varlist(args.count)
 
-        return self.database.run([("INSERT OR REPLACE INTO pinned_top_sites (url, pinDate, title, historyID, guid, domain) VALUES \(arglist)", args)])
+        return self.database.run([("INSERT OR REPLACE INTO pinned_top_sites (url, pinDate, title, historyID, domain) VALUES \(arglist)", args)])
         >>== {
             self.notificationCenter.post(name: .TopSitesUpdated, object: self)
             return succeed()

diff --git a/firefox-ios/Storage/ThirdParty/SwiftData.swift b/firefox-ios/Storage/ThirdParty/SwiftData.swift
@@ -283,8 +283,10 @@ private class SQLiteDBStatement {
                 status = sqlite3_bind_double(pointer, Int32(index+1), obj as! Double)
             } else if obj is Int64 {
                 status = sqlite3_bind_int64(pointer, Int32(index+1), Int64(obj as! Int64))
+            } else if obj is Int32 {
+                status = sqlite3_bind_int(pointer, Int32(index+1), Int32(obj as! Int32))
             } else if obj is Int {
-                status = sqlite3_bind_int(pointer, Int32(index+1), Int32(obj as! Int))
+                status = sqlite3_bind_int64(pointer, Int32(index+1), Int64(obj as! Int))
             } else if obj is Bool {
                 status = sqlite3_bind_int(pointer, Int32(index+1), (obj as! Bool) ? 1 : 0)
             } else if obj is String {

diff --git a/firefox-ios/firefox-ios-tests/Tests/StorageTests/TestSQLitePinnedSites.swift b/firefox-ios/firefox-ios-tests/Tests/StorageTests/TestSQLitePinnedSites.swift
@@ -59,8 +59,8 @@ class TestSQLitePinnedSites: XCTestCase {
         func checkPinnedSites() -> Success {
             return pinnedSites.getPinnedTopSites() >>== { pinnedSites in
                 XCTAssertEqual(pinnedSites.count, 2)
-                XCTAssertEqual(pinnedSites[0]!.url, site2.url)
-                XCTAssertEqual(pinnedSites[1]!.url, site1.url, "The older pinned site should be last")
+                XCTAssertEqual(pinnedSites[0]?.url, site2.url)
+                XCTAssertEqual(pinnedSites[1]?.url, site1.url, "The older pinned site should be last")
                 return succeed()
             }
         }
@@ -69,7 +69,7 @@ class TestSQLitePinnedSites: XCTestCase {
             return pinnedSites.removeFromPinnedTopSites(site2) >>== {
                 return pinnedSites.getPinnedTopSites() >>== { pinnedSites in
                     XCTAssertEqual(pinnedSites.count, 1, "There should only be one pinned site")
-                    XCTAssertEqual(pinnedSites[0]!.url, site1.url, "Site2 should be the only pin left")
+                    XCTAssertEqual(pinnedSites[0]?.url, site1.url, "Site1 should be the only pin left")
                     return succeed()
                 }
             }
@@ -79,7 +79,7 @@ class TestSQLitePinnedSites: XCTestCase {
             return pinnedSites.addPinnedTopSite(site1) >>== {
                 return pinnedSites.getPinnedTopSites() >>== { pinnedSites in
                     XCTAssertEqual(pinnedSites.count, 1, "There should not be a dupe")
-                    XCTAssertEqual(pinnedSites[0]!.url, site1.url, "Site2 should be the only pin left")
+                    XCTAssertEqual(pinnedSites[0]?.url, site1.url, "Site1 should still be the only pin")
                     return succeed()
                 }
             }
@@ -91,7 +91,7 @@ class TestSQLitePinnedSites: XCTestCase {
             >>> dupePinnedSite
             >>> done
 
-        waitForExpectations(timeout: 10.0) { error in
+        waitForExpectations(timeout: 3) { error in
             return
         }
     }
@@ -125,27 +125,80 @@ class TestSQLitePinnedSites: XCTestCase {
         func checkPinnedSites() -> Success {
             return pinnedSites.getPinnedTopSites() >>== { pinnedSites in
                 XCTAssertEqual(pinnedSites.count, 2)
-                XCTAssertEqual(pinnedSites[0]!.url, site2.url)
-                XCTAssertEqual(pinnedSites[1]!.url, site1.url, "The older pinned site should be last")
+                XCTAssertEqual(pinnedSites[0]?.url, site2.url)
+                XCTAssertEqual(pinnedSites[1]?.url, site1.url, "The older pinned site should be last")
                 return succeed()
             }
         }
 
         func removePinnedSites() -> Success {
             return pinnedSites.removeFromPinnedTopSites(site2) >>== {
                 return pinnedSites.getPinnedTopSites() >>== { pinnedSites in
-                    XCTAssertEqual(pinnedSites.count, 1, "There should only be one pinned site")
-                    XCTAssertEqual(pinnedSites[0]!.url, site1.url, "Site2 should be the only pin left")
+                    XCTAssertEqual(pinnedSites.count, 0, "Duplicate pinned domains are removed with a fuzzy search")
                     return succeed()
                 }
             }
         }
 
         addPinnedSites()
             >>> checkPinnedSites
+            >>> checkPinnedSites
+            >>> removePinnedSites
+            >>> done
+
+        waitForExpectations(timeout: 3) { error in
+            return
+        }
+    }
+
+    func testPinnedTopSites_idOfMaxSizeInt64() {
+        let database = BrowserDB(
+            filename: "testPinnedTopSitesDuplicateDomains.db",
+            schema: BrowserSchema(),
+            files: files
+        )
+        let prefs = MockProfilePrefs()
+        let pinnedSites = BrowserDBSQLite(database: database, prefs: prefs)
+
+        // create pinned sites with a same domain name
+        let siteId = Int.max
+        let site = Site(id: Int.max, url: "http://site.com/foo1", title: "A domain")
+
+        let expectation = self.expectation(description: "Add site")
+        func done() -> Success {
+            expectation.fulfill()
+            return succeed()
+        }
+
+        func addPinnedSite() -> Success {
+            return pinnedSites.addPinnedTopSite(site)
+        }
+
+        func checkPinnedSite() -> Success {
+            return pinnedSites.getPinnedTopSites() >>== { pinnedSites in
+                XCTAssertEqual(pinnedSites.count, 1)
+                XCTAssertEqual(pinnedSites[0]?.id, siteId)
+                XCTAssertEqual(pinnedSites[0]?.url, site.url)
+                XCTAssertEqual(pinnedSites[0]?.title, site.title)
+                return succeed()
+            }
+        }
+
+        func removePinnedSite() -> Success {
+            return pinnedSites.removeFromPinnedTopSites(site) >>== {
+                return pinnedSites.getPinnedTopSites() >>== { pinnedSites in
+                    XCTAssertEqual(pinnedSites.count, 0, "There should be no pinned sites")
+                    return succeed()
+                }
+            }
+        }
+
+        addPinnedSite()
+            >>> checkPinnedSite
+            >>> removePinnedSite
             >>> done
 
-        waitForExpectations(timeout: 10.0) { error in
+        waitForExpectations(timeout: 3) { error in
             return
         }
     }