Skip to content

WIP Bugfix FXIOS-15339 [Tab management] Tab closed crashes when media presentations is present #33032

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

Draft
lmarceau wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

WIP Bugfix FXIOS-15339 [Tab management] Tab closed crashes when media presentations is present #33032

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4d5144f
We need to await the media to be closed before closing the tab
lmarceau Apr 8, 2026
47917f8
Fix tests
lmarceau Apr 10, 2026
a6958d5
Adjust closing private tabs unit tests
lmarceau Apr 10, 2026
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
9 changes: 3 additions & 6 deletions firefox-ios/Client/ApplicationHelper.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,12 @@ protocol ApplicationHelper: Sendable {
@MainActor
func open(_ url: URL, inWindow: WindowUUID)

@MainActor
func closeTabs(_ urls: [URL]) async
}

/// UIApplication.shared wrapper
struct DefaultApplicationHelper: ApplicationHelper {
@MainActor
func openSettings() {
UIApplication.shared.open(URL(string: UIApplication.openSettingsURLString)!, options: [:])
}
Expand All @@ -30,7 +30,6 @@ struct DefaultApplicationHelper: ApplicationHelper {
/// On iPadOS if more than one window is open, the OS will
/// determine which UIScene the URL is delivered to.
/// - Parameter url: the URL to open.
@MainActor
func open(_ url: URL) {
UIApplication.shared.open(url)
}
Expand All @@ -44,7 +43,6 @@ struct DefaultApplicationHelper: ApplicationHelper {
/// - Parameters:
/// - url: the URL to open.
/// - inWindow: the UUID of the window to open the URL.
@MainActor
func open(_ url: URL, inWindow: WindowUUID) {
let foundTargetScene = UIApplication.shared.connectedScenes.contains(where: {
guard let delegate = $0.delegate as? SceneDelegate,
Expand All @@ -63,11 +61,10 @@ struct DefaultApplicationHelper: ApplicationHelper {
///
/// - Parameters:
/// - urls: an array of URLs requested to be closed
@MainActor
func closeTabs(_ urls: [URL]) {
func closeTabs(_ urls: [URL]) async {
let windowManager = AppContainer.shared.resolve() as WindowManager
for tabManager in windowManager.allWindowTabManagers() {
tabManager.removeTabs(by: urls)
await tabManager.removeTabs(by: urls)
}
}
}
5 changes: 4 additions & 1 deletion ...Frontend/Browser/BrowserViewController/Extensions/BrowserViewController+KeyCommands.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,10 @@ extension BrowserViewController {
extras: ["action": "close-tab"])
guard let currentTab = self.tabManager.selectedTab else { return }
self.tabsPanelTelemetry.tabClosed(mode: currentTab.isPrivate ? .private : .normal)
self.tabManager.removeTab(currentTab.tabUUID)
Task {
// Laurie
Copy link
Copy Markdown
Contributor

@yoanarios yoanarios Apr 15, 2026

Choose a reason for hiding this comment

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

Clean up comment 😄

await self.tabManager.removeTab(currentTab.tabUUID)
}
self.keyboardPressesHandler().reset()
}
}
Expand Down
6 changes: 5 additions & 1 deletion ...er/BrowserViewController/Extensions/BrowserViewController+ToolBarActionMenuDelegate.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,11 @@ extension BrowserViewController: PhotonActionSheetProtocol {
iconType: .Image) { _ in
if let tab = self.tabManager.selectedTab {
self.tabsPanelTelemetry.tabClosed(mode: tab.isPrivate ? .private : .normal)
self.tabManager.removeTab(tab.tabUUID)

// Legacy Photon menu will be gone with FXIOS-15138
Task { @MainActor in
await self.tabManager.removeTab(tab.tabUUID)
}
store.dispatch(
GeneralBrowserAction(
windowUUID: self.windowUUID,
Expand Down
20 changes: 13 additions & 7 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 @@ -177,7 +177,7 @@ extension BrowserViewController: WKUIDelegate {
// Need to wait here in case we're waiting for a pending `window.open()`.
try await Task.sleep(nanoseconds: NSEC_PER_MSEC * 100)
tabsPanelTelemetry.tabClosed(mode: tab.isPrivate ? .private : .normal)
tabManager.removeTab(tab.tabUUID)
await tabManager.removeTab(tab.tabUUID)
}
}
}
Expand Down Expand Up @@ -652,17 +652,23 @@ extension BrowserViewController: WKNavigationDelegate {
if isOpened {
UIApplication.shared.open(url, options: [:])
}
// If a new window was opened for this URL, close it
if let currentTab = self?.tabManager.selectedTab,
currentTab.historyList.count == 1,
self?.isStoreURL(currentTab.historyList[0]) ?? false {
self?.tabsPanelTelemetry.tabClosed(mode: currentTab.isPrivate ? .private : .normal)
self?.tabManager.removeTab(currentTab.tabUUID)
Task { @MainActor in
await self?.removeTab()
}
}
}
}

private func removeTab() async {
// If a new window was opened for this URL, close it
guard let currentTab = tabManager.selectedTab,
currentTab.historyList.count == 1,
isStoreURL(currentTab.historyList[0]) else { return }

tabsPanelTelemetry.tabClosed(mode: currentTab.isPrivate ? .private : .normal)
await tabManager.removeTab(currentTab.tabUUID)
}

private func handleMailToNavigation(url: URL) {
showExternalAlert(withText: .ExternalMailLinkConfirmation) { _ in
if let mailToMetadata = url.mailToMetadata(),
Expand Down
14 changes: 9 additions & 5 deletions firefox-ios/Client/Frontend/Browser/BrowserViewController/Views/BrowserViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3490,13 +3490,17 @@ class BrowserViewController: UIViewController,
store.dispatch(action)
}

// TODO: FXIOS-TODO - Make closeAllPrivateTabs async
Copy link
Copy Markdown
Contributor Author

@lmarceau lmarceau Apr 10, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Need to create a ticket once this PR is approved. I think closeAllPrivateTabs should be async and the Task introduced at a higher level

func closeAllPrivateTabs() {
tabManager.removeTabs(tabManager.privateTabs)
guard let tab = mostRecentTab(inTabs: tabManager.normalTabs) else {
tabManager.selectTab(tabManager.addTab())
return
Task { @MainActor in
await tabManager.removeTabs(tabManager.privateTabs)

guard let tab = mostRecentTab(inTabs: tabManager.normalTabs) else {
tabManager.selectTab(tabManager.addTab())
return
}
tabManager.selectTab(tab)
}
tabManager.selectTab(tab)
}

func switchToTabForURLOrOpen(
Expand Down
46 changes: 27 additions & 19 deletions firefox-ios/Client/Frontend/Browser/Tabs/Middleware/TabManagerMiddleware.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,13 +119,15 @@ final class TabManagerMiddleware: FeatureFlaggable,
copyURL(tabID: tabUUID, uuid: action.windowUUID)

case TabPeekActionType.closeTab:
// TODO: verify if this works for closing a tab from an unselected tab panel
Copy link
Copy Markdown
Contributor Author

@lmarceau lmarceau Apr 10, 2026

Choose a reason for hiding this comment

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

Removed the todo since this is working since a long time, and there's no ticket number

guard let tabsState = state.componentState(TabsPanelState.self,
for: .tabsPanel,
window: action.windowUUID) else { return }
tabPeekCloseTab(with: tabUUID,
uuid: action.windowUUID,
isPrivate: tabsState.isPrivateMode)

Task {
await tabPeekCloseTab(with: tabUUID,
uuid: action.windowUUID,
isPrivate: tabsState.isPrivateMode)
}
default:
break
}
Expand Down Expand Up @@ -222,9 +224,11 @@ final class TabManagerMiddleware: FeatureFlaggable,

case TabPanelViewActionType.closeTab:
guard let tabUUID = action.tabUUID else { return }
closeTabFromTabPanel(with: tabUUID,
uuid: action.windowUUID,
isPrivate: action.panelType == .privateTabs)
Task {
await closeTabFromTabPanel(with: tabUUID,
uuid: action.windowUUID,
isPrivate: action.panelType == .privateTabs)
}

case TabPanelViewActionType.undoClose:
undoCloseTab(state: state, uuid: action.windowUUID)
Expand All @@ -236,11 +240,15 @@ final class TabManagerMiddleware: FeatureFlaggable,
)

case TabPanelViewActionType.confirmCloseAllTabs:
closeAllTabs(state: state, uuid: action.windowUUID)
Task {
await closeAllTabs(state: state, uuid: action.windowUUID)
}

case TabPanelViewActionType.deleteTabsOlderThan:
guard let period = action.deleteTabPeriod else { return }
deleteNormalTabsOlderThan(period: period, uuid: action.windowUUID)
Task {
await deleteNormalTabsOlderThan(period: period, uuid: action.windowUUID)
}

case TabPanelViewActionType.undoCloseAllTabs:
undoCloseAllTabs(uuid: action.windowUUID)
Expand Down Expand Up @@ -424,7 +432,7 @@ final class TabManagerMiddleware: FeatureFlaggable,
/// - Parameters:
/// - tabUUID: UUID of the tab to be closed/removed
/// - Returns: If is the last tab to be closed used to trigger dismissTabTray action
private func closeTab(with tabUUID: TabUUID, uuid: WindowUUID, isPrivate: Bool) -> Bool {
private func closeTab(with tabUUID: TabUUID, uuid: WindowUUID, isPrivate: Bool) async -> Bool {
tabsPanelTelemetry.tabClosed(mode: isPrivate ? .private : .normal)
let tabManager = tabManager(for: uuid)
// In non-private mode, if:
Expand All @@ -434,14 +442,14 @@ final class TabManagerMiddleware: FeatureFlaggable,
let isLastActiveTab = isPrivate
? tabManager.privateTabs.count == 1
: tabManager.normalTabs.count == 1
tabManager.removeTab(tabUUID)
await tabManager.removeTab(tabUUID)
return isLastActiveTab
}

/// Close tab and trigger refresh
/// - Parameter tabUUID: UUID of the tab to be closed/removed
private func closeTabFromTabPanel(with tabUUID: TabUUID, uuid: WindowUUID, isPrivate: Bool) {
let shouldDismiss = self.closeTab(with: tabUUID, uuid: uuid, isPrivate: isPrivate)
private func closeTabFromTabPanel(with tabUUID: TabUUID, uuid: WindowUUID, isPrivate: Bool) async {
let shouldDismiss = await closeTab(with: tabUUID, uuid: uuid, isPrivate: isPrivate)
triggerRefresh(uuid: uuid, isPrivate: isPrivate)

if isPrivate && tabManager(for: uuid).privateTabs.isEmpty {
Expand Down Expand Up @@ -539,14 +547,14 @@ final class TabManagerMiddleware: FeatureFlaggable,
store.dispatch(scrollAction)
}

private func closeAllTabs(state: AppState, uuid: WindowUUID) {
private func closeAllTabs(state: AppState, uuid: WindowUUID) async {
let tabManager = tabManager(for: uuid)
guard let tabsState = state.componentState(TabsPanelState.self, for: .tabsPanel, window: uuid) else { return }

tabsPanelTelemetry.closeAllTabsSheetOptionSelected(option: .all, mode: tabsState.isPrivateMode ? .private : .normal)
let normalCount = tabManager.normalTabs.count
let privateCount = tabManager.privateTabs.count
tabManager.removeAllTabs(isPrivateMode: tabsState.isPrivateMode)
await tabManager.removeAllTabs(isPrivateMode: tabsState.isPrivateMode)

triggerRefresh(uuid: uuid, isPrivate: tabsState.isPrivateMode)

Expand All @@ -572,10 +580,10 @@ final class TabManagerMiddleware: FeatureFlaggable,
}
}

private func deleteNormalTabsOlderThan(period: TabsDeletionPeriod, uuid: WindowUUID) {
private func deleteNormalTabsOlderThan(period: TabsDeletionPeriod, uuid: WindowUUID) async {
tabsPanelTelemetry.deleteNormalTabsSheetOptionSelected(period: period)
let tabManager = tabManager(for: uuid)
tabManager.removeNormalTabsOlderThan(period: period, currentDate: .now)
await tabManager.removeNormalTabsOlderThan(period: period, currentDate: .now)

// We are not closing the tab tray, so we need to refresh the tabs on screen
let model = getTabsDisplayModel(for: false, uuid: uuid)
Expand Down Expand Up @@ -692,8 +700,8 @@ final class TabManagerMiddleware: FeatureFlaggable,
UIPasteboard.general.url = tabManager.getTabForUUID(uuid: tabID)?.canonicalURL
}

private func tabPeekCloseTab(with tabID: TabUUID, uuid: WindowUUID, isPrivate: Bool) {
closeTabFromTabPanel(with: tabID, uuid: uuid, isPrivate: isPrivate)
private func tabPeekCloseTab(with tabID: TabUUID, uuid: WindowUUID, isPrivate: Bool) async {
await closeTabFromTabPanel(with: tabID, uuid: uuid, isPrivate: isPrivate)
}

private func changePanel(_ panel: TabTrayPanelType, appState: AppState, uuid: WindowUUID) {
Expand Down
7 changes: 6 additions & 1 deletion firefox-ios/Client/Frontend/Browser/TopTabDisplayManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,9 +338,14 @@ class TopTabDisplayManager: NSObject {
func performCloseAction(for tab: Tab) {
guard !isDragging else { return }

// TODO: FXIOS-TODO - Why do we call get tabs here, can we remove it
Copy link
Copy Markdown
Contributor Author

@lmarceau lmarceau Apr 10, 2026

Choose a reason for hiding this comment

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

Need to create a ticket for this, I don't understand what this _ = getTabs() is doing, and I am wondering if we can just remove it. Might be a side-effect needed for something else? Who knows. But having a separate task for this makes more sense than trying to bundle it here

_ = getTabs()
tabsPanelTelemetry.tabClosed(mode: tab.isPrivate ? .private : .normal)
tabManager.removeTab(tab.tabUUID)

// TODO: FXIOS-TODO Make performCloseAction async
Copy link
Copy Markdown
Contributor Author

@lmarceau lmarceau Apr 10, 2026

Choose a reason for hiding this comment

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

Need to create a ticket once this PR is approved. I think performCloseAction should be async and the Task introduced at a higher level

Task {
Copy link
Copy Markdown
Contributor

@yoanarios yoanarios Apr 15, 2026

Choose a reason for hiding this comment

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

async-await question here we use Task { versus Task { @MainActor because the function is marked as MainActor and should inherit the context?

await tabManager.removeTab(tab.tabUUID)
}
}

// When using 'Close All', hide all the tabs so they don't animate their deletion individually
Expand Down
12 changes: 8 additions & 4 deletions firefox-ios/Client/TabManagement/Tab.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -600,12 +600,16 @@ class Tab: NSObject,
guard let currentlyOpenUrl = lastKnownUrl ?? historyList.last else { return }

url = currentlyOpenUrl
close()

// TODO: FXIOS-TODO Make clearAndResetTabHistory async
Copy link
Copy Markdown
Contributor Author

@lmarceau lmarceau Apr 10, 2026

Choose a reason for hiding this comment

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

Need to create a ticket once this PR is approved. I think clearAndResetTabHistory should be async and the Task introduced at a higher level

Task {
await close()
}
}

func close() {
webView?.pauseAllMediaPlayback {}
webView?.closeAllMediaPresentations {}
func close() async {
await webView?.pauseAllMediaPlayback()
Copy link
Copy Markdown
Contributor

@yoanarios yoanarios Apr 15, 2026

Choose a reason for hiding this comment

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

this is what originates most of the changes I guess?

Copy link
Copy Markdown
Contributor Author

@lmarceau lmarceau Apr 15, 2026

Choose a reason for hiding this comment

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

Yeah exactly!

await webView?.closeAllMediaPresentations()
webView?.stopLoading()
Comment on lines +610 to 613
Copy link
Copy Markdown
Contributor Author

@lmarceau lmarceau Apr 10, 2026

Choose a reason for hiding this comment

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

This is the fix for the bug. We need to await that media are closed before deiniting the tab. A completion handler version is also available, but as I said in the description I think we should move towards async/await instead. Let me know what you think


contentScriptManager.uninstall(tab: self)
Expand Down
10 changes: 5 additions & 5 deletions firefox-ios/Client/TabManagement/TabManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,18 +74,18 @@ protocol TabManager: AnyObject {

/// Remove tab option using tabUUID.
/// - Parameter tabUUID: UUID from the tab
func removeTab(_ tabUUID: TabUUID)
Copy link
Copy Markdown
Contributor Author

@lmarceau lmarceau Apr 10, 2026

Choose a reason for hiding this comment

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

Basically anything that wants to remove a tab in the tab manager will be async now

func removeTab(_ tabUUID: TabUUID) async

/// Remove all tabs indicating if is on private mode or not
/// - Parameter isPrivateMode: Is private mode enabled or not
func removeAllTabs(isPrivateMode: Bool)
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])
func removeTabs(_ tabs: [Tab])
func removeTabs(by urls: [URL]) async
func removeTabs(_ tabs: [Tab]) async

/// Remove normal tabs older than a certain period of time
func removeNormalTabsOlderThan(period: TabsDeletionPeriod, currentDate: Date)
func removeNormalTabsOlderThan(period: TabsDeletionPeriod, currentDate: Date) async

// MARK: - Undo Close
func undoCloseTab()
Expand Down
Loading
Loading