Closed Bug 965452 Opened 6 years ago Closed 6 years ago

Limit HomeProvider addon data storage

Categories

(Firefox for Android :: Awesomescreen, defect, P2)

All
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 32

People

(Reporter: mcomella, Assigned: Margaret)

References

(Blocks 3 open bugs)

Details

Attachments

(1 file)

We may want to do some benchmarks to determine performance characteristics.

Some possibilities via my thoughts and etherpad:
  * (mcomella) User configurable
    * (mfinkle) "user configurable" means "never configured" or "set at default forever". We need to set reasonable limits by default.
  * (mcomella) Prompt the user once an addon exceeds some small threshold X if it's okay for the add-on to continue saving stuff

Note:
  * (margaret) if we want to support a "delete everything, then re-add all your data" model for updating, that would perform better with small data sets
  * (mcomella) We limit the number of history items shown on the home page - perhaps we should have similar limits for custom panels? They were probably selected for a reason
Summary: Limit addon data storage → Limit home panel addon data storage
Summary: Limit home panel addon data storage → Limit HomeProvider addon data storage
Priority: -- → P1
Priority: P1 → P2
Assignee: nobody → margaret.leibovic
I started investigating this, and I found that when I stored 100,000 items in one dataset, I would run into database locked exceptions because it would take too long to query.

So, I think we should limit how many items can be in a given dataset. I think 1000 items seems plenty reasonable, given that the purpose of this is storage is to back a list that the user will scroll through.
Actually, we limit the history list to only 100 items:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/MostRecentPanel.java#155

I tested with 1000 items in a dynamic panel and it was really fast (although that was on an N4), but maybe we should consider making this even smaller.
While this isn't a perfect solution (an add-on could still call save a bunch of times in a row), it at least creates some guidelines around how many items are expected to be stored in a dataset, since I also decided to limit us to showing 100 items at once. I decided to be conservative and go with 100 because that's the number we use to limit items in the history panel.

If we decided to also implement some solution to guarantee there aren't more than N items saved for a given dataset, we'll probably need that number to be higher than 100, since a dataset that uses filters could store more than 100 items, but have them all displayed as different filters are applied.
Attachment #8419092 - Flags: review?(rnewman)
Duplicate of this bug: 965457
I think we should write a second patch to limit the total number of items stored per dataset, but I'm not sure of the best way to do that. Should we just query for the total number of items on ever save call? That seems inefficient.

rnewman, what do you think?
Flags: needinfo?(rnewman)
Comment on attachment 8419092 [details] [diff] [review]
Limit number of items saved at once

Review of attachment 8419092 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/modules/HomeProvider.jsm
@@ +319,5 @@
>     * @return Promise
>     * @resolves When the operation has completed.
>     */
>    save: function(data) {
> +    if (data.length > MAX_SAVE_COUNT) {

data && data.length ...
Attachment #8419092 - Flags: review?(rnewman) → review+
Post-review post-comment: extra points if you add some documentation for add-on authors to consume, explaining the limits and how to work around them if they really need to.

Regarding your question: there are really two actors at play here that care about how much data an add-on is storing -- Fennec itself, and the add-on. That implies that a solution which gives all decision-making power to only one of those things would be flawed.

I think it's fair to follow the memory pressure model, and have either HomeStorage or HomeProvider call back to the add-on with a pressure notification (whether that's a real observer notification, a response code from requestSync or save, or a called callback doesn't matter to me).

An appropriate final fallback would be to delete all of an add-on's data; the notifications are a courtesy, but if you get up to a few thousand items and ignore the notifications, we're going to record some kind of telemetry event and then delete everything out from under you.

As for when in particular we should try to find out how many items there are: my guess is at the end of (or before; doesn't matter) a save call. SELECT COUNT(*) is cheap, particularly compared to an INSERT, and the number will only increase after save has been called.
Flags: needinfo?(rnewman)
I updated the docs here:
https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/API/HomeProvider.jsm/HomeStorage#save%28%29

I'll file a follow-up for the storage notifications (and fallback of deleting items). I think it's fair to make add-ons responsible for keeping their data in check. I also remembered that bug 987238 was filed about creating a telemetry probe to track the amount of data stored.
Blocks: 1007793
https://hg.mozilla.org/mozilla-central/rev/fe4eefbc4cef
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Setting P2 hub bugs to block hub v2 EPIC bug (targeting fx31 release).

Filter on epic-hub-bugs.
Blocks: 1014030
You need to log in before you can comment on or make changes to this bug.