630398 - Session store doesn't save session data quickly enough

Status: VERIFIED FIXED

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

Description

[Benjamin Stover (:stechz)]

STR on Linux desktop
1. Go to any website (call it website A), and wait for it to load.
2. killall plugin-container
3. Reload tab
4. Go to a different website (call it website B), and wait for it to load.
5. killall plugin-container
6. Reload tab

Expected:
The page we were on last, website B, is restored

Actual:
Website A is restored

Comment 1

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

We need to weigh the desire to capture the session as fast as possible against the performance issues of trying to do that. Firefox uses the same delay mechanism as Fennec. The only time I see Fx dropping the delay is when a new window opens, and even then the delay drops from 15 secs to 2 secs.

Fennec uses a delay of 10 secs, by default. We could drop the delay to 2 secs when a new tab finishes loading.

Comment 5

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

FWIW, the delay in Fx is for saving the file. Our data in memory is updated right away and that's what's used when reopening a closed tab/window.

Comment 6

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

Attachment: WIP 1

This patch makes some small changes, but some impact is large:
1. Use NetUtil.asycnFetch to load the sessionstore JSON after a crash, so we don't block the main thread with file I/O.
2. Adds support for "sessionstore-state-read" notification like desktop Firefox
3. Save new page loads using the DOMContentLoaded event instead of the "pageshow" event
4. Save the session state file ASAP after a new page load is detected

#3 and #4 are the big changes. Using DOMContentLoaded means the page is not even fully loaded when we save the new state. And since we save the state ASAP, we will be saving state while a page is loading. Multi-process and async file saving should help minimize the Tp affect, but we'll certainly need to test it.

What is the impact of #3? If the page crashes during the load, we still capture it in the session state. That could be a bad thing if the page always causes a crash. It's a good thing if the app just used too much memory or crashed for some other reason.

Comment 7

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

Upon further reading, only bug 604268 cares about saving data before the tab is fully loaded. And we don't need to save that case to disk, only to memory. I can change the patch so we still use "pageload" to save to disk, but use "DOMContentLoaded" to save to memory. That should make Undo Closed Tab work.

Comment 8

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

Attachment: patch

This patch uses DOMContentLoaded to save the state to memory, so Undo Close Tab will work much better when closing tabs of loading pages. We don't save the state though. We wait for "pageshow" - when the page is finished loading. We do save the state right away though - no delay.

We will need to watch for Tp regressions. If we hit some, we can try to back off the saving of the state to disk for a small delay. Not the 2 to 15 secs we do now.

Comment 9

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

pushed:
http://hg.mozilla.org/mozilla-central/rev/0063bd47e5ba

Comment 10

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

This patch regressed Ts, which makes sesne since we are knowingly saving the state ASAP after a pageload. I'll look at ways to save time:

Regression Ts increase 3.53% on Nokia n900 mobile
----------------------------------------------------
    Previous: avg 6463.911 stddev 59.452 of 30 runs up to revision eb730526b7ca
    New     : avg 6692.378 stddev 38.252 of 5 runs since revision cc24b61149ed
    Change  : +228.467 (3.53% / z=3.843)
    Graph   : http://mzl.la/eKbKwj

Changeset range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=eb730526b7ca&tochange=cc24b61149ed

Comment 11

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

Attachment: followup patch

I found that the transient "about:blank" that is used when adding a new tab is causing session to be written to disk. This is even before we start to load the actual page. This patch ignores transient "about:blank" pages.

Comment 12

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

pushed followup:
http://hg.mozilla.org/mozilla-central/rev/e76e1efdc3a0

Comment 13

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Android; Linux armv71; rv:6.0a1) Gecko/20110501 Firefox/6.0a1 Fennec/6.0a1

Comment 15

[CoJaBo]

As of the 07-15-11 nightly mobile, I am still having frequent cases where, after switching away then back to the browser, one or more tabs will reload prior pages in their history rather than the last page that was navigated to.
Has this issue actually been fixed or has it regressed?

Comment 16

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

You'll need to give a lot more detail in your steps to reproduce. Timing is critical and the state of the pages (loading or finished) is important too.