329741 - history.dat, formhistory.dat, downloads.rdf should be deleted when the user clears private data

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[Jesse Ruderman]

<Jesse> does history.dat ever get deleted after switching to places?
<Ryan> I don't think so...

If not, it could be a privacy problem -- the sites you visited in the weeks before you upgraded to Firefox 2 will be listed on your computer forever, whether you clear history or not.

Comment 1

[Brett Wilson]

No we never delete history.dat. Right now this is by design so people can run places and non-places builds side-by-side, and also because we keep changing the database, requiring people to re-import.

For release, we should have some kind of plan. Perhaps when you clear your history the first time we go and delete it.

Comment 2

[Jesse Ruderman]

> No we never delete history.dat.  Right now this is by design so people can
> run places and non-places builds side-by-side, and also because we keep 
> changing the database, requiring people to re-import.

That's reasonable for alphas.

> For release, we should have some kind of plan. Perhaps when you clear your
> history the first time we go and delete it.

But then if you never clear history, your history from the ~2 weeks before you upgraded to Firefox 2 will be on your HD forever.

Comment 3

[Brett Wilson]

Maybe a better idea is to notice when we import history.dat and set a preference that indicates the expiration date of this file. This would be today + the number of days your history is valid for. On shutdown we check this preference, and if we are past the date, we delete history.dat and the preference.

Comment 4

[Brett Wilson]

*** Bug 333199 has been marked as a duplicate of this bug. ***

Comment 5

[Brett Wilson]

We should be careful for people who use multiple versions of the browser with
the same profile. If the files are in use (perhaps because they've been changed
since we upgraded) then we shouldn't do any automatic deleting.

Comment 6

[Brett Wilson]

*** Bug 334201 has been marked as a duplicate of this bug. ***

Comment 7

[Tuukka Tolvanen (sp3000)]

For history.dat, modification date of the file + the usual history expiration pref dtrt, since the contents would have expired by then even when using a version using that file.

Comment 8

[(not reading, please use [e-mail] instead)]

*** Bug 362003 has been marked as a duplicate of this bug. ***

Comment 9

[Justin Dolske [:Dolske]]

FWIW: I'm looking at some password manager changes, which will probably include moving the saved passwords to a MozStorage (SQL) file. So it will have the same problem as Places with respect to old files.

Comment 10

[Mike Connor [:mconnor]]

Need a clear policy here, for the pwmgr file format changes we deleted on upgrade, but that was higher-risk.

Comment 11

[(not reading, please use [e-mail] instead)]

we should not delete bookmarks.html, as we still write out bookmark data to that file which can be used by other browsers, and by ourselves on force places db migration (on schema version change).

Comment 12

[Mike Connor [:mconnor]]

Need to have a solution in place by beta 1, though we might not activate that solution until we get to the RCs

Comment 13

[Dietrich Ayala (:dietrich)]

re-targeting to what might be beta 1.

Comment 14

[Dietrich Ayala (:dietrich)]

(In reply to comment #7)
> For history.dat, modification date of the file + the usual history expiration
> pref dtrt, since the contents would have expired by then even when using a
> version using that file.
> 

this was reasonable when the default history expiration was 9 days. however, it has recently changed to 180 days.

Comment 15

[Dietrich Ayala (:dietrich)]

Attachment: wip

here's a patch that deletes the old history file at 9 days past last-modified time.

a couple of possible improvements:

1. if the history days pref is not 180, use it, else use 9.

2. delete the old history file whenever all of history is cleared.

does this approach sound good?

Comment 16

[Dietrich Ayala (:dietrich)]

Attachment: fix v1

Comment 17

[(not reading, please use [e-mail] instead)]

Comment on attachment 275137 [details] [diff] [review]
fix v1

instead of 180, can you check if mExpireDays is equal to the default value of the expire pref (from the default pref branch?)

180 may change on us.  (We have to hard code 9, however.)

Comment 18

[Dietrich Ayala (:dietrich)]

Attachment: fix v2

Comment 19

[(not reading, please use [e-mail] instead)]

Comment on attachment 275188 [details] [diff] [review]
fix v2

r=sspitzer

consider a comment or verbose #define (DEFAULT_EXPIRE_DAYS_IN_FIREFOX_2) to explain the 9 constant.

Comment 20

[Dietrich Ayala (:dietrich)]

Attachment: for checkin

Comment 21

[Dietrich Ayala (:dietrich)]

(In reply to comment #12)
> Need to have a solution in place by beta 1, though we might not activate that
> solution until we get to the RCs
> 

holding off on checking this in.

Comment 22

[(not reading, please use [e-mail] instead)]

dietrich / sdwilsh / dwitte:  what about cookies.txt and downloads.rdf?  It seems like we should delete those as well.

Comment 23

[Shawn Wilsher :sdwilsh]

My concern is what happens if the user goes back to 2.0?  Do we just nuke it and leave them "high and dry"?

Comment 24

[[e-mail]]

i decided to have cookies.txt be deleted upon successful migration to cookies.sqlite on trunk, for precisely this reason... i figured the downside of having orphaned private data outweighed the dataloss risk/inconvenience of not being able to go back and forth between trunk/branch and such. so, not an issue with cookies.

Comment 25

[Mike Connor [:mconnor]]

Moving to M11 since we want to do this close to the end.

Comment 26

[Mike Connor [:mconnor]]

Do we want to land this before or after b3?  I guess immediately after?

Comment 27

[(not reading, please use [e-mail] instead)]

> Do we want to land this before or after b3?  I guess immediately after?

note, the last patch in this bug only covers history.dat, and not other files.

keeping in mind that users might switch back and forth between fx 2 / 3 (for legit reasons, especially at first, for example if an add-on is not supported).

Note, we are currently leaving bookmarks.html alone for this reason.

Perhaps what we really want is to remove these files, if they exist, when the user choose to clear private data, in particular the private data that was associated with those fx 2 files.

(So when you clear history, we will attempt to remove history.dat, instead of doing it in ::Init() of the history service, like the attached patch does.)

something similar would happen for downloads.rdf and formhistory.dat

Comment 28

[(not reading, please use [e-mail] instead)]

Comment on attachment 275199 [details] [diff] [review]
for checkin

morphing bug and taking back my r+, because I think what we really want to do is remove these files when clearing private data.

I guess one scenario where this could bite us is if the user attempted to manually clear private data by selecting all history and deleting it, but then again, they aren't guaranteed that will clear private data (like Tools | Clear Private Data... should do)

mconnor/dietrich/jesse, what do you guys think?

Comment 29

[Dietrich Ayala (:dietrich)]

Sure, we should add that. However, the CPD approach doesn't cover users who don't clear private data ever, but expect their history to be gone in XXX days.

About switching back and forth between Fx2/3: This patch uses the last-modified-time of the history.dat on disk to determine whether it should be deleted, so if it's getting used, we won't remove it.

Comment 30

[Tuukka Tolvanen (sp3000)]

Comment on attachment 275199 [details] [diff] [review]
for checkin

>+    // if expired, remove
>+    PRInt64 expireDays = (mExpireDays == defaultExpireDays) ?
>+                         DEFAULT_EXPIRE_DAYS_IN_FIREFOX_2 : mExpireDays;

nitpickery I suppose, but why is this necessary when the default is being increased? There doesn't seem to be a privacy reason to not keep history.dat around for as long as the current implementation is keeping history regardless of whether that value comes from defaults, so isn't this extent of simulating when history.dat contents would have expired when using fx2 is superfluous?

Comment 31

[Justin Dolske [:Dolske]]

Note to self: bug 412381 added support in the password manager for deleting old signons.txt files when clearing private data.

Comment 32

[Mike Connor [:mconnor]]

Attachment: patch, no tests yet

adds cleanup code to the clear all functions in the right places, so consumers don't have to think about it.  no tests yet, but throwing this out there for now.

Comment 33

[Mike Connor [:mconnor]]

Attachment: patch with tests

Comment 34

[Shawn Wilsher :sdwilsh]

I think you mean copyright 2008 in the license blocks ;)

Comment 35

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

Comment on attachment 309005 [details] [diff] [review]
patch with tests

>Index: toolkit/components/places/src/nsNavHistory.cpp

>+  nsresult rv = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR,
>+                                       getter_AddRefs(oldHistoryFile));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  rv = oldHistoryFile->Append(NS_LITERAL_STRING("history.dat"));
>+  NS_ENSURE_SUCCESS(rv, rv);

Why not use NS_APP_HISTORY_50_FILE to match the code in nsNavHistory::Init?

>Index: toolkit/components/satchel/test/unit/head_satchel.js

>+cleanUpFormHist();
>+  
>+  
>\ No newline at end of file

Add one, get rid of the extra trailing spaces?

In general you're treating failure to remove the file as a fatal error - should we do that last in these methods, to ensure that we at least clear the "live" data before possibly failing to remove the old files? I suppose the removal is unlikely to fail, so it probably doesn't matter.

Comment 36

[[e-mail]]

(In reply to comment #35)
> (From update of attachment 309005 [details] [diff] [review])
> >Index: toolkit/components/places/src/nsNavHistory.cpp
> 
> >+  nsresult rv = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR,
> >+                                       getter_AddRefs(oldHistoryFile));
> >+  NS_ENSURE_SUCCESS(rv, rv);

also, please use |if (NS_FAILED(rv)) return rv;| here, because this function frequently fails (eg when running some kinds of test) and adds to debug warning spew.

Comment 37

[Mike Connor [:mconnor]]

Attachment: as checked in

comments addressed

Comment 38

[Mike Connor [:mconnor]]

Checking in toolkit/components/satchel/Makefile.in;
/cvsroot/mozilla/toolkit/components/satchel/Makefile.in,v  <--  Makefile.in
new revision: 1.3; previous revision: 1.2
done
Checking in toolkit/components/satchel/src/nsStorageFormHistory.cpp;
/cvsroot/mozilla/toolkit/components/satchel/src/nsStorageFormHistory.cpp,v  <--  nsStorageFormHistory.cpp
new revision: 1.21; previous revision: 1.20
done
RCS file: /cvsroot/mozilla/toolkit/components/satchel/test/Makefile.in,v
done
Checking in toolkit/components/satchel/test/Makefile.in;
/cvsroot/mozilla/toolkit/components/satchel/test/Makefile.in,v  <--  Makefile.in
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/components/satchel/test/unit/formhistory.dat,v
done
Checking in toolkit/components/satchel/test/unit/formhistory.dat;
/cvsroot/mozilla/toolkit/components/satchel/test/unit/formhistory.dat,v  <--  formhistory.dat
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/components/satchel/test/unit/head_satchel.js,v
done
Checking in toolkit/components/satchel/test/unit/head_satchel.js;
/cvsroot/mozilla/toolkit/components/satchel/test/unit/head_satchel.js,v  <--  head_satchel.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/components/satchel/test/unit/test_bug_329741.js,v
done
Checking in toolkit/components/satchel/test/unit/test_bug_329741.js;
/cvsroot/mozilla/toolkit/components/satchel/test/unit/test_bug_329741.js,v  <--  test_bug_329741.js
initial revision: 1.1
done
Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.274; previous revision: 1.273
done
RCS file: /cvsroot/mozilla/toolkit/components/places/tests/unit/history.dat,v
done
Checking in toolkit/components/places/tests/unit/history.dat;
/cvsroot/mozilla/toolkit/components/places/tests/unit/history.dat,v  <--  history.dat
initial revision: 1.1
done
Checking in toolkit/components/places/tests/unit/test_history.js;
/cvsroot/mozilla/toolkit/components/places/tests/unit/test_history.js,v  <--  test_history.js
new revision: 1.10; previous revision: 1.9
done
Checking in toolkit/components/downloads/src/nsDownloadManager.cpp;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp,v  <--  nsDownloadManager.cpp
new revision: 1.172; previous revision: 1.171
done
RCS file: /cvsroot/mozilla/toolkit/components/downloads/test/unit/test_bug_329741.js,v
done
Checking in toolkit/components/downloads/test/unit/test_bug_329741.js;
/cvsroot/mozilla/toolkit/components/downloads/test/unit/test_bug_329741.js,v  <--  test_bug_329741.js
initial revision: 1.1
done

Comment 39

[jag (Peter Annema)]

This is causing various tests to fail.

../../../../_tests/xpcshell-simple/test_places/unit/test_adaptive.js: FAIL
../../../../_tests/xpcshell-simple/test_places/unit/test_browserhistory.js: FAIL
../../../../_tests/xpcshell-simple/test_places/unit/test_expiration.js: FAIL
../../../../_tests/xpcshell-simple/test_places/unit/test_frecency.js: FAIL
../../../../_tests/xpcshell-simple/test_places/unit/test_history.js: FAIL

http://tinderbox.mozilla.org/showlog.cgi?tree=Firefox&errorparser=unittest&logfile=1205846207.1205848462.18696.gz&buildtime=1205846207&buildname=WINNT%205.2%20qm-win2k3-01%20dep%20unit%20test&fulltext=

NEXT ERROR ../../../../_tests/xpcshell-simple/test_places/unit/test_adaptive.js: FAIL
../../../../_tests/xpcshell-simple/test_places/unit/test_adaptive.js.log:
>>>>>>>
*** test pending
Test 0 same count, diff rank, same term; no search
[Exception... "'Failure' when calling method: [nsIDirectoryServiceProvider::getFile]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: ../../../../_tests/xpcshell-simple/test_places/unit/test_adaptive.js :: prepTest :: line 192"  data: no]
*** FAIL ***
Exception: [Exception... "Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIFile.remove]"  nsresult: "0x80520015 (NS_ERROR_FILE_ACCESS_DENIED)"  location: "JS frame :: ../../../../_tests/xpcshell-simple/test_places/unit/head_bookmarks.js :: clearDB :: line 92"  data: no]
<<<<<<<

Comment 40

[jag (Peter Annema)]

Though I've been told that that snippet might be par for the course and simply not output unless the test failed.

Comment 41

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

Attachment: bustage fix

nsNavHistory::Init now depends on NS_APP_HISTORY_50_FILE, this should hopefully fix all the bustage (worked on my machine, unit test boxes are still cycling).

Comment 42

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv