Make session saving more reliable

RESOLVED FIXED in Firefox 28

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: bnicholson, Assigned: bnicholson)

Tracking

Trunk
Firefox 30
All
Android
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox28 fixed, firefox29 fixed, firefox30 fixed, b2g-v1.3 fixed)

Details

Attachments

(6 attachments)

Sometimes, Fennec sessions aren't restored to the state they were left at. For example, if I close a tab, sometimes that tab reappears in a restore. If I go back in history, sometimes the tab is restored to the wrong index. This happens because of the saveStateDelayed calls in SessionStore.js; they improve performance by reducing I/O, but they can also cause state to be lost.

There are a couple of tweaks we can make to SessionStore.js to make session saving more reliable:
1) Always flush out the session to disk in onPause to prevent any pending data from getting lost
2) Use onLocationChange instead of pageshow to detect tab loads, which will trigger earlier state saves for new pages
Assignee

Comment 1

6 years ago
Handles #1 above. Uses a pendingWrite flag to make sure we only do I/O when there's pending data to be written.
Attachment #8367126 - Flags: review?(mark.finkle)
Assignee

Comment 2

6 years ago
Handles #2 above. Improves handling of pages that are loaded shortly before Fennec is killed.
Attachment #8367127 - Flags: review?(mark.finkle)
Comment on attachment 8367126 [details] [diff] [review]
Synchronously write session on application-background

Can you use this._saveTimer as a substitute for pendingWrite?
Comment on attachment 8367127 [details] [diff] [review]
Replace SessionStore pageshow listener with onLocationChange listener


>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>     // Browser webapps may load content inside iframes that can not reach across the app/frame boundary
>     // i.e. even though the page is loaded in an iframe window.top != webapp
>     // Make cure this window is a top level tab before moving on.
>     if (BrowserApp.getBrowserForWindow(contentWin) == null)
>       return;
> 
>+    Services.obs.notifyObservers(null, "Session:UpdateTab", this.id);

Session:StoreTab  ?

Let's make sure we test that the "moving back/forward through bfcache" is working correctly.
Attachment #8367127 - Flags: review?(mark.finkle) → review+
Assignee

Comment 5

6 years ago
(In reply to Mark Finkle (:mfinkle) from comment #3)
> Comment on attachment 8367126 [details] [diff] [review]
> Synchronously write session on application-background
> 
> Can you use this._saveTimer as a substitute for pendingWrite?

I don't think so, because there are two situations we'll want to set pendingWrite. The first obvious one is when we call saveStateDelayed() (which _saveTimer would handle). The second more subtle case we need the flag is when we call saveState(). Even though saveState() triggers a write "immediately" without using a timer, that write happens asynchronously. Since it's asynchronous and off the main Gecko thread, Android's onPause callback won't wait for it, and there's no guarantee it will happen before Android kills us.
Comment on attachment 8367126 [details] [diff] [review]
Synchronously write session on application-background

OK, as much as I hate new booleans, I guess it makes sense.

Can we rename: saveStateNow -> flushPendingState ?
saveState and saveStateNow seem similar, but I don't think they are anymore. flushPendingState seems to capture the behavior a little better.
Attachment #8367126 - Flags: review?(mark.finkle) → review+
Assignee

Comment 9

5 years ago
There are 3 different failures reported by TBPL. One of them is just because the test needs to be updated slightly, but the other two are actual failures caught by our session restore tests. Hooray tests.

The first (and most common) of the three failures:
failed to verify session JSON - org.mozilla.gecko.tests.SessionTest$AssertException: Assertion failed: title in JSON matches session title | got , expected page6

This happens because onLocationChange happens before pageshow, so the tab doesn't have a title by the time we write the file. This patch fixes this by adding a DOMTitleChange listener to update the session state object.

Note: I'll fold these patches into the existing ones before landing, but I figured these changes would be easier to review individually.
Attachment #8371217 - Flags: review?(mark.finkle)
Assignee

Comment 10

5 years ago
The second failure:
22 INFO TEST-UNEXPECTED-FAIL | testSessionOOMSave | failed to verify session JSON - org.mozilla.gecko.tests.SessionTest$AssertException: Assertion failed: selected tab matches | got 3, expected 2

This is a nasty intermittent bug, so I'm glad our tests hit this. As I said before, there's a delay between the saveState() call and the actual file write because saveState() is asynchronous. So if, during this small window, another saveState() is called, we'll hit a race: _pendingWrite gets set to false in the I/O callback, but we still have state that needs to be saved out.

Rather than using a simple boolean, we can use an int that we increment with every outgoing state. If the int changes between calling saveState() its callback, we know not to reset pendingWrite since there's another write on the way.
Attachment #8371221 - Flags: review?(mark.finkle)
Assignee

Comment 11

5 years ago
Final failure:
22 INFO TEST-UNEXPECTED-FAIL | testSessionOOMSave | Error readingsessionstore.js - java.io.FileNotFoundException: /mnt/sdcard/tests/profile/sessionstore.js: open failed: ENOENT (No such file or directory)

This is just a case where we need to update the test. Since we changed onTabLoad to use saveStateDelayed() instead of saveState(), sessionstore.js won't be written to immediately, and the file won't always exist when we try to read it. This change prevents the testSessionOOMSave from failing when sessionstore.js is missing.

I was able to reproduce all three of these failures locally, and I can confirm that these failures are all fixed with these patches applied.
Attachment #8371222 - Flags: review?(mark.finkle)
Comment on attachment 8371217 [details] [diff] [review]
Listen for DOMTitleChange to update page title in session data

I worry that adding a new event could slow things down. I wonder if DOMTitleChanged is called all the time and soon after onLocationChanged. Maybe we could we remove the onLocationChange trigger.

r+ but my spidersense is on "low tingle"
Attachment #8371217 - Flags: review?(mark.finkle) → review+
Attachment #8371221 - Flags: review?(mark.finkle) → review+
Attachment #8371222 - Flags: review?(mark.finkle) → review+
Assignee

Comment 13

5 years ago
(In reply to Mark Finkle (:mfinkle) from comment #12)
> Comment on attachment 8371217 [details] [diff] [review]
> Listen for DOMTitleChange to update page title in session data
> 
> I worry that adding a new event could slow things down. I wonder if
> DOMTitleChanged is called all the time and soon after onLocationChanged.
> Maybe we could we remove the onLocationChange trigger.
> 
> r+ but my spidersense is on "low tingle"

Huh. I thought that DOMTitleChanged would fire only for pages that had a <title> element, but it looks like it's fired everywhere (including about: pages, unresolvable URLs, bf cache entries, etc.). It does come after onLocationChange, but soon enough that the delay shouldn't matter much, so it's nice to get rid of the added complexity. And like onLocationChange, DOMTitleChanged fires way before pageshow. Thanks for the suggestion!

Relanded with all of these changes:
https://hg.mozilla.org/integration/fx-team/rev/bf45888656ad
https://hg.mozilla.org/integration/fx-team/rev/c08cc49abbe6
https://hg.mozilla.org/mozilla-central/rev/bf45888656ad
https://hg.mozilla.org/mozilla-central/rev/c08cc49abbe6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Assignee

Updated

5 years ago
Depends on: 969700
Assignee

Comment 15

5 years ago
Session restoring for older devices that OOM frequently have been a source of complaints for awhile, so we should get this fixed. Here's an uplift patch with the changes from this bug and bug 969700.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): None
User impact if declined: After coming back to Firefox, the state of the browser may not be the same as you left it. For example, added tabs may be lost, removed tabs may reappear, the wrong page in history may be selected, etc.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): Low risk
String or IDL/UUID changes made by this patch: None
Attachment #8374369 - Flags: approval-mozilla-beta?
Attachment #8374369 - Flags: approval-mozilla-aurora?
Attachment #8374369 - Flags: approval-mozilla-beta?
Attachment #8374369 - Flags: approval-mozilla-beta+
Attachment #8374369 - Flags: approval-mozilla-aurora?
Attachment #8374369 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.