currently, the only way to clear saved form data is by completely clearing it, unlike saved history items, which expire after a user-defined number of days. an expiration time period should be included in the prefs ui (similar to the "remember visited pages for the last ___ days" dialog for history) and saved form data should be removed after that period of time. the "age" of saved form data should be based on the last use by the user, just like saved history items. naturally, the default option should be to have no expiration time limit for saved form data, just like it is currently. this option would be particularly useful for users who may repeatedly use the same form while entering different values on each use, which can result in excessively long or slow-rendering autocomplete lists.

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

I've heard from one other person and believe I have also benefited browser startup time by clearing form history data. We should evaluate this along with other data storage accumulation issues that may impact Firefox performance after lots of use.

It might be less of a problem now that form history is sqlite rather than mork.

This probably isn't a perf issue any more, but the privacy aspect remains. We should probably just use browser.history_expire_days (180 days, currently). It's sufficiently close in purpose, and overlaps in some cases (forms with method=GET). Storing "last used" date on history entries would help for expiring old fields, as well as enabling some new uses in the future (eg, show most-recently-used in the autocomplete dropdown, and awesomebar-like frecency weightings).

But if you implement this, please don't make this silently or subsume it under the normal history timeout. I for one don't want my form history to expire, ever.

Proof of concept. Untested, but it compiles. [So maybe not such a great POC. :)] Not yet hooked up to a daily expire timer. I was concerned about performance of the first time a big chunk of form data expires (eg, when the thousands of entries not modified since they were initially timestamped suddenly become X days old), but I don't think it's a big deal... My daily-use profile has 26,000 form history entries, 23,000 of which are older than 120 days. Expiring them in a single statement took about 0.3 seconds.

This is done, except for one piece. :( I found that the VACUUM wasn't working; it fails because the DB is busy. That ends up being the fault of the stupid StartCache() stuff. I've talked with sdwilsh about this before, we both think it should just be removed. I verified that calling StopCache() in ExpireOldEntries() makes it work. So I'm going to nuke the cache stuff in a separate bug. Then this bug can be modified to (1) actually test the VACUUM worked and (2) add a "DROP TABLE moz_dummy_table" to the v2 upgrade code (this is the empty table the cache used). But it's good enough for a review now. :)

Comment on attachment 372901 [details] [diff] [review] Patch v.2 > nsresult >+nsFormHistory::ExpireOldEntries() >+{ >+ nsresult rv; >+ PRInt64 expireTime; >+ PRInt32 expireDays, beginningCount, endingCount; Normally we don't do C-style declaration of variables but, do so at the first use of the variable. Any reason you did it this way here? >+ nsresult rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING( >+ "SELECT COUNT(*) FROM moz_formhistory"), >+ getter_AddRefs(stmt)); nit: you have some strange indenting in general with your new SQL statements >+ PRBool hasMore; nit: really "hasResult" here >+ PRInt32 count = 0; >+ if (hasMore) { >+ stmt->GetInt32(0, &count); just use |count = stmt->AsInt32(0);| r=sdwilsh, but I didn't look terribly closely at the unit tests.

Nits fixed. This patch is now on top of bug 488571, so it adds a DROP TABLE to the v2 upgrade code and tests to make sure the vacuum worked (by checking the file size).

Also verified, manually, that older builds are able to deal with a missing moz_dummy_table. The code was designed to detect that and create it when needed, and it's working as expected. (bug 488571 removes it, since it's no longer needed)

Comment on attachment 372988 [details] [diff] [review] Patch v.3 >diff --git a/toolkit/components/satchel/src/nsStorageFormHistory.cpp b/toolkit/components/satchel/src/nsStorageFormHistory.cpp >+nsFormHistory::ExpireOldEntries() >+ nsresult rv; >+ nsCOMPtr<nsIPrefService> prefService = do_GetService(NS_PREFSERVICE_CONTRACTID, &rv); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ nsCOMPtr<nsIPrefBranch> prefBranch; >+ rv = prefService->GetBranch(nsnull, getter_AddRefs(prefBranch)); >+ NS_ENSURE_SUCCESS(rv, rv); You can skip the call to GetBranch and just do_GetService(NS_PREFSERVICE_CONTRACTID) into a nsCOMPtr<nsIPrefBranch> directly.

I wouldn't mind it if you added an explicit check for browser.history_expire_days = -1 (or <0), and just not doing expiration in that case, though.

Fixed nits. After IRC discussion, decided to add support for a browser.formfill.expire_days pref that can be used to control expiration independently of the Places pref that's normally used (browser.history_expire_days).

Pushed http://hg.mozilla.org/mozilla-central/rev/5eb5a07df2e0

Backed out because of this: TEST-UNEXPECTED-FAIL | e:\builds\moz2_slave\mozilla-central-win32-unittest\build\objdir\_tests\xpcshell\test_satchel\unit\test_expire.js | false == true Mac and Linux passed.

(In reply to comment #13) > Created an attachment (id=373264) [details] > Patch v.4 > > Fixed nits. After IRC discussion, decided to add support for a > browser.formfill.expire_days pref that can be used to control expiration > independently of the Places pref that's normally used > (browser.history_expire_days). Stupid question from a non-coder: is 'browser.formfill.expire_days' a 'hidden pref' ? I'm using an hourly that has the patch prior to being backed out, and I don't see that pref in about:config

Comment on attachment 373264 [details] [diff] [review] Patch v.4 Please renominate when it clears tests :)

(In reply to comment #15) > Backed out because of this: > ... Oops. :-( Well, the fix is simple. The test's nsIFile that points to the DB was caching the filesize, so the test thought the file wasn't shrinking after the VACUUM. Adding "dbFile = dbFile.clone()" to the test makes it pass on Windows. Will reland when the tree's green. (In reply to comment #16) > Stupid question from a non-coder: is 'browser.formfill.expire_days' a 'hidden > pref' ? I'm using an hourly that has the patch prior to being backed out, and > I don't see that pref in about:config Yes. There's no default value for the pref, so you won't see it in about:config until you create it.

Pushed http://hg.mozilla.org/mozilla-central/rev/7694718fd556

Comment on attachment 373264 [details] [diff] [review] Patch v.4 a191=beltzner

Pushed to 191: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/26fb6a3994e4

How will this affect extensions that use nsIFormHistory2 to store (some) of their data? Hopefully this will not lead to mysterious data-loss problems.

(In reply to comment #22) > How will this affect extensions that use nsIFormHistory2 to store (some) of > their data? Hopefully this will not lead to mysterious data-loss problems. The API never guaranteed that it'd be permanently stored, so it seems to me like they were abusing the API. There are lots of other safe ways to permanently store data from an add-on that they should be using.