Closed Bug 630398 Opened 11 years ago Closed 10 years ago

Session store doesn't save session data quickly enough


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



(Not tracked)

Firefox 6


(Reporter: stechz, Assigned: mfinkle)



(Whiteboard: [fennec-6][fennec-sessionstore])


(2 files, 1 obsolete file)

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

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

Website A is restored
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.
Summary: Session restore doesn't retrieve session data quickly enough → Session store doesn't save session data quickly enough
Priority: -- → P3
Whiteboard: [fennec-6]
Duplicate of this bug: 628979
Duplicate of this bug: 604268
Duplicate of this bug: 605164
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.
Attached patch WIP 1 (obsolete) — Splinter Review
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.
Assignee: nobody → mark.finkle
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.
Attached patch patchSplinter Review
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.
Attachment #527252 - Attachment is obsolete: true
Attachment #527467 - Flags: review?(mbrubeck)
Attachment #527467 - Flags: review?(mbrubeck) → review+
Closed: 10 years ago
Resolution: --- → FIXED
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   :

Changeset range:
Attached patch followup patchSplinter Review
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.
Attachment #528527 - Flags: review?(mbrubeck)
Attachment #528527 - Flags: review?(mbrubeck) → review+
Whiteboard: [fennec-6] → [fennec-6][fennec-sessionstore]
Depends on: 653669
Verified fix on Mozilla/5.0 (Android; Linux armv71; rv:6.0a1) Gecko/20110501 Firefox/6.0a1 Fennec/6.0a1
Duplicate of this bug: 654980
Depends on: 656923
Target Milestone: --- → Firefox 6
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?
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.
You need to log in before you can comment on or make changes to this bug.