Refactor FXIOS-13450 [Swift 6] Fix strict concurrency warnings in Tab Manager #29257

Cramsden · 2025-09-10T20:05:17Z

📜 Tickets

💡 Description

This PR cleans up a lot of the functionality in the TabManager that is marked as async but does not need to be in order to simplify the general logic.

This PR also isolates the TabManager protocol to the MainActor

This PR fixes any other warnings that were in TabManager with strict concurrency on

🎥 Demos

Before	After

Demo

📝 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 needed, I updated documentation and added comments to complex code
If needed, I added a backport comment (example @Mergifyio backport release/v150.0)

lmarceau

First round of review! I want to pull the PR locally as well tomorrow morning

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

firefox-ios/Client/Frontend/Browser/TopTabDisplayManager.swift

firefox-ios/Client/TabManagement/TabManager.swift

lmarceau · 2025-09-10T20:25:36Z

firefox-ios/Client/TabManagement/TabManagerImplementation.swift

+    private nonisolated let tabDataStore: TabDataStore
    private let tabSessionStore: TabSessionStore
-    private let imageStore: DiskImageStore?
+    private nonisolated let imageStore: DiskImageStore?


Same question with tabDataStore and imageStore, feels like we'll need to work on those to improve?

These are both actors so they are sendable and should not need to be isolated to the main actor

Can we mark these 2 protocols as requiring an Actor then to be safe? 👀

protocol Foo : Actor { }

I also noticed that if we don't do that, we don't get this error, which actually sounds like something we should care about?

EXCEPT I noticed that fetchWindowDataUUIDs is marked nonisolated in the implementation, so it should also be in the protocol, and then that warning goes away.

hmmm I don't see this warning at all. I agree the protocol method should be marked as nonisolated. I dont know if I totally see the benefit of enforcing the protocol to be an actor. It shouldn't have to be it just needs to be Thread safe in one way or another.

When I tried marking the protocol as Actor isolated, I noticed the compiler picked up places we called the actor (via the let tabDataStore: TabDataStore property) and suggested that isolated methods needed an await. If you add the Actor to TabDataStore protocol you'll see what I mean. Does this mean we could potentially be calling actor-isolated methods without an await, if the protocol doesn't specify that an actor implements it?? 😬 That's my fear!

I think that this actually works because of the structure of the protocol... and we we actually don't need to specify the nonisolated because of the way the protocol is structured. All isolated methods are async and all non isolated methods are not marked as async. If the protocol has a function that breaks the actor isolation we will get a compiler error:

firefox-ios/Client/TabManagement/TabManagerImplementation.swift

lmarceau · 2025-09-10T20:27:51Z

firefox-ios/Client/Telemetry/SearchTelemetry.swift

@@ -138,7 +139,8 @@ enum SearchTelemetryValues {
    }
 }

-class SearchTelemetry {
+// TODO: FXIOS Make SearchTelemetry actually sendable


Needs ticket number 👀

firefox-ios/Client/Telemetry/SearchTelemetry.swift

Cramsden · 2025-09-10T20:54:05Z

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

+        tabManager.undoCloseInactiveTabs()
+        let inactiveTabs = self.refreshInactiveTabs(uuid: uuid)
+        let refreshAction = TabPanelMiddlewareAction(inactiveTabModels: inactiveTabs,
+                                                     windowUUID: uuid,
+                                                     actionType: TabPanelMiddlewareActionType.refreshInactiveTabs)
+        store.dispatchLegacy(refreshAction)


@cyndichin I removed this task since I was hoping that now that this is all isolated to the main actor we might not need it anymore? Would you be willing to help me test?

ih-codes

First pass, I'll also test more tomorrow!

firefox-ios/Client/AccountSyncHandler.swift

ih-codes · 2025-09-10T21:02:41Z

firefox-ios/Client/Frontend/Browser/Search/SearchViewModel.swift

@@ -185,30 +186,32 @@ class SearchViewModel: FeatureFlaggable, LoaderListener {
        let tempSearchQuery = searchQuery
        suggestClient?.query(searchQuery,


I'm hoping later we can make the callback @MainActor

firefox-ios/Client/Frontend/Browser/TopTabDisplayManager.swift

ih-codes · 2025-09-10T21:15:39Z

firefox-ios/Client/TabManagement/TabManagerImplementation.swift

+    private nonisolated let tabDataStore: TabDataStore
    private let tabSessionStore: TabSessionStore
-    private let imageStore: DiskImageStore?
+    private nonisolated let imageStore: DiskImageStore?


Can we mark these 2 protocols as requiring an Actor then to be safe? 👀

protocol Foo : Actor { }

I also noticed that if we don't do that, we don't get this error, which actually sounds like something we should care about?

EXCEPT I noticed that fetchWindowDataUUIDs is marked nonisolated in the implementation, so it should also be in the protocol, and then that warning goes away.

ih-codes · 2025-09-10T21:17:46Z

firefox-ios/Client/TabManagement/TabManagerImplementation.swift

@@ -554,20 +537,17 @@ class TabManagerImplementation: NSObject,

    private func restoreTabs() {
        tabs = [Tab]()
-        Task {
+        Task { @MainActor in


ih-codes · 2025-09-10T21:18:28Z

firefox-ios/Client/TabManagement/TabManagerImplementation.swift

-            }
+            buildTabRestore(window: windowData)
+            TabErrorTelemetryHelper.shared.validateTabCountAfterRestoringTabs(windowUUID)
+            // Log on main thread, where computed `tab` properties can be accessed without risk of races


Guess we don't need this comment anymore probably 😌

ih-codes · 2025-09-10T21:21:33Z

firefox-ios/firefox-ios-tests/Tests/ClientTests/TabManagement/TabManagerTests.swift

@@ -53,6 +53,7 @@ class TabManagerTests: XCTestCase {
        super.tearDown()
    }

+    @MainActor


This might be a good file to just annotate the entire test class since all the tab manager tests need @MainActor

My personal preference was that each test be marked as main actor so tests aren't automatically put on the main thread if they don't mean to be? but I can make this update if you prefer.

I don't have a strong preference, but it just seems easier if the entire test suite is for an object that is main actor isolated to mark the whole test class instead of a lot of functions. But I agree if it's just a few test cases it's better to mark just the tests. I don't have strong feelings either way!

ih-codes · 2025-09-10T21:24:15Z

...ios-tests/Tests/ClientTests/Frontend/Browser/BrowserViewControllerWebViewDelegateTests.swift

@@ -187,7 +187,9 @@ class BrowserViewControllerWebViewDelegateTests: XCTestCase {
        subject.webView(tab.webView!,
                        decidePolicyFor: MockNavigationAction(url: url,
                                                              type: .linkActivated)) { _ in
-            XCTAssertNotNil(subject.pendingRequests[url.absoluteString])
+            ensureMainThread {
+                XCTAssertNotNil(subject.pendingRequests[url.absoluteString])


This test seems odd, without an expectation I don't imagine this ever executes anyway 😆

lol ya this test does nothing.

ih-codes · 2025-09-11T15:42:56Z

I think your latest warning fixes look good! ✅

I think my comment here should be addressed though. This is a bit of an interesting case... if an actor implements a protocol it seems that the compiler doesn't know that via the protocol definition so it might not know to await calls. 👀 I'll add a topic to the agenda for this since it's the first time I'm seeing it!

FilippoZazzeroni

@Cramsden just some question and curiosities around the implementation. Overall looks good to me, but maybe extra eye are needed.

FilippoZazzeroni · 2025-09-11T15:50:38Z

firefox-ios/Client/AccountSyncHandler.swift

@@ -82,9 +82,15 @@ class AccountSyncHandler: TabEventHandler {
    /// To prevent multiple tab actions to have a separate syncs, we sync after 5s of no tab activity
    private func performClientsAndTabsSync() {
        guard profile.hasSyncableAccount() else { return }
-        debouncer.call { [weak self] in self?.storeTabs() }
+        debouncer.call { [weak self] in
+            guard let self = self else {return }


Question, do we need to retain self here ?

I also don't think this solves the compiler warning from before @Cramsden, I'm still seeing the Sendable self issue

ya it doesn't... I will take it out

FilippoZazzeroni · 2025-09-11T15:52:22Z

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

@@ -24,6 +24,7 @@ final class TabManagerMiddleware: FeatureFlaggable {
    private let summarizerServiceFactory: SummarizerServiceFactory
    private let tabsPanelTelemetry: TabsPanelTelemetry

+    @MainActor


Just a curiosity, why in these cases we need MainActor on all methods marking the class isn't enough right ?

It is preferred for middlewares to isolate the whole class to the main actor but at this point I don't want the provider to be main actor isolated because we will want the protocol for middlewares to be isolated to the main actor and that will effect them all. This is just an interim step!

ih-codes

I'm going to approve because I want these changes on my branch and any other concerns I had can be resolved later! 👌 Nice work!! This was an annoying one to fix I bet haha.

mobiletest-ci-bot · 2025-09-11T17:31:54Z

	Messages
📖	Project coverage: 37.29%
📖	Edited 39 files
📖	Created 0 files

Client.app: Coverage: 36.96

File	Coverage
BrowserViewController+TabToolbarDelegate.swift	1.85%	⚠️
TabManagerMiddleware.swift	27.5%	⚠️
TabErrorTelemetryHelper.swift	12.71%	⚠️
TopTabDisplayManager.swift	15.53%	⚠️
SearchTelemetry.swift	5.17%	⚠️
NoImageModeHelper.swift	12.12%	⚠️
NightModeHelper.swift	30.77%	⚠️
ReaderPanel.swift	29.33%	⚠️
SearchViewModel.swift	52.68%	✅
AccountSyncHandler.swift	86.47%	✅
MessageCardMiddleware.swift	98.63%	✅
TabToolbarHelper.swift	47.81%	⚠️
WindowManager.swift	84.87%	✅
SummarizeMiddleware.swift	84.62%	✅
TabManager.swift	80.0%	✅
WindowSimpleTabsCoordinator.swift	95.0%	✅
ToolbarMiddleware.swift	93.01%	✅
TabManagerImplementation.swift	53.45%	✅

libStorage.a: Coverage: 56.74

File	Coverage
DiskImageStore.swift	67.61%	✅

Generated by 🚫 Danger Swift against cacb5db

lmarceau · 2025-09-11T19:01:27Z

There seems to be one issue with the changes. If I have a lot of tabs open and then "Close all tabs" from the tab tray, the UI is frozen. You can open a bunch of tabs by using the option under the debug menu. I clicked it 3 times (and then you need to wait a bit so all the tabs are opened before you try to close all tabs).

Here's a video. After I click to close all tabs the UI is frozen for a good chunk of time.

Simulator.Screen.Recording.-.iPhone.16.-.2025-09-11.at.15.00.38.mp4

…y warnings

Cramsden · 2025-09-11T19:23:04Z

Looks like I got some additional commits in here that shouldn't be. Cleaning up the commit history now.

lmarceau · 2025-09-11T19:24:52Z

There seems to be one issue with the changes. If I have a lot of tabs open and then "Close all tabs" from the tab tray, the UI is frozen. You can open a bunch of tabs by using the option under the debug menu. I clicked it 3 times (and then you need to wait a bit so all the tabs are opened before you try to close all tabs). [...]

Turns out this is not a new issue, fun! LGTM with the PR changes, since Isabella said there will be follow up changes as well

mattreaganmozilla

I haven't run through this branch exhaustively yet, but nothing major jumped out. Just a few minor nits that we can address later.

mattreaganmozilla · 2025-09-11T19:50:56Z

firefox-ios/Client/Frontend/Browser/Toolbars/Redux/ToolbarMiddleware.swift

+        guard Thread.isMainThread else {
+            self.logger.log(
+                "MessageCardMiddleware is not being called from the main thread!",
+                level: .fatal,
+                category: .tabs
+            )


Very minor nit but this is probably worth an assert to make it more obvious and catch it during dev if it happens. NAB though

mattreaganmozilla · 2025-09-11T19:55:53Z

firefox-ios/firefox-ios-tests/Tests/ClientTests/DependencyInjection/DependencyHelperMock.swift

-                                                       uuid: ReservedWindowUUID(uuid: windowUUID, isNew: false),
-                                                       windowManager: windowManager)
+
+        var tabManager: TabManager!


Nit: I know it's annoying to point this out but ideally we should probably avoid this IUO even when we know it's safe, if for no other reason than consistent hygiene/etiquette. Definitely NAB though

Even in tests? I guess that is fair that crashing is not as clear as failing?

mattreaganmozilla · 2025-09-11T19:57:06Z

firefox-ios/firefox-ios-tests/Tests/ClientTests/DependencyInjection/DependencyHelperMock.swift

        if Thread.isMainThread {
            MainActor.assumeIsolated {
+                tabManager = injectedTabManager ?? TabManagerImplementation(
+                    profile: profile,
+                    uuid: ReservedWindowUUID(uuid: windowUUID, isNew: false),
+                    windowManager: windowManager
+                )
                AppContainer.shared.register(service: MockThemeManager() as ThemeManager)
            }
        } else {
            DispatchQueue.main.sync {
+                tabManager = injectedTabManager ?? TabManagerImplementation(
+                    profile: profile,
+                    uuid: ReservedWindowUUID(uuid: windowUUID, isNew: false),
+                    windowManager: windowManager
+                )
                AppContainer.shared.register(service: MockThemeManager() as ThemeManager)
            }
        }


Is the reason we can't use ensureMainThread here because of the sync (instead of async)?

Yes -- I originally put this in when doing the ThemeManager stuff. We have an open ticket for hopefully coming up with a better solution, I know you and I talked a bit about this too 😅 FXIOS-13151

I hypothesized that we'd want to ensure the setup is complete before running a test (hence the sync), especially tests and setUp() methods isolated to the main actor. (I didn't actually test to see if this causes flakiness though)

It is my hope that this can be isolated to the main actor but I think that is going to be a pretty big change.

dataports · 2025-09-12T00:40:39Z

firefox-ios/Client/Frontend/Toolbar+URLBar/TabToolbarHelper.swift

        ) { [weak self] _ in
-            self?.longPressGestureRecognizers.forEach { gesture in
-                gesture.minimumPressDuration = longPressDuration
+            guard let self = self else { return }


unimportant nit but you can shorten this to guard let self else { return }

dataports · 2025-09-12T00:41:56Z

firefox-ios/Client/Telemetry/SearchTelemetry.swift

-                                                        userInfo: nil,
-                                                        repeats: false)
+        impressionTelemetryTimer = Timer.scheduledTimer(withTimeInterval: 1.0, repeats: false, block: { [weak self] _ in
+            guard let self = self else { return }


unimportant nit but you can shorten this to guard let self else { return }

Cramsden requested a review from a team as a code owner September 10, 2025 20:05

Cramsden requested review from thatswinnie, dataports, lmarceau and ih-codes and removed request for a team September 10, 2025 20:05

lmarceau reviewed Sep 10, 2025

View reviewed changes

Cramsden commented Sep 10, 2025

View reviewed changes

ih-codes reviewed Sep 10, 2025

View reviewed changes

lmarceau self-requested a review September 10, 2025 23:16

Cramsden requested a review from FilippoZazzeroni September 11, 2025 15:12

FilippoZazzeroni reviewed Sep 11, 2025

View reviewed changes

ih-codes force-pushed the epic-branch/swift-migration branch from 3a72799 to 0dd835e Compare September 11, 2025 16:03

ih-codes requested a review from issammani as a code owner September 11, 2025 16:03

ih-codes approved these changes Sep 11, 2025

View reviewed changes

ih-codes mentioned this pull request Sep 11, 2025

Refactor FXIOS-12796 [Swift 6 Migration] Fix strict concurrency errors in QuickActions #29295

Open

7 tasks

Cramsden requested a review from mattreaganmozilla September 11, 2025 18:53

Cramsden added 9 commits September 11, 2025 13:22

Remove removeTabWithCompletion

ade2f1d

Isolate tab manager to main actor and fix remaining strict concurrenc…

ca6bdb2

…y warnings

additional clean up

dbb7db0

fix tests

9d373d2

more test clean up

1567958

clean up warnings

9c628a9

additional clean up

2d4d8d0

Additional clean up

b04fd94

remove unneeded main actor isolation

cacb5db

lmarceau approved these changes Sep 11, 2025

View reviewed changes

Cramsden force-pushed the cr/FXIOS-13450_strict-concurrency-in-tab-manager branch from f00aeb8 to cacb5db Compare September 11, 2025 19:29

mattreaganmozilla approved these changes Sep 11, 2025

View reviewed changes

dataports reviewed Sep 12, 2025

View reviewed changes

dataports approved these changes Sep 12, 2025

View reviewed changes

		@@ -185,30 +186,32 @@ class SearchViewModel: FeatureFlaggable, LoaderListener {
		let tempSearchQuery = searchQuery
		suggestClient?.query(searchQuery,

Refactor FXIOS-13450 [Swift 6] Fix strict concurrency warnings in Tab Manager #29257

Are you sure you want to change the base?

Refactor FXIOS-13450 [Swift 6] Fix strict concurrency warnings in Tab Manager #29257

Conversation

Cramsden commented Sep 10, 2025

📜 Tickets

💡 Description

🎥 Demos

📝 Checklist

Uh oh!

lmarceau left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ih-codes commented Sep 11, 2025

Uh oh!

FilippoZazzeroni left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ih-codes left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

ih-codes left a comment •

edited

Loading

mobiletest-ci-bot commented Sep 11, 2025 •

edited

Loading

lmarceau commented Sep 11, 2025 •

edited

Loading