Bugfix FXIOS-13351 [Widgets] Wait for tab restoration before closing private tabs from the widget

[devzahirul]

📜 Tickets

Jira ticket
Github issue

💡 Description

Root cause. The widget's Close Private Tabs action arrives as the .action(.closePrivateTabs) route and ends in BrowserViewController.closeAllPrivateTabs(), which removes the tabs currently in tabManager.privateTabs. With the deeplink-optimization refactor enabled, BrowserCoordinator.checkBrowserIsReady() no longer waits for !tabManager.isRestoringTabs (the legacy gate did). So on a cold launch (app force-closed → widget tapped) the route is handled while tab restoration is still in flight: removeTabs runs against the not-yet-restored (empty) in-memory list, and applyRestorationResult then merges the restored private tabs back — they survive. This matches the QA notes exactly: reproducible only when the app was force closed (cold start + restore race), not when backgrounded (tabs already in memory), and not in prod (legacy readiness gate waits for restore).

The refactor's snapshot-and-merge restore explicitly supports additive pre-restore deeplinks (deeplink tabs land last), but a destructive pre-restore action wasn't accounted for.

Fix. closeAllPrivateTabs() now defers itself until restoration completes when tabManager.isRestoringTabs is true, using AppEventQueue.wait(for: .tabRestoration(windowUUID)) — the same pattern switchToTabForURLOrOpen uses for the equivalent race (FXIOS-10916). No behavior change on the legacy path, where the readiness gate already guarantees restore has finished before the route is handled.

Tests. Two new tests in BrowserViewControllerTests: the not-restoring path removes private tabs immediately, and the restoring path performs no removal until .tabRestoration completes (deterministic — EventQueue.started re-arms the event, and the queue executes inline on the main thread). MockTabManager.removeTabs now records its calls to support these. The existing route-level test (BrowserCoordinatorTests, .action(.closePrivateTabs) → WindowManager) is unaffected and still passes that seam.

Verification ask. I could not run the app or the unit tests locally (machine is on Xcode 26.0.1; the repo requires 26.5, and current main does not compile on 26.0.1), so CI is the authoritative test run. Since the reproduction needs a real device (force close → tap widget's Close Private Tabs → reopen), it would be great if QA or the reference person could verify the scenario on this branch.

📝 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 — two new unit tests cover both paths; authoritative suite run deferred to CI (see verification note)
• If working on UI, I checked and implemented accessibility (Dynamic Text and VoiceOver) — n/a, no UI change
• If adding telemetry, I read the data stewardship requirements and will request a data review — n/a
• If adding or modifying strings, I read the guidelines and will request a string review from l10n — n/a
• If needed, I updated documentation and added comments to complex code — added a comment referencing the ticket at the race-condition guard

[mobiletest-ci-bot]

	Warnings
⚠️	Detected tab related changes in: firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockTabManager.swift (cc @yoanarios)

	Messages
📖	Project coverage: 44.67%

💪 Quality guardian

2 tests files modified. You're a champion of test coverage! 🚀

🧹 Tidy commit

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

💬 Description craftsman

Great PR description! Reviewers salute you 🫡

🦊 BrowserViewController Check

We’re tracking the size of BrowserViewController.swift to keep it healthy.

• ✨ Change in file size: +10 lines

✅ Code coverage

• No new files detected so the coverage gate wasn't run.
• No modified files had significant enough changes for the coverage gate to run.

Client.app: Coverage: 43.55

File	Coverage
BrowserViewController.swift	38.57%	⚠️

Generated by 🚫 Danger Swift against 8b66767

[lmarceau]

Does this bug happens if you have the feature flag "Deeplink Optimization Refactor" turned ON? Does it happen only when it's OFF? I would confirm this before making any changes. If the refactor work fixes the problem, then let's not add a new AppEventQueue.wait.

[devzahirul]

@lmarceau (replying here — I can't reply inline on the review thread from my tooling)

Does this bug happens if you have the feature flag "Deeplink Optimization Refactor" turned ON? Does it happen only when it's OFF? I would confirm this before making any changes. If the refactor work fixes the problem, then let's not add a new AppEventQueue.wait.

Good question — I traced both flag states before making the change, and it's the opposite: the bug only reproduces when "Deeplink Optimization Refactor" is ON. With it OFF the legacy path is unaffected.

Flag OFF (legacy, current release): checkBrowserIsReady() requires browserIsReady && !tabManager.isRestoringTabs, so the widget route is kept in savedRoute and replayed by tabManagerDidRestoreTabs(_:) once restoration finishes → private tabs close correctly.

Flag ON (refactor): the check is only browserIsReady, which flips true in viewDidAppear (browserHasLoaded()) while the restoreTabs() kicked off in viewDidLoad is still reading from disk. The route is dispatched immediately — that's the point of the refactor — so closeAllPrivateTabs() runs against the not-yet-populated tab list, and restoration then brings the private tabs back.

This also matches the channel matrix QA reported in Jira (reproduces on main/beta, prod fine): deeplinkOptimizationRefactorFeature.yaml defaults are enabled: false with true on beta and developer. So the refactor won't fix it on its own — once the flag ships to release, prod would regress too.

The refactor path already handles this exact race for the neighbouring deeplinks with the same pattern: switchToTabForURLOrOpen (FXIOS-10916) and handle(url:isPrivate:options:) (FXIOS-14406), both a few lines above this function. This change applies the established guard to the one widget action that was missed. When the flag is OFF the guard is inert, since the coordinator only dispatches the route after restoration (isRestoringTabs is already false here).

Happy to move the wait up into the coordinator's readiness check for this route instead, if you'd prefer it there.