1020831 - Firefox loses my tabs every once in a while

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[David Bruant]

I apologize for this vague summary, but it's the best I can do.
It happens every month, maybe every three weeks. Firefox would start and instead of loading my tabs, it just opens a single tab with the home page.

I've noticed that every single time I open Firefox, it first looks like a single tab is opened, but then it usually opens all previously open tabs... expect for the few times where it doesn't. This is why I'm suggesting there might be a subtle race conditions that makes that sometimes, it properly loads all tabs and sometimes it just remains on the one "fakely" loaded tab.

Since the first time I've noticed this, I've reset Firefox and it didn't solve the problem.
I'm on Aurora, but it's been several versions that it's been this way, so I don't believe it's an Aurora problem.

I'm sorry again for the vagueness, but this is the best I have.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

I don't have a solution, but once bug 883609 has landed, it should become easier to run some forensics on your (lost) sessionstore.js – as well as to recover it from its backup.

Comment 2

[David Bruant]

(In reply to David Rajchenbach Teller [:Yoric] from comment #1)
> I don't have a solution, but once bug 883609 has landed, it should become
> easier to run some forensics on your (lost) sessionstore.js
Yes, by the way, where are my tabs? Are they only in sessionstore.js? Is it safe to replace this file with an older version or would it create inconsistencies with other part of a sqlite database or else?
At the very least so I can write a quick script that historizes it up every ~hour.

> as well as to recover it from its backup.
Sounds good, thanks for the tip! It'll be already better than the current situation where I feel helpless each time I loose my tabs. Thanks :-)

I'll keep this bug open at least to report each time I loose my tabs so I see if there is a pattern in the frequency and if I find a pattern in my use that might explain why this happens only every once in a while.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

(In reply to David Bruant from comment #2)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #1)
> > I don't have a solution, but once bug 883609 has landed, it should become
> > easier to run some forensics on your (lost) sessionstore.js
> Yes, by the way, where are my tabs? Are they only in sessionstore.js? Is it
> safe to replace this file with an older version or would it create
> inconsistencies with other part of a sqlite database or else?
> At the very least so I can write a quick script that historizes it up every
> ~hour.

Restoring from an old sessionstore.js should work.

Comment 4

[Wayne Mery (:wsmwk)]

I think it's presumptuous that this has roots in a race condition.

Question - is sessionstore.js in good condition?  
In other words when you see this problem if you close or kill firefox, and start it again, does about:sessionrestore show up correctly?

Comment 5

[David Bruant]

(In reply to Wayne Mery (:wsmwk) from comment #4)
> I think it's presumptuous that this has roots in a race condition.
Perhaps. I'm trying to give both information as well as my best guess to compensate my lack of knowledge otherwise. I'm obviously open to any other explanation that would be more correct than my guess :-)
 
> Question - is sessionstore.js in good condition?  
> In other words when you see this problem if you close or kill firefox, and
> start it again, does about:sessionrestore show up correctly?
No it doesn't. From what I can remember, it always happened after I normally closed Firefox. So, about:sessionrestore should have no reason to show up. That's what bothers me.
If I was violently closing Firefox often, I'd find unfortunate that my tabs don't get restored, but I'd accept it, but I'm not.

Comment 6

[David Bruant]

I lost them again. Still no idea of why, a pattern, pre-configuration or precondition that would cause it. Adding a mark just to note it and see if there is a long term timing pattern.

Comment 7

[David Bruant]

Lost the tabs again.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Bruant from comment #7)
> Lost the tabs again.

:( So just to make sure, you're on Firefox 33+, right? We thought that bug 883609 would fix these kinds of things. But there seems to be different issue.

Comment 9

[David Bruant]

Lots my tabs again.

(In reply to Tim Taubert [:ttaubert] from comment #8)
> (In reply to David Bruant from comment #7)
> > Lost the tabs again.
> 
> :( So just to make sure, you're on Firefox 33+, right? We thought that bug
> 883609 would fix these kinds of things. But there seems to be different
> issue.
Right now, Firefox 34.

I googled and came across http://www.ghacks.net/2014/06/27/mozilla-launches-improved-session-restore-firefox-33/ and read:
"the Session Restore feature of Firefox is far from perfect. While it does a good job normally, I sometimes experienced issues in regards to that.

Sessions would not be loaded but the Firefox home page would."

I AM NOT ALOOOOOOONE IN THIS UNIVEEEERSE!!!!!!

Along the way, I learned about sessionstore-backups (and sessionstore.bak [1] which I was oblivious to) and that's been of great help to recover a very recent session.

Still no clue on the trigger. For some time, I've tried to keep the number of tabs small and it helped... maybe.
It really feels like some sort of race condition based on whether sessionstore.js is loaded fast enough.

[1] http://www.ghacks.net/2013/06/03/how-to-restore-firefox-sessions-if-session-restore-is-not-working-correctly/

Comment 10

[David Teller [:Yoric] - still alive but not very active]

I wouldn't rule out a race condition entirely. The startup code of Session Restore is still pretty wild territory.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Just talked to :glandium who had a similar occurrence on Aurora and he was so nice to take some time to debug this on his machine. Turns out that all windows somehow ended up in .closedWindows and sessionstore just wasn't seeing anything to restore. This was on Linux.

That's at least one pointer, should think about what needs to go wrong for that to happen.

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Ahah. That sounds like a race condition during shutdown. Normally, we perform a final collection + write of Session Restore during shutdown before closing all windows, then deactivate Session Restore. Sounds like we're not doing this correctly.

Comment 13

[Mike Hommey [:glandium]]

My case involved a weird situation involving gnome shell telling me the application was not responding, then with all windows closed and the getusermedia screen-hanger left, and me killing the remaining process. So I wouldn't read too much from my case. Although it hinted at other session restore problems that Tim is looking at.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Ok, I think I have an idea. I looked into what would make .getCurrentState() return an empty set of windows. When closing windows on Linux/Windows one by one, all windows end up in _closedWindows[]. At least one of those will however be resurrected before returning if STATE_QUITTING. The only thing that can actually be wrong here is the state so I thought about how that could be out-of-sync. Assume the following flow of events:

1) Last window is closed and put in _closedWindows[].
2) nsIAppStartup.quit() is called and sets .isShuttingDown = true
3) nsIAppStartup fires quit-application-granted
4) SessionStore receives quit-application-granted and sets STATE_QUITTING
5) SessionSaver.run() is called on quit-application

Now the important part here is that if there is some code in between (2) and (4) that spins the event loop, e.g. sync or any other code, then any scheduled "delayed saves" will be executed and call .getCurrentState(). Due to nsIAppStartup.isShuttingDown returning true this will be the last write we allow and (5) won't be able to correct what just happened. I confirmed that spinning the event loop between (2) and (4) does indeed fire a pending timeout. To me that sounds likely enough and at the same time rare enough to explain the few occurrences. What do you think?

An easy solution might be to just use .isShuttingDown instead of tracking STATE_QUITTING ourselves...

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

That theory would btw be also a great explanation for some of the data losses that people reported when enabling e10s. Sync is especially active in the first seconds after startup and so there's a good chance it's currently spinning the event loop when shutting down.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Turns out that .shuttingDown is stupid and unreliable. We should stick to what we do now but propage STATE_QUITTING in some way to SessionFile.jsm.

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0003-Bug-1020831-Make-SessionStore.jsm-and-SessionFile.js.patch

This patch introduces LoadState.jsm as an easy way to share the correct load state between JSMs that don't want to know about each other.

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0003-Bug-1020831-Make-SessionStore.jsm-and-SessionFile.js.patch, v2

Updated LoadState.jsm to use Object.freeze() like we do in other modules.

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

This affects Windows and Linux.

Comment 20

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8496401 [details] [diff] [review]
0003-Bug-1020831-Make-SessionStore.jsm-and-SessionFile.js.patch, v2

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

Good catch. I remember wondering whether `isShuttingDown` was the right thing to use, a long time ago, and then completely forgetting about the question.

::: browser/components/sessionstore/LoadState.jsm
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +

Could you explain what this module is for?

Also, nit: is this really a "Load" state? Maybe a "Run" state, rather?

@@ +10,5 @@
> +
> +Cu.import("resource://gre/modules/Services.jsm", this);
> +
> +// We're initially stopped.
> +let state = "stopped";

Just in case we need to extend this later, perhaps use named constants instead of literal strings?

@@ +21,5 @@
> +// Listen for when the application is quitting.
> +Services.obs.addObserver(observer, "quit-application-granted", false);
> +
> +this.LoadState = Object.freeze({
> +  get isStopped() {

Please document the meaning of stopped, running and quitting.

::: browser/components/sessionstore/SessionFile.jsm
@@ +268,5 @@
>        return Promise.reject(new Error("SessionFile is closed"));
>      }
>  
>      let isFinalWrite = false;
> +    if (LoadState.isQuitting) {

Do I understand correctly that this line is the actual bugfix?

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #20)
> >      let isFinalWrite = false;
> > +    if (LoadState.isQuitting) {
> 
> Do I understand correctly that this line is the actual bugfix?

Yes. Had to create the module only to share the state between modules.

Thanks for all the other comments, will address them.

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

[Tracking Requested - why for this release]:

I would like to track and uplift this to Firefox 33 + 34. We didn't know what was causing this very rare data loss but I think we have a good pointer now. It was originally reported for 31 and fixing this for 33 would be great.

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Tim, I am afraid it is a too big change for 33 beta 8, don't you think?
I am afraid that might have some side effects and it has been there for 2 releases. This could wait for 34, don't you think?

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/7349529e5a81

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #24)
> Tim, I am afraid it is a too big change for 33 beta 8, don't you think?
> I am afraid that might have some side effects and it has been there for 2
> releases. This could wait for 34, don't you think?

Maybe, yeah. I'm quite confident this shouldn't break anything but I too would like this to have more bake time than two weeks on beta.

Comment 27

[Sylvestre Ledru [:Sylvestre]]

Anyway, happy to take an uplift to aurora to increase the coverage :)

Comment 28

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8496401 [details] [diff] [review]
0003-Bug-1020831-Make-SessionStore.jsm-and-SessionFile.js.patch, v2

Approval Request Comment
[Feature/regressing bug #]: bug 914581, when we started using nsIAppStartup.shuttingDown
[User impact if declined]: rare data loss that you *can* recover from but it's not easy and involves copying files around
[Describe test coverage new/current, TBPL]: no tests, we have no way to test startup/shutdown paths :(
[Risks and why]: Risk is rather low, we actually reverted the code to use the same signals we used for years before
[String/UUID change made/needed]: None

Comment 29

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8496401 [details] [diff] [review]
0003-Bug-1020831-Make-SessionStore.jsm-and-SessionFile.js.patch, v2

I spoke with Tim on irc. We don't have str for this bug. I'm going to take this on 34, which will hit beta in a couple of weeks. Tim is on the hook to respond to any fallout from this change.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/719d5e69451d