1219748 - Crash when opening multiple tabs in background

Status: RESOLVED WORKSFORME

(Firefox for iOS :: General, defect)

Description

[Catalin Suciu [:csuciu]]

Attachment: Client 29-10-15 14-14.crash

One time crash on Aurora 1112 on iPad Air 2 (iOS 9.0.2)
Happened while opening bbc.com articles in background tabs

Incident Identifier: 234221C6-2634-40A4-8095-35EC7F096C4A
CrashReporter Key:   15f116bde971c0638a432fe6d472b1cd7df90666
Hardware Model:      iPad5,3
Process:             Client [3881]
Path:                /var/mobile/Containers/Bundle/Application/41170C58-A4F1-406E-B82D-C6B45150A746/Client.app/Client
Identifier:          org.mozilla.ios.FennecAurora
Version:             1113 (1.1)
Code Type:           ARM-64 (Native)
Parent Process:      launchd [1]

Date/Time:           2015-10-29 14:14:14.14 +0200
Launch Time:         2015-10-29 14:11:59.59 +0200
OS Version:          iOS 9.0.2 (13A452)
Report Version:      105

Exception Type:  00000020
Exception Codes: 0x000000008badf00d
Exception Note:  SIMULATED (this is NOT a crash)
Highlighted by Thread:  0

Application Specific Information:
org.mozilla.ios.FennecAurora failed to exit after 5.00s

Elapsed total CPU time (seconds): 5.880 (user 5.880, system 0.000), 39% CPU 
Elapsed application CPU time (seconds): 0.000, 0% CPU

Filtered syslog:
None found

Thread 0 name:  Dispatch queue: com.apple.main-thread
Thread 0:
0   libxpc.dylib                  	0x000000019bb3983c xpc_get_type + 0
1   libxpc.dylib                  	0x000000019bb45a60 xpc_connection_get_pid + 20
2   WebKit                        	0x000000018cacfc44 WebKit::WebProcessProxy::didFinishLaunching(WebKit::ProcessLauncher*, IPC::Connection::Identifier) + 268
3   WebKit                        	0x000000018c98efd8 WebKit::ProcessLauncher::didFinishLaunchingProcess(int, IPC::Connection::Identifier) + 100
4   WebKit                        	0x000000018c991978 _ZNSt3__110__function6__funcIZZN6WebKitL16connectToServiceERKNS2_15ProcessLauncher13LaunchOptionsEbPS3_MS3_FviN3IPC10Connection10IdentifierEEPNS2_12_GLOBAL__N_110UUIDHolderEEUb_E3$_0NS_9allocatorISG_EEFvvEEclEv + 60
5   JavaScriptCore                	0x0000000188297300 WTF::RunLoop::performWork() + 916
6   JavaScriptCore                	0x00000001882977ac WTF::RunLoop::performWork(void*) + 36
7   CoreFoundation                	0x00000001866c05a4 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 24
8   CoreFoundation                	0x00000001866c0038 __CFRunLoopDoSources0 + 540
9   CoreFoundation                	0x00000001866bdd38 __CFRunLoopRun + 724
10  CoreFoundation                	0x00000001865ecdc0 CFRunLoopRunSpecific + 384
11  GraphicsServices              	0x0000000191580088 GSEventRunModal + 180
12  UIKit                         	0x000000018bcc6f44 UIApplicationMain + 204
13  Client                        	0x000000010005dea0 0x10002c000 + 204448
14  libdyld.dylib                 	0x000000019b9268b8 start + 4

Comment 1

[Richard Newman [:rnewman]]

If we can get STR, this would make a great upstream bug.

Comment 2

[Richard Newman [:rnewman]]

Catalin, can you find STR?

Comment 3

[Catalin Suciu [:csuciu]]

I've tried all kind of scenarios based on the 'steps' from the description, but I couldn't reproduce the crash.

Comment 4

[Catalin Suciu [:csuciu]]

I'm able to reproduce this crash using the following steps:

1. Open Firefox
2. Visit www.ziare.com
3. Long tap on a link and 'Open in New Tab'
4. Repeat step 3 for ~15 times 

Result: Firefox crashes

Comment 5

[Catalin Suciu [:csuciu]]

Attachment: Client 04-01-16 14-26.crash

Comment 6

[Emily Toop (:fluffyemily)]

Each new background webpage adds 4MB to the memory stack. This fluctuates a little but doesn't really get cleaned up. The app crashes at around 102/103MBs memory on my device. This looks like a springboard crash as no crash stack gets logged anywhere on the device it seems.

Comment 7

[Stephan Leroux [:sleroux]]

I know that Safari limits the number of tabs that can be opened to ~28. Going by the math this looks like 28 * 4mb = 112mb which looks similar to our upper limit. I wonder if limiting number of tabs is something we should consider.

Comment 8

[Emily Toop (:fluffyemily)]

I am all for that. Exceedingly large numbers of tabs make little sense IMHO on mobile. On desktop, sure. I tend to have 2-3 windows each with 15-20 open tabs on them but I also have the space to make sense of that level of compartmentalisation and organisation. On a mobile screen I just don't. The only reason I have many tabs open in Safari is that it just keeps opening them when I click on something in another app.

Which begs the question. If we limit our tabs to, say 20, what do we do when someone selects ViewLater or sends a tab from another device? Do we fail to open those tabs? Do we replace the oldest or least recently used tab? Or do we just say 'you've reached the tab limit, please close some tabs before we can open your queued tabs'?

Comment 9

[Stephan Leroux [:sleroux]]

> If we limit our tabs to, say 20, what do we do when someone selects ViewLater or sends a tab from another device? Do we fail to open those tabs? Do we replace the oldest or least recently used tab? Or do we just say 'you've reached the tab limit, please close some tabs before we can open your queued tabs'?

I think conserving the tabs you already have open is more important than opening new ones. Removing a tab should always be an explicit action. It does get challenging though having to provide error UX across various errors such as ViewLater though. I remember trying to add a 'Sent!' toast but wasn't able to because action extensions don't normally have UI unless presenting in a full modal view controller. 

Seems like this plays into our tab management story as well because we should make it easier for users to know about their open tabs and remove all of them at once. Total side note, for example, it would be cool if we could close the current tab I'm on without having to navigate to the tab tray. I run into the same situation with Safari all the time where it just keeps opening tabs for sessions I don't want to be long-lived.

NI'ing Robin - any thoughts?

Comment 10

[Emily Toop (:fluffyemily)]

Not sure if conserving the tabs you already have is always the priority. If I have just ViewLatered a tab, that means I actually want to read that soon. However tabs may already exist for a site I opened 2 months ago, never looked at again and have utterly forgotten about. I can't help thinking some way of tracking which tabs are our most recent/frequent and then using that to determine which tabs we can clean up (especially at restoration etc) and which our user is most likely to want around would be useful. 

We could then do things like

'You've reached your tab limit. Would you like to close <3 least recently used tabs> in order to open the tabs you've sent from your desktop?'

which would feel very personal.

Comment 11

[Richard Newman [:rnewman]]

(In reply to Stephan Leroux [:sleroux] from comment #9)

> I think conserving the tabs you already have open is more important than
> opening new ones.

I'd rather we didn't force the user to choose. Pushing them into an active resource management role -- "oh, if I want to click that link, I need to go close some open tabs" -- feels very Windows Mobile.

(That's why I suggest zombies: the limit's high enough that it won't bother most users, it works fine if you _do_ actively manage tabs, and it's better than a big STOP sign.)

> Seems like this plays into our tab management story as well because we
> should make it easier for users to know about their open tabs and remove all
> of them at once. Total side note, for example, it would be cool if we could
> close the current tab I'm on without having to navigate to the tab tray. I
> run into the same situation with Safari all the time where it just keeps
> opening tabs for sessions I don't want to be long-lived.

Both of those -- current tab, and close all -- are part of FIOS-224, which I think Robin's already thinking about.

Comment 12

[Richard Newman [:rnewman]]

Can we avoid the need to address a limit in most of the app, and certainly the UI -- both of which seem like a horrifying ball of wax -- and instead just re-zombify tabs oldest-first?

So you could have 10 or 15 live tabs, altering the limit based on memory pressure, and if you open more we start putting them to sleep at the oldest end of the queue.

That logic could presumably be jammed into TabManager without significant pain.

Re-zombifying would involve pickling the back stack (which we already do?) and killing the webview so it releases all of that retained memory, including the retained memory on the other side of the IPC bridge.

Comment 13

[Richard Newman [:rnewman]]

(Yeah, that comment didn't post because I mid-aired with Steph, so imagine I posted that at Comment 8.)

Comment 14

[Richard Newman [:rnewman]]

(In reply to Emily Toop (:fluffyemily) from comment #8)
> I am all for that. Exceedingly large numbers of tabs make little sense IMHO
> on mobile.

As a user, I expect to be able to open as many tabs as I want, and close them when I want. It's not uncommon for me to churn through a forum looking for threads about a problem, opening a dozen or more tabs to read in the background.

We have a "99+" label for the Android tabs tray, and it does see use. On Android we zombify tabs when we run out of memory.

It's also worth thinking about iPad, here. The iPad is a desktop replacement for a lot of people.


> The only reason I have many tabs open in Safari is that it just keeps opening
> them when I click on something in another app.

And it makes closing them a huge pain in the ass with its stupid rolodex layout.

Comment 15

[Emily Toop (:fluffyemily)]

> Can we avoid the need to address a limit in most of the app, and certainly
> the UI -- both of which seem like a horrifying ball of wax -- and instead
> just re-zombify tabs oldest-first?

Just to confirm the zombie theory (which we all pretty much know anyway), I just repeated the same test but with each background tab being opened as a zombie. our memory footprint never went over 50MB and I managed to open > 30 tabs with no crashy crashy.

Shame they all display as blank tiles with no titles in the tab tray.

Comment 16

[Richard Newman [:rnewman]]

(In reply to Emily Toop (:fluffyemily) from comment #15)

> Shame they all display as blank tiles with no titles in the tab tray.

That seems like a solvable problem! :D

Comment 17

[Emily Toop (:fluffyemily)]

(In reply to Richard Newman [:rnewman] from comment #16)
> (In reply to Emily Toop (:fluffyemily) from comment #15)
> 
> > Shame they all display as blank tiles with no titles in the tab tray.
> 
> That seems like a solvable problem! :D

Indeed. Attempting to solve that right now.

Comment 18

[Brian Nicholson (:bnicholson)]

Just want to point out that WKWebView already participates in the OS didReceiveMemoryWarning in some cases. If I open 40+ Google tabs, then go back and select one I opened earlier, the tab is gray, but then reloads along with the back stack. This zombification is done automatically, though the unzombification (reload-on-select) is triggered by us at [1].

*Note that the above is only true if you run the app outside of Xcode.* For reasons unknown, the OS seems to maintain a tight grip on content processes when running from Xcode, and didReceiveMemoryWarning is never fired. Generally, I've noticed the tab limit before crashing is much lower when running from Xcode versus opening directly from SpringBoard.

That said, I'm still able to reproduce the steps in comment 4 when not running from Xcode, and it still crashes at <15 tabs. It could be worth investigating why the OS events aren't kicking in in this situation (or if they are, why aren't they working?). We already know that we can't restore the back stack on our own because of bug 1238006, so I'd urge us to avoid homemade solutions to this if possible.

[1] https://github.com/mozilla/firefox-ios/blob/a707d76fc44f46825e029557df888dd451638f5b/Client/Frontend/Browser/TabManager.swift#L642

Comment 19

[Brian Nicholson (:bnicholson)]

(In reply to Brian Nicholson (:bnicholson) from comment #18)
> For reasons unknown, the OS seems to maintain a tight grip on content processes
> when running from Xcode, and didReceiveMemoryWarning is never fired.

Sorry, that should have been didReceiveMemoryWarning is never *handled* (by the WKWebView). It's still fired as we see in the Xcode log.

Comment 20

[Robin Andersen [:tecgirl]]

(In reply to Stephan Leroux [:sleroux] from comment #9)
> > If we limit our tabs to, say 20, what do we do when someone selects ViewLater or sends a tab from another device? Do we fail to open those tabs? Do we replace the oldest or least recently used tab? Or do we just say 'you've reached the tab limit, please close some tabs before we can open your queued tabs'?
> 
> I think conserving the tabs you already have open is more important than
> opening new ones. Removing a tab should always be an explicit action. It
> does get challenging though having to provide error UX across various errors
> such as ViewLater though. I remember trying to add a 'Sent!' toast but
> wasn't able to because action extensions don't normally have UI unless
> presenting in a full modal view controller. 
> 
> Seems like this plays into our tab management story as well because we
> should make it easier for users to know about their open tabs and remove all
> of them at once. Total side note, for example, it would be cool if we could
> close the current tab I'm on without having to navigate to the tab tray. I
> run into the same situation with Safari all the time where it just keeps
> opening tabs for sessions I don't want to be long-lived.
> 
> NI'ing Robin - any thoughts?

My first thought is to keep all tabs and zombify. We should not replace tabs, ever.

I can absolutely see adding "Close Tab" to the upcoming Menu as an option—we're already offering it on the tabs tray (p&p). I assume if you close the tab you would expect to then be on the tabs tray? Then the question becomes; if you have this long grid of tabs, would you expect to be at the beginning or the end? Or would you expect to be on the next tab?

Tab management would help in this regard. If we could group tabs, we could automatically cache (even for offline) and zombify the group.

Stephan, have you seen how Pocket throws a toast up from Tweetbot? That's what I loosely based ours on.

Comment 21

[Stephan Leroux [:sleroux]]

> I can absolutely see adding "Close Tab" to the upcoming Menu as an option—we're already offering it on the tabs tray (p&p). I assume if you close the tab you would expect to then be on the tabs tray? Then the question becomes; if you have this long grid of tabs, would you expect to be at the beginning or the end? Or would you expect to be on the next tab?

One option is when we select 'Close Tab' from the menu while viewing a tab, we perform the animation to to tab tray then perform the deletion swipe animation on the tab. The result would be putting the user in the same place as if they went to the tab tray to delete the tab. Some benefits would be:

1. The user would have more context as to where the tab was deleted from.
2. Some browsers navigate automatically to the next tab after closing one but I don't think that's what users want to do after closing a tab. Anecdotally from my experience, if I'm closing a tab I either want to close all the tabs or I want to navigate to another that is usually not just the 'next' tab. I think taking the user back to the tab tray solves both of these if we were include a 'Clear all' option in the tab tray.

> Stephan, have you seen how Pocket throws a toast up from Tweetbot? That's what I loosely based ours on.

Yup I remember looking into putting up a toast but ran into difficulties being able to show something from an action extension. I think the best thing we can do is start animating a view from the bottom modally then cancel it right away to trick the extension to display the toast. I'd like to go back and take another look at this because we definitely should have some visual feedback.

Comment 22

[Emily Toop (:fluffyemily)]

So, I have tracked this bug down to this line of code:

webViewContainer.insertSubview(webView, atIndex: 0)

at line 1423 in BVC browser(browser: Browser, didCreateWebView webView: WKWebView)

Remove this one line and all our multiple tab issues go away. Adding a subview to the webViewContainer for every tab we open regardless of whether we're displaying it or not seems silly, especially as we add it as a subview again at tabManager(tabManager: TabManager, didSelectedTabChange selected: Browser?, previous: Browser?)

Problem is, when I remove that line, some of our background opened tabs then open scrolled part way down the page when you select that tab, but this feels like a solvable problem.

Comment 23

[Emily Toop (:fluffyemily)]

(In reply to Emily Toop (:fluffyemily) from comment #22)

> 
> Remove this one line and all our multiple tab issues go away. 

Well, removing this one line and the snapkit stuff below it and then relocating the snapkit stuff to didSelectedTabChange

Comment 24

[Emily Toop (:fluffyemily)]

Attachment: Pull request

Comment 25

[Emily Toop (:fluffyemily)]

Comment on attachment 8712782 [details] [review]
Pull request

no test added for this as the bug causes the app to get killed with no errors and I thought that would screw up the whole test suite rather than just fail the one test, which might not be a desirable outcome. 

Happy to add a test if you think it would be useful here though. I was kinda 50/50 and landed on the side of less work ;)

Comment 26

[Brian Nicholson (:bnicholson)]

Comment on attachment 8712782 [details] [review]
Pull request

Nice, it makes sense that this would fix the Open in New Tab part of the STR since the web view content process isn't actually created until it's attached to a superview (you can see this when switching tabs -- the progress bar appears and starts loading only after selecting them). Effectively, this is like passing zombie: true to addTab() in TabManager.swift, causing creation/adding of the web view to be delayed until it's selected.

I'm still a bit concerned about the underlying crashes/memory pressure from having N active web views; that is, once the tab has been selected once and its web view has been initialized, we're still at the mercy of the OS to zombify the web view (actually selecting these tabs still causes my iPod touch to crash at ~10 tabs when running from Xcode, for example). Not running from Xcode, I definitely notice some perf improvements with this PR applied, though -- maybe detaching the web views from the container hints the OS to zombify them sooner?

In any case, this seems like a good idea. One suggestion: if we notice perf improvements with fewer attached web views, seems like we might attach as few as possible (i.e., only one child on the container at a time).

Also, tabs opened via Open in New Tab will no longer have thumbnails since they aren't loaded. I'd file a follow-up so it's on the radar, though I'm not sure how we can fix it if we go this route.

Comment 27

[Emily Toop (:fluffyemily)]

So it looks like I should reduce my tab cleanup max tabs code to start cleaning up tabs after 5 open tabs rather than 20. 

How about we actually create a UIView subclass for the webViewContainer that handles it's own webView stack? Then it doesn't matter where we add new webViews it will always ensure that there are only the requisite number of webViews to prevent a memory issue. Then, if we add a webView later that is already on the stack we can just move it to the top rather than re-adding it. We can then go back to adding on new webView to get the screenshot to the tab.

Comment 28

[Stephan Leroux [:sleroux]]

Comment on attachment 8712782 [details] [review]
Pull request

Looks good to me just one question.

I like the idea of extracting out the container management logic from the BVC and into it's own thing. I don't think we would want to subclass UIView to add in business logic for removing/adding subviews though since UIView should focus more on view-specific stuff. What about creating an NSObject subclass that contains the logic and owns a container view that we reference from the BVC instead?

Comment 29

[Stephan Leroux [:sleroux]]

Comment on attachment 8712782 [details] [review]
Pull request

Looked at the new commit adding the container class and looks good - just added some comments which you've already addressed.

While I was running the app in the simulator I did notice that the memory used of the app was still going up pretty significantly so I'm not sure how much of an impact this patch seems to have. I ran instruments on the simulator to get an idea of what else might be going on and found some interesting things.

I've attached a couple of screenshots: the first being on app launch (base) and the second after opening 16 tabs in the simulator. It looks like we are dramatically allocating and not releasing memory that is being created from SELdidClickAddTab and the animationTransition calls. Digging in a bit further, it looks like both of these calls are related to screenshoting. The SELdidClickAddTab call generates the screenshot using the extension method on UIView but doesn't seem to ever release it. Looks like some other people have been noticing this as well:

http://stackoverflow.com/questions/5121120/uigraphicsgetimagefromcurrentimagecontext-memory-leak-pdf-previews

http://stackoverflow.com/questions/30993485/swift-uigraphicsgetimagefromcurrentimagecontext-cant-release-memory

Not sure yet but I think looking at how we capture the image and refer to it within our app might lead to some more clues.

Additionally, the animationTransition call makes reference to the transition snapshot we take to do the animation which I think is also keeping the snapshot around indefinitely after the animation if finished. I feel like if we address these two address we'll be able to dramatically improve our memory situation.

Lastly, I've also included the profile dump generated from instruments. I feel like this patch should also get added since it could have been also accounting for some of the memory but we should also look into this snapshot stuff.

Comment 30

[Brian Nicholson (:bnicholson)]

(In reply to Emily Toop (:fluffyemily) from comment #27)
> So it looks like I should reduce my tab cleanup max tabs code to start
> cleaning up tabs after 5 open tabs rather than 20. 

Why 5?

Comment 31

[Stephan Leroux [:sleroux]]

Attachment: Memory Profiling

Comment 32

[Emily Toop (:fluffyemily)]

(In reply to Brian Nicholson (:bnicholson) from comment #30)
> (In reply to Emily Toop (:fluffyemily) from comment #27)
> > So it looks like I should reduce my tab cleanup max tabs code to start
> > cleaning up tabs after 5 open tabs rather than 20. 
> 
> Why 5?

Because it's a small enough number that it should never cause a crash problem on any device, and large enough that we retain the ability to go back to the previous tab without having to do a whole bunch of extra work. When I allowed only 1 web view on the container view then viewing the tab previous to the one I am currently on caused it to reload and re-render, whereas if I don't remove and re-add I can return to my previous tab without a refresh. 5 seemed like a good number to keep around to ensure your most recent tabs are around. 

Basically it was an arbitrary number.

Comment 33

[Richard Newman [:rnewman]]

(In reply to Stephan Leroux [:sleroux] from comment #29)

> Not sure yet but I think looking at how we capture the image and refer to it
> within our app might lead to some more clues.
> 
> Additionally, the animationTransition call makes reference to the transition
> snapshot we take to do the animation which I think is also keeping the
> snapshot around indefinitely after the animation if finished. I feel like if
> we address these two address we'll be able to dramatically improve our
> memory situation.

My 2¢: I think this is worth looking into before or instead of introducing another layer of indirection and putting the number '5' anywhere :D

Comment 34

[Emily Toop (:fluffyemily)]

:bnicholson I've updated the PR with the changes I mentioned above if you want to re-review.

Comment 35

[Brian Nicholson (:bnicholson)]

About the arbitrary 5 limit: I tested the didReceiveMemoryWarning callback at [1], and I do see the URL bar updated with "LOW:MEMORY" when adding multiple tabs (both when running from Xcode and independently). Can you see if this is triggered for you? If so, this could be a good option for deciding when to detach those views.

[1] https://github.com/thebnich/firefox-ios/commit/905b3a3ac584b0f4b652ec082bb5bb8ed444d017

Comment 36

[Brian Nicholson (:bnicholson)]

Comment on attachment 8712782 [details] [review]
Pull request

Doing some testing on my iPod touch, I'm actually not seeing any improvements with this PR over master. On both builds:

When running from Xcode, I'm able to exactly reproduce comment 4: Open in New Tab about 15 times on m.ziare.com, and the browser crashes.

When not running from Xcode, I don't crash, but things get really pretty slow after 15 tabs or so are opened. This is evidence that the OS zombification is kicking in.

Flipping the r+ since I think this still needs some more investigation.

Comment 37

[Emily Toop (:fluffyemily)]

hiding webViews makes no difference to the crashes here.

Comment 38

[Emily Toop (:fluffyemily)]

(In reply to Brian Nicholson (:bnicholson) (on PTO through February 3) from comment #35)
> About the arbitrary 5 limit: I tested the didReceiveMemoryWarning callback
> at [1], and I do see the URL bar updated with "LOW:MEMORY" when adding
> multiple tabs (both when running from Xcode and independently). Can you see
> if this is triggered for you? If so, this could be a good option for
> deciding when to detach those views.
> 
> [1]
> https://github.com/thebnich/firefox-ios/commit/
> 905b3a3ac584b0f4b652ec082bb5bb8ed444d017

Interestingly, when I apply this patch I don't see "LOW:MEMORY" anywhere and when outside of a debugger (in case the debugger is the thing that is preventing the low memory warning being fired) with this patch and a call to clear all the background webviews when the low memory warning is fired, the crash still occurs. On my 5s the app gets slower and slower and slower as it approaches 16 tabs, but still doesn't display the LOW:MEMORY warning.

It appears things are happening a little differently on your device than mine.

Comment 39

[Emily Toop (:fluffyemily)]

Further investigation:

It is definitely the adding of the WKWebViews as a subview that is causing the problems. Adding a UIWebView or simple UIView at the same place does not crash the app. Removing all delegates, helpers & configurations and adding a simple empty WKWebView with no request loaded does crash everything though.

However, when creating a sample app that just adds as many WKWebViews to a UIViewController as requested doesn't cause a crash.

So, it's not screenshotting (that only occurs when URL is loaded and everything crashes even if I remove screenshotting), it's not loading of URLs, it's not anything else we do with delegates (as I've removed all of them) and it's not helpers. And yet we get a crash.

Examining the WKWebView source right now in an attempt to understand what happens when the WKWebView renders.