589441 - Settings "clear browsing history when Minefield closes" causes crashed sessions to not be restored

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0b5pre) Gecko/20100820 Minefield/4.0b5pre
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b5pre) Gecko/20100820 Minefield/4.0b5pre

 

Reproducible: Always

Steps to Reproduce:
go to "Options" => "Privacy" => enable "Clear history when Minefield closes" and click "Settings..."=> enable "Browsing History"
Actual Results:  
Crashed or saved session isn't restored

Expected Results:  
Crashed or saved session will be restored

Comment 1

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Working fine with stable Firefox 3.6.8

Comment 2

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

This is expected (privacy is trumping here).

(In reply to comment #1)
> Working fine with stable Firefox 3.6.8

Are you sure? It shouldn't be.

Comment 3

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

(In reply to comment #2)
> (In reply to comment #1)
> > Working fine with stable Firefox 3.6.8
> 
> Are you sure? It shouldn't be.
Yes, tested now with Firefox Portable 3.6.8

(In reply to comment #2)
> This is expected (privacy is trumping here).
I don't thinks so, it's same like remembering browsing history which I have disabled, and latest closed tabs and windows or undo latest closed tab work...

It's clearly a bug! Even I have info after I install some add-on, that are you want your tabs restored on next startup or sth like this

Comment 4

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

bump, 1 week and still unconfirmed...
Guys don't sleep ;)

Comment 5

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

Hmm, confirming (Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b5pre) Gecko/20100830 Firefox/4.0b5pre ID:20100830030743). I guess we should be restoring from crashes regardless. The file is there, so I'm not completely sure what's happening.

BernesB - would you be willing to try to get a regression range? You just need to try out several builds (easy to test the betas, then narrow it down with nightlies if need be).

Comment 6

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

I agree that we should be restoring from a crash and for a browser restart for app/add-on update/install, but we should not restore from a "saved session". In the past we made it such that if you select that all history should be cleared on shutdown, we simply didn't offer the "Save and Quit" feature anymore.

Comment 7

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Works - https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2010/07/2010-07-01-04-mozilla-central/

Fails - https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2010/07/2010-07-02-04-mozilla-central/

Comment 8

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

(In reply to comment #7)
> Works -
> https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2010/07/2010-07-01-04-mozilla-central/
> 
> Fails -
> https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2010/07/2010-07-02-04-mozilla-central/

Translates to:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=82edf5bd1abe&tochange=c173731c9d90

Not seeing any obvious culprits, though the component registration change in there (shouldn't be  guilty, but omgchange)

Comment 9

[Maniac Vlad Florin (:vladmaniac)]

This sounds like bug 577652

Comment 10

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Yep, partially, but on crash and add-on update session should be also restored

Comment 11

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Any chances to get this fixed before shipping a RC ?

Comment 12

[Johnathan Nightingale [:johnath]]

Discussed in The Grand Retriage - we believe the intent communicated by toggling this setting is "don't record my browsing, I don't want them discovered by others" and that intent outweighs the "helpfulness" offered by crash recovery.

Comment 13

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

OK, I understand.
But should we in this case remove also this prompting messages to recover tabs on next run when updating addons or on crash? Because they don't work in this state and cause misunderstanding. Nice will be also informing ppl about this change in changelog, when Fx4 will be released :)

Comment 15

[Michael Kraft [:morac]]

I'll mention that this bug is not a dupe of bug 622379 because that bug only deals with restarts.  The fix for that bug won't have solve the problem as documented here, meaning it won't restore crashed or saved sessions when the "clear browsing history..." setting is set.

Comment 16

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Ahh, sorry, seems that I didn't understand it fully on my first reading then, thanks for clarification.

I understand why saved session isn't restored like Mike Beltzner said in comment 6 (https://bugzilla.mozilla.org/show_bug.cgi?id=589441#c6) and this is a good change.

But we should restore crashes sessions as well as restarted session like in bug #622379 (Mike Beltzner mention it too), because user didn't want to lose all work, it was just simply a program fault.

So REOPENED this for further reconsidering it as regression.

Comment 17

[Michael Kraft [:morac]]

The problem with crashes is that a "browser:purge-session-history" is sent out when Minefield is run after a crash.  So basically SessionStart sets up the crash data for SessionStore, but a "browser:purge-session-history" comes in and wipes it all out before SessionStore can restore the crashed session.  

The "browser:purge-session-history" notification is being triggered after a crash because the "privacy.sanitize.didShutdownSanitize" preference is not set to true; it gets set on shut down.

That really has nothing to do with the bug fix in bug 622379 which is a fix in SessionStore.  This problem is with sanitize.js since that's where this preference is used to send the notification

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/sanitize.js#488

It actually looks like this was done by design.  My guess is it's another issue with observer notification order.

Comment 18

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

I'm not sure what the right solution is here; as mentioned in several triage comments, there's a bad interplay between privacy and convenience.

My feeling is that this should remain WONTFIX, but I don't like the idea of the behaviour being this way because of a regression. I support the behaviour as it is in Firefox 4 now, though, since a user might be away from their machine when it crashes, and having that session be restored could leak private data.

Marking [hardblocker] since we need a decision on this. It would help me a lot to know how the behaviour change was caused.

Comment 19

[Michael Kraft [:morac]]

I'll note that in the case of a crash the session data is actually already saved on the disk. It isn't until the browser is started again that the data is deleted off the disk.  A knowlegable person can recover the session data between the time of the crash and the time it's deleted and trick Firefox 4 into loading it. 

So basically privacy is already compromised at the time of the crash.   Wiping the session data at run time simply gives the illusion of privacy. 

As a compromise though maybe the last update time could be used to determine if the session dat is "new" enough to be restored. "Old" crashed sessions can be deleted at startup in fhis case.  The last update time is stored in the sessionstore.js file already.

Comment 20

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

I'm not completely sure where I am here either, but I'll throw out that after I looked at Sanitizer, it seems apparent that the reason we send out the purge-session-history notification was to cover the case of crashes.

From http://mxr.mozilla.org/mozilla-central/source/browser/base/content/sanitize.js#475: "we check for unclean exit with pending sanitization"

So if anything what we're doing now is more right.

As to what caused this change... The regression range above leads me to believe that somehow the component registration change made sessionstartup open earlier that it was before. My theory is the previously nsBrowserGlue got the "final-ui-startup" notification first, which then triggered "browser:purge-session-history", which nsSessionStartup got and then it wiped the session data it had. Only it hadn't been notified of "final-ui-startup" yet so it hadn't actually read the file from disk yet. It then gets notified of "final-ui-startup" and reads the file from disk. But now nsSessionStartup is getting notified first and so reads the file, then nsBrowserGlue gets notified and sends out the purge notification.

That's the only theory I've come up with that (a) fits with the regression range, (b) fits in with the fact that I never intentionally changed (nor saw others change) this behavior in session restore, and (c) isn't entirely crackpot. I'll test this theory tomorrow.

If that's the case though... what else might have unintentionally changed from the component registration change?

Comment 21

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

(In reply to comment #20)
it seems apparent that the reason we send out the purge-session-history notification [at startup] was to cover the case of crashes.

Comment 22

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

(In reply to comment #20)
AWESOME. So that's exactly what happened.

Pre-compreg-change
nsSessionStartup OBSERVED app-startup
nsBrowserGlue OBSERVED app-startup
nsBrowserGlue OBSERVED final-ui-startup
nsBrowserGlue OBSERVED browser:purge-session-history
nsSessionStartup OBSERVED final-ui-startup
nsSessionStartup OBSERVED domwindowopened
nsBrowserGlue OBSERVED places-init-complete
nsBrowserGlue OBSERVED distribution-customization-complete
nsBrowserGlue OBSERVED sessionstore-windows-restored

Post-compreg-change
nsBrowserGlue OBSERVED app-startup
nsSessionStartup OBSERVED app-startup
nsSessionStartup OBSERVED final-ui-startup
nsBrowserGlue OBSERVED final-ui-startup
nsSessionStartup OBSERVED browser:purge-session-history
nsBrowserGlue OBSERVED browser:purge-session-history
nsSessionStartup OBSERVED domwindowopened
nsBrowserGlue OBSERVED places-init-complete
nsBrowserGlue OBSERVED distribution-customization-complete
nsBrowserGlue OBSERVED sessionstore-windows-restored

Comment 23

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

Bug 457195 introduced the observer in nsSessionStartup to ensure the previous session wasn't shown in about:sessionrestore after the purge-session-history notification. It doesn't seem that the startup case was considered (and when testing would have been done, crashes would restore based on notification order). So I maintain that this was entirely an unintentional change.

I keep going back and forth about what is right here. But I've been talking for a while now, so I'm going to shutup until I have a solid opinion or somebody else does.

FWIW, This should be pretty straightforward to fix if we decide it needs to be.

Comment 24

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

In my opinion, with not detailed info for coder like you are putting here ;p
I want that Firefox with enabled "clear browsing history when Minefield closes":

-will restore my latest crashed session
-other crashed sessions will be automatically deleted when this will be saved after crash

This will be a compromise in privacy and user friendly action.

We can also consider disable all restoring like you firstly want, but only in Private Browsing mode.

Comment 25

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

Attachment: Patch v0.1

Moves the addition of the browser:purge-session-history observer to after sessionstore does it's thing.

I'm more sure we should take this and go back to what we were doing forever. Not 100%, but ~90%. The crash while you're away from your computer isn't great, but oh well - the chance of you having Firefox set to clear history on quit AND crashing AND not being in front of your computer at the time is pretty small.

Comment 26

[Dietrich Ayala (:dietrich)]

Comment on attachment 504778 [details] [diff] [review]
Patch v0.1

Hrm, probably only testable via mozmill or litmus. Please follow those avenues up.

Comment 27

[Michael Kraft [:morac]]

I noticed that the browser.sessionstore.privacy_level preference in Firefox 4 was switched from 1 (unencrypted only) to 0 (everywhere), meaning that SSL pages are now stored.

Because of that I'm thinking that this should remain WONTFIX since it could result in banking or other information being recovered from a crashed session.  Even if that's 10% of the time, that's bad.  On the flip side, crashed session data isn't saved when private browsing is enable and one would think someone doing banking on a public computer would use that.  Still it's something to think about.

Comment 28

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

OK, there's two different things going on here:

1. An unintentional behavioural change as a side effect from bug 568691 - this needs to be fixed, as it may have *other* unintentional behaviours.

2. A proposed change to the existing behaviour that matches the unintentional change.

It seems clear that we should fix the side effect / regression, and return to how things were. So we should take the patch here and close this bug out.

Paul's point about this being a pretty rare occurrence is fair, though we should remember that we interpret a force-restart from Windows as a "crash" as our process is killed; so if you leave your Windows 7 computer and it restarts for a critical update, that means that you could leak data as mentioned in comment 27. However, that's the way it's been for all of Firefox 3.6's lifetime, at least, and to change that behaviour we should file a bug (and nominate for blocking) to cover that specific change.

Make sense?

Comment 29

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

Pushed http://hg.mozilla.org/mozilla-central/rev/c5f632c83007. Bug 627548 was filed as a followup for the discussion about changing this behavior.

Comment 30

[u279076]

Verified using Minefield nightly from 2011-01-22.

Comment 31

[Steve Scott (pxbugz)]

Verified fixed on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre

Comment 32

[u279076]

RE: in-litmus? request

We have the following test which was created for bug 483566:
https://litmus.mozilla.org/show_test.cgi?id=11353

Paul, can you please review and let me know what implications this bug has on that test.

Comment 33

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

I think that test case is still valid and covers this. It actually would have failed were this bug not fixed since this was a regression. Thanks for looking.

Comment 34

[u279076]

Marking in-litmus+ based on comment 33.