1386874 - Startup crash in TabManager.removeTab()

Status: RESOLVED FIXED

(Firefox for iOS :: General, defect, P1)

Description

[Doug Hill]

Attachment: Client 8-2-17, 4-26 PM.crash

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170628075643

Steps to reproduce:

Attempt to launch the app.

Tap the home screen icon for the app. 


Actual results:

App immediately quits.


Expected results:

App launches successfully.

Firefox for iOS: org.mozilla.ios.Firefox
Version: 8.1

iPhone SE (iPhone8,4)
iOS 19.3.2

Comment 1

[Doug Hill]

Attachment: Client 8-2-17, 4-20 PM.crash

Comment 2

[Doug Hill]

Correction for configuration:

iOS 10.3.2

Comment 3

[Doug Hill]

Attachment: Firefox iOS crash (screen recording movie)

Comment 4

[Doug Hill]

I also have the same crash on my iPad Air.

Latest OS and Firefox iOS.

Comment 5

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

0   Client                        	0x000000010020d228 specialized TabManager.removeTab(Tab, flushToDisk : Bool, notify : Bool) -> () (TabManager.swift:0)
1   Client                        	0x0000000100201d98 TabManager.removeAllPrivateTabsAndNotify(Bool) -> () (TabManager.swift:0)
2   Client                        	0x000000010020d368 specialized TabManager.selectTab(Tab?, previous : Tab?) -> () (TabManager.swift:178)
3   Client                        	0x0000000100206434 TabManager.restoreTabsInternal() -> () (TabManager.swift:0)
4   Client                        	0x0000000100213d68 partial apply for TabManager.(restoreTabs() -> ()).(closure #1) (TabManager.swift:0)
5   Client                        	0x000000010001ca94 -[Try initWithTry:catch:] (Try.m:19)
6   Client                        	0x000000010020684c TabManager.restoreTabs() -> () (TabManager.swift:0)
7   Client                        	0x0000000100206a60 @objc TabManager.restoreTabs() -> () (TabManager.swift:0)
8   Client                        	0x0000000100247968 BrowserViewController.viewWillAppear(Bool) -> () (BrowserViewController.swift:559)
9   Client                        	0x0000000100247c90 @objc BrowserViewController.viewWillAppear(Bool) -> () (BrowserViewController.swift:0)
10  UIKit                         	0x000000018f723bf4 -[UIViewController _setViewAppearState:isAnimating:] + 632
11  UIKit                         	0x000000018f723964 -[UIViewController __viewWillAppear:] + 156
12  UIKit                         	0x000000018f7c3458 -[UINavigationController _startTransition:fromViewController:toViewController:] + 752
13  UIKit                         	0x000000018f7c2c2c -[UINavigationController _startDeferredTransitionIfNeeded:] + 856
14  UIKit                         	0x000000018f7c27e0 -[UINavigationController __viewWillLayoutSubviews] + 64
15  UIKit                         	0x000000018f7c2744 -[UILayoutContainerView layoutSubviews] + 188
16  UIKit                         	0x000000018f70907c -[UIView(CALayerDelegate) layoutSublayersOfLayer:] + 1200
17  QuartzCore                    	0x000000018c8f9274 -[CALayer layoutSublayers] + 148
18  QuartzCore                    	0x000000018c8edde8 CA::Layer::layout_if_needed(CA::Transaction*) + 292
19  QuartzCore                    	0x000000018c8edca8 CA::Layer::layout_and_display_if_needed(CA::Transaction*) + 32
20  QuartzCore                    	0x000000018c86934c CA::Context::commit_transaction(CA::Transaction*) + 252
21  QuartzCore                    	0x000000018c8903ac CA::Transaction::commit() + 504
22  QuartzCore                    	0x000000018c890e78 CA::Transaction::observer_callback(__CFRunLoopObserver*, unsigned long, void*) + 120
23  CoreFoundation                	0x00000001895889a8 __CFRUNLOOP_IS_CALLING_OUT_TO_AN_OBSERVER_CALLBACK_FUNCTION__ + 32
24  CoreFoundation                	0x0000000189586630 __CFRunLoopDoObservers + 372
25  CoreFoundation                	0x00000001894b6dc4 CFRunLoopRunSpecific + 456
26  UIKit                         	0x000000018f776384 -[UIApplication _run] + 652
27  UIKit                         	0x000000018f771058 UIApplicationMain + 208
28  Client                        	0x000000010001cf98 main (main.swift:0)
29  libdyld.dylib                 	0x00000001884c559c start + 4

Comment 6

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

This seems related to:

TabManager.swift:176

        // Make sure to wipe the private tabs if the user has the pref turned on
        if shouldClearPrivateTabs(), !(tab?.isPrivate ?? false) {
            removeAllPrivateTabsAndNotify(false)
        }

Comment 7

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Doug, do you know when this started happening? What was the last known good version that you used before you saw this crash? Was it 6.0 or 7.x?

Comment 8

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

To get past this, maybe try this: turn on airplane mode, start the app. close tabs manually. Let us know if that made a difference, that may be a good hint for the root cause of this.

Comment 9

[Doug Hill]

(In reply to Stefan Arentz [:st3fan] from comment #7)
> Doug, do you know when this started happening? What was the last known good
> version that you used before you saw this crash? Was it 6.0 or 7.x?

I apologize, but I don't remember which version last was working. I wasn't keeping track of the app updates.

Comment 10

[Doug Hill]

(In reply to Stefan Arentz [:st3fan] from comment #8)
> To get past this, maybe try this: turn on airplane mode, start the app.
> close tabs manually. Let us know if that made a difference, that may be a
> good hint for the root cause of this.

On my iPhone SE, I turned on Airplane mode, then launched the app.

Same crash behavior.

Comment 11

[Aaron Train [:aaronmt]]

Tried inducing a forced crash with various private and regular tabs open but have not had luck thus far. Need clearer steps to reproduce this crash. E.g, was 'Close Private Tabs' when leaving PB enabled? e.t.c,

Comment 12

[Richard Newman [:rnewman]]

I notice we call `websiteDataStore.removeData` with no completion handler, and we do so in a loop for each private tab.

We also flush _all tabs_ to disk for each private tab closed, which flushes their screenshots _asynchronously_ into the image store. Closing multiple private tabs could generate racing writes on the image store.

The image store is at the top of one of the threads in Doug's crash.

Suggestions to repro:

- Open several tabs (5?). Make sure they have thumbnails. Ideally they will also have saved cookies or local storage.
- Open several private browsing tabs (5?).
- Turn on Close Private Tabs, and force-kill the app without leaving Settings.
- Launch the app.

Comment 13

[Doug Hill]

(In reply to Aaron Train [:aaronmt] from comment #11)
> Tried inducing a forced crash with various private and regular tabs open but
> have not had luck thus far. Need clearer steps to reproduce this crash. E.g,
> was 'Close Private Tabs' when leaving PB enabled? e.t.c,

FWIW, all I do is launch the app and it crashes. There are no other steps for me.

Comment 14

[Doug Hill]

Also, FWIW, I am an iOS developer so if there's anything I can do to help debug this, let me know. Feel free to contact me privately.

Comment 15

[Richard Newman [:rnewman]]

Thanks for your help, Doug.

Did you have any private tabs open when this crash started?

Do you have Close Private Tabs When Leaving Private Browsing turned on (that you remember)?

If you open Safari and navigate to

  firefox://open-url/?url=http://reddit.com/

(say yes when prompted), does Firefox still crash when it opens?

Comment 16

[Doug Hill]

(In reply to Richard Newman [:rnewman] from comment #15)
> Thanks for your help, Doug.
> 
> Did you have any private tabs open when this crash started?
> 
> Do you have Close Private Tabs When Leaving Private Browsing turned on (that
> you remember)?
> 
> If you open Safari and navigate to
> 
>   firefox://open-url/?url=http://reddit.com/
> 
> (say yes when prompted), does Firefox still crash when it opens?

Crashes on launch of Firefox.

Comment 17

[Doug Hill]

Here's an interesting tidbit of information…

I just noticed that the Firefox app is still running, as I see it in the Springboard app switcher (e.g. double-click home button).

Selecting the Firefox window in the app switcher brings the app to the foreground, then immediately exit back to the home screen. However, I see that it shows the Firefox window with tabs. Compare this towhen the app is launched by tapping the home screen icon, which only shows the launch screen

Comment 18

[Doug Hill]

Attachment: Firefox iOS Crash - Springboard - 720.mov

Comment 19

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

This is a P1 bug without an assignee. 

P1 are bugs which are being worked on for the current release cycle/iteration/sprint. 

If the bug is not assigned by Monday, 28 August, the bug's priority will be reset to '--'.

Comment 20

[Doug Hill]

FWIW, I still can't run Firefox on both my phone and my iPad.

Comment 21

[Justin D'Arcangelo [:justindarc]]

Still haven't come up with reproducible STR, but `settings.closePrivateTabs` needs to be set to true.

Comment 22

[Justin D'Arcangelo [:justindarc]]

Attachment: GitHub Pull Request

Comment 23

[Justin D'Arcangelo [:justindarc]]

This PR fixes the behavior where the app doesn't honor the `settings.closePrivateTabs` flag on startup. This should hopefully avoid the code path where the crash is occurring.

Comment 24

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8920314 [details] [review]
GitHub Pull Request

Moving this PR to Bug 1211348.

Comment 25

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

We are waiting to see if bug 1211348 resolved this issue. If that is the case, if this crash does not show up anymore, then we can close this or dupe this.

Comment 26

[Justin D'Arcangelo [:justindarc]]

(In reply to Stefan Arentz [:st3fan] from comment #25)
> We are waiting to see if bug 1211348 resolved this issue. If that is the
> case, if this crash does not show up anymore, then we can close this or dupe
> this.


It doesn't seem as though that has resolved this bug. I'm seeing at least one occurrence in Xcode crash reporter for the latest beta.

Comment 27

[Justin D'Arcangelo [:justindarc]]

Attachment: GitHub Pull Request

Although I'm still unable to reproduce this crash, this PR drastically simplifies and removes the code where the crash occurs. My assumption is that it is possible that the crash is a result of the way we were removing elements from the `tabs` array while iterating over it. This PR instead performs a single `filter` operation on the array and invokes the `tabManagerDidRemoveAllTabs` delegate method. Tested and verified working on both iPhone and iPad with top-tabs.

Comment 28

[Justin D'Arcangelo [:justindarc]]

Landed on master:

https://github.com/mozilla-mobile/firefox-ios/commit/5117327101c4b4a03fd3bcc78bba159e0a382fec

Landed on v10.x:

https://github.com/mozilla-mobile/firefox-ios/commit/0845439edfe95c5b83d2605e3a8f09ae6f711d50