Skip to content
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.

Sign up for GitHub

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

Refactor FXIOS-11147 [Tab Optomization Phase 1] Additional Tab Manager cleanup #24560

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+95 −111
Open

Refactor FXIOS-11147 [Tab Optomization Phase 1] Additional Tab Manager cleanup #24560

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3ab08db
Clean up remove tab logic in Tab Manager
Cramsden Feb 4, 2025
8ec54c5
call remove tab for removing one tab
Cramsden Feb 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions BrowserKit/Sources/Shared/NotificationConstants.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,6 @@ extension Notification.Name {

public static let WallpaperDidChange = Notification.Name("WallpaperDidChange")

public static let UpdateLabelOnTabClosed = Notification.Name("UpdateLabelOnTabClosed")

public static let TopTabsTabClosed = Notification.Name("TopTabsTabClosed")

public static let ShowHomepage = Notification.Name("ShowHomepage")
Expand Down
16 changes: 9 additions & 7 deletions ...Frontend/Browser/BrowserViewController/Extensions/BrowserViewController+KeyCommands.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,13 +195,15 @@ extension BrowserViewController {

@objc
func closeTabKeyCommand() {
TelemetryWrapper.recordEvent(category: .action,
method: .press,
object: .keyCommand,
extras: ["action": "close-tab"])
guard let currentTab = tabManager.selectedTab else { return }
tabManager.removeTab(currentTab)
keyboardPressesHandler().reset()
Task {
TelemetryWrapper.recordEvent(category: .action,
method: .press,
object: .keyCommand,
extras: ["action": "close-tab"])
guard let currentTab = tabManager.selectedTab else { return }
await tabManager.removeTab(currentTab.tabUUID)
keyboardPressesHandler().reset()
}
}

@objc
Expand Down
7 changes: 4 additions & 3 deletions ...d/Browser/BrowserViewController/Extensions/BrowserViewController+TabToolbarDelegate.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,9 +319,10 @@ extension BrowserViewController: TabToolbarDelegate, PhotonActionSheetProtocol {
iconString: StandardImageIdentifiers.Large.cross,
iconType: .Image) { _ in
if let tab = self.tabManager.selectedTab {
self.tabManager.removeTab(tab)
self.updateTabCountUsingTabManager(self.tabManager)
self.showToast(message: .TabsTray.CloseTabsToast.SingleTabTitle, toastAction: .closeTab)
self.tabManager.removeTabWithCompletion(tab.tabUUID) {
self.updateTabCountUsingTabManager(self.tabManager)
self.showToast(message: .TabsTray.CloseTabsToast.SingleTabTitle, toastAction: .closeTab)
}
}
}.items
}
Expand Down
11 changes: 6 additions & 5 deletions ...end/Browser/BrowserViewController/Extensions/BrowserViewController+WebViewDelegates.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,10 +186,11 @@ extension BrowserViewController: WKUIDelegate {
}

func webViewDidClose(_ webView: WKWebView) {
if let tab = tabManager[webView] {
// Need to wait here in case we're waiting for a pending `window.open()`.
DispatchQueue.main.asyncAfter(deadline: .now() + .milliseconds(100)) {
self.tabManager.removeTab(tab)
Task {
if let tab = tabManager[webView] {
// Need to wait here in case we're waiting for a pending `window.open()`.
try await Task.sleep(nanoseconds: NSEC_PER_MSEC * 100)
await tabManager.removeTab(tab.tabUUID)
Comment on lines +189 to +193
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was not able to test this because I am not sure how to get this function to fire https://mozilla.slack.com/archives/C05C9RET70F/p1738709743883439

}
}
}
Expand Down Expand Up @@ -549,7 +550,7 @@ extension BrowserViewController: WKNavigationDelegate {
if let currentTab = self?.tabManager.selectedTab,
currentTab.historyList.count == 1,
self?.isStoreURL(currentTab.historyList[0]) ?? false {
self?.tabManager.removeTab(tab)
self?.tabManager.removeTabWithCompletion(tab.tabUUID, completion: nil)
}
}
}
Expand Down
7 changes: 4 additions & 3 deletions firefox-ios/Client/Frontend/Browser/TopTabDisplayManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,12 +318,13 @@ class TopTabDisplayManager: NSObject {
// If it is the last tab of regular mode we automatically create an new tab
if !self.isPrivate,
tabsToDisplay.count == 1 {
self.tabManager.removeTabs([tab])
self.tabManager.selectTab(self.tabManager.addTab())
self.tabManager.removeTabWithCompletion(tab.tabUUID) {
self.tabManager.selectTab(self.tabManager.addTab())
}
return
}

self.tabManager.removeTab(tab)
self.tabManager.removeTabWithCompletion(tab.tabUUID, completion: nil)
}
}

Expand Down
2 changes: 1 addition & 1 deletion firefox-ios/Client/Frontend/Browser/TopTabsViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ class TopTabsViewController: UIViewController, Themeable, Notifiable, FeatureFla
}

deinit {
tabManager.removeDelegate(self.topTabDisplayManager)
tabManager.removeDelegate(self.topTabDisplayManager, completion: nil)
}

override func viewDidLoad() {
Expand Down
85 changes: 41 additions & 44 deletions firefox-ios/Client/TabManagement/TabManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,60 +18,62 @@ protocol TabManager: AnyObject {
var isRestoringTabs: Bool { get }
var delaySelectingNewPopupTab: TimeInterval { get }
var recentlyAccessedNormalTabs: [Tab] { get }
var tabs: [Tab] { get }
var count: Int { get }

var selectedTab: Tab? { get }
var backupCloseTab: BackupCloseTab? { get set }

var tabs: [Tab] { get }
var normalTabs: [Tab] { get } // Includes active and inactive tabs
var normalActiveTabs: [Tab] { get }
var inactiveTabs: [Tab] { get }
var privateTabs: [Tab] { get }

subscript(index: Int) -> Tab? { get }
subscript(webView: WKWebView) -> Tab? { get }

// MARK: - Add/Remove Delegate
func addDelegate(_ delegate: TabManagerDelegate)
func addNavigationDelegate(_ delegate: WKNavigationDelegate)
func removeDelegate(_ delegate: TabManagerDelegate, completion: (() -> Void)?)

// MARK: - Select Tab
func selectTab(_ tab: Tab?, previous: Tab?)
func addTab(_ request: URLRequest?, afterTab: Tab?, isPrivate: Bool) -> Tab
func addTabsForURLs(_ urls: [URL], zombie: Bool, shouldSelectTab: Bool, isPrivate: Bool)
func removeTab(_ tab: Tab, completion: (() -> Void)?)
func removeTabs(_ tabs: [Tab])
func undoCloseTab()
func getMostRecentHomepageTab() -> Tab?
func clearAllTabsHistory()
func cleanupClosedTabs(_ closedTabs: [Tab], previous: Tab?, isPrivate: Bool)
func reorderTabs(isPrivate privateMode: Bool, fromIndex visibleFromIndex: Int, toIndex visibleToIndex: Int)
func preserveTabs()
func restoreTabs(_ forced: Bool)
func startAtHomeCheck() -> Bool
func getTabForUUID(uuid: TabUUID) -> Tab?
func getTabForURL(_ url: URL) -> Tab?
func expireSnackbars()
@discardableResult
func switchPrivacyMode() -> SwitchPrivacyModeResult
func addPopupForParentTab(profile: Profile, parentTab: Tab, configuration: WKWebViewConfiguration) -> Tab

// MARK: - Add Tab
func addTabsForURLs(_ urls: [URL], zombie: Bool, shouldSelectTab: Bool, isPrivate: Bool)
@discardableResult
func addTab(_ request: URLRequest?,
afterTab: Tab?,
zombie: Bool,
isPrivate: Bool) -> Tab
// MARK: TabTray refactor interfaces

/// Async Remove tab option using tabUUID. Replaces direct usage of removeTab where the whole Tab is needed
// MARK: - Remove Tab
// TODO: FXIOS-11272 Remove this function in favor of the async remove tab.
/// GCD remove tab option using tabUUID with completion
/// - Parameters:
/// - tabUUID: UUID from the tab
/// - completion: closure called after remove tab completes on main thread
func removeTabWithCompletion(_ tabUUID: TabUUID, completion: (() -> Void)?)

/// Async Remove tab option using tabUUID.
/// - Parameter tabUUID: UUID from the tab
func removeTab(_ tabUUID: TabUUID) async

/// Async Remove all tabs indicating if is on private mode or not
/// - Parameter isPrivateMode: Is private mode enabled or not
func removeAllTabs(isPrivateMode: Bool) async

/// Removes all tabs matching the urls, used when other clients request to close tabs on this device.
func removeTabs(by urls: [URL]) async
func removeTabs(_ tabs: [Tab])

// MARK: - Undo Close
func undoCloseTab()
/// Undo close all tabs, it will restore the tabs that were backed up when the close action was called.
func undoCloseAllTabs()

/// Removes all tabs matching the urls, used when other clients request to close tabs on this device.
func removeTabs(by urls: [URL]) async
// MARK: Inactive Tabs

/// Get inactive tabs from the list of tabs based on the time condition to be considered inactive.
/// Replaces LegacyInactiveTabModel and related classes
Expand All @@ -84,38 +86,33 @@ protocol TabManager: AnyObject {

/// Undo all inactive tabs closure. All inactive tabs are added back to the list of tabs
func undoCloseInactiveTabs() async

// MARK: Get Tab
func getTabForUUID(uuid: TabUUID) -> Tab?
func getTabForURL(_ url: URL) -> Tab?
func getMostRecentHomepageTab() -> Tab?

// MARK: Other Tab Actions
func clearAllTabsHistory()
func reorderTabs(isPrivate privateMode: Bool, fromIndex visibleFromIndex: Int, toIndex visibleToIndex: Int)
func preserveTabs()
func restoreTabs(_ forced: Bool)
func startAtHomeCheck() -> Bool
func expireSnackbars()
@discardableResult
func switchPrivacyMode() -> SwitchPrivacyModeResult
func addPopupForParentTab(profile: Profile, parentTab: Tab, configuration: WKWebViewConfiguration) -> Tab
}

extension TabManager {
func removeDelegate(_ delegate: TabManagerDelegate) {
removeDelegate(delegate, completion: nil)
}

func selectTab(_ tab: Tab?) {
selectTab(tab, previous: nil)
}

func removeTab(_ tab: Tab) {
let uuid = windowUUID
removeTab(tab) {
NotificationCenter.default.post(name: .UpdateLabelOnTabClosed,
object: nil,
userInfo: uuid.userInfo)
}
}

func restoreTabs(_ forced: Bool = false) {
restoreTabs(forced)
}

func cleanupClosedTabs(_ closedTabs: [Tab],
previous: Tab?,
isPrivate: Bool = false) {
cleanupClosedTabs(closedTabs,
previous: previous,
isPrivate: isPrivate)
}

@discardableResult
func addTab(_ request: URLRequest? = nil,
afterTab: Tab? = nil,
Expand Down
19 changes: 6 additions & 13 deletions firefox-ios/Client/TabManagement/TabManagerImplementation.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,8 +238,10 @@ class TabManagerImplementation: NSObject, TabManager, FeatureFlaggable, TabEvent
)
}

func removeTab(_ tab: Tab, completion: (() -> Void)? = nil) {
guard let index = tabs.firstIndex(where: { $0 === tab }) else { return }
func removeTabWithCompletion(_ tabUUID: TabUUID, completion: (() -> Void)? = nil) {
guard let index = tabs.firstIndex(where: { $0.tabUUID == tabUUID }) else { return }
let tab = tabs[index]

DispatchQueue.main.async { [weak self] in
self?.removeTab(tab, flushToDisk: true)
self?.updateSelectedTabAfterRemovalOf(tab, deletedIndex: index)
Expand Down Expand Up @@ -269,9 +271,7 @@ class TabManagerImplementation: NSObject, TabManager, FeatureFlaggable, TabEvent
return urls.contains(url)
}
for tab in tabsToRemove {
await withCheckedContinuation { continuation in
removeTab(tab) { continuation.resume() }
}
await removeTab(tab.tabUUID)
}
}

Expand Down Expand Up @@ -1016,19 +1016,12 @@ class TabManagerImplementation: NSObject, TabManager, FeatureFlaggable, TabEvent
tabToSelect = addTab(request, afterTab: selectedTab, isPrivate: selectedTab.isPrivate)
}
selectTab(tabToSelect)
removeTab(selectedTab)
Task {
await removeTab(selectedTab.tabUUID)
await tabSessionStore.deleteUnusedTabSessionData(keeping: [])
Comment on lines -1019 to 1021
Copy link
Collaborator

@mattreaganmozilla mattreaganmozilla Feb 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Cramsden Doesn't this create a potential race condition? removeTab is @MainActor (I believe), so it will run on the main thread (which is good since tabs is not thread-safe), but the Task can still be suspended and will allow other code to potentially execute on the MT before removeTab actually runs (at an arbitrary point in the future). LMK if I may be misunderstanding though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattreaganmozilla it is very possible. Help me understand what the race condition you would expect would be? Like you go to clear your tabs history and the tab is not removed immediately? The other implementation of this same function dispatches the call to the main thread asynchronously as well so it might also not execute right away?

My hope here with this work was to at least minimize the places where we are calling async/await and gcd. Clearly I didn't actually accomplish this because the call site of clearAllTabsHistory also has a DispatchQueue.main.async.

My naive strategy has been if we are already using async/await to try to bubble it up as far as possible so that we can insure an isolated context or at least make the places where we are wrapping work in tasks as visible as possible. So maybe here clearAllTabsHistory should also be async as well? Or none of it should be async. Who even knows. I surely don't

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Help me understand what the race condition you would expect would be? Like you go to clear your tabs history and the tab is not removed immediately?

Quick disclaimer that I am not a Swift concurrency expert so I could be totally mistaken on some of this. 😅 But AFAIU the await creates a suspension point that Swift can suspend as long as it thinks it needs to. This is an extreme/unlikely example, but for the sake of illustration I believe it's theoretically possible that someone could delete a tab, and then quickly hit a command key on iPad to undo the delete, and the actual deletion Task is performed after the undo code runs. At least, the fundamental problem is similar, which is that we could potentially have code running before this delete finishes its work that we weren't expecting.

The other implementation of this same function dispatches the call to the main thread asynchronously as well so it might also not execute right away?

If the former code is DispatchQueue.main.async that is possibly also problematic, though maybe slightly less dangerous since it's basically guaranteed to be enqueued immediately, and the main queue is serial. I don't think an await suspension point is exactly the same; the Task is enqueued/scheduled but I don't believe there's any guarantee of when it runs in relation to other code on the main thread.

My hope here with this work was to at least minimize the places where we are calling async/await and gcd. Clearly I didn't actually accomplish this because the call site of clearAllTabsHistory also has a DispatchQueue.main.async. My naive strategy has been if we are already using async/await to try to bubble it up as far as possible so that we can insure an isolated context or at least make the places where we are wrapping work in tasks as visible as possible. So maybe here clearAllTabsHistory should also be async as well? Or none of it should be async. Who even knows. I surely don't

Yeah this is all fantastic and I'm glad we're digging more into this. 👍 I am a little concerned that the way we're using Tasks is potentially unsafe and might be contributing to weird/rare edge case bugs.

}
}

func closeTab(by url: URL) {
// Find the tab with the specified URL
if let tabToClose = tabs.first(where: { $0.url == url }) {
self.removeTab(tabToClose)
}
}

func reorderTabs(isPrivate privateMode: Bool, fromIndex visibleFromIndex: Int, toIndex visibleToIndex: Int) {
let currentTabs = privateMode ? privateTabs : normalActiveTabs

Expand Down
13 changes: 2 additions & 11 deletions firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockTabManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,13 +61,6 @@ class MockTabManager: TabManager {
}
}

func addTab(_ request: URLRequest?, afterTab: Tab?, isPrivate: Bool) -> Tab {
let profile = MockProfile()
let tab = Tab(profile: profile, isPrivate: isPrivate, windowUUID: windowUUID)
tabs.append(tab)
return tab
}

func getMostRecentHomepageTab() -> Tab? {
return addTab(nil, afterTab: nil, isPrivate: false)
}
Expand All @@ -85,11 +78,11 @@ class MockTabManager: TabManager {

func reAddTabs(tabsToAdd: [Tab], previousTabUUID: String) {}

func removeTab(_ tab: Tab, completion: (() -> Void)?) {}
func removeTabWithCompletion(_ tabUUID: TabUUID, completion: (() -> Void)?) {}

func removeTabs(_ tabs: [Tab]) {}

func removeTab(_ tabUUID: String) async {}
func removeTab(_ tabUUID: TabUUID) async {}

func removeAllTabs(isPrivateMode: Bool) async {}

Expand All @@ -105,8 +98,6 @@ class MockTabManager: TabManager {

func willSwitchTabMode(leavingPBM: Bool) {}

func cleanupClosedTabs(_ closedTabs: [Tab], previous: Tab?, isPrivate: Bool) {}

func reorderTabs(isPrivate privateMode: Bool, fromIndex visibleFromIndex: Int, toIndex visibleToIndex: Int) {}

func preserveTabs() {}
Expand Down
Loading