Closed Bug 558425 Opened 15 years ago Closed 10 years ago

Make backups of sessionstore.bak

Categories

(Firefox :: Session Restore, defect)

defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 883609

People

(Reporter: zpao, Assigned: kanru, Mentored)

References

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 file, 1 obsolete file)

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.
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
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.
Whiteboard: [good first bug][mentor=zpao]
Assignee: nobody → indiasuny000
Assignee: indiasuny000 → nobody
Assignee: nobody → brijesh3105
Assignee: brijesh3105 → nobody
Whiteboard: [good first bug][mentor=zpao] → [good first bug][mentor=zpao][lang=js]
Assignee: nobody → michaelkohler
Status: NEW → ASSIGNED
Working on a patch, but it's not ready yet. I will upload a WIP patch which works shortly.
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #642408 - Flags: review?(paul)
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 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 :)
Attachment #642408 - Flags: review?(paul) → review-
(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.
OS.File is quite ready, by the way.
Michael, are you still working on this?
Flags: needinfo?(mkohler)
Blocks: 589095
(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 :)
Flags: needinfo?(mkohler)
Michael: Which Saturday is that? :)
Flags: needinfo?(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.
Assignee: mkohler → nobody
Flags: needinfo?(mkohler)
I'd like to finish this.
Assignee: nobody → kchen
Any progress, kanru?
Flags: needinfo?(kchen)
Attached patch Patch v2Splinter Review
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.
Attachment #642408 - Attachment is obsolete: true
Attachment #829862 - Flags: review?(paul)
Flags: needinfo?(kchen)
Attachment #829862 - Flags: review?(paul) → review?(dteller)
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().
Attachment #829862 - Flags: review?(dteller) → feedback+
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?
Flags: needinfo?(ttaubert)
Flags: needinfo?(smacleod)
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..
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.
Flags: needinfo?(ttaubert)
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 ^
Flags: needinfo?(smacleod)
See Also: → 883609
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.
(In reply to peter blicher from comment #24) > Maybe this is redundant, maybe not... > Hmm, maybe my comment should be part of Bug 883609.
Normally, bug 883609 should fix most of this.
Mentor: paul
Whiteboard: [good first bug][mentor=zpao][lang=js] → [good first bug][lang=js]
I think this can be marked as a dupe now. Please reopen if you think Bug 883609 didn't fix the problem fully.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: