Bugfix FXIOS-9998 [inactive tabs] Fix crash when deleting and the undoing the inactive tabs multiple times in a row (related improvements FXIOS-9954, FXIOS-10010, FXIOS-9999) #22075
Conversation
…naming for clarity.
…case an active tab has been generated.
…ved tab which does not require saving a subarray index prior to removing a the tab. Add unit tests.
…ng a tab, which properly takes into account inactive tabs. Cleaned up some old inactive tabs nimbus experiment code.
…ions for normal active tabs, normal inactive tabs, and private tabs.
| self.updateIndexAfterRemovalOf(tab, deletedIndex: index, viableTabsIndex: viableTabsIndex) | ||
| DispatchQueue.main.async { [weak self] in | ||
| self?.removeTab(tab, flushToDisk: true) | ||
| self?.updateSelectedTabAfterRemovalOf(tab, deletedIndex: index) |
There was a problem hiding this comment.
I was able to simplify things a bit by calculating this "viableTabsIndex" inside findRightOrLeftTab instead of holding onto it and passing it in later.
| isSelected: selectedTab?.tabUUID == tab.tabUUID) | ||
| } | ||
| self.removeTab(tab, flushToDisk: true) | ||
| self.updateIndexAfterRemovalOf(tab, deletedIndex: index, viableTabsIndex: viableTabsIndex) | ||
| self.updateSelectedTabAfterRemovalOf(tab, deletedIndex: index) // Crash path |
There was a problem hiding this comment.
Same simplification here for "viableTabsIndex"
| @@ -819,56 +824,96 @@ class LegacyTabManager: NSObject, FeatureFlaggable, TabManager, TabEventHandler | |||
| privateConfiguration.preferences.javaScriptCanOpenWindowsAutomatically = allowPopups | |||
| } | |||
|
|
|||
| // returns all activate tabs (private or normal) | |||
| private func viableTabs(isPrivate: Bool = false) -> [Tab] { | |||
There was a problem hiding this comment.
This method didn't really seem useful anymore. There's a separate task to clean up the inactiveTabs refactor.
| @@ -905,24 +950,6 @@ class LegacyTabManager: NSObject, FeatureFlaggable, TabManager, TabEventHandler | |||
| storeChanges() | |||
| } | |||
|
|
|||
| // Select the most recently visited tab, IFF it is also the parent tab of the closed tab. | |||
| private func selectParentTab(afterRemoving tab: Tab) -> Bool { | |||
There was a problem hiding this comment.
This was only used in one place, so this logic is essentially moved to updateSelectedTabAfterRemovalOf (formerly called updateIndexAfterRemovalOf) instead.
| var normalActiveTabs: [Tab] { get } | ||
| var inactiveTabs: [Tab] { get } | ||
| var normalInactiveTabs: [Tab] { get } |
There was a problem hiding this comment.
Renamed for clarity since private tabs can't be inactive technically, or at least not as we are using "inactive". :)
There was a problem hiding this comment.
Kind of a nit, but I vote keeping this to inactiveTabs because there's a lot of references to "inactiveTabs" without the normal modifier (in other variables, enums, etc) in the codebase that it might actually just be more mental overhead to change the name at this point. Maybe a comment here stating that private tabs can't be inactive would be better?
There was a problem hiding this comment.
I can revert this pretty easily for now if we want to have a larger discussion on the value of this change!
I agree, other naming should be updated as well in this case. It could be a separate task if we want to go forward with a change like this.
For me, it was the lack of symmetry between normalActiveTabs and inactiveTabs that was a bit confusing. And that an "active tab" is assumed normal and not private, while an "inactive tab" is assumed also normal and not private. But a "private tab" can't be "inactive tab"—except actually, it can be, we just don't differentiate in the UI or have special private inactive tab logic haha. It starts to get a little confusing to know exactly what each subarray contains as there are now 2 flags at play. 😂
While I was writing the tests I had to be very explicit with this, which was why I thought the naming could maybe be improved across the three tab types we really care about: normal active tabs, normal inactive tabs, and private tabs (technically either active or inactive).
There was a problem hiding this comment.
Yeah maybe revert for now and lets have a larger discussion - I agree that something is not quite right with the naming for the tabs. Maybe we can discuss in the iOS devs channel and make a ticket to change the naming from that? You could basically just put what you wrote here.
If we're gonna fix it, we might as well fix it everywhere in the project 😆
There was a problem hiding this comment.
Sounds good to me! I've started a discussion in slack and made a proposal task for us to discuss! 👍 Will revert this change in the meantime.
| @@ -189,16 +189,16 @@ class TabManagerImplementation: LegacyTabManager, Notifiable, WindowSimpleTabsPr | |||
| level: .debug, | |||
| category: .tabs) | |||
|
|
|||
| selectTab(tabToSelect) | |||
There was a problem hiding this comment.
There was an edge case where tabToSelect might be nil so I thought we should handle that explicitly.
| let windowData = WindowData(id: windowUUID, | ||
| activeTabId: activeTabID, | ||
| activeTabId: self.selectedTabUUID ?? UUID(), |
There was a problem hiding this comment.
@OrlaM This is the spot where we see selectedTab as nil each time. Just turn on inactive tabs and close a bunch at once, it's 100% reproducible. I've filed a separate ticket as I do not think this is related to the crash I was investigating.
There was a problem hiding this comment.
Forgot to link the ticket: https://mozilla-hub.atlassian.net/browse/FXIOS-10059
firefox-ios/Client/Frontend/Browser/Tabs/Middleware/TabManagerMiddleware.swift
Show resolved
Hide resolved
…additional discussion with team.
| @@ -132,6 +131,15 @@ class LegacyTabManager: NSObject, FeatureFlaggable, TabManager, TabEventHandler | |||
| return tabs[_selectedIndex] | |||
| } | |||
|
|
|||
| var selectedTabUUID: UUID? { | |||
| guard let selectedTab = self.selectedTab, | |||
| let uuid = UUID(uuidString: selectedTab.tabUUID) else { | |||
There was a problem hiding this comment.
minor nit, but maybe else { return nil } on a new line here? Not married to it though.
Probably something we should talk about in the iOS weekly meeting one day and add stuff to the style guide for guard syntax since I don't see anything there atm
There was a problem hiding this comment.
Yeah, let's have a larger discussion. I know Roux has voiced preference to this stylistic choice. I personally find it harder to read when the return paths are not all left aligned. 😅
| // Viable tabs for selection are private tabs and normal active tabs. We never want to select a normal inactive tab. | ||
| let viableTabsToSelect = removedTab.isPrivate | ||
| ? privateTabs | ||
| : normalActiveTabs |
There was a problem hiding this comment.
I really like this spacing for ternary operator syntax btw, much easier to parse for me
…closing last inactive tabs. Add additional unit tests.
|
Working on some bugfixes on this branch... don't bother reviewing until I work through them tomorrow! 🙏 Will close the PR just so no one wastes time on this until it's ready. |
|
Cleaned this up a bit, fixed the edge case @dataports spotted, and added more tests to catch that situation. I think this is ready for review again. 🙏 |
Client.app: Coverage: 30.36
Generated by 🚫 Danger Swift against 1cc5604 |
…ose-inactive-tabs-crash
ih-codes
had a problem deploying
to
github-pages
GitHub Actions
Failure
ih-codes
changed the title
|
@mergify backport release/v131 |
✅ Backports have been created
|
…e tabs multiple times in a row (related improvements FXIOS-9954, FXIOS-10010, FXIOS-9999) (#22075) * Add TabManager / Tab helper getters and improve documentation. Minor improvements. Refactored naming for clarity. * Fix to refresh the active tab area when all inactive tabs are closed, in case an active tab has been generated. * Add logic for findRightOrLeftTab to find a neighbouring tab to a removed tab which does not require saving a subarray index prior to removing a the tab. * Reworked logic for calculating the "next" index to select when removing a tab, which properly takes into account inactive tabs. * Cleaned up some old inactive tabs nimbus experiment code. * Add a ton of removeTab unit tests which check edge cases and common situations for normal active tabs, normal inactive tabs, and private tabs. * Make sure TabManagerTests do not use debug timeout for inactive tabs. * Bitrise test workaround for main thread issues related to FXIOS-10110. * Rewrite unrelated failing test not to rely on the global notification center. * Bump timeout for Danger. * Bypass failing UI test to expedite PR, FIXME ticket FXIOS-10122 (cherry picked from commit 5e949c4) # Conflicts: # firefox-ios/Client/TabManagement/Legacy/LegacyTabManager.swift # firefox-ios/Client/TabManagement/TabManagerImplementation.swift # firefox-ios/firefox-ios-tests/Tests/ClientTests/TabTests.swift
…oing the inactive tabs multiple times in a row (related improvements FXIOS-9954, FXIOS-10010, FXIOS-9999) (backport #22075) (#22217) * Bugfix FXIOS-9998 Fix crash when deleting and the undoing the inactive tabs multiple times in a row (related improvements FXIOS-9954, FXIOS-10010, FXIOS-9999) (#22075) * Add TabManager / Tab helper getters and improve documentation. Minor improvements. Refactored naming for clarity. * Fix to refresh the active tab area when all inactive tabs are closed, in case an active tab has been generated. * Add logic for findRightOrLeftTab to find a neighbouring tab to a removed tab which does not require saving a subarray index prior to removing a the tab. * Reworked logic for calculating the "next" index to select when removing a tab, which properly takes into account inactive tabs. * Cleaned up some old inactive tabs nimbus experiment code. * Add a ton of removeTab unit tests which check edge cases and common situations for normal active tabs, normal inactive tabs, and private tabs. * Make sure TabManagerTests do not use debug timeout for inactive tabs. * Bitrise test workaround for main thread issues related to FXIOS-10110. * Rewrite unrelated failing test not to rely on the global notification center. * Bump timeout for Danger. * Bypass failing UI test to expedite PR, FIXME ticket FXIOS-10122 (cherry picked from commit 5e949c4) # Conflicts: # firefox-ios/Client/TabManagement/Legacy/LegacyTabManager.swift # firefox-ios/Client/TabManagement/TabManagerImplementation.swift # firefox-ios/firefox-ios-tests/Tests/ClientTests/TabTests.swift * Resolve merge conflicts. --------- Co-authored-by: Isabella <[email protected]>
📜 Tickets
Jira ticket FXIOS-9998
Github issue 21953
This will also fix (or improve at least) some other lingering inactive tab bugs:
Jira ticket FXIOS-9954 -- Improvement but not 100% fix
Github issue 21863
Jira ticket FXIOS-9954
Github issue 21971
Jira ticket FXIOS-9954
Github issue 21954
💡 Description
I couldn't reproduce the crash in FXIOS-9998, but after inspecting the crash log it was clear there was something odd going on with wiping and restoring multiple inactive tabs.
So I cleaned up a lot of this logic and reduced dependencies to some of the inner methods (e.g. we don't need to calculate the "viable" deletion index prior to removing a tab, hold onto that index, and then pass it back into
updateSelectedTabAfterRemovalOflater. We can just do some math infindRightOrLeftTab.)Regrettably, the logic for
updateSelectedTabAfterRemovalOfis more complicated now, but that's unavoidable as inactive tabs just weren't fully accounted for prior. I tried to document the complexity as best I could.I also added a huge swath of unit tests because of this complex logic. If anyone has suggestions on making these prettier while still retaining their usefulness, my ears are open. The tests are a bit long so the bot might shame me for PR length. 😅
Actual code changes to review are much smaller!!
📝 Checklist
You have to check all boxes before merging
@Mergifyio backport release/v120)