Session store shouldn't clobber its data file after failed restore

RESOLVED FIXED in Firefox 3.1b2

Status

()

Firefox
Session Restore
--
critical
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: bz, Assigned: Simon Bünzli)

Tracking

({dataloss})

Trunk
Firefox 3.1b2
dataloss
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

I just had my computer shut down due to low battery.  When I turned it back on, sessionstore failed to restore the things I had open.  Worse yet, it then immediately clobbered the sessionstore.js with one that has a single entry pointing to the "updated nightly" page.

It would be really nice if we'd at least move the old file to a backup (just one level deep) so that there's some hope of recovering data in situations where sessionstore totally fails like this...

Comment 1

10 years ago
In what way did it fail to restore things the first time?
(Assignee)

Comment 2

10 years ago
We already make a backup when Firefox crashes (i.e. when you're offered to either restore or start a new session): sessionstore.bak

But from your description, it sounds like Firefox was not shut down forcefully, nor did you tell it to restore your "windows and tabs" - in which case there just was nothing to restore in the first place. In case I'm wrong: please post some more detailed steps to reproduce...
Simon, Firefox was not shut down forcefully, but I have it set to open my windows and tabs from last time, and at quit tim I told it to save the windows and tabs.  So yes, there should have been restoring going on.

I wish I could give you more detailed steps to reproduce, but it's not reliably reproducible.  And reproducing the failure is irrelevant to what this bug is about which is not clobbering the data file.  In addition to making sure that sessionstore never fails to restore (which is a laudable goal, but one this bug is useless for) we should make sure that when it _does_ fail we don't lose the restore data.

Jesse, it failed in every possible way.  None of the tabs were restored.  All it showed was the default "new nightly" page, and automatically brought up the add-on manager for some reason.  The infobar in this claimed that two extensions were installed, and the main area was blank.  I had not in fact installed any extensions before quitting.  The extension manager thing might or might not be related to the sessionstore behavior.
(Assignee)

Comment 4

10 years ago
(In reply to comment #3)
> I have it set to open my windows and tabs from last time, and at quit tim
> I told it to save the windows and tabs.

That sounds wrong, nonetheless - you shouldn't get asked anything when quitting when Firefox is set to resume the session, anyway (unless this behavior has changed through a very recent check-in). Could it be that you've somehow managed to get your prefs.js corrupted (or not correctly updated)?

> we should make sure that when it _does_ fail we don't lose the restore data.

I'm actually not quite sure why we don't update sessionstore.bak at every startup. My original code did that, but it could quite well be that because of privacy concerns we've decided not to for SessionStore.

Maybe Dietrich can shed some more light on this decision and on what chance there is of reverting to always create sessionstore.bak.
> you shouldn't get asked anything when quitting

Sure you should.  You get asked whether to save the tabs, unless you've checked the "never ask me again" box.

> Could it be that you've somehow managed to get your prefs.js corrupted

No idea.  It's pretty irrelevant to this bug, in any case.
(Assignee)

Comment 6

10 years ago
(In reply to comment #5)
> Sure you should.

You really shouldn't as long as you've set Firefox to automatically resume your browsing session and bug 419009 hasn't been fixed yet. Then again, that's just as irrelevant for this bug as filed.
Hmm.  You're right, I have that setting on "home page".  I do get the prompt on quit, and select "save and quit".
(Assignee)

Updated

10 years ago
(Assignee)

Comment 8

9 years ago
(In reply to comment #4)
> Maybe Dietrich can shed some more light on this decision and on what chance
> there is of reverting to always create sessionstore.bak.

Dietrich: Ping?

Alternatively we could also introduce a new pref browser.sessionstore.max_backups and then just keep backups of the last max_backups sessions around (default: 2).
(Assignee)

Comment 9

9 years ago
Created attachment 342767 [details] [diff] [review]
always create a backup

With the changes from bug 448741, we have to always create a backup, anyway. Otherwise we might either inadvertently leak data when an extension encrypts sessionstore.js but we write sessionstore.bak as plain text file; or not create a backup at all when an extension cancels the restoration of a crashed session.
Assignee: nobody → zeniko
Attachment #342767 - Flags: review?(dietrich)
Comment on attachment 342767 [details] [diff] [review]
always create a backup



> 
>     // remove the session data files if crash recovery is disabled
>     if (!this._resume_from_crash)
>       this._clearDisk();
>+    // otherwise create a backup if the session data file exists
>+    try {
>+      if (this._sessionFileBackup.exists())
>+        this._sessionFileBackup.remove(false);
>+      if (this._sessionFile.exists())
>+        this._sessionFile.copyTo(null, this._sessionFileBackup.leafName);
>+    }
>+    catch (ex) { Cu.reportError(ex); } // file was write-locked?
>     

put this in an |else| block, to avoid the unnecessary filesystem hit if we've just cleared the files from disk.

(and remove that whitespace underneath since you're in the area)
Attachment #342767 - Flags: review?(dietrich) → review+
(Assignee)

Comment 11

9 years ago
Created attachment 344846 [details] [diff] [review]
for check-in
[Checkin: Comment 12]
Attachment #342767 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
Comment on attachment 344846 [details] [diff] [review]
for check-in
[Checkin: Comment 12]

http://hg.mozilla.org/mozilla-central/rev/1c977c8c9c67
Attachment #344846 - Attachment description: for check-in → for check-in [Checkin: Comment 12]
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
OS: Mac OS X → All
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.1b2
You need to log in before you can comment on or make changes to this bug.