Persist the tab URLs when the application is killed

RESOLVED FIXED

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: bnicholson, Assigned: jhugman)

Tracking

unspecified
All
iOS 8
Dependency tree / graph

Firefox Tracking Flags

(fennec+)

Details

Attachments

(3 attachments)

(Reporter)

Description

4 years ago
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.
Duplicate of this bug: 1139960
(Reporter)

Comment 2

4 years ago
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.
Summary: Persist/restore application state when the app is killed → Persist the tab URLs when the application is killed
(Reporter)

Comment 3

4 years ago
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!
Assignee: nobody → jhugman
Status: NEW → ASSIGNED
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.
(Reporter)

Comment 5

4 years ago
Filed bug 1141240 for restoring tab history.
Blocks: 1141847
(Assignee)

Comment 6

4 years ago
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).
Flags: needinfo?(bnicholson)
(Reporter)

Comment 7

4 years ago
(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)
Flags: needinfo?(bnicholson) → needinfo?(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.
Flags: needinfo?(rnewman)
(Assignee)

Comment 9

4 years ago
> 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.
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.
Also note that Desktop and Android versions of the Session also track "closed tabs" so we can undo a closed tab.
Created attachment 8578696 [details]
example sessionstore.js from Android

Here is a simple session file I grabbed from Android
Duplicate of this bug: 1145391

Updated

4 years ago
Duplicate of this bug: 1146376

Updated

4 years ago
tracking-fennec: --- → ?
Duplicate of this bug: 1146395
(Assignee)

Comment 16

4 years ago
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. :)
tracking-fennec: ? → +
(Assignee)

Comment 17

3 years ago
Created attachment 8584925 [details] [review]
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.
Attachment #8584925 - Flags: review?(sarentz)
Attachment #8584925 - Flags: review?(rnewman)
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.
(Assignee)

Comment 19

3 years ago
This is useful, thanks :st3fan.

This appears to be able to avoid a JSON blob on disk which is perfect.
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.
Attachment #8584925 - Flags: review?(sarentz)
Attachment #8584925 - Flags: review?(rnewman)
(Assignee)

Comment 21

3 years ago
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
Attachment #8584925 - Flags: review-
(Assignee)

Comment 22

3 years ago
Comment #21 wrong bug.
(Assignee)

Comment 23

3 years ago
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.
Attachment #8584925 - Flags: review-
Attachment #8584925 - Flags: feedback?(sarentz)
Attachment #8584925 - Flags: feedback?(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.)
Attachment #8584925 - Flags: feedback?(rnewman) → feedback+
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.
Attachment #8584925 - Flags: feedback?(sarentz) → feedback+
Blocks: 1154554
No longer blocks: 1141847
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 27

3 years ago
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.
Opening a new bug for comment #26
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
(Reporter)

Comment 29

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

(bug 1159754)
Created attachment 8601627 [details]
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.
You need to log in before you can comment on or make changes to this bug.