"Close All Tabs" crashes app intermittently

clarmso commented 1 year ago

Steps to reproduce

Open a website.
Tab tabs button. Open a new (normal) tab.
Open a website on the new tab.
Tab tabs button again.
Tap trash can icon 🗑️.
Tap "Close All Tabs".

The steps are reproducible manually and via the XCUITest testCloseAllTabsUndo(). The same crash is shown on both iPhone and iPad. https://github.com/mozilla-mobile/firefox-ios/blob/main/Tests/XCUITests/TopTabsTest.swift#L121

I have not reproduced the issue on private tabs.

Expected behavior

Tabs are closed. The homepage with the "Undo" button is displayed.

Actual behavior

The app crashes about 1/3 of the time after tapping "Close All Tabs". When I open the app again, the tabs are preserved.

Device & build information

Device: iOS simulator iOS 17.0. Physical iPad device.
OS: 17.0 (Not an issue on 16.x)
Build version: 9000 (35105)
First seen version: Oct 11ish?
1. Notes Attachments:
  https://github.com/mozilla-mobile/firefox-ios/assets/1740517/49ec0ddb-f3ed-4e7f-b334-3130cb32ea2a

┆Issue is synchronized with this Jira Task

clarmso commented 1 year ago

@yoanarios Is there any work related to this issue? If not I can close this ticket. 😄

yoanarios commented 1 year ago

The fix has been merged and I moved the ticket to QA needed if is not reproducible anymore I think we are good to close it

yoanarios commented 1 year ago

Reopening the issue because the crash is reproducible again.

This issue was first fixed in this commit and I cannot reproduce the crash anymore but in the current main branch (commit sha: b8062cde711b34b3188f5c80379d44d1b37d8d09) is reproducible again so I will investigate the possible causes

clarmso commented 1 year ago

In the meanwhile, DesktopModeTestsIpad/testLongPressReload (iPad only) and DesktopModeTestsIphone/testLongPressReload (iPhone only) at "Close All Tabs".

yoanarios commented 1 year ago

@clarmso do you mean the iPad tests are also failing? I'm able to reproduce the crash using testLongPressReload test for iPhone it looks like the original fix was just a partial one that didn't account for these steps but the remaining crash is harder to reproduce than the original so I'm still investigating

clarmso commented 1 year ago

@clarmso do you mean the iPad tests are also failing?

@yoanarios Yes, the corresponding test for iPad fails in the same way too, but the test for iPad isn't a part of smoke test running on Bitrise.

data-sync-user commented 1 year ago

➤ Yoana Rios Diaz commented:

The crash is still happening but it is harder to reproduce manually. It is recommended to run testLongPressReload repeatedly to test possible fixes. The cause seems to be the same inconsistency in JumpBackIn Viewmodel/Adaptor when all tabs are deleted

clarmso commented 1 year ago

I could mitigate the intermittent failures on testLongPressReload() by opening a new tab first. 😑 https://github.com/mozilla-mobile/firefox-ios/pull/17133

data-sync-user commented 11 months ago

➤ Dianna Smith commented:

Looks like introduced in Fx120.0 and will be reverted in 120.3

data-sync-user commented 11 months ago

➤ Matthew Reagan commented:

Orla Mitchell Can you please confirm how we should update status of this ticket? Revert was merged today for hotfix but I wasn’t entirely clear if we should update to QA Needed for validation etc. Thank you

data-sync-user commented 11 months ago

➤ Andrei Bodea commented:

Hello, please note that I tried on v120.3 (36471) with the following devices:

iPhone 13 Pro Max (15.7.1) - I had 50 tries of close all tabs but FF never crashed
iPhone SE (16.6) - At least 25+ tries of close all tabs but FF never crashed
iPhone 14 (17) - FF crashes at least 2 times out of 10, I had like 50 tries and ~every 10 tries I had 2 crashes.

Adding this information for when we will need to verify this issue.

data-sync-user commented 11 months ago

➤ Andrei Bodea commented:

Small update, I was able to reproduce this issue on v9000 (36834) with iPhone 15 Pro Max (17.1.2), and iPhone 15+ (17.0.2). The issue is still intermittent, but I could repro it ~3 times out of 10 tries.

dragosb01 commented 10 months ago

The steps affected by this crash for testLongPressReload, testCloseAllTabs and testCloseAllTabsUndo tests have been disabled until the issue will be fixed.

dragosb01 commented 10 months ago

@clarmso @yoanarios test3AutocompleteDeletingChars automated test started to fail on the same steps from this issue, causing the app to crash.

data-sync-user commented 10 months ago

➤ Dragos Bigu commented:

matt i am reopening this issue because it is reproducing on our automation tests (test3AutocompleteDeletingChars). cc Clare So Yoana Rios Diaz

dragosb01 commented 9 months ago

Disabled testRemoveAllTabsButtonRecentlyClosedHistory and workaround for test3AutocompleteDeletingChars and testTopSitesRemoveAllExceptPinnedClearPrivateData tests to avoid failures caused by this crash

abodea commented 6 months ago

I was able to reproduce this issue 2 times out of 10 tries with the iPhone 15 Pro (17.4.1) on v125 (40715). CC: @afurlan-firefox

https://github.com/mozilla-mobile/firefox-ios/assets/42831109/839f2a04-4c99-4d3d-bdbb-4196beaa155f

data-sync-user commented 6 months ago

➤ Nishant Bhasin commented:

Nishant Bhasin Norberto Andres Furlan as fyi

mozilla-mobile / firefox-ios