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

[Cramsden]

📜 Tickets

Jira ticket
Github issue

💡 Description

I set out with the intention of simplifying the TabManager protocol by removing duplicate functions. I did not accomplish that goal at all. I did:

• Move function calls that made sense to the async version
• Did some general code organization
• Made function names more explicit

📝 Checklist

You have to check all boxes before merging

• Filled in the above information (tickets numbers and description of your work)
• Updated the PR name to follow our PR naming guidelines
• Wrote unit tests and/or ensured the tests suite is passing
• When working on UI, I checked and implemented accessibility (minimum Dynamic Text and VoiceOver)
• If needed, I updated documentation / comments for complex code and public methods
• If needed, added a backport comment (example @Mergifyio backport release/v120)

[Cramsden]

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

[lmarceau]

nit; PR title should follow guidelines 😄

[mergify]

This pull request has conflicts when rebasing. Could you fix it @Cramsden? 🙏

[mattreaganmozilla]

@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.

[Cramsden]

@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

[mattreaganmozilla]

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.

[dataports]

Changes look good to me. Unfortunately not sure about the concurrency changes but maybe we can mitigate risk by getting it into a beta early and noting it to QA?