Refactor FXIOS-12831 [Swift 6 Migration] MainActor TabManagerMiddleware #29768

lmarceau · 2025-10-03T00:54:16Z

📜 Tickets

💡 Description

Second attempt at this, first one was here.

Now we do get the compiler errors. This is under tab peek (if I remove the ensureMainThread which was added a week ago or so):

And this is another error we didn't catch as well last time around:

cc: @Cramsden @ih-codes

📝 Checklist

I filled in the ticket numbers and a description of my work
I updated the PR name to follow our PR naming guidelines
I ensured unit tests pass and wrote tests for new code
If working on UI, I checked and implemented accessibility (Dynamic Text and VoiceOver)
If adding telemetry, I read the data stewardship requirements and will request a data review
If adding or modifying strings, I read the guidelines and will request a string review from l10n
If needed, I updated documentation and added comments to complex code

mobiletest-ci-bot · 2025-10-03T13:21:43Z

	Messages
📖	Project coverage: 38.42%

🧹 Tidy commit

Just 1 file(s) touched. Thanks for keeping it clean and review-friendly!

💬 Description craftsman

Great PR description! Reviewers salute you 🫡

❌ Per-file test coverage gate

The following changed file(s) are below 35.0% coverage:

File	Coverage	Required
`firefox-ios/Client/Frontend/Browser/Tabs/Middleware/TabManagerMiddleware.swift`	25.6%	35.0%

Client.app: Coverage: 37.2

File	Coverage
TabManagerMiddleware.swift	25.6%	⚠️

Generated by 🚫 Danger Swift against 52db608

ih-codes

Nice cleanup, and thank goodness for finally seeing the correct compiler errors! 👏

firefox-ios/Client/Frontend/Browser/Tabs/Middleware/TabManagerMiddleware.swift

lmarceau · 2025-10-06T15:27:02Z

Those same two tests fails every time, but they pass locally. I am not sure what's up there

clarmso

ReadingListsTests fail on my end as well. The tests are known to be passing consistently.

ih-codes

I've pulled down to test the crash and it's pretty crazy to me the compiler gives zero warnings on this. I want to test a bit in a brand new Swift 6.2 project and/or playground just to replicate the issue to see if I get anything there.

What do you think changing the group.notify to dispatch to the main thread? That seems the real issue here (the mix of GCD and Concurrency). Then we could put an assert instead of ensureMainThread in the fetchProfileTabInfo completion... because if concurrency was working correctly for us here we shouldn't have to do that. 😭

private func fetchProfileTabInfo(...) {
   ...
   group.notify(queue: dataQueue) // <- main queue instead?

ih-codes · 2025-10-08T16:38:25Z

firefox-ios/Client/Frontend/Browser/Tabs/Middleware/TabManagerMiddleware.swift

    ///   - isPrivate: if the tab should be created in private mode or not
    private func addNewTab(with urlRequest: URLRequest?, isPrivate: Bool, showOverlay: Bool, for uuid: WindowUUID) {
-        assert(Thread.isMainThread)
+        MainActor.assertIsolated("Expected to be called only on main actor.")


I had to double check that this gets removed at prod compile time, it sounds like it does!

clarmso

🚀 Woohoo the issue in tests has been resolved.

ih-codes

Just wanted to update here for visibility. After some testing, it seems the only scenario we do NOT get the appropriate build warnings in Xcode is due to the GCD DispatchGroup.notify call. For whatever reason, this call does not require the closure to be marked @Sendable, which in turn would have let us know that the closure might not execute on the main thread (which is what caused the crash @lmarceau saw in the UI tests). If we used instead DispatchQueue.global().async we get the expected warnings to at least tip us off to a runtime crash problem.

I've opened a discussion on the Apple Developer Forums about this last week... haven't heard anything yet.

Anyway, for now I think I'd like to see the dataLoadingCompletion closure marked @Sendable in the fetchProfileTabInfo function definition, and for the notify to dispatch deliberately to the main thread (instead of the background queue). It's a little weird to thread hop for that like we're doing anyway.

Maybe we can also put a main thread assertion in the fetchProfileTabInfo callback implementation and add a comment referring back to this PR for the discussion points. (Also still good to continue to use the ensureMainThread as well as backup 👌 ).

ih-codes

Thanks @lmarceau for your iterations on this one! 🚀

lmarceau · 2025-10-14T21:11:14Z

Created the task for follow up investigation on notify here

…re (#29768) * Tab manager middleware * fetchProfileTabInfo completion needs to be called on the main thread * Update following comment

@mainactor

…Actor` isolation (#30208) * Refactor FXIOS-12831 [Swift 6 Migration] MainActor TabManagerMiddleware (#29768) * Tab manager middleware * fetchProfileTabInfo completion needs to be called on the main thread * Update following comment * Refactor FXIOS-12831 [Swift 6 Migration] MainActor ToolbarMiddleware (#30014) Toolbar middleware Missing microsurvey to be tested * Ih/fxios 12831 search engine selection middleware 2 (#30026) * Isolate the SearchEngineSelectionMiddleware to the main actor. * Update unit tests. * Tweak mocks so the SearchEnginesManagerTests unit tests pass under the same threading assumptions. * Refactor FXIOS-12831 [Swift 6 Migration] MainActor TopSitesMiddleware & HomepageMiddleware (#30039) Migration for those two middlewares * Refactor FXIOS-12831 [Swift 6 Migration] MainActor SummarizerMiddleware (#30042) Migration * Refactor FXIOS-12831 [Swift 6 Migration] MainActor MainMenuMiddleware (#30013) main menu middleware is now main actor * Refactor FXIOS-12831 [Swift 6 Migration] MainActor TermsOfUseMiddleware & StartAtHomeMiddleware (#30061) Migration * Refactor FXIOS-12831 [Swift 6 Migration] @mainactor MessageCardMiddleware & ShortcutsLibraryMiddleware (#30064) Migration * Refactor FXIOS-12831 [Swift 6 Migration] MainActor MicrosurveyMiddleware & MicrosurveyPromptMiddleware (#30041) * MIgration * Adjust test * Refactor FXIOS-12831 [Swift 6 Migration] MainActor ThemeMiddleware & FakeReduxMiddleware (#30040) Migration * Refactor FXIOS-12831 [Swift 6 Migration] FeltPrivacyMiddleware & BookmarksMiddleware (#30083) Migration * Refactor FXIOS-12831 [Swift 6 Migration] NativeErrorPageMiddleware & TrackingProtectionMiddleware (#30088) * Migration * Native error page VC migration * Fix VC/coordinator not getting deinit * Revert dispatch legacy changes * Refactor FXIOS-12912 [Swift 6] Make Redux States sendable and Reducers @mainactor isolated (#30081) * Make Redux State Sendable and Reducers MainActor isolated * disambiguate passthrough state and default state * Fix tests * PR feedback * Refactor FXIOS-12831 [Swift 6 Migration] RemoteTabsPanelMiddleware & WallpaperMiddleware MainActor (#30117) Migration * Refactor FXIOS-12831 [Swift 6 Migration] PasswordGeneratorMiddleware actor isolation (#30085) * Isolate PasswordGeneratorMiddleware to the main actor. * Add @mainactor annotations to our WebKit JS evaluation wrappers. * Refactor FXIOS-12831 [Swift 6 Migration] MerinoMiddleware actor isolation (#30084) Isolate MerinoMiddleware to the main actor. --------- Co-authored-by: lmarceau <[email protected]> Co-authored-by: Carson Ramsden <[email protected]>

lmarceau requested a review from ih-codes October 3, 2025 00:54

lmarceau requested a review from a team as a code owner October 3, 2025 00:54

ih-codes approved these changes Oct 3, 2025

View reviewed changes

firefox-ios/Client/Frontend/Browser/Tabs/Middleware/TabManagerMiddleware.swift Show resolved Hide resolved

lmarceau force-pushed the lm/TabManagerMiddleware branch from 57ea2c1 to b80d510 Compare October 3, 2025 17:45

lmarceau requested review from a team and issammani as code owners October 3, 2025 17:45

lmarceau requested review from clarmso and removed request for a team October 3, 2025 17:45

lmarceau force-pushed the lm/TabManagerMiddleware branch 2 times, most recently from 84f7941 to a2c6dc6 Compare October 6, 2025 14:50

clarmso requested changes Oct 6, 2025

View reviewed changes

lmarceau force-pushed the lm/TabManagerMiddleware branch from a2c6dc6 to 5b10e38 Compare October 8, 2025 14:25

lmarceau requested review from clarmso and ih-codes October 8, 2025 16:06

ih-codes reviewed Oct 8, 2025

View reviewed changes

clarmso approved these changes Oct 9, 2025

View reviewed changes

ih-codes requested changes Oct 14, 2025

View reviewed changes

lmarceau added 3 commits October 14, 2025 15:25

Tab manager middleware

a102f0c

fetchProfileTabInfo completion needs to be called on the main thread

25147ed

Update following comment

52db608

lmarceau force-pushed the lm/TabManagerMiddleware branch from 5b10e38 to 52db608 Compare October 14, 2025 19:47

lmarceau requested a review from ih-codes October 14, 2025 19:47

ih-codes approved these changes Oct 14, 2025

View reviewed changes

lmarceau merged commit 9aebabd into epic-branch/swift-6-redux-migration Oct 14, 2025
9 checks passed

lmarceau deleted the lm/TabManagerMiddleware branch October 14, 2025 21:09

lmarceau added a commit that referenced this pull request Oct 16, 2025

Refactor FXIOS-12831 [Swift 6 Migration] MainActor TabManagerMiddlewa…

50dd6fb

…re (#29768) * Tab manager middleware * fetchProfileTabInfo completion needs to be called on the main thread * Update following comment

lmarceau added a commit that referenced this pull request Oct 16, 2025

Refactor FXIOS-12831 [Swift 6 Migration] MainActor TabManagerMiddlewa…

ed0e5e5

…re (#29768) * Tab manager middleware * fetchProfileTabInfo completion needs to be called on the main thread * Update following comment

lmarceau added a commit that referenced this pull request Oct 17, 2025

Refactor FXIOS-12831 [Swift 6 Migration] MainActor TabManagerMiddlewa…

b81779e

…re (#29768) * Tab manager middleware * fetchProfileTabInfo completion needs to be called on the main thread * Update following comment

lmarceau added a commit that referenced this pull request Oct 20, 2025

Refactor FXIOS-12831 [Swift 6 Migration] MainActor TabManagerMiddlewa…

599b246

…re (#29768) * Tab manager middleware * fetchProfileTabInfo completion needs to be called on the main thread * Update following comment

ih-codes pushed a commit that referenced this pull request Oct 28, 2025

Refactor FXIOS-12831 [Swift 6 Migration] MainActor TabManagerMiddlewa…

63389b7

…re (#29768) * Tab manager middleware * fetchProfileTabInfo completion needs to be called on the main thread * Update following comment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Refactor FXIOS-12831 [Swift 6 Migration] MainActor TabManagerMiddleware #29768

Refactor FXIOS-12831 [Swift 6 Migration] MainActor TabManagerMiddleware #29768

Uh oh!

lmarceau commented Oct 3, 2025

Uh oh!

mobiletest-ci-bot commented Oct 3, 2025 •

edited

Loading

Uh oh!

ih-codes left a comment

Uh oh!

Uh oh!

lmarceau commented Oct 6, 2025

Uh oh!

clarmso left a comment

Uh oh!

ih-codes left a comment

Uh oh!

ih-codes Oct 8, 2025

Uh oh!

clarmso left a comment

Uh oh!

ih-codes left a comment

Uh oh!

ih-codes left a comment

Uh oh!

Uh oh!

lmarceau commented Oct 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Refactor FXIOS-12831 [Swift 6 Migration] MainActor TabManagerMiddleware #29768

Refactor FXIOS-12831 [Swift 6 Migration] MainActor TabManagerMiddleware #29768

Uh oh!

Conversation

lmarceau commented Oct 3, 2025

📜 Tickets

💡 Description

📝 Checklist

Uh oh!

mobiletest-ci-bot commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🧹 Tidy commit

💬 Description craftsman

❌ Per-file test coverage gate

Client.app: Coverage: 37.2

Uh oh!

ih-codes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lmarceau commented Oct 6, 2025

Uh oh!

clarmso left a comment

Choose a reason for hiding this comment

Uh oh!

ih-codes left a comment

Choose a reason for hiding this comment

Uh oh!

ih-codes Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

clarmso left a comment

Choose a reason for hiding this comment

Uh oh!

ih-codes left a comment

Choose a reason for hiding this comment

Uh oh!

ih-codes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lmarceau commented Oct 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mobiletest-ci-bot commented Oct 3, 2025 •

edited

Loading