801412 - Add a preference to always restore the last session

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Gordon P. Hemsley [:GPHemsley]]

I believe it used to be the case that loading an external link into Firefox when it wasn't open would also open about:home, so that you could recover your tabs from last time.

However, on the latest version or two of Firefox for Android Beta, loading an external link into the browser when it is is closed will only open that single tab for the link you are loading.

This makes it difficult to recover your tabs from last time, as you have to manually load about:home.

Comment 1

[Paul Feher]

The behavior described here is not encountered even for the earlier version Nightly 12.0a1 (2012-01-01) so i don't think this is a regression. This issue could be treated as an possible enhancement but for the moment i think this is the expected behavior. Aaron, could we have you input on this?

Comment 2

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

I agree with Paul. I am pretty sure we did this on purpose, to make startup speed as fast as possible.

Getting back to the Home page is a common request, and we have a bug filed for that.

Comment 3

[Gordon P. Hemsley [:GPHemsley]]

As far as I'm concerned, this is something akin to losing data.

It's bad enough that the tabs already aren't restored automatically when the browser restarts.

But if I also have to manually navigate to about:home to request them to be restored just adds insult to injury.

And I still remember there being some point where the about:home page was loaded automatically on startup.

Also, it would be nice if there were further explanation about the decision to WONTFIX this almost a month after the previous comments were made (by other people).

Comment 4

[Kartikaya Gupta (email:[e-mail])]

:bnicholson, correct me if I'm wrong but I think this is WONTFIX because it is really a dupe of bug 759011?

Comment 5

[Gordon P. Hemsley [:GPHemsley]]

(In reply to Kartikaya Gupta (:kats) from comment #4)
> :bnicholson, correct me if I'm wrong but I think this is WONTFIX because it
> is really a dupe of bug 759011?

Maybe, maybe not.

My main problem here is that I lose my tabs when Firefox closes unexpectedly. They are recoverable from about:home, but that is merely luck.

Keep in mind that closing Firefox is not always voluntary on the part of the user, nor is it always the result of a problem with Firefox.

In particular, a few reasons Firefox may close unexpectedly that are not the fault of the user or Firefox:

* Another app has some unrecoverable error that causes Android to restart on its own. (This is frustratingly not uncommon.)
* Firefox gets closed by Android in order to automatically update. (This is also frustrating in that it can do so even if you're in the middle of using it!)
* The device battery dies.

Etc.

Comment 6

[Brian Nicholson (:bnicholson)]

(In reply to Kartikaya Gupta (:kats) from comment #4)
> :bnicholson, correct me if I'm wrong but I think this is WONTFIX because it
> is really a dupe of bug 759011?

Yeah, bug 759011 would address the comments originally posted in this bug (I didn't dupe it because bug 759011 is different from the feature requested here). I marked the bug as WONTFIX because this behavior is by design - see comment 2. If Fennec is launching an external URL, the expected behavior is to open just that URL. This is the same behavior observed on desktop Firefox. This was all under the assumption that we're talking about clean quits and normal startups, where there's no expectation that your previous tabs will be automatically restored (but users may still want to see them, and they could visit about:home to do so).

But if the issue is actually about restoring the session after a crash as described in comment 5, this is a bit different. I think we used to create an about:home tab only if both a) we're restoring from a crash, and b) the tabs weren't restored automatically (due to our 30 second restore timeout or avoiding a crash loop). This might have changed as a side effect of bug 722661.

This might be a good opportunity to review our session restore behavior after a crash/force quit, which has never been great IMO. Our current design is explained in https://bugzilla.mozilla.org/show_bug.cgi?id=806454#c2 (also see bug 718240 for the reasoning behind the 30 second timeout). I think options to consider are:

1) Always restoring all tabs after any crash/kill (we still may have to worry about crash loops, and this could slow down startups)
2) Never restoring any tabs after any crash/kill (and perhaps ensuring an about:home tab is always created to allow access to tabs from last time)
3) Restore all tabs when we detect that fennec crashed, but don't restore tabs if Fennec was force killed by the user (this is the behavior I originally wanted in bug 718240, but we still don't have a way to differentiate between kills and actual crashes AFAIK).

CC'ing Ian for input.

Comment 7

[Brian Nicholson (:bnicholson)]

After talking with Ian, we were able to go with option #3 in comment 6, which was implemented in bug 735399.

Even with that change, we still haven't fixed this bug (having easy access to "tabs from last time" after doing a session restore). Reading through these bugs, it seems like some users want to always do a session restore, others want to not do a restore (see bug 718240), and our current implementation is somewhere in the middle. To satisfy everyone, maybe we could have a pref for session restore that gives users three options: 1) always restore, 2) never restore, or 3) restore after crashes?

Comment 8

[Mounir Lamouri (:mounir)]

(In reply to Brian Nicholson (:bnicholson) from comment #7)
> Even with that change, we still haven't fixed this bug (having easy access
> to "tabs from last time" after doing a session restore). Reading through
> these bugs, it seems like some users want to always do a session restore,
> others want to not do a restore (see bug 718240), and our current
> implementation is somewhere in the middle. To satisfy everyone, maybe we
> could have a pref for session restore that gives users three options: 1)
> always restore, 2) never restore, or 3) restore after crashes?

Something like that would be great. Currently, Nightly doesn't restore after an update but seems to restore after being killed which makes the behaviour undefined in the user perspective and pretty frustrating when you keep some tabs open for later.

Comment 9

[Brian Nicholson (:bnicholson)]

Attachment: Always restore tabs after Fennec is closed

In addition to adding a pref for restoring, we should consider changing the current behavior to restore more aggressively. Currently, the only time we restore is after being killed in the background or after a crash. We don't restore tabs if we close Fennec through other means (swipe to close, kill via adb, force close in Android, restart for updates, etc.)

This patch makes us always do a session restore, regardless of how the browser was closed -- the one exception is using the Quit menu on Froyo (or QuitNow), which will not restore tabs on next launch. This follows the behavior of the stock browser and Chrome, which, as far as I can tell, always restore tabs.

Comment 10

[Brian Nicholson (:bnicholson)]

Test build here: https://dl.dropboxusercontent.com/u/35559547/fennec-always-restore.apk

Comment 11

[Ian Barlow (:ibarlow)]

I'm not sure if our default behaviour should be to restore more aggressively. As Mark mentioned in comment 2, we want to try to keep startup speed as quick as possible, so we should be careful about deciding to restore absolutely everything all the time.

Having said that, I agree with the idea of providing a way for more advanced users to decide how they want the browser to behave -- whether it should always restore, never restore, or only restore after crashes. 

For everyone else, we should try to provide the most useful defaults we can. I've listed them out below, let me know what you think. 

Restore session after a crash
- Should happen automatically, but should have a pref that can be switched off

Restore session after an update
- This should always happen, no matter what. An update should not interfere with the state the user left their browser in

Restore session every time, no matter how Firefox is closed
- Optional, should be a pref that is manually turned on

Seeing my tabs from last time
- This will be solved by the about:home redesign bug 862793, which will make the start page much easier to access.

Comment 12

[Russell Haley]

The Android UI convention is that there is no difference between an app that is resident in memory and one that is not.  Otherwise, it would be practically impossible to get anything done on a java-based system with only 693 MB of system memory.

Tabs should always be restored, down to the form contents and scroll position, unless the user intentionally closes them.  There should be no need for a home screen with a "tabs from last time" list.

If not losing state substantially impacts startup time, perhaps we should look into why reading in a few tens of page titles is so much more expensive than it should be.

Comment 13

[Brian Nicholson (:bnicholson)]

Unfortunately, I'm not sure there's any reliable way to tell when we've done an upgrade. There are PACKAGE_REMOVED/PACKAGE_REPLACED intents fired whenever an APK is replaced, and I tried creating a BroadcastReceiver for them, but the problem is that the receiver's callback sometimes isn't executed for several seconds after the upgrade. That means that if Fennec is launched before the receiver runs, we won't be able to detect an upgrade.

We can restore tabs after an upgrade, but we'd also have to restore tabs for the other cases too. In other words, the restore behavior must be all-or-nothing: we can either restore tabs after an upgrade while also restoring after a force close, or we do a restore for neither. I'm leaning toward the former since it's what other browsers do and is less destructive. Ian, what do you think?

Sadly, this also means we can't have a pref for session restore (unless we wanted to give the option of never restoring tabs that included upgrades).

Comment 14

[Brian Nicholson (:bnicholson)]

We might be look at the version codes from AndroidManifest, save them to a shared pref, and compare them to the previous version at startup. I'll see if this works.

Comment 15

[Brian Nicholson (:bnicholson)]

There's really two tasks that need to be done for this bug. The first is make sure that the session is always restored after an app upgrade and the second is to allow customizing restore behavior. I filed bug 889554 for the upgrade part.

Comment 16

[Brian Nicholson (:bnicholson)]

Attachment: Add pref to always restore session

This is built on top of the patch in bug 889554.

Comment 17

[Brian Nicholson (:bnicholson)]

This can't be uplifted due to string changes.

Comment 18

[Wesley Johnston (:wesj)]

Comment on attachment 770516 [details] [diff] [review]
Add pref to always restore session

Review of attachment 770516 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/GeckoApp.java
@@ +1664,5 @@
>  
>              restoreMode = RESTORE_NORMAL;
> +        } else if (savedInstanceState != null ||
> +                PreferenceManager.getDefaultSharedPreferences(this)
> +                .getBoolean("android.not_a_preference.restoreSession", false)) {

I'd just put this on one line. Its confusing to read like this (looks like two different conditions to me).

::: mobile/android/base/GeckoPreferences.java
@@ +74,4 @@
>      private static String PREFS_UPDATER_AUTODOWNLOAD = "app.update.autodownload";
>      private static String PREFS_GEO_REPORTING = "app.geo.reportdata";
>      private static String PREFS_HEALTHREPORT_LINK = NON_PREF_PREFIX + "healthreport.link";
> +    private static String PREFS_RESTORE_SESSION = NON_PREF_PREFIX + "restoreSession";

Can we make this a public const and use it in GeckoApp?

@@ +457,5 @@
>              // Translate boolean value to int for geo reporting pref.
>              PrefsHelper.setPref(prefName, (Boolean) newValue ? 1 : 0);
>              return true;
> +        } else if (PREFS_RESTORE_SESSION.equals(prefName)) {
> +            // Do nothing; this pref will be read at startup.

Not sure what the "this pref will be read at startup" part of this is trying to say.

Comment 19

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

Good to ride the trains

Comment 20

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7dabbee8085a

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7dabbee8085a

Comment 22

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
Build: Firefox for Android (2013-07-22)
Device: Samsung Galaxy Nexus
OS: Android 4.1.1

Comment 23

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
Build: Firefox for Android 25.0a1 (2013-07-22)
Device: Samsung Galaxy Nexus
OS: Android 4.1.1

Comment 24

[Aaron Train [:aaronmt]]

We'll want a MozTrap test covering this.

Comment 25

[Teodora Vermesan (:TeoVermesan)]

Test Case added in moztrap:
https://moztrap.mozilla.org/manage/caseversion/54517/

Comment 27

[sbporter]

I'm not seeing any option in Firefox 25 Beta. Was the fix backed out, forgotten in the reorganization of the settings menu, or am I not looking hard enough?

Comment 28

[u421692]

Session restore preferences we're removed from Firefox 25, see bug 905371.