ContentPrefs is storing float as timestamp and not expiring
Categories
(Toolkit :: Preferences, defect, P3)
Tracking
()
People
(Reporter: mak, Unassigned)
Details
Contentprefs is apparently storing Date.now() / 1000 as timestamp, that ends up storing a uselessly space taking float, while it was supposed to be an integer timestamp. That is likely to take double the space (8 bytes VS 4)
Additionally, I cannot find a point where we use this information to expire old entries.
If I run SELECT date(timestamp, 'unixepoch') FROM prefs ORDER BY 1 ASC I can find stuff from 4 years ago.
Based on the use case for this I suspect limiting to the last 180 days would be sufficient (it may depend on the setting though, so we may want to use an allowList or such, at least for recent download folder it makes sense to expire, for zoom I'm not sure).
Expiration could even just happen on idle-daily.
One concern is potentially privacy, as this is keeping some sort of history of the last 4 years of downloads.
Potentially we could also evaluate a different approach where origins are one-way hashed before being stored, but that'd be for a follow-up.
|
||
Comment 1•2 years ago
|
||
What content prefs do we use this timestamp for? And what's the impact of the bad storage / how hard would it be to fix?
|
Reporter | |
Comment 2•2 years ago
•
|
||
(In reply to :Gijs (he/him) from comment #1)
What content prefs do we use this timestamp for? And what's the impact of the bad storage / how hard would it be to fix?
We use the timestamp to clear these settings for a certain time frame, e.g. clear site settings for the last hour.
We could use it also for expiration, if we implement it.
The impact of the bad storage is mostly wasted space on disk, looking at my old database it contains about 700 entries, wasting 4 bytes per entry means we're wasting about 3KiB. Of course that's a trivial space waste on todays' disks, even more if we implement some kind of expiration.
This is more about code lying as timestamp is not THE timestamp.
It should be easy to fix with a migration that CASTs the column to INTEGER and fixing the code to store an integer + a unit test.
Expiration may be a bit more complicated, though if we decide we only want to expire lastDir, it won't be that bad. The problem is anything can be stored here, so maybe the consumers should be able to declare a setting as "expirable"
|
|
||
Comment 3•2 years ago
|
||
OK. I think this likely doesn't rise to the level where we'd prioritize unless I'm missing something? Happy to be corrected. :-)
Description
•