Refactor FXIOS-12796 [Swift 6 Migration] Fix concurrency warnings in history related classes and FxAWebViewModel #30259
Conversation
💪 Quality guardian3 tests files modified. You're a champion of test coverage! 🚀 🥇 Perfect PR sizeSmaller PRs are easier to review. Thanks for making life easy for reviewers! ✨ 💬 Description craftsmanGreat PR description! Reviewers salute you 🫡 ❌ Per-file test coverage gateThe following changed file(s) are below 35.0% coverage:
Client.app: Coverage: 37.1
Generated by 🚫 Danger Swift against bd7a120 |
| viewModel.reloadData { [weak self] success in | ||
| DispatchQueue.main.async { | ||
| viewModel.reloadData { success in | ||
| DispatchQueue.main.async { [weak self] in |
There was a problem hiding this comment.
I don't think you need the weak self here too but I guess it is also fine if you want to have it. Do we want to be using ensureMainThread here?
There was a problem hiding this comment.
I just left it in because it was already there (but moved because of a warning).
Looks like the viewModel calls an async fetch to get fresh history items though, so it's probably good to keep this weak in case the user navigates away from the screen while loading is happening.
I can update to ensureMainThread but it will never be called on the main thread from that fetch from the looks of it! (rust Deferred code on background queues I believe)
| final class HistoryDeletionUtility: HistoryDeletionProtocol, Sendable { | ||
| private let profile: Profile | ||
| // FIXME: FXIOS-13957 All properties should be truly Sendable | ||
| private nonisolated(unsafe) let gleanWrapper: GleanWrapper |
There was a problem hiding this comment.
How come this needs to be nonisolated(unsafe)?
There was a problem hiding this comment.
The underlying Glean type is not Sendable. 😅
I did ask them if the Glean singleton was Sendable a while back and they were like 94% sure it was thread safe. 👀 Here is the bugzilla: https://bugzilla.mozilla.org/show_bug.cgi?id=1978613
There was a problem hiding this comment.
I think I'll move the nonisolated(unsafe) up one layer so the GleanWrapper appears Sendable in the meantime. Makes more sense I think!
Cramsden left a comment
Cramsden left a comment There was a problem hiding this comment.
two small questions but seems fine to me!
| Hm.
But in Xcode if I remove the Glean is definitely not  I guess I need to update to Xcode 26.0.1 to see if that's the cause of the discrepancy. |
ih-codes force-pushed the ih/FXIOS-12796-history-and-fxAWebViewModel-errors branch from 583e112 to 80d1fab Compare | Ok, I updated to Xcode 26.0.1 and I see the warning now. Seems more like my local Glean did not update until I did a deep clean. Anyway, I think this should be fixed now, we'll see what Bitrise says. 🤞 |
ih-codes deleted the ih/FXIOS-12796-history-and-fxAWebViewModel-errors branch 
| 🚀 PR merged to |
📜 Tickets
Jira ticket
Github issue
💡 Description
Just picking up some random warnings here and there. This PR fixes strict concurrency warnings in history related classes, as well as FxAWebViewModel.
Examples of some of the warnings fixed:
Created a few followup tickets in Epic 2
cc @Cramsden @lmarceau @dataports | Swift 6 Migration
📝 Checklist