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

RESOLVED FIXED in Firefox 3 beta5

Status

()

Firefox
Bookmarks & History
P2
normal
RESOLVED FIXED
13 years ago
8 years ago

People

(Reporter: Jesse Ruderman, Assigned: mconnor)

Tracking

({privacy})

Trunk
Firefox 3 beta5
privacy
Points:
---
Bug Flags:
blocking-firefox3 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [has patch])

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

13 years ago
<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

13 years ago
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.
Priority: -- → P5
Summary: history.dat is never deleted after Places import (?) → history.dat should be deleted at some point
Target Milestone: --- → Firefox 2 beta2
(Reporter)

Comment 2

13 years ago
> 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

13 years ago
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

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

Comment 5

12 years ago
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.
Summary: history.dat should be deleted at some point → history.dat, formhistory.dat, and bookmarks.html should be deleted at some point

Comment 6

12 years ago
*** Bug 334201 has been marked as a duplicate of this bug. ***
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.
OS: Mac OS X 10.2 → All
Hardware: Macintosh → All
Flags: blocking-firefox3?
Target Milestone: Firefox 2 beta2 → ---
*** Bug 362003 has been marked as a duplicate of this bug. ***
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.
(Assignee)

Comment 10

11 years ago
Need a clear policy here, for the pwmgr file format changes we deleted on upgrade, but that was higher-risk.
Flags: blocking-firefox3? → blocking-firefox3+
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).
Summary: history.dat, formhistory.dat, and bookmarks.html should be deleted at some point → history.dat, formhistory.dat should be deleted at some point
(Assignee)

Comment 12

11 years ago
Need to have a solution in place by beta 1, though we might not activate that solution until we get to the RCs
Target Milestone: --- → Firefox 3 beta1
re-targeting to what might be beta 1.
Target Milestone: Firefox 3 M7 → Firefox 3 M8
Assignee: nobody → 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.
Created attachment 274996 [details] [diff] [review]
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?
Created attachment 275137 [details] [diff] [review]
fix v1
Attachment #274996 - Attachment is obsolete: true
Attachment #275137 - Flags: review?(sspitzer)
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.)
Attachment #275137 - Flags: review?(sspitzer)
Created attachment 275188 [details] [diff] [review]
fix v2
Attachment #275137 - Attachment is obsolete: true
Attachment #275188 - Flags: review?(sspitzer)
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.
Attachment #275188 - Flags: review?(sspitzer) → review+
Created attachment 275199 [details] [diff] [review]
for checkin
Attachment #275188 - Attachment is obsolete: true
(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.
Status: NEW → ASSIGNED
Assignee: dietrich → nobody
Status: ASSIGNED → NEW
Target Milestone: Firefox 3 M8 → Firefox 3 M9
Assignee: nobody → dietrich
dietrich / sdwilsh / dwitte:  what about cookies.txt and downloads.rdf?  It seems like we should delete those as well.
Summary: history.dat, formhistory.dat should be deleted at some point → history.dat, formhistory.dat, downloads.rdf should be deleted at some point
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

11 years ago
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.
Blocks: 362410
(Assignee)

Updated

11 years ago
Target Milestone: Firefox 3 M9 → Firefox 3 M10
Whiteboard: [has patch]
(Assignee)

Comment 25

11 years ago
Moving to M11 since we want to do this close to the end.
Target Milestone: Firefox 3 M10 → Firefox 3 M11
(Assignee)

Updated

11 years ago
Priority: P5 → P3
(Assignee)

Comment 26

11 years ago
Do we want to land this before or after b3?  I guess immediately after?
Whiteboard: [has patch] → [has patch][ready to land when needed]
> 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

Whiteboard: [has patch][ready to land when needed]
Summary: history.dat, formhistory.dat, downloads.rdf should be deleted at some point → history.dat, formhistory.dat, downloads.rdf should be deleted when the user clears private data
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?
Attachment #275199 - Flags: review-
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 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?
Target Milestone: Firefox 3 beta3 → ---
(Assignee)

Updated

10 years ago
Assignee: dietrich → mconnor
Priority: P3 → P2
Note to self: bug 412381 added support in the password manager for deleting old signons.txt files when clearing private data.
(Assignee)

Comment 32

10 years ago
Created attachment 308981 [details] [diff] [review]
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.
Attachment #275199 - Attachment is obsolete: true
(Assignee)

Comment 33

10 years ago
Created attachment 309005 [details] [diff] [review]
patch with tests
Attachment #308981 - Attachment is obsolete: true
Attachment #309005 - Flags: review?(gavin.sharp)
(Assignee)

Updated

10 years ago
Whiteboard: [has patch][needs review gavin]
I think you mean copyright 2008 in the license blocks ;)
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.
Attachment #309005 - Flags: review?(gavin.sharp) → review+

Comment 36

10 years ago
(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.
(Assignee)

Comment 37

10 years ago
Created attachment 310241 [details] [diff] [review]
as checked in

comments addressed
Attachment #309005 - Attachment is obsolete: true
(Assignee)

Comment 38

10 years ago
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
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 39

10 years ago
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

10 years ago
Though I've been told that that snippet might be par for the course and simply not output unless the test failed.
Whiteboard: [has patch][needs review gavin] → [has patch]
Target Milestone: --- → Firefox 3 beta5
Created attachment 310257 [details] [diff] [review]
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).
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.