saved form data should expire after a time period defined by user

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Toolkit
Form Manager
--
enhancement
RESOLVED FIXED
13 years ago
3 years ago

People

(Reporter: ratman, Assigned: Dolske)

Tracking

({fixed1.9.1, perf, privacy})

Trunk
mozilla1.9.2a1
fixed1.9.1, perf, privacy
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

13 years ago
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.
Assignee: dveditz → nobody
*** Bug 309899 has been marked as a duplicate of this bug. ***

Updated

10 years ago
Keywords: perf, privacy

Updated

10 years ago
OS: Windows XP → All
Hardware: PC → All

Comment 2

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

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

Updated

9 years ago
Component: Form Manager → Form Manager
Product: Core → Toolkit
QA Contact: form.manager
(Assignee)

Comment 4

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

Updated

9 years ago
Depends on: 463154

Comment 5

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

Updated

8 years ago
Depends on: 483096
(Assignee)

Comment 6

8 years ago
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.
Assignee: nobody → dolske
(Assignee)

Comment 7

8 years ago
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. :)
Attachment #367702 - Attachment is obsolete: true
Attachment #372901 - Flags: review?(sdwilsh)

Updated

8 years ago
Attachment #372901 - Flags: review?(sdwilsh) → review+
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.
(Assignee)

Updated

8 years ago
Depends on: 488571
(Assignee)

Comment 9

8 years ago
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).
Attachment #372901 - Attachment is obsolete: true
Attachment #372988 - Flags: review?(gavin.sharp)
(Assignee)

Comment 10

8 years ago
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)
Attachment #372988 - Flags: review?(gavin.sharp) → review+
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.
(Assignee)

Comment 13

8 years ago
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).
Attachment #372988 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #373264 - Flags: approval1.9.1?
(Assignee)

Updated

8 years ago
Target Milestone: --- → mozilla1.9.2a1
(Assignee)

Comment 14

8 years ago
Pushed http://hg.mozilla.org/mozilla-central/rev/5eb5a07df2e0
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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
Attachment #373264 - Flags: approval1.9.1?
Comment on attachment 373264 [details] [diff] [review]
Patch v.4

Please renominate when it clears tests :)
(Assignee)

Comment 18

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

Comment 19

8 years ago
Pushed http://hg.mozilla.org/mozilla-central/rev/7694718fd556
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
(Assignee)

Updated

8 years ago
Attachment #373264 - Flags: approval1.9.1?
Comment on attachment 373264 [details] [diff] [review]
Patch v.4

a191=beltzner
Attachment #373264 - Flags: approval1.9.1? → approval1.9.1+
(Assignee)

Comment 21

8 years ago
Pushed to 191: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/26fb6a3994e4
Keywords: fixed1.9.1

Comment 22

8 years ago
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.
Blocks: 518509
You need to log in before you can comment on or make changes to this bug.