Closed Bug 1020831 Opened 6 years ago Closed 6 years ago

Firefox loses my tabs every once in a while

Categories

(Firefox :: Session Restore, defect)

31 Branch
defect
Not set
normal
Points:
5

Tracking

()

RESOLVED FIXED
Firefox 35
Iteration:
35.2
Tracking Status
firefox32 --- wontfix
firefox33 - wontfix
firefox34 + fixed
firefox35 --- fixed

People

(Reporter: bruant.d, Assigned: ttaubert)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
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.
(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.
(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.
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?
Flags: needinfo?(bruant.d)
Summary: Firefox looses my tabs every once in a while likely due to some race condition at startup time → Firefox looses my tabs every once in a while
(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.
Flags: needinfo?(bruant.d)
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.
Lost the tabs again.
(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.
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/
I wouldn't rule out a race condition entirely. The startup code of Session Restore is still pretty wild territory.
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.
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.
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.
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...
Flags: needinfo?(dteller)
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.
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.
Depends on: 1073502
Depends on: 1073513
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.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #8495943 - Flags: review?(dteller)
Flags: needinfo?(dteller)
Updated LoadState.jsm to use Object.freeze() like we do in other modules.
Attachment #8495943 - Attachment is obsolete: true
Attachment #8495943 - Flags: review?(dteller)
Attachment #8496401 - Flags: review?(dteller)
This affects Windows and Linux.
OS: Linux → All
Hardware: x86_64 → All
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?
Attachment #8496401 - Flags: review?(dteller) → review+
(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.
Iteration: --- → 35.2
Points: --- → 5
Flags: qe-verify-
Flags: firefox-backlog+
[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.
Depends on: 1073992
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?
https://hg.mozilla.org/mozilla-central/rev/7349529e5a81
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
(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.
Anyway, happy to take an uplift to aurora to increase the coverage :)
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
Attachment #8496401 - Flags: approval-mozilla-aurora?
Summary: Firefox looses my tabs every once in a while → Firefox loses my tabs every once in a while
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.
Attachment #8496401 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.