558425 - Make backups of sessionstore.bak

Status: RESOLVED DUPLICATE of bug 883609

(Firefox :: Session Restore, defect)

Description

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

So I've seen a few people mention that sessionstore was losing windows when restoring from a crash (which they then crash during restore). If we want to look at the original sessionstore.bak, we can't because upon starting again, sessionstore.bak is overwritten.

Now if for some reason that first restore after crashing didn't do something correctly, we have absolutely no way of finding out, or recovering any of that data.

So my proposal is that we do 1 of these:
* Make an explicitly named sessionstore.TIMESTAMP.bak when recovering from a crash. Maybe limit to N files?
* Backup the last N sessions into a folder, like bookmarks does.

I'm liking the first option there much better. To most people it will be invisible, but for diagnosing the causes of a crashy session, or bad restore, or even recovering from the accidental "start new session" click, it will be invaluable.

Comment 1

[Daniel Mota Leite]

i vote for the second, make the last ... say 3.. session restores backups to a folder

i just closes FF just after opening it (my fault of course), before recovering the session, losing all my searches for the past few days... 
i have no way to get any of the previous sessions other that getting luck in the history, as i only have the current sessionstore.js in the profile folder

Other way, that i even like it more, is to put the session restore inside the bookmarks, this way people could restore the previous bookmark to get the previous (or other) session restore.

this way might be simpler, as all the backup routine is already setup, only needs a way to insert the session in the bookmarks

Comment 4

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Marking as [good first bug] in case anybody has interest in picking this up. A good starting point would be to see how bookmarks backups are made (there's some l10n/date formatting to have nice file names, but I'm inclined to just have sessionstore-YYYY-MM-DD.js).

If you're interested in working on this, please assign it to yourself and feel free to ask me any questions.

Comment 5

[Michael Kohler [:mkohler]]

Working on a patch, but it's not ready yet. I will upload a WIP patch which works shortly.

Comment 6

[Michael Kohler [:mkohler]]

Attachment: Patch v1

Comment 7

[:Gavin Sharp [email: [e-mail]]]

I don't think we should add any additional sync IO (i.e. the nsIFile.remove() calls in this patch, and the directory iteration in _getBackupEntries) now that we have OS.File. It's probably best to first port this code to that (possibly dependent on bug 729057), and then focus on this bug afterwards.

Comment 8

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 642408 [details] [diff] [review]
Patch v1

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

Thanks for getting this here Michael!

A couple comments.
1. I agree with Gavin that we should not be adding more sync IO in the startup path, so let's figure that part out
2. We especially don't need to enforce the max_backups at startup. We can move that to an idle timer or something. Even async at startup will hurt.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +442,5 @@
> +    if (maxBackups !== undefined && maxBackups > -1) {
> +      if (!this._sessionFileBackupDir.exists()) {
> +        this._sessionFileBackupDir.create(Ci.nsIFile.DIRECTORY_TYPE, 0700);
> +        if (!this._sessionFileBackupDir.exists())
> +          throw("Unable to create bookmarks backup folder");

This needs to be handled gracefully or be caught & handled. As is, if it throws we're going to break session restore in a bad way.

@@ +491,5 @@
> +
> +  // returns the prefix which is used for the session backup files
> +  get _sessionBackupFilePrefix() {
> +    return "sessionbackup";
> +  },

Just have _filenamesRegex and _sessionBackupFilePrefix defined as constants. You're pretty much there anyway :)

Comment 9

[Michael Kohler [:mkohler]]

(In reply to Paul O'Shannessy [:zpao] (no longer moco, slower to respond) from comment #8)
> Comment on attachment 642408 [details] [diff] [review]
> [...]
> 1. I agree with Gavin that we should not be adding more sync IO in the
> startup path, so let's figure that part out
> 2. We especially don't need to enforce the max_backups at startup. We can
> move that to an idle timer or something. Even async at startup will hurt.

I agree, let's wait until we can use OS.File.

> ::: browser/components/sessionstore/src/SessionStore.jsm
> @@ +442,5 @@
> > +    if (maxBackups !== undefined && maxBackups > -1) {
> > +      if (!this._sessionFileBackupDir.exists()) {
> > +        this._sessionFileBackupDir.create(Ci.nsIFile.DIRECTORY_TYPE, 0700);
> > +        if (!this._sessionFileBackupDir.exists())
> > +          throw("Unable to create bookmarks backup folder");
> 
> This needs to be handled gracefully or be caught & handled. As is, if it
> throws we're going to break session restore in a bad way.
> 
> @@ +491,5 @@
> > +
> > +  // returns the prefix which is used for the session backup files
> > +  get _sessionBackupFilePrefix() {
> > +    return "sessionbackup";
> > +  },
> 
> Just have _filenamesRegex and _sessionBackupFilePrefix defined as constants.
> You're pretty much there anyway :)

I will address this in the next patch version which I will do when OS.File is ready.

Comment 10

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

OS.File is quite ready, by the way.

Comment 12

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

Michael, are you still working on this?

Comment 13

[Michael Kohler [:mkohler]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #12)
> Michael, are you still working on this?

I think I'll finish v2 of the patch on Saturday :)

Comment 14

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

Michael: Which Saturday is that? :)

Comment 15

[Michael Kohler [:mkohler]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #14)
> Michael: Which Saturday is that? :)

sorry about that :) I don't have a lot of time right now, so it would probably better if someone else would finish this.

Comment 16

[Kan-Ru Chen [:kanru] (UTC+9)]

I'd like to finish this.

Comment 17

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

Any progress, kanru?

Comment 18

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Patch v2

Sorry that I almost forget that I took this bug... The SessionStore.jsm changed a lot so I take the weekend to rewrite the patch based on patch v1 and current SessionWorker.

Comment 19

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

Comment on attachment 829862 [details] [diff] [review]
Patch v2

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

This looks good.
Not r+ yet, because we'll need to discuss the interactions of this bug and bug 883609. It might be that bug 883609 is not needed anymore if we land this patch.

::: browser/components/sessionstore/src/SessionWorker.js
@@ +73,5 @@
>    // The path to sessionstore.bak
>    backupPath: OS.Path.join(OS.Constants.Path.profileDir, "sessionstore.bak"),
>  
> +  // The path to sessionbackups
> +  backupDirPath: OS.Path.join(OS.Constants.Path.profileDir, "sessionbackups"),

Please mention that it's a directory.

@@ +296,5 @@
> +    let exn;
> +    let entries = [];
> +    if (!OS.File.exists(this.backupDirPath)) {
> +      return entries;
> +    }

You can get the information "for free" with the directory iterator, without having to call exists().

Comment 20

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

We need to come up with a strategy between this bug and bug 883609.
This bug is about making backups of sessionstore.bak. Bug 883609 is about repurposing sessionstore.bak to always be the latest version of sessionstore.js that we know we have written completely.
We could even do a bit of both, i.e. have both the latest version of sessionstore.js that was written completely and a backup of the session during the latest start.

Kanru, Tim, Smacleod, any thought?

Comment 21

[Kan-Ru Chen [:kanru] (UTC+9)]

My only concern is that sometimes we write empty sessionstore.js on top of the not yet restored sessionstore.js. I think we could do both. With bug 883609 sessionstore.bak is the latest known good version of sessionstore.js, and when we write new sessionstore.bak we push the old one to sessionbackups/. Then we could possibly offer the users to select to restore from a revision, but that is another story..

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

So how about implementing bug 883609 and morphing this one to be more like bug 876168 in that instead of keeping upgrade backups we just keep a configurable number of backups that are created on startup? This way we could also address bug 942471.

Comment 23

[Steven MacLeod [:smacleod]]

It should be noted that once Bug 887780 lands, the backup won't be written at startup (if it's not an upgrade) since we will no longer immediately write.

How about we:
a) Keep a configurable number of startup backups (distinct from sessionstore.bak)
b) Move sessionstore.js to sessionstore.bak before every write (So we always have a pretty recent backup)

Then on restore we can 1) restore from sessionstore.js 2) if that fails restore from sessionstore.bak (which might be only slightly older)

We also have the other number of startup backups in case something goes wrong with ^

Comment 24

[peter]

Maybe this is redundant, maybe not...

I have noticed on a few occasions that if the OS (in my case either win xp or win 7) crashes, then sometimes ff cannot restore from sessionstore.

One problem is like the current bug, in that sessionstore.bak gets clobbered when exiting the session that should have restored previous state but did not.

However, there appears to be another problem, even when sessionstore.bak exists and has not been clobbered by saving a new null session.  Namely, it appears that sometimes sessionstore.bak is corrupted (probably because the OS crashed while it was being written), and the symptom to me as a user is that even if I manually copy sessionstore.bak to sessionstore.js, which clearly has lots of real data in it, when I start up FF, no session restoration or offer to restore occurs (yes, crash recovery is on, tabmixplus is installed and set to use FF native session manager).  This can be cured by copying in an old version of sessionstore.js that has been backed up by my backup system.

So, a related bug is that FF does not report to the user that sessionstore.js is corrupted.  (Most likely, sessionstore.js has been truncated due to system crash?)

Of course, if more than one version of sessionstore.bak were kept, this would be mitigated, assuming that far enough back a non-corrupted sessionstore.js was saved and kept.

Comment 25

[peter]

(In reply to peter blicher from comment #24)
> Maybe this is redundant, maybe not...
> 

Hmm, maybe my comment should be part of Bug 883609.

Comment 26

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

Normally, bug 883609 should fix most of this.

Comment 27

[Florian Bender]

I think this can be marked as a dupe now. Please reopen if you think Bug 883609 didn't fix the problem fully.