487219 - Current session data is lost upon browser restart if the option to clear browsing history upon shutdown is set

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect, P2)

Description

[Michael Kraft [:morac]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090406 Firefox/3.5b4pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090406 Firefox/3.5b4pre

If the option to "always clear my private data when I close <browser>" is set and includes the "Browsing History", then restarting the browser will result in the current session being lost. 

I understand that if the clear private data on shutdown option is set, that the session data should be deleted when the browser is closed regardless of the browser startup options (see bug 398817), but the session data should not be cleared when the browser is restarted as most users would not consider a restart to be "closing the browser".

As is, if the setting to clear private data on shutdown is set, then the current session will be lost if a user installs an add-on, upgrades the browser, disables/enables an add-on, etc and then clicks "Restart" when prompted.

Reproducible: Always

Steps to Reproduce:
1. Check the "always clear my private data when I close <browser>" option and make sure it includes "Browsing History"
2. Go to addons.mozilla.org and install any add-on.
3. When prompted to restart the browser, do so.
Actual Results:  
The current session is lost.

Expected Results:  
The current session should be maintained on a restart, regardless of the clear private data on shutdown setting.

This issue was caused by the fix to bug 483566 and exists in the Firefox 3.5 and trunk branches.

See bug 398817 for the initial discussion about clearing the session data on shutdown.

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

Blocking final. Connor, feel free to re-assign if you think someone else (adw?) can take this. We're able to determine the difference between a restart and a shutdown, right?

Comment 2

[Michael Kraft [:morac]]

On a restart, the aData for the quit-application and quit-application-requested notifications will be "restart".  At least it should be for all restart cases.  

It should be fairly simple to check for that.

Comment 3

[Johnathan Nightingale [:johnath]]

(Adding ETA from conversation today)

Comment 4

[Johnathan Nightingale [:johnath]]

mconnor - ping? This is ETA today, but I don't know the state of the work.

Comment 5

[Mike Connor [:mconnor]]

Yeah, my laptop issues have been screwing with me.  Still no dev tools, but I think the easiest fix is to just add an explicit bit at http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.js#329 so that it reads:

  if (aData == "restart") {
    this._prefBranch.setBoolPref("sessionstore.resume_session_once", true);
    this._clearingOnShutdown = false;
  }

This is the simplest/safest fix, so if someone wants to turn that into a patch and get review, that'd be great, or I can do that sometime tomorrow.

Comment 6

[Johnathan Nightingale [:johnath]]

Attachment: _clearOnShutdown should be false in restart case

Per mconnor's description. No test, because I don't know a good way to test across restarts, but I will flag for litmus.

Comment 7

[Simon Bünzli]

Comment on attachment 377522 [details] [diff] [review]
_clearOnShutdown should be false in restart case

Thanks. We don't have a reasonable way to test session resuming, so a Litmus test will have to do for now.

Comment 8

[Johnathan Nightingale [:johnath]]

I'll try to land this tomorrow, but in case I don't, tagging it checkin-needed

Comment 9

[Johnathan Nightingale [:johnath]]

http://hg.mozilla.org/mozilla-central/rev/1bb09e3c69e8

Comment 10

[Johnathan Nightingale [:johnath]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0f482d814a11

Comment 11

[Marcia Knous [:marcia]]

Verified fixed on the 1.9.1 branch using : Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1pre) Gecko/20090521 Shiretoko/3.5pre and the latest Mac nightly. Adding keyword. Litmus test on the way...

Comment 12

[Marcia Knous [:marcia]]

https://litmus.mozilla.org/show_test.cgi?id=7738 added to both BFT and FFT.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed on trunk with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090711 Minefield/3.6a1pre ID:20090711031617

Comment 14

[Michael Kraft [:morac]]

This bug has regressed under Firefox 4.0.  I filled bug 622379 documenting the regression.