580892 - Checking 'clear history when minefield closes' is not clearing cache on shutdown but on startup

Status: VERIFIED FIXED

(Firefox :: Private Browsing, defect)

Description

[Luke Iliffe (Harlequin99)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9.2.7) Gecko/20100713 Firefox/3.6.7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9.2.7) Gecko/20100713 Firefox/3.6.7

In a new profile, checking the option to 'clear history when minefield closes' in the privacy pref pane, and ensuring 'cache' is selected in settings, it appears the cache is not cleared when minefield exits, but when it starts up again.

Note history, cookies etc do get cleared on shutdown.

This would have privacy implications allowing someone's cache to be viewed after they existed minefield and walked away.

Discussion on Mozillazine: http://forums.mozillazine.org/viewtopic.php?f=23&t=1950401&start=0

They mention bug 579334 and bug 580374 but I am not technical enough to really know what those bugs are talking about.

Reproducible: Always

Steps to Reproduce:
1. New profile, Firefox button, Options, Privacy, check 'clear history when minefield closes'. Check settings button to ensure 'cache' is selected (default)
2. Open your OS file manager & go to the cache location (On XP %USERPROFILE%\Local Settings\Application Data\Mozilla\Firefox\Profiles\<PROFILENAME>\Cache). Verify the cache files present.
3.Browse to some sites, e.g. google, images, maps to generate some cache content.
4. Refresh your OS file manager and note the newly created cache files.
5. Close minefield. Verify the process has exited in task manager.
6. Cache files are still present in the OS file manager.
7. Restart minefield and as it opens watch the OS file manager, the cache files disappear!
Actual Results:  
Cache is cleared on startup.

Expected Results:  
Cache is cleared on shutdown.

I have tested on XP, Mozilla/5.0 (Windows; Windows NT 5.1; rv:2.0b3pre) Gecko/20100721 Minefield/4.0b3pre

Others on Mozillazine appear to be using Windows 7 and Linux

Comment 1

[Luke Iliffe (Harlequin99)]

Mozilla Central Regression Range:
Works 2010-04-21 http://hg.mozilla.org/mozilla-central/rev/cdc8bf25220e

Broken 2010-04-22 http://hg.mozilla.org/mozilla-central/rev/cb8f60ee7c23

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cdc8bf25220e&tochange=cb8f60ee7c23

There are a number of possible candidates in the list but I would be just guessing, will leave to others more knowledgeable to work out the offender.

Comment 2

[(mostly gone) XtC4UaLL [:xtc4uall]]

Mak, could this be a Regression by Bug 529821?

Comment 3

[Johnathan Nightingale [:johnath]]

Blocking, and assigning to mak for investigation. Marco, if this needs to go elsewhere, feel free to reassign.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #2)
> Mak, could this be a Regression by Bug 529821?

that bug regressed clear history and we fixed it in bug 562644.

Comment 0 has been posted with 3.6.7, but is it relative to latest nightly right?
I can exclude bug 580374.

History and cache are not cleared by the same code clearly, need to check if cache is cleared at the wrong time.
bug 562644 could be culprit by the fact it moved earlier some shutdown stuff, but has to be verified, even if at first look is the only valid candidate.

Will try to figure out something.

Comment 5

[Luke Iliffe (Harlequin99)]

(In reply to comment #4)
> Comment 0 has been posted with 3.6.7, but is it relative to latest nightly
> right?

Yes I posted the bug using 3.6.7 but tested with latest nightly, and trunk builds for the regression range. This does NOT occur in 3.6.7 (I did check that too for sanity).

Comment 6

[(no longer active)]

Marco, could bug 578892 also be a dupe of this one?

Comment 7

[logos]

just a confirmation of what the OP of this thread observed. No clearing of private data when set to do so at closing time. Things weirdly happen when reopening the browser. This concerns cache, forms history, and cookies too, but not browsing history. Browsing history gets cleared when closing FF, as expected, if set to do so.
 The issue with cookies is a bit more complicated, because there's a separate setting interface >>> if cookies are set to be kept until Firefox closes, as opposed "when they expire", then they will be deleted when closing Firefox. But if they are set to be kept until expiration, and on the other hand they are set to be deleted from the general "clearing history when Firefox closes" interface, then they're not deleted at all when closing Firefox.

Comment 8

[logos]

just to add that downloads and passwords if set to be cleared at closing time are deleted as expected, and when expected.Behavior is okay like for history.

Comment 9

[logos]

(In reply to comment #8)
> just to add that downloads and passwords if set to be cleared at closing time
> are deleted as expected, and when expected.Behavior is okay like for history.

this was tested on my side with 4.0b2 build1

Comment 10

[Alice0775 White]

I reverted to changeset 80e39b33fc3a in local win build, the problem still happens,
And reverted to changeset ab4c6ba5ff70, the problem does not happen.

So, landing of changeset 80e39b33fc3a(Bug 529821) seems to cause a problem.

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

in bug 588651 Neil noticed that there is no event loop spinning between profile-before-change and xpcom-shutdown, so I had some wrong assumption.
My current plan would be to shutdown Places synchronously at profile-change-teardown, and to finally close the connection (And do the final sync) at profile-before-change.
I also suspect fixing this will show Ts Shutdown regressions, but I'm not much worried about that because it's most likely previous improvements were partly lying to us.

My only doubts are regarding the Sanitizer, to work correctly with clear history it should run at profile-change-teardown, but there are other parties involved (like cache for example) that could try to add data after this notification, making it leaving some stuff behind.

Another possibility could involve adding an enqueueNotifyObservers() api to the observer service, that will notify a topic as soon as current one has finished before giving back control to the caller, so it would be possible to avoid having nested topics.

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

At first glance, looking at mxr for profile-before-change listeners does not look like there could be issues calling sanitize() at profile-change-teardown.

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: wip v1.0

something like this.

Comment 14

[logos]

just to mention that the issue is still unsolved in 4.0b4, but I suppose you guys know it and are working on it ;)

Comment 15

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

yes we are aware of the issue and it's a blocker, I'll be back on it as soon as I can.

Comment 16

[logos]

okay thanks ;)

Comment 17

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.0

Comment 18

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.1

Added a test for cache too.
Locally all tests are passing, but I'll do a tryserver run to check them and Talos since changing shutdown is always challenging. As soon as tryserver reopens.

Comment 19

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

once fixed, should update Places page on mdc that reports notifications.

Comment 20

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

this should probably land sooner than later, beta5 would be perfect. While there should be no risk since we were already shutting down even earlier (quit-application) as Neil pointed out on IRC, shutdown changes could always bring some unwanted effect.

Comment 21

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.2

ready to qimport

Comment 22

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

This could possibly show a fake Ts shutdown regression, I see something on Windows on tryserver even if it's a bit oscillating. this is not changing amount of work we do, but it's moving stuff that could have been ignored (was after xpcom-shutdown) to a more decent and correct position. So the regression, if present, is ignorable.

Comment 23

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

This was pushed a few days ago:
http://hg.mozilla.org/mozilla-central/rev/7557b96e9e02

I haven't heard of any shutdown regressions (but I'm behind on mail).

Comment 24

[Luke Iliffe (Harlequin99)]

Verified fixed on XP based on my STR in comment 0. Sanity checked common things like cookies and browsing history still behave as expected and they do.

Would be nice if someone could verify on linux , OSX and Win7

Comment 25

[Eric Shepherd [:sheppy]]

I've documented the places-connection-closed, places-will-close-connection, and places-connection-closing notifications:

https://developer.mozilla.org/en/Observer_Notifications#Places

Comment 26

[pal-moz]

(In reply to comment #24)
> Verified fixed on XP based on my STR in comment 0. Sanity checked common things
> like cookies and browsing history still behave as expected and they do.
> 
> Would be nice if someone could verify on linux , OSX and Win7

fixed on Windows 7 also.

Comment 27

[logos]

yeah confirming this is fixed since beta5; I'm running beta6pre now and it works just...well as fine as it always use to since I use Firefox (W7/64 here)

Comment 28

[VanillaMozilla]

Is this actually fixed?  I don't see any evidence of approval or checkin, and a year later, there is a credible report of an incompletely deleted cache, here:  http://forums.mozillazine.org/viewtopic.php?f=38&t=2376317 .

I can't reproduce the problem, but the user reports that _CACHE_001,CACHE_002, _CACHE_003, and _CACHE_MAP are not deleted.

Comment 29

[Michael Lefevre]

(In reply to VanillaMozilla from comment #28)
> Is this actually fixed?  I don't see any evidence of approval or checkin,

It was, see comment 23 (and several comments verifying the fix)

> and a year later, there is a credible report [...]

Given that this was fixed and it is a year later, whether it is the same issue or a different issue, it should be reported as a new bug.

Comment 30

[VanillaMozilla]

(In reply to Michael Lefevre from comment #29)
> Given that this was fixed and it is a year later, whether it is the same
> issue or a different issue, it should be reported as a new bug.

This is bug 706224.

Comment 31

[VanillaMozilla]

I forgot to mention, I think Preferences is the wrong component for that bug, but I don't want to change it because I'm not entirely sure about Private Browsing either (does it cover just Private Browsing Mode or all privacy matters?).