-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bugfix FXIOS-10996 Fix app crashes in HistoryPanel due to duplicate Site identifiers #24116
base: main
Are you sure you want to change the base?
Bugfix FXIOS-10996 Fix app crashes in HistoryPanel due to duplicate Site identifiers #24116
Conversation
…wift Int ID's larger than Int32 will crash.
… Improved existing tests and refactored force unwraps. Fixed misleading assert messages.
… protocol. This allows for automatic conformance to Equatable and Hashable, which should reduce programmer error and crashes related to diffable data sources.
…to use the new Site type.
…er, TabManagerMiddleware, ContextMenuHelper, SponsoredTileTelemetry, UnifiedAdsCallbackTelemetry, JumpBackInViewModel, HistoryPanelViewModel, and the WidgetExtension to use the new Site type.
…tension factory helper for creating a sponsored site using a Contile in the Client target,
…10996-refactor-history-panel-hashable-conformance # Conflicts: # firefox-ios/Storage/SQL/SQLiteHistoryFactories.swift
…ration from previous versions). Add documentation.
…tor-history-panel-hashable-conformance # Conflicts: # firefox-ios/Client/Frontend/Home/Homepage Rebuild/TopSites/TopSitesManager.swift # firefox-ios/Client/Frontend/Home/Homepage Rebuild/TopSites/TopSitesMiddleware.swift # firefox-ios/Client/Frontend/Library/Bookmarks/BookmarksViewController.swift # firefox-ios/Storage/SQL/SQLiteHistoryFactories.swift # firefox-ios/firefox-ios-tests/Tests/StorageTests/TestSQLitePinnedSites.swift
Client.app: Coverage: 32.38
CredentialProvider.appex: Coverage: 21.41
NotificationService.appex: Coverage: 25.99
ShareTo.appex: Coverage: 31.44
WidgetKitExtension.appex: Coverage: 7.07
libStorage.a: Coverage: 56.71
Generated by 🚫 Danger Swift against 3f75507 |
// MARK: - Encode & Decode | ||
|
||
public static func encode(with encoder: JSONEncoder, data: [Site]) throws -> Data { | ||
let storage = data.map { site in | ||
return site.storage | ||
} | ||
return try encoder.encode(storage) | ||
} | ||
|
||
public static func decode(from decoder: JSONDecoder, data: Data) throws -> [Site] { | ||
let storage = try decoder.decode([Storage].self, from: data) | ||
return storage.map { | ||
return Site(from: $0) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the previous custom encoding logic
// Created since to avoid making Sites Codable which involes making also PageMetadata and Visit Codable too | ||
private struct Storage: Codable { | ||
let resource: SiteResource? | ||
let title: String | ||
let id: Int? | ||
let guid: String? | ||
let url: String | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was used for the custom encoding logic and was removed
// MARK: - Hashable | ||
extension Site: Hashable { | ||
public func hash(into hasher: inout Hasher) { | ||
// The == operator below must match the same requirements as this method | ||
hasher.combine(id) | ||
hasher.combine(guid) | ||
hasher.combine(title) | ||
hasher.combine(url) | ||
hasher.combine(faviconResource) | ||
} | ||
|
||
public static func == (lhs: Site, rhs: Site) -> Bool { | ||
// The hash method above must match the same requirements as this operator | ||
return lhs.id == rhs.id | ||
&& lhs.guid == rhs.guid | ||
&& lhs.title == rhs.title | ||
&& lhs.url == rhs.url | ||
&& lhs.faviconResource == rhs.faviconResource | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These were the removed methods we want to auto synthesize to hopefully help with the crash
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some thoughts!
firefox-ios/Client/Frontend/Home/Homepage Rebuild/TopSites/TopSitesManager.swift
Show resolved
Hide resolved
self.viewModel.profile.places.deleteBookmarksWithURL(url: site.url) >>== { | ||
site.setBookmarked(false) | ||
} | ||
self.viewModel.profile.places.deleteBookmarksWithURL(url: site.url) >>== {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This syntactical sugar is beyond me but do we still need the deferment chaos here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😅 Looks like these are the options:
_ = self.viewModel.profile.places.deleteBookmarksWithURL(url: site.url)
and
self.viewModel.profile.places.deleteBookmarksWithURL(url: site.url) >>== {}
I'll go with the first one since it's slightly shorter / more obvious... and going to add a comment too.
// Return our bundled G icon for all of the Google Suite. | ||
// Parse example: "https://drive.google.com/drive/home" > "drive.google.com" > "google" | ||
imageResource = GoogleTopSiteManager.Constants.faviconResource | ||
switch topSite.site.type { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I kinda wish this var was topSiteState instead of topsite or we could put a var on TopSiteState
:
var type: SiteType {
return site.type
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, will add that!
case .pinnedSite, .suggestedSite: | ||
imageResource = topSite.site.faviconResource | ||
default: | ||
if let siteURL = URL(string: siteURLString), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this piece actually work? Isn't the google site a pinned site?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was to override ALL google favicons with our embedded google image (since google has notoriously low quality favicons for some reason)... The pinned google tile should have a faviconResource
already attached. (Actually, funny story, it didn't, but thanks to this comment I double checked and fixed it at the call site in GoogleTopSiteManager.swift 😂 )
firefox-ios/Client/Frontend/Home/TopSites/DataManagement/TopSitesDataAdaptor.swift
Outdated
Show resolved
Hide resolved
...os/Client/Frontend/Home/TopSites/DataManagement/UnifiedAds/UnifiedAdsCallbackTelemetry.swift
Outdated
Show resolved
Hide resolved
if let domainMap = DefaultSuggestedSites.urlMap[site.url], | ||
let localizedURL = domainMap[locale.identifier] { | ||
return Site(copiedFromSite: site, withLocalizedURLString: localizedURL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be a suggested Site instead of the default initializer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call out, since this looks wrong at a glance. But I think we're ok here. This copies the current Site
(so .suggestedSite
remains the same) and just updates the (nonmutable) url
property on Site
... a little hokey but I really didn't want url
to be a var
on the struct.
@@ -722,7 +722,7 @@ extension RustPlaces { | |||
} | |||
// Note: FXIOS-10740 Necessary to have unique Site ID iOS 18 HistoryPanel crash with diffable data sources | |||
let hashValue = "\(info.url)_\(info.timestamp)".hashValue | |||
let site = Site(id: hashValue, url: info.url, title: title) | |||
var site = Site(id: hashValue, url: info.url, title: title, type: .basic) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sometimes we are using the default initializer and sometimes we are using the static funcs to initialize... should we just pick one and make the others private?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm yes I see what you mean...
This is one of the rare places we explicitly want to define the ID rather than auto generate one, which is why it's like this...
I'm going to make the default init private, and we can add an optional first ID param to the createBasicSite factory method.
return URL(string: url, invalidCharacters: false) ?? URL(string: "about:blank")! | ||
default: | ||
return URL(string: url, invalidCharacters: false)?.domainURL ?? URL(string: "about:blank")! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this preferred to just having an optional url? I just worry if this is nil then the issue is obfuscated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I honestly have no idea. Everyone wants to remove the force unwraps but I just copied what was already in the app and moved it here... 😬 I'm open to suggestions. 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm I guess I would alternatively just have the optional value returned here and then we could unwrap it somewhere else more meaningful? But also this is fine for now because I imagine that is a bigger change.
public var isPinnedSite: Bool { | ||
switch self { | ||
case .pinnedSite: | ||
return true | ||
default: | ||
return false | ||
} | ||
} | ||
|
||
public var isSponsoredSite: Bool { | ||
switch self { | ||
case .sponsoredSite: | ||
return true | ||
default: | ||
return false | ||
} | ||
} | ||
|
||
public var isSuggestedSite: Bool { | ||
switch self { | ||
case .suggestedSite: | ||
return true | ||
default: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be overkill but it might be nice to have these helpers in Site so that instead of accessing site.type.isSuggestedSite
we could just access site.isSuggestedSite
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, I'll add that! 👍
This pull request has conflicts when rebasing. Could you fix it @ih-codes? 🙏 |
…s to create sites.
…tor-history-panel-hashable-conformance # Conflicts: # firefox-ios/Storage/DefaultSuggestedSites.swift
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, made the requested changes, ready for another look! cc @Cramsden
// Return our bundled G icon for all of the Google Suite. | ||
// Parse example: "https://drive.google.com/drive/home" > "drive.google.com" > "google" | ||
imageResource = GoogleTopSiteManager.Constants.faviconResource | ||
switch topSite.site.type { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, will add that!
self.viewModel.profile.places.deleteBookmarksWithURL(url: site.url) >>== { | ||
site.setBookmarked(false) | ||
} | ||
self.viewModel.profile.places.deleteBookmarksWithURL(url: site.url) >>== {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😅 Looks like these are the options:
_ = self.viewModel.profile.places.deleteBookmarksWithURL(url: site.url)
and
self.viewModel.profile.places.deleteBookmarksWithURL(url: site.url) >>== {}
I'll go with the first one since it's slightly shorter / more obvious... and going to add a comment too.
...os/Client/Frontend/Home/TopSites/DataManagement/UnifiedAds/UnifiedAdsCallbackTelemetry.swift
Outdated
Show resolved
Hide resolved
if let domainMap = DefaultSuggestedSites.urlMap[site.url], | ||
let localizedURL = domainMap[locale.identifier] { | ||
return Site(copiedFromSite: site, withLocalizedURLString: localizedURL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call out, since this looks wrong at a glance. But I think we're ok here. This copies the current Site
(so .suggestedSite
remains the same) and just updates the (nonmutable) url
property on Site
... a little hokey but I really didn't want url
to be a var
on the struct.
return URL(string: url, invalidCharacters: false) ?? URL(string: "about:blank")! | ||
default: | ||
return URL(string: url, invalidCharacters: false)?.domainURL ?? URL(string: "about:blank")! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I honestly have no idea. Everyone wants to remove the force unwraps but I just copied what was already in the app and moved it here... 😬 I'm open to suggestions. 😆
public var isPinnedSite: Bool { | ||
switch self { | ||
case .pinnedSite: | ||
return true | ||
default: | ||
return false | ||
} | ||
} | ||
|
||
public var isSponsoredSite: Bool { | ||
switch self { | ||
case .sponsoredSite: | ||
return true | ||
default: | ||
return false | ||
} | ||
} | ||
|
||
public var isSuggestedSite: Bool { | ||
switch self { | ||
case .suggestedSite: | ||
return true | ||
default: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, I'll add that! 👍
firefox-ios/Client/Frontend/Home/TopSites/DataManagement/TopSitesDataAdaptor.swift
Outdated
Show resolved
Hide resolved
@@ -722,7 +722,7 @@ extension RustPlaces { | |||
} | |||
// Note: FXIOS-10740 Necessary to have unique Site ID iOS 18 HistoryPanel crash with diffable data sources | |||
let hashValue = "\(info.url)_\(info.timestamp)".hashValue | |||
let site = Site(id: hashValue, url: info.url, title: title) | |||
var site = Site(id: hashValue, url: info.url, title: title, type: .basic) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm yes I see what you mean...
This is one of the rare places we explicitly want to define the ID rather than auto generate one, which is why it's like this...
I'm going to make the default init private, and we can add an optional first ID param to the createBasicSite factory method.
case .pinnedSite, .suggestedSite: | ||
imageResource = topSite.site.faviconResource | ||
default: | ||
if let siteURL = URL(string: siteURLString), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was to override ALL google favicons with our embedded google image (since google has notoriously low quality favicons for some reason)... The pinned google tile should have a faviconResource
already attached. (Actually, funny story, it didn't, but thanks to this comment I double checked and fixed it at the call site in GoogleTopSiteManager.swift 😂 )
firefox-ios/Client/Frontend/Home/Homepage Rebuild/TopSites/TopSitesManager.swift
Show resolved
Hide resolved
@Cramsden Argh, looks like some more merge conflicts on this branch -- will fix tomorrow morning. Just FYI if you re-review tonight. |
This pull request has conflicts when rebasing. Could you fix it @ih-codes? 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are some merge conflicts that need to be resolved but the additional changes look good to me!
📜 Tickets
Jira ticket
Github issue
💡 Description
This PR refactors the
Site
class and its inheriting classes to instead be aSite
struct which synthesizes itsEquatable
andHashable
methods.The purpose of this change is to clean up some old code, and ultimately ensure all
Site
s are unique and strictlyIdentifiable
for diffing (as required on iOS 18 with the History Panel diffable data source).This work should hopefully resolve crashes we are seeing on Sentry due to "duplicate" Sites, which is a regression on a past fix in this PR: #23494 (possibly caused by a backport related to the xcode 16 upgrade).
Main Changes:
Site
object was built as a class, with several types inheriting from it and adding additional functionality (suggested sites, sponsored tiles, pinned sites…). This felt a bit non-swifty. I tried making Site a protocol at first, but ran into issues with making other types that hadany Site
Equatable
, as a concrete type was needed. So I opted to make an enum to identify eachSiteType
.Site
is inside theStorage
target, it was necessary for me to move the associated types for suggested, sponsored, and pinned sites to Storage as well (as Storage cannot access Client).Site
implementation to useCodable
instead of custom encode/decode methods (used for theWidgetExtension
)Site
implementation to automatically synthesizeHashable
andEquatable
conformance vs. explicit implementations, which has caused bugs in the past due to the implementations not being symmetricalTesting
Test cases are listed in the attached JIRA ticket. 🙏
📝 Checklist
You have to check all boxes before merging
@Mergifyio backport release/v120
)