1205004 - Use new webViewWebContentProcessDidTerminate API

Status: RESOLVED FIXED

(Firefox for iOS :: General, defect)

Description

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

In iOS 9 we have a new API available to detect dead content processes. The WKNavigationDelegate has the following method:

  - (void)webViewWebContentProcessDidTerminate:(WKWebView *)webView

And there is also a new WKErrorCode:

  WKErrorJavaScriptResultTypeIsUnsupported

These can hopefully be used to detect dead backend processes that are currently resulting in blank tabs.

See bug 1194726 for the story about the blank tabs.

Comment 1

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

After implementing webViewWebContentProcessDidTerminate: in the TabManager, I see that it gets called when the blank tab of death appears. Thinking about a simple strategy now to reload the tab.

Comment 2

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

Attachment: PR: https://github.com/mozilla/firefox-ios/pull/1075

Work in Progress

Comment 3

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

Comment on attachment 8661862 [details] [review]
PR: https://github.com/mozilla/firefox-ios/pull/1075

This patch reloads the tab if we get a notification that the tab's content process has terminated.

Comment 4

[Brian Nicholson (:bnicholson)]

Copying some thoughts/questions from IRC:

4:26:49 PM <bnicholson> st3fan: so we've been using webkit's automatic webview termination for zombification on low memory
4:27:09 PM <bnicholson> which plays pretty nicely with iOS since we reload whenever we select those tabs again later
4:27:17 PM <bnicholson> what effects will your PR have there?
4:29:24 PM <bnicholson> st3fan: i used to hit this bug all the time in 8.4, but haven't seen it at all in 9...still wonder if apple fixed it for us
4:29:43 PM <bnicholson> st3fan: AaronMT: could we get a confirmation that this is even still happening?
4:31:42 PM <AaronMT> bnicholson: I'll try with tabs left open overnight tonight

Just want to make sure we aren't trying to fix a problem that doesn't exist :) NEEDINFO'ing Aaron for the QA results.

Comment 5

[Aaron Train [:aaronmt]]

Yeah, I wasn't able to reproduce this on 9.0.2 (iPhone 6). Tried again with ~45 min this morning and same result.

Comment 6

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

This patch deals with the new `webViewWebContentProcessDidTerminate` delegate method. If that delegate method is called, we do the following:

We find the `Browser` (tab) instance that owns the `WKWebView` that has just died. If that `Browser` is the currently selected one, we reload it right away. If it is not then we set its `webContentProcessDidTerminate` flag and reload it when it becomes active again.


This needs a thorough review before it lands. Added both :bnicholson and :sleroux since they probably know this code the best.

Comment 7

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

(In reply to Aaron Train [:aaronmt] from comment #5)
> Yeah, I wasn't able to reproduce this on 9.0.2 (iPhone 6). Tried again with
> ~45 min this morning and same result.

I was actually able to reproduce this on iOS 9.0: I got the blank tab and I saw that new webViewWebContentProcessDidTerminate: method being called as a result of that.

If we think this is not needed anymore then I am happy to just let the PR sit there for a while until we have a better understanding.

Personally I think that new delegate method exists for a reason and we need to handle it. But, I agree that it is all poorly documented and hard to reproduce.

Comment 8

[Stephan Leroux [:sleroux]]

Comment on attachment 8661862 [details] [review]
PR: https://github.com/mozilla/firefox-ios/pull/1075

Just one nit.

Comment 9

[Stephan Leroux [:sleroux]]

I don't see how this patch would add any additional risk and seems like a possible solution so I'd say go ahead and land it.

Comment 10

[Brian Nicholson (:bnicholson)]

(In reply to Stefan Arentz [:st3fan] from comment #7)
> If we think this is not needed anymore then I am happy to just let the PR
> sit there for a while until we have a better understanding.

Personally, I'd vote for this since I'm also not able to reproduce this myself. I've left my 9.0 idle a few times overnight, and the tabs that I switch to are gray, not white. They immediately load when I select them.

Is this a speculative fix, or have you verified that this works? As mentioned before, we already do this here: https://github.com/mozilla/firefox-ios/blob/a3dfe50aec56ef98a855fd2844cad30dd5e678d0/Client/Frontend/Browser/BrowserViewController.swift#L1356. Your PR also just reloads the tab, so I can't see how this could possibly fix anything new...

> Personally I think that new delegate method exists for a reason and we need
> to handle it. But, I agree that it is all poorly documented and hard to
> reproduce.

I agree it exists for a reason: apps have the option of handling this however they want to. They could update the tab titles, visual state, send some telemetry pings, etc. WKWebViews die by design: it would be too much memory pressure to keep a bunch of unused pages around.

If reloading to force the web view to stay alive was the right solution, I think Apple would have just prevented them from dying in the first place rather than exposing this delegate, no? Forcing them to stay alive seems to be against the spirit of the API.

(In reply to Stephan Leroux [:sleroux] from comment #9)
> I don't see how this patch would add any additional risk and seems like a
> possible solution so I'd say go ahead and land it.

See above comments and comment 4...if we immediately reload all tabs when the system kills them, that means we'll never have *any* zombified tabs. I've seen lots of users that collect 30+ tabs in a session. 30+ active browsers on a phone will almost certainly cause the app to OOM.

Comment 11

[Brian Nicholson (:bnicholson)]

Comment on attachment 8661862 [details] [review]
PR: https://github.com/mozilla/firefox-ios/pull/1075

Oh hey, you changed the PR from when I last looked! Sorry, I thought this was still using the brute force reload-when-terminated approach. I agree that this seems fairly risk-free.

This seems to be a better fix for the hack at https://github.com/mozilla/firefox-ios/blob/a3dfe50aec56ef98a855fd2844cad30dd5e678d0/Client/Frontend/Browser/BrowserViewController.swift#L1356 -- should we consider just removing that now?

Comment 12

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

Landed as a simplified patch that only reloads the selected tab. Not background tabs. THat case is already covered by other code that checks if webView.URL goes nil.