1134904 - Persist the tab URLs when the application is killed

Status: RESOLVED FIXED

(Firefox for iOS :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

iOS apps can be killed in low memory pressure situations. To prevent users from losing their sessions, we need some kind of basic tab serialization/deserialization for V1.

Comment 2

[Brian Nicholson (:bnicholson)]

This can be broken down a bit more, so I'll morph this into simply persisting the list of URLs. We can worry about history for each tab separately.

Comment 3

[Brian Nicholson (:bnicholson)]

James, think you could take a look at this? I don't know if anyone on the team has a very strong grasp with application state/persistence on iOS yet, so it'd be great if you could take over. Perhaps there's some event we're guaranteed to get before the app is closed, and we could dump out a list of URLs there?

I've worked on session restore on the Android side, so happy to help if needed!

Comment 4

[Richard Newman [:rnewman]]

N.B., open tabs is something we will be syncing, so do loop me in on this; it'll be a particular approach to persistence. I have some opinions about necessary flags and schema.

Comment 5

[Brian Nicholson (:bnicholson)]

Filed bug 1141240 for restoring tab history.

Comment 6

[James Hugman [:jhugman] [@jhugman]]

Initial pass on this: store it in response to UIApplicationDelegate events.

However, if we're going to be re-using the data for sync (and others) we should be touching Storage already.

(I'm a bit reluctant to be touching NSUserDefaults which would be the cheap and cheerful and wrong way of doing it).

Comment 7

[Brian Nicholson (:bnicholson)]

(In reply to James Hugman [:jhugman] [@jhugman] from comment #6)

> Initial pass on this: store it in response to UIApplicationDelegate events.

Yep, looks like we'll want to use applicationDidEnterBackground() since applicationWillTerminate() isn't guaranteed to be called [1]. So these are the Android onPause() and onDestroy() equivalents for iOS.

> However, if we're going to be re-using the data for sync (and others) we
> should be touching Storage already.
> 
> (I'm a bit reluctant to be touching NSUserDefaults which would be the cheap and cheerful and wrong way of doing it).

Yeah, I'd avoid using prefs. How about just writing out a JSON file? It's flexible, and it's what we use on desktop/Android, so it seems like the logical choice here. You could add a write() method to FileAccessor.swift, then write out the session file using profile.files.write().

As a first pass, I'd just write out the bare minimum we need to restore the set of URLs. Something like this:
{selectedIndex: 0, tabs: [{url: "http://www.google.com"}, {url: "http://www.yahoo.com"}, ...]}

> N.B., open tabs is something we will be syncing, so do loop me in on this;
> it'll be a particular approach to persistence. I have some opinions about
> necessary flags and schema.

Care to elaborate?

For an initial approach, I imagine we'd be fine simply writing the minimum possible (an array of URLs) and expanding from there. Session data is transient, so I hope we can just tack on any necessary Sync-specific additions when they're needed.

[1] https://developer.apple.com/library/ios/documentation/UIKit/Reference/UIApplicationDelegate_Protocol/#//apple_ref/occ/intfm/UIApplicationDelegate/applicationWillTerminate:(In reply to Richard Newman [:rnewman] from comment #4)

Comment 8

[Richard Newman [:rnewman]]

(In reply to Brian Nicholson (:bnicholson) from comment #7)

> > N.B., open tabs is something we will be syncing, so do loop me in on this;
> > it'll be a particular approach to persistence. I have some opinions about
> > necessary flags and schema.
> 
> Care to elaborate?

We need to support the following:

* A way for us to find out if the set of tabs has changed since we last synced. That can be as simple as a switch that the browser flips every time it writes, and Sync flips back when it's uploaded.

* Individual records need to support these fields:

http://docs.services.mozilla.com/sync/objectformats.html#tabs

That means we should be writing out:

    title:         title of the current page.
    urlHistory:    array of page URLs in the tab’s history.
    icon:          favicon URI of the tab.
    lastUsed:      Unix epoch (in seconds) at which the tab was last accessed.

The simplest version of this is a single URI in a one-element array for urlHistory, no known icon, and 0 for lastUsed.


Sync uploads the entire bundle of tabs at once, so a snapshot approach is convenient.

Note that incoming synced tabs will be in this format, too; a shared representation might make things simpler somewhere.

Comment 9

[James Hugman [:jhugman] [@jhugman]]

> Care to elaborate?
Pragmatism aside (famous last words), I'm inclined to want to put tab data (esp tab history) in to Storage at the time that the tab loads a new URL.

The sync file should be generated from Storage (and parsed and moved into Storage) with the sync code.

This stops us having to touch the filesystem at the time when we least want to.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Firefox on Desktop and Android conceptually share the session store mechanism. Some notes:

* Session is stored in a JSON file
* Session data is written in two ways: delayed and immediate
  * Delayed is the default method and used for various triggers:
    * Loading a URL
    * Opening/Closing a Tab
    * Switching Tabs
  * Immediate is used only when we feel the app could be killed soon:
    * When app goes into background
* Delayed writes wait a number of seconds before saving to disk and any new delayed write request resets the timer.
* Immediate writes check to see if a Delayed write is queued and forces an immediate flush to disk.

We do not try to make the Session JSON format match any other data format, i.e. Synced Tabs, since the two have already diverged on Desktop and Android.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Also note that Desktop and Android versions of the Session also track "closed tabs" so we can undo a closed tab.

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: example sessionstore.js from Android

Here is a simple session file I grabbed from Android

Comment 16

[James Hugman [:jhugman] [@jhugman]]

My preferred option was to listen to the onLocationChange events and update an OpenTabs object hanging off the profile in Storage.xcodeproj.

However: the TabManager keeps track of tab indexes, rather than tab ids, which means reconstructing the current of TabManager isn't possible with this approach.

Two alternatives: 

1. add ids, and closed datetime to the tabs. The book keeping to restore a tab session, may not be as bad as I fear.
2. serialize the tab manager more directly, without creating any extra data structures. Panic write a JSON file when needed. This may get ugly when it comes to re-inflating tabs later on. It may also get ugly when we start taking closed tabs into consideration.

For implementation speed going with 2. If this turns out to be adequate, it'll be a genius idea and we did the right thing.

If it's as ugly as I expect, we'll need to re-write it, but we did the right thing re: time-to-market. :)

Comment 17

[James Hugman [:jhugman] [@jhugman]]

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

Implemented using a JSON object written to disk.

A new Storage object called OpenTabs. It doesn't actually store anything, just facilitates writing a JSON blob to disk by serializing TabManager.

It's hacky because:
a) we need something fast
b) we will want something that's not horrible in the future

Also added a restore.

Comment 18

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

Why is this not implemented with the *State Preservation and Restoration* infrastructure that is part of the UIApplication lifecycle?

This is documented at https://developer.apple.com/library/ios/documentation/iPhone/Conceptual/iPhoneOSProgrammingGuide/StrategiesforImplementingYourApp/StrategiesforImplementingYourApp.html#//apple_ref/doc/uid/TP40007072-CH5-SW2

There is no need to manually send notifications around because the application delegate will receive the following two callbacks:

* `application:shouldSaveApplicationState:`
* `application:shouldRestoreApplicationState:`

And for `UIViewController` instances there is the `UIViewControllerRestoration` protocol that needs to be implemented.

I think it is a bad idea to reinvent this.

Specially because the application can also launch in the background for different reasons. Like periodic sync or network callbacks made from app extensions. In those cases we do not actually want to restore the UI. And I think the current approach does do that. While if we follow the official protocols for this, iOS will prevent that from happening because the state management is done by the OS at the right time.

Comment 19

[James Hugman [:jhugman] [@jhugman]]

This is useful, thanks :st3fan.

This appears to be able to avoid a JSON blob on disk which is perfect.

Comment 20

[Richard Newman [:rnewman]]

Comment on attachment 8584925 [details] [review]
https://github.com/mozilla/firefox-ios/pull/285

I left some drive-by comments, as did Stefan. I strongly agree with him: you should hook into the application lifecycle interface, rather than rolling our own.

Yes, there's a small impedance mismatch between that and real-time tab syncing... but honestly, I'd rather this code be rock-solid and work well for its primary purpose than have quicker turnaround for tabs, particularly when we'll probably sync those on exit, not during use.

I'll give more substantial feedback after that rework.

Comment 21

[James Hugman [:jhugman] [@jhugman]]

Comment on attachment 8584925 [details] [review]
https://github.com/mozilla/firefox-ios/pull/285

Happy to check canOpenUrl() for any scheme that we can't open. 

However, we need to be able to open up for non-scheme based URLs.

Related docs:
https://bugzilla.mozilla.org/show_bug.cgi?id=1118378

Comment 22

[James Hugman [:jhugman] [@jhugman]]

Comment #21 wrong bug.

Comment 23

[James Hugman [:jhugman] [@jhugman]]

Comment on attachment 8584925 [details] [review]
https://github.com/mozilla/firefox-ios/pull/285

Re-worked based on feedback, and discussion with :mfinkle. Much simpler implementation which does the same, but is not tied to Profile.

Known bad: Whitespace and comments change in an unrelated file.

Comment 24

[Richard Newman [:rnewman]]

Comment on attachment 8584925 [details] [review]
https://github.com/mozilla/firefox-ios/pull/285

I'm happy to land this after cleanup (maybe just cherry-picking!), but it's definitely a stopgap.

(We can use the same hooks, and we'll probably still use this mechanism to persist more UI-level stuff, but tabs are a meatier deal than this can solve alone.)

Comment 25

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

Comment on attachment 8584925 [details] [review]
https://github.com/mozilla/firefox-ios/pull/285

James, I like this approach. Looks clean.

Can you rebase your PR branch and squash the commits into one? Then I'd like to just take one more peek to be sure. Right now there is a lot of stuff in the PR that should not. Probably because of the PR is a bit stale.

Ping me on irc if you need help with git magic please.

Comment 26

[Aaron Train [:aaronmt]]

This doesn't work.

Correct me if I'm wrong please

* I have open tabs
* I swipe fennec off my running application list
* I re-launch the browser, none of my tabs are restored

Comment 27

[James Hugman [:jhugman] [@jhugman]]

This is working for me in all the scenarios I've tried so far.

I have open tabs.

I've stopped the app in multiple ways: 

 * returning to the springboard (this doesn't kill the app)
 * swiping from the running apps list (which Fennec is foregrounded and backgrounded)
 * power cycling the device.

Each time the previously open tabs are restored.

This is an iPad Mini 3, iOS 8.3.

Comment 28

[Aaron Train [:aaronmt]]

Opening a new bug for comment #26

Comment 29

[Brian Nicholson (:bnicholson)]

(In reply to Aaron Train [:aaronmt] from comment #28)
> Opening a new bug for comment #26

(bug 1159754)

Comment 30

[Robin Andersen [:tecgirl]]

Attachment: ios-tab-crash-2.mov

this is possibly related to the app trying to load a Reader View URL, but I'm currently stuck in a crash pattern. Here you can see the app load, the thumbnails not load, then the app crashes.