Closed Bug 589441 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox :: Session Restore, defect, blocker)

defect
Not set
blocker

Tracking

()

VERIFIED FIXED
Firefox 4.0b10
Tracking Status
blocking2.0 --- final+

People

(Reporter: Virtual, Assigned: zpao)

References

(Blocks 1 open bug)

Details

(Keywords: dataloss, nightly-community, regression, Whiteboard: [hardblocker][has patch][fx4-fixed-bugday])

Attachments

(1 file)

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
blocking2.0: --- → ?
Version: unspecified → Trunk
QA Contact: general → session.restore
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.
(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
bump, 1 week and still unconfirmed...
Guys don't sleep ;)
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).
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: x86 → All
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.
blocking2.0: ? → final+
This sounds like bug 577652
Yep, partially, but on crash and add-on update session should be also restored
Assignee: nobody → paul
Any chances to get this fixed before shipping a RC ?
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.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
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 :)
Status: RESOLVED → VERIFIED
No longer blocks: 622379
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.
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.
Blocks: 622379
Status: VERIFIED → REOPENED
Resolution: DUPLICATE → ---
Summary: Settings "clear browsing history when Minefield closes" causing to not restoring any crashed or saved session → Settings "clear browsing history when Minefield closes" causes crashed sessions to not be restored
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.
No longer blocks: 622379
Keywords: dataloss
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.
Whiteboard: [hardblocker]
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.
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?
(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.
(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
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.
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.
Attached patch Patch v0.1Splinter Review
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.
Attachment #504778 - Flags: review?(dietrich)
No longer depends on: 577652
Whiteboard: [hardblocker] → [hardblocker][re-WONTFIX?][has patch][needs review dietrich]
Comment on attachment 504778 [details] [diff] [review]
Patch v0.1

Hrm, probably only testable via mozmill or litmus. Please follow those avenues up.
Attachment #504778 - Flags: review?(dietrich) → review+
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.
Whiteboard: [hardblocker][re-WONTFIX?][has patch][needs review dietrich] → [hardblocker][re-WONTFIX?][has patch]
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?
Pushed http://hg.mozilla.org/mozilla-central/rev/c5f632c83007. Bug 627548 was filed as a followup for the discussion about changing this behavior.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Flags: in-litmus?(anthony.s.hughes)
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b10
Verified using Minefield nightly from 2011-01-22.
Verified fixed on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre
Status: RESOLVED → VERIFIED
Whiteboard: [hardblocker][re-WONTFIX?][has patch] → [hardblocker][re-WONTFIX?][has patch][fx4-fixed-bugday]
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.
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.
Marking in-litmus+ based on comment 33.
Flags: in-litmus?(anthony.s.hughes) → in-litmus+
Whiteboard: [hardblocker][re-WONTFIX?][has patch][fx4-fixed-bugday] → [hardblocker][has patch][fx4-fixed-bugday]
You need to log in before you can comment on or make changes to this bug.