Last Comment Bug 243136 - saved form data should expire after a time period defined by user
: saved form data should expire after a time period defined by user
Status: RESOLVED FIXED
: fixed1.9.1, perf, privacy
Product: Toolkit
Classification: Components
Component: Form Manager (show other bugs)
: Trunk
: All All
: -- enhancement with 3 votes (vote)
: mozilla1.9.2a1
Assigned To: Justin Dolske [:Dolske]
:
Mentors:
: 309899 (view as bug list)
Depends on: 463154 483096 488571
Blocks: 518509
  Show dependency treegraph
 
Reported: 2004-05-09 16:50 PDT by ratman
Modified: 2013-12-27 14:20 PST (History)
15 users (show)
dolske: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v.1 (WIP) (6.61 KB, patch)
2009-03-16 19:03 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Review
Patch v.2 (35.01 KB, patch)
2009-04-15 10:32 PDT, Justin Dolske [:Dolske]
sdwilsh: review+
Details | Diff | Review
Patch v.3 (35.32 KB, patch)
2009-04-15 15:13 PDT, Justin Dolske [:Dolske]
gavin.sharp: review+
Details | Diff | Review
Patch v.4 (35.81 KB, patch)
2009-04-16 21:33 PDT, Justin Dolske [:Dolske]
mbeltzner: approval1.9.1+
Details | Diff | Review

Description ratman 2004-05-09 16:50:01 PDT
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.
Comment 1 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-10-22 21:31:06 PDT
*** Bug 309899 has been marked as a duplicate of this bug. ***
Comment 2 Asa Dotzler [:asa] 2007-05-22 19:06:34 PDT
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. 
Comment 3 Jesse Ruderman 2007-05-22 19:38:38 PDT
It might be less of a problem now that form history is sqlite rather than mork.
Comment 4 Justin Dolske [:Dolske] 2008-10-06 15:24:13 PDT
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).
Comment 5 Peter Weilbacher 2008-11-16 05:00:42 PST
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.
Comment 6 Justin Dolske [:Dolske] 2009-03-16 19:03:48 PDT
Created attachment 367702 [details] [diff] [review]
Patch v.1 (WIP)

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.
Comment 7 Justin Dolske [:Dolske] 2009-04-15 10:32:02 PDT
Created attachment 372901 [details] [diff] [review]
Patch v.2

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 8 Shawn Wilsher :sdwilsh 2009-04-15 14:23:03 PDT
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.
Comment 9 Justin Dolske [:Dolske] 2009-04-15 15:13:07 PDT
Created attachment 372988 [details] [diff] [review]
Patch v.3

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).
Comment 10 Justin Dolske [:Dolske] 2009-04-15 15:42:53 PDT
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 11 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-04-16 16:03:00 PDT
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.
Comment 12 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-04-16 20:09:59 PDT
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.
Comment 13 Justin Dolske [:Dolske] 2009-04-16 21:33:00 PDT
Created attachment 373264 [details] [diff] [review]
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).
Comment 14 Justin Dolske [:Dolske] 2009-04-17 00:11:40 PDT
Pushed http://hg.mozilla.org/mozilla-central/rev/5eb5a07df2e0
Comment 15 Dão Gottwald [:dao] 2009-04-17 03:21:25 PDT
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.
Comment 16 Jim Jeffery not reading bug-mail 1/2/11 2009-04-17 06:57:13 PDT
(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 17 Mike Beltzner [:beltzner, not reading bugmail] 2009-04-17 11:32:21 PDT
Comment on attachment 373264 [details] [diff] [review]
Patch v.4

Please renominate when it clears tests :)
Comment 18 Justin Dolske [:Dolske] 2009-04-17 14:31:15 PDT
(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.
Comment 19 Justin Dolske [:Dolske] 2009-04-17 14:37:09 PDT
Pushed http://hg.mozilla.org/mozilla-central/rev/7694718fd556
Comment 20 Mike Beltzner [:beltzner, not reading bugmail] 2009-04-17 15:17:03 PDT
Comment on attachment 373264 [details] [diff] [review]
Patch v.4

a191=beltzner
Comment 21 Justin Dolske [:Dolske] 2009-04-17 15:32:42 PDT
Pushed to 191: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/26fb6a3994e4
Comment 22 Philip Chee 2009-05-13 23:00:07 PDT
How will this affect extensions that use nsIFormHistory2 to store (some) of their data? Hopefully this will not lead to mysterious data-loss problems.
Comment 23 Shawn Wilsher :sdwilsh 2009-05-14 09:56:44 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.