433975 - delete sessionstore.js when starting a new session

Status: RESOLVED INVALID

(Firefox :: Session Restore, defect)

Description

[juan becerra [:juanb]]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9) Gecko/2008051202 Firefox/3.0 (10.5.2)

While going through the session store test cases in litmus (test case 4275), it looks like you can restore to the wrong session, to the restored session prior to to starting new session after abnormal exit.

Steps to reproduce:
1. add this to your prefs.js file: user_pref("browser.sessionstore.interval", 1);
2. Start the browser and load the latest headlines in all tabs
3. kill -9 the process corresponding to your Firefox instance
4. Relaunch it. When the dialog to restore session comes up, select Start New Session.
5. kill -9 it too.
6. Relaunch it. When the dialog comes up, choose Restore Session.

Expected: Browser should launch and open the default Start page (assuming this is a clean profile where no home page customizations have been done).

Actual: You rever to the session in step 2, with the latest headlines open in all tabs.

It looks like upon relaunching the browser with a new session in step 4 the sessionstore files are not being deleted.

Comment 1

[Mike Connor [:mconnor]]

the timing of step 5 is probably critical.  We don't try to save your session in the first while (30 seconds?) to avoid broken state from loading tabs, and since its unlikely to be much data from the first 30 seconds anyway.  This is unlikely to be a regression, but we could delete the old files immediately on "start new session" to avoid this case.

Comment 2

[Simon Bünzli]

Attachment: delete sessionstore.js at startup if we don't restore from it

Comment 3

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

Comment on attachment 326926 [details] [diff] [review]
delete sessionstore.js at startup if we don't restore from it

>diff -r 8a770f50ccd3 browser/components/sessionstore/src/nsSessionStartup.js

>+    // delete the session file if it isn't wanted anymore to prevent it
>+    // from accidentally being used again in a follow-up crash
>+    if (!this.doRestore() && sessionFile.exists()) {
>+      try {
>+        sessionFile.remove(false);
>+      }
>+      catch (ex) { dump(ex + '\n'); } // couldn't remove the file - what now?
>     }

Hrm, I think I've relied on this bug before when I'd accidentally hit "new session". Should we move it to sessionstore.bak instead?

>     if (this._sessionType != Ci.nsISessionStartup.NO_SESSION) {

unrelated, but wouldn't this be clearer as if (this.doRestore())?

Comment 4

[Simon Bünzli]

Comment on attachment 326926 [details] [diff] [review]
delete sessionstore.js at startup if we don't restore from it

(In reply to comment #3)
> Hrm, I think I've relied on this bug before when I'd accidentally hit "new
> session". Should we move it to sessionstore.bak instead?

We should indeed. I'd even like to always move sessionstore.js to sessionstore.bak at this point (also when we're restoring according to a pref) so that we always have the current session and the previous one. Who'd have to decide such a change?

> unrelated, but wouldn't this be clearer as if (this.doRestore())?

It would and will be.

Comment 5

[Simon Bünzli]

Resolving as INVALID as with bug 448976 fixed, we can't just delete sessionstore.js without risking dataloss (other pages might already be open beside the Restore Session page).