on shutdown, expire history entries based on the browser.history_expire_days

RESOLVED WONTFIX

Status

()

Firefox
Bookmarks & History
P2
normal
RESOLVED WONTFIX
11 years ago
6 years ago

People

(Reporter: (not reading, please use seth@sspitzer.org instead), Unassigned)

Tracking

({relnote})

Trunk
relnote
Points:
---
Bug Flags:
blocking-firefox3 -
blocking-firefox3.5 -
wanted-firefox3 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 obsolete attachments)

[places] on shutdown, expire history entries based on the browser.history_expire_days

from ria:

Setting browser.history_expire_days to 0 has no effect since Bug 355738 landed,
it does not delete the history entries from the sqlite file; history is still
present in the sidebar. Although it stops gathering new history.
The sanitize function (Tools > Clear Private Data) works however.
Could be that this is an independent bug, for I see it also without imported
history.

So the steps are:
- Start Firefox in a new profile
- Visit www.cnn.com
- Set Tools > Options > Privacy > History > Remember visited pages to 0 days
- Close the browser
When you start the browser you'll see that www.cnn.com is still present; should
be empty. 

Firefox 2.0 appears to do this work in nsGlobalHistory::CloseDB(), see
http://lxr.mozilla.org/seamonkey/source/toolkit/components/history/src/nsGlobalHistory.cpp#3136

minefield doesn't seem to be doing this.  

also, I wonder if we should expire history entries when we observe the pref changes, and not on shutdown.
See bug 328598 - you don't want to expire on shutdown.
thanks for the pointer, phil.

so, what about the idea of doing the expiration in the pref observer (in nsNavHistory.cpp) when the user changes the pref (but only if the "new" expire days is less than the "old" expire days.)

we'll still be paying a price for doing the expiration, but at least it will be associated with the action taken, and not on shutdown.

note, firefox 2.0 does not behave this way, but I think this would be more correct, as disabling the "Remember pages visited" pref would immediately clear history.

comments?

Comment 3

11 years ago
To avoid delaying shutdown, places expires history as you browse. On shutdown, it does some extra work to catch some edgecases (the *Paranoid functions). This code is in toolkit/components/places/src/nsNavHistoryExpire.cpp

I'm not sure if the 0 days case is handled properly. In this case, OnAddURI
may never get called, and this is the function that schedules incremental expiration. OnExpirationChanged should handle all non-zero cases, although the items won't immediately be deleted (I think it's 5 items deleted per page visited).

For the 0 day case, you may want to call nsNavHistoryExpire::ClearHistory in OnExpirationChanged when the days are 0.

If it's really important that when the user decreases the expiration time those items get deleted immediately (I doubt this is important), you may want to modify ExpireItems. It takes the maximum number of things to expire as a parameter. Currently, we use 0 to mean expire all history. You may want to do something like making -1 expire all history, and 0 expire as many items as possible given the current expiration time. Then call this from OnExpirationChanged.
Problem is (also) that none of the history is deleted when I change browser.history_expire_days from 600 days to 1 day. I changed it 24 hours ago and al my history is still there.
Flags: blocking-firefox3?
Target Milestone: --- → Firefox 3
Flags: blocking-firefox3? → blocking-firefox3+

Updated

10 years ago
Target Milestone: Firefox 3 → Firefox 3 beta1

Updated

10 years ago
Target Milestone: Firefox 3 M7 → Firefox 3 M8
Might need to relnote this for M8, moving out to M9.
Keywords: relnote
Target Milestone: Firefox 3 M8 → Firefox 3 M9

Updated

10 years ago
Component: History → Places
OS: Windows XP → All
QA Contact: history → places
Hardware: PC → All
Assignee: nobody → dietrich
Created attachment 285002 [details] [diff] [review]
wip

the problem was that we currently do zero expiration at shutdown in some cases, even when there might be history to expire.

when that was combined with our really slow incremental expiration (which also doesn't expire oldest to newest btw), it basically would never catch up.

this fix does the following:
- always run expiration at shutdown
- do expiration when 15 mins idle, hooking into the history vacuum timer for this (might want to alternate or something)

expiring 6mos+ of history took about 20 secs w/ a debug build. i'd prefer to err on the side of privacy here though, and fix perf in bug 395066.
Created attachment 285053 [details] [diff] [review]
more
Attachment #285002 - Attachment is obsolete: true

Updated

10 years ago
Target Milestone: Firefox 3 M9 → Firefox 3 M10
Priority: -- → P2

Updated

10 years ago
Priority: P2 → P3
Target Milestone: Firefox 3 M10 → Firefox 3 M11
Attachment #285053 - Attachment is obsolete: true
Target Milestone: Firefox 3 beta3 → Firefox 3 beta4
Target Milestone: Firefox 3 beta4 → Firefox 3

Updated

9 years ago
Priority: P3 → P4
Not blocking on this bug for final ship. Would take a safe enough patch if one comes through.
Flags: wanted-firefox3+
Flags: blocking-firefox3-
Flags: blocking-firefox3+
(In reply to comment #6)
> the problem was that we currently do zero expiration at shutdown in some cases, even when there might be history to expire.

i'm not sure this is still true

> (which also doesn't expire oldest to newest btw)

this is no more true after latest changes
(In reply to comment #4)
> Problem is (also) that none of the history is deleted when I change
> browser.history_expire_days from 600 days to 1 day. I changed it 24 hours ago
> and al my history is still there.
> 

filed bug 433893 for this
Fixing this bug requires a full and complete truncation of history to match the prefs at shutdown, overriding the partial expiration mechanisms we have in place now.
Summary: [places] on shutdown, expire history entries based on the browser.history_expire_days → on shutdown, expire history entries based on the browser.history_expire_days
Target Milestone: Firefox 3 → ---
Since we're printing this as a known issue for Firefox 3, we should look at fixing for 3.1.
Flags: wanted-firefox3.1?
Flags: blocking-firefox3.1?
Approving blocking request, this should be fixed. as well as at shutdown, the user expects history to be pruned after changing the setting.

We could do this w/o blocking UI by moving the expiration queries into the background.
Flags: wanted-firefox3.1?
Flags: blocking-firefox3.1?
Flags: blocking-firefox3.1+
Priority: P4 → P2
Target Milestone: --- → Firefox 3.1
Aaaannnnd removing the blocking flag: no major problems reported since the 3.0 release, so this doesn't need to block 3.1. We're making more progress on moving non-browsing-synchronous tasks to the background, so fixing this bug is becoming feasible.
Flags: blocking-firefox3.1+ → blocking-firefox3.1-
Target Milestone: Firefox 3.1 → Firefox 3.2a1
Duplicate of this bug: 476868
Target Milestone: Firefox 3.6a1 → ---
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
Assignee: dietrich → nobody

Updated

8 years ago
Depends on: 520165

Comment 17

6 years ago
Does https://bugzilla.mozilla.org/show_bug.cgi?id=520165 make this wont fix?
(In reply to comment #17)
> Does https://bugzilla.mozilla.org/show_bug.cgi?id=520165 make this wont fix?

yes
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.