719195 - about:Home displayed briefly before session restore

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Johnathan Nightingale [:johnath]]

I was all set to admire the improvements to thumbnailing on about:home, but about a second after it displayed, I was whisked away into my session restored tabs. If we're restoring session, we shouldn't load/display about:home.

Tested on Jan-18 aurora

Comment 1

[:Margaret Leibovic]

Has there been any UX discussion around when we do session restore, as opposed to just showing "Tabs From Last Time" on about:home? I haven't been paying complete attention to these features, but it seems like if session restore is perceptibly slow, we'd be better off taking advantage the instant-ness of about:home.

Comment 2

[Lucas Rocha (:lucasr)]

From bug 712970, about the "Tabs from Last Time" in about:home:

"- this is only when we're not restoring tabs automatically, because the user explicitly quit or we crashed on startup multiple times (the "well, this is embarrassing" case)"

Comment 3

[Dietrich Ayala (:dietrich)]

Seeing this on restart after nightly update on 1-23 Aurora.

Comment 4

[Madhava Enros [:madhava]]

Certainly, we shouldn't show you the start page for a second or two only to whisk you to something else. This is very very distracting.

The spec cited in comment 2 about using session restore unless the browser has crashed or was quit -- this assumes a fast session restore. If it's going to be very slow, then perhaps we should not do it, and use the tabs list instead.

Comment 5

[Brian Nicholson (:bnicholson)]

Attachment: patch

Checks to see if sessionstore.bak exists. If it does, this means are doing a session restore, so don't immediately show about:home.

Comment 6

[Lucas Rocha (:lucasr)]

(In reply to Brian Nicholson (:bnicholson) from comment #5)
> Created attachment 592001 [details] [diff] [review]
> patch
> 
> Checks to see if sessionstore.bak exists. If it does, this means are doing a
> session restore, so don't immediately show about:home.

Isn't the exists() call likely to cause startup regressions? As part of bug 721008, I'm working on a patch that allows us to figure out if we want to restore a session without necessarily having to touch the sessionstore.bak file by just adding some info to the bundle. Maybe wait to see how we can combine those things?

Comment 7

[Lucas Rocha (:lucasr)]

Comment on attachment 592001 [details] [diff] [review]
patch

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

::: mobile/android/base/GeckoApp.java
@@ +1498,5 @@
>  
> +        if (mLastUri == null || mLastUri.equals("") || mLastUri.equals("about:home")) {
> +            // show about:home if we aren't restoring previous session
> +            File profileDir = getProfileDir();
> +            if (profileDir == null || !new File(profileDir, "sessionstore.bak").exists())

This will conflict a bit with SessionRestore.js's sessionstore.bak expiration code. IIUC, it checks the last modified date of sessionstore.js to decide if it will actually restore session. See:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/SessionStore.js#103

So, if the sessionstore.bak is expired and exists, you'll avoid showing about:home unnecessarily. In this case, about:home will only be shown once Gecko starts and explicitly loads the about:home URL.

Not something that will happen all the time but definitely something to keep in mind.

Comment 8

[Brian Nicholson (:bnicholson)]

(In reply to Lucas Rocha (:lucasr) from comment #6)
> Isn't the exists() call likely to cause startup regressions? As part of bug
> 721008, I'm working on a patch that allows us to figure out if we want to
> restore a session without necessarily having to touch the sessionstore.bak
> file by just adding some info to the bundle. Maybe wait to see how we can
> combine those things?

How is that possible? There's two kinds of cases where we initiate the session restore:
1) We are killed by android OOM. This is already handled by the session restore boolean we save in the bundle and pass along to GeckoThread.
2) We crash. As far as I'm aware, there's no way to detect this on the Java side without either communicating with Gecko or checking for the existence of the sessionstore.bak/js file.

How would you detect case #2 with a bundle?

Comment 9

[Brian Nicholson (:bnicholson)]

(In reply to Lucas Rocha (:lucasr) from comment #7)
> Comment on attachment 592001 [details] [diff] [review]
> patch
> 
> Review of attachment 592001 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/GeckoApp.java
> @@ +1498,5 @@
> >  
> > +        if (mLastUri == null || mLastUri.equals("") || mLastUri.equals("about:home")) {
> > +            // show about:home if we aren't restoring previous session
> > +            File profileDir = getProfileDir();
> > +            if (profileDir == null || !new File(profileDir, "sessionstore.bak").exists())
> 
> This will conflict a bit with SessionRestore.js's sessionstore.bak
> expiration code. IIUC, it checks the last modified date of sessionstore.js
> to decide if it will actually restore session. See:
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/
> SessionStore.js#103
> 
> So, if the sessionstore.bak is expired and exists, you'll avoid showing
> about:home unnecessarily. In this case, about:home will only be shown once
> Gecko starts and explicitly loads the about:home URL.
> 
> Not something that will happen all the time but definitely something to keep
> in mind.

Yep, I did see this case. It should be exceptionally rare though: a crash (not an OOM kill), followed by an hour or more of not restoring Fennec. The result is just a delayed (1-2 seconds) about:home screen. IMO, this isn't something to worry about optimizing - right?

Comment 10

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

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

> > Not something that will happen all the time but definitely something to keep
> > in mind.
> 
> Yep, I did see this case. It should be exceptionally rare though: a crash
> (not an OOM kill), followed by an hour or more of not restoring Fennec. The
> result is just a delayed (1-2 seconds) about:home screen. IMO, this isn't
> something to worry about optimizing - right?

If it becomes common, we'll see a bug and need to fix it. We can read the sessionstore.bak file date, or check for exists, in a background thread while loading Java app, in onCreate or somewwhere, too. That would allow Java to know exactly what the Gecko session restore is when we start.

Comment 11

[Brian Nicholson (:bnicholson)]

Actually, it'll be a bit more common than I suggested because of task killers. I don't know whether we should optimize for task killers or not since users really shouldn't be using them - we even have a Quit button.

Comment 12

[Brian Nicholson (:bnicholson)]

Landed on inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/9c391570dbe1

As per Doug's recommendation, I included zerdatime log messages before/after the exists() check.

Comment 13

[Joe Drew (not getting mail)]

https://hg.mozilla.org/mozilla-central/rev/9c391570dbe1

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

Brian, request aurora approval

Comment 15

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

Comment on attachment 592001 [details] [diff] [review]
patch

[Triage Comment]
This missed fx11, which is now beta

Comment 16

[Justin Wood (:Callek)]

fyi was going to land this:

Justin@ORION /d/sources/mozilla-beta
$ hg qpush
applying abouthome_sessionrestore.patch
patching file mobile/android/base/GeckoApp.java
Hunk #1 FAILED at 1446
Hunk #2 FAILED at 1488
2 out of 2 hunks FAILED -- saving rejects to file mobile/android/base/GeckoApp.java.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh abouthome_sessionrestore.patch

Comment 17

[Matt Brubeck (:mbrubeck)]

Comment on attachment 592001 [details] [diff] [review]
patch

Clearing approval for Aurora 12 and Beta 11 because we are not currently planning a Native Fennec release of these versions.  If this changes in the future, we will likely do a mass uplift of all native fennec changes.  For now, let's get these bugs off the channel triage radar.

[Filter on the string "mbrubeck-bugspam" if you want to delete all of these emails at once.]

Comment 18

[Cristian Nicolae (:xti)]

Verified fixed on:

Firefox 13.0a1 (2012-02-28)
20120228031102
http://hg.mozilla.org/mozilla-central/rev/7ce4d9b55863

--
Device: Samsung Galaxy S2
OS: Android 2.3.4