965137 - Make session saving more reliable

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

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

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: Synchronously write session on application-background

Handles #1 above. Uses a pendingWrite flag to make sure we only do I/O when there's pending data to be written.

Comment 2

[Brian Nicholson (:bnicholson)]

Attachment: Replace SessionStore pageshow listener with onLocationChange listener

Handles #2 above. Improves handling of pages that are loaded shortly before Fennec is killed.

Comment 3

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

Comment on attachment 8367126 [details] [diff] [review]
Synchronously write session on application-background

Can you use this._saveTimer as a substitute for pendingWrite?

Comment 4

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

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.

Comment 5

[Brian Nicholson (:bnicholson)]

(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 6

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

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.

Comment 7

[Brian Nicholson (:bnicholson)]

Landed with comments addressed:

https://hg.mozilla.org/integration/fx-team/rev/580de0df77e9
https://hg.mozilla.org/integration/fx-team/rev/12ca455a3f09

Comment 8

[Brian Nicholson (:bnicholson)]

Backed out for session restore failures: https://tbpl.mozilla.org/?tree=Fx-Team&rev=12ca455a3f09

https://hg.mozilla.org/integration/fx-team/rev/cd1cbc8f72c2

Comment 9

[Brian Nicholson (:bnicholson)]

Attachment: Listen for DOMTitleChange to update page title in session data

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.

Comment 10

[Brian Nicholson (:bnicholson)]

Attachment: Use a counter instead of a boolean for pendingWrite

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.

Comment 11

[Brian Nicholson (:bnicholson)]

Attachment: Handle missing sessionstore.js in testSessionOOMSave

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.

Comment 12

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

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"

Comment 13

[Brian Nicholson (:bnicholson)]

(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

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bf45888656ad
https://hg.mozilla.org/mozilla-central/rev/c08cc49abbe6

Comment 15

[Brian Nicholson (:bnicholson)]

Attachment: Folded patch for uplift

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/363ca16c69f4
https://hg.mozilla.org/releases/mozilla-beta/rev/8cb5df1f8bc8

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/8cb5df1f8bc8