Closed Bug 744961 Opened 8 years ago Closed 7 years ago

Add some form of history expiration

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 19
Tracking Status
fennec 19+ ---

People

(Reporter: mfinkle, Assigned: wesj)

References

Details

Attachments

(5 files, 5 obsolete files)

We removed the outright truncation of history, but we should still consider "expiring" history so it doesn't grow unbounded.

We could look at what desktop does for expiration. I don't think we should try to expire items on each DB access. We should wait for other triggers, like shutdown or going into the background.
There's code like this which will quickly get unpleasant now that we don't expire history:

https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GlobalHistory.java#84
tracking-fennec: --- → ?
Current Nightly on my Galaxy Nexus is taking up 209MB, while the app itself is taking 19,24MB. I do a lot of fuzz testing on that device with the Nightly.
In comparison, Firefox Beta takes up 30,08MB and the app itself 18,10MB.

I also noticed that it can take 5 seconds or so before about:home content is populated or the awesomebar.
I wouldn't suggest that this should block release, but I'd like to see it fixed sooner rather than later, even during beta.
Is that 200M really browser.db? Are you sure you aren't seeing things like: bug 754575 ?
I'll take this.
Assignee: nobody → mbrubeck
Group: mozilla-corporation-confidential
Group: mozilla-corporation-confidential
Interim hack for those who want to get rid of old history on their device, and are using Sync:

* Open about:config
* Open a privileged Web Console (Cmd-Opt-K on Mac)
* Paste the following:

--
      Components.utils.import("resource://gre/modules/PlacesUtils.jsm");
      let days = 400;                                       // How many days to keep.
      let end = new Date(Date.now() - (days * 86400000));
      end.setHours(0);
      end.setMinutes(0);
      end.setSeconds(0);
      PlacesUtils.history.removeVisitsByTimeframe(0, (end.getTime() * 1000));
--

This will beachball for a while, then you'll have a big sync on both devices.

Vacuuming after removing 50,000 old records on desktop dropped places.sqlite by about 30MB. You'll see a smaller improvement on mobile unless you Reset Sync > Replace Others from desktop.
(In reply to Gian-Carlo Pascutto (:gcp) from comment #5)
> Is that 200M really browser.db? Are you sure you aren't seeing things like:
> bug 754575 ?

Bug 754575 is now fixed, in today's trunk build, Nightly takes a total of 216MB.
I'll repeat my question if this is really browser.db. Of course if you are injecting random URLs during fuzz testing you can make the URL database arbitrarily large. I don't think that's a relevant use case.

A year of real browsing history is about 70M in Places and (IIRC) less than half that in LocalBrowserDB.
Yes, I'm injecting random URLs during fuzz testing.
(In reply to Gian-Carlo Pascutto (:gcp) from comment #9)
> I'll repeat my question if this is really browser.db. Of course if you are
> injecting random URLs during fuzz testing you can make the URL database
> arbitrarily large. I don't think that's a relevant use case.

FWIW, my regular browsing profile with a year of history causes Beta to occupy 35MB on my device. Between Firefox, TouchDown, and Facebook, I regularly get "Storage Low" warnings.

(As Android devices age, their free memory shrinks, most likely due to all of the in-firmware apps that have been updated via Play. Users with year-old phones will suffer if we regularly consume 30MB+ of memory.)

Note that we *could* listen for this intent and prune accordingly:

http://stackoverflow.com/questions/8117881/when-is-the-intent-action-device-storage-low-broadcasted
Assignee: mbrubeck → gpascutto
Whiteboard: [ARMv6]
Whiteboard: [ARMv6]
Any update on this, gcp?
blassey judged this as very low priority as we('re supposed to) have tools to move the profiles to the SD card again. I don't plan to work on this in the near future.
Thanks for the update! 

blassey, presumably you'll still take a patch for this? I still field user queries about slow awesomebar or lack of space with big synced profiles, so I might be motivated to put something together myself.
Assignee: gpascutto → nobody
See Also: → 785945
yes, we'd take a patch. Though it should be behind a pref and it would be up to product and UX whether it would be default on or not.
Richard - Want to take this one?
tracking-fennec: ? → 19+
(In reply to Mark Finkle (:mfinkle) from comment #16)
> Richard - Want to take this one?

Eventually! :D
Blocks: 760653
Attached patch WIP, no tests. v1 (obsolete) — Splinter Review
WIP.

Logic goes in BrowserProvider, so it gets access to the raw DB object to run an arbitrary delete.

We order by frecency ascending, so we delete least-frecent records first.

Accepts limits for both retention and recency, both of which are set to arbitrary MAGIC_CONSTANTS for now, but can easily be pulled out of the request URL.

Method added to BrowserDB, so this just needs to get called from somewhere.

No tests.

f? to mfinkle to answer or dispatch as he chooses :)
Attachment #663275 - Flags: feedback?(mark.finkle)
Comment on attachment 663275 [details] [diff] [review]
WIP, no tests. v1

Using "old" does not make me think "expired". Maybe use "expired" in places where you are using "old" ?

Overall looks good to me. Any thoughts on when to perform expiration?

Adding Lucas to feedback (and reviews) too, to keep him in the loop.
Attachment #663275 - Flags: feedback?(mark.finkle)
Attachment #663275 - Flags: feedback?(lucasr.at.mozilla)
Attachment #663275 - Flags: feedback+
Comment on attachment 663275 [details] [diff] [review]
WIP, no tests. v1

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

Nice. Where would you be calling expireHistory from?

::: mobile/android/base/db/BrowserProvider.java.in
@@ +1336,5 @@
> +        final long now = System.currentTimeMillis();
> +        debug("Expiring at most " + toRemove + " rows earlier than " + keepAfter + ".");
> +
> +        final String age = "(" + Combined.DATE_LAST_VISITED + " - " + now + ") / 86400000";
> +        final String sortOrder = Combined.VISITS + " * MAX(1, 100 * 225 / (" + age + "*" + age + " + 225)) ASC";

Maybe this sortOrder should be shared through a constant between the content provider and LocalBrowserDB?

@@ +1343,5 @@
> +        if (keepAfter > 0) {
> +            sql = "DELETE FROM " + TABLE_HISTORY + " " +
> +                  "WHERE MAX(" + History.DATE_LAST_VISITED + ", " + History.DATE_MODIFIED +
> +                             ") < " + keepAfter +
> +                  "ORDER BY (" + sortOrder + ") LIMIT " + toRemove;

Oh, sqlite's DELETE supports LIMIT? Nice. I assume it's supported on all Android versions Fennec supports.

@@ +1455,5 @@
>          return deleted;
>      }
>  
>      @SuppressWarnings("fallthrough")
> +    public int deleteInTransaction(SQLiteDatabase db, Uri uri, String selection, String[] selectionArgs) {

I'd probably change the deleteInTransaction() arguments in a separate patch for clarity. No big deal though.
Attachment #663275 - Flags: feedback?(lucasr.at.mozilla) → feedback+
(In reply to Mark Finkle (:mfinkle) from comment #19)

> Using "old" does not make me think "expired". Maybe use "expired" in places
> where you are using "old" ?

My thinking: the URI "history/old" identifies 'old' records, ordered by irrelevance. You expire them when you ask to delete at that URI -- that is, "old records" is the noun, and "expire" is the verb. They become "expired records" when we delete them. In other words:

  db.expireHistory() == DELETE FROM history/old.

Clarifying observation: we don't delete the entire contents of the set when we expire; there's a count limit and a time limit. "Expiration candidates" would be accurate, but is kinda long :D

Not a big deal if you still disagree.

> Overall looks good to me. Any thoughts on when to perform expiration?

On low memory (aggressive), if queries start taking some multiple of a 'good' time for that device (aggressive), and perhaps periodically (mild: delete fewer records; just attempting to avoid the two previous situations!).
 
> Adding Lucas to feedback (and reviews) too, to keep him in the loop.

Excellent.

I won't have much time to work on this for the next week or so, so if someone else wants to pick this up, feel free.
Let's have a try build that will actually run tests.

https://tbpl.mozilla.org/?tree=Try&rev=fcbf98f585d6
I'm gonna pick this up if you don't mind.
(In reply to Wesley Johnston (:wesj) from comment #23)
> I'm gonna pick this up if you don't mind.

Go for it!
Attached patch Patch 1/2 - Expire methods (obsolete) — Splinter Review
This is basically the same patch with the ability to request a "priority" for the expiration (I hate that term, but can't find anything better). Normal is items that are more than 4 weeks old, up to leaving  (at least) 2000 remaining. Aggressive is items that are more than 2 weeks old, leaving (at least) 500 entries. I like "/old" better than expiring too, so I left that bit around.

Android's sqlite doesn't support SORT or LIMIT, so I had to rewrite the query. It also seemed to have problems with the "WHERE MAX(date,modified) < something" line if I didn't dynamically bind the something bit (I assume some incorrect conversion?).
Attachment #663275 - Attachment is obsolete: true
Attachment #671597 - Flags: review?(lucasr.at.mozilla)
Attached patch Patch 2/2 - use it (obsolete) — Splinter Review
I'm proposing we be kinda aggressive here. This expires a lot on memory pressure, and a little every time we're put in the background. I'm a bit worried about that, but it should happen often enough that we're never expiring much. Happy to hear opinions there.

Worked on some tests, but Robocop seems impossible to run locally right now. I'll keep working on them with some tricks/try.
Attachment #671602 - Flags: review?(mark.finkle)
wesj: you may also be interested in my patch on bug 789923. In particular, the code to run the expiration should probably be thrown into a Runnable and run on the background thread. I like your ExpirePriority thing, I might want to do the same for thumbnails, actually.
Comment on attachment 671597 [details] [diff] [review]
Patch 1/2 - Expire methods

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

Looks good but...
- How does this expiration interact with sync? Will we get the deleted history entries re-added to the DB on next sync somehow?
- This code needs tests.

Giving r- because this code involves expiring/deleting user data and we need tests to be confident that we're doing the right thing.

::: mobile/android/base/db/BrowserDB.java
@@ +22,5 @@
>          public static String VISITS = "visits";
>          public static String KEYWORD = "keyword";
>      }
>  
> +    public enum ExpirePriority {

"ExpirationMode" maybe?

::: mobile/android/base/db/BrowserProvider.java.in
@@ +1355,5 @@
> +        final long toRemove = rows - retain;
> +        final long now = System.currentTimeMillis();
> +        debug("Expiring at most " + toRemove + " rows earlier than " + keepAfter + ".");
> +
> +        final String age = "(" + Combined.DATE_LAST_VISITED + " - ?1) / 86400000";

It would be nice if we avoided duplicating this query here. Maybe add static constants to BrowserProvider or even BrowserContract?

::: mobile/android/base/db/LocalBrowserDB.java
@@ +288,5 @@
>      }
>  
> +    public void expireHistory(ContentResolver cr, BrowserDB.ExpirePriority priority) {
> +        Uri url = mHistoryExpireUriWithProfile;
> +        url = url.buildUpon().appendQueryParameter("priority", priority.toString()).build();

This param should be defined in BrowserContract (see how we define PARAM_LIMIT, PARAM_PROFILE, etc)
Attachment #671597 - Flags: review?(lucasr.at.mozilla) → review-
Comment on attachment 671602 [details] [diff] [review]
Patch 2/2 - use it

Seems reasonable
Attachment #671602 - Flags: review?(mark.finkle) → review+
Comment on attachment 671602 [details] [diff] [review]
Patch 2/2 - use it


>diff --git a/mobile/android/base/MemoryMonitor.java b/mobile/android/base/MemoryMonitor.java

>     public void onReceive(Context context, Intent intent) {
>         if (Intent.ACTION_DEVICE_STORAGE_LOW.equals(intent.getAction())) {
>             Log.d(LOGTAG, "Device storage is low");
>             mStoragePressure = true;
>+            BrowserDB.expireHistory(context.getContentResolver(),
>+                                    BrowserDB.ExpirePriority.AGGRESSIVE);

Wait. Shouldn't this be in a background thread too? I don't know if the onReceive is called on the main thread or not. Seems safer to use a background runnable.
Attachment #671602 - Flags: review+ → review-
Heh. Yes. I was waiting for kat's stuff to land and figured I'd move it in the same background runnable he uses. I'll throw up a new patch. Have some tests for the other patch that pass locally. Running them through try now.
Attached patch Tests (obsolete) — Splinter Review
Green on try: https://tbpl.mozilla.org/?tree=Try&rev=1a5c721893d5
Attachment #672950 - Flags: review?(lucasr.at.mozilla)
Attached patch Patch 1/3Splinter Review
Expire methods:
> "ExpirationMode"
Mode implies something different in my head. Maybe we'd be better with TIMEFRAME and something like "RECENT", "OLD" but that seems kinda deceptive too (i.e. I like the word aggressive). I left priority for now.

> It would be nice if we avoided duplicating this query here.
I have a second patch for this. We don't use this query that often according to an mxr search for "86400000", and there are slight differences in those places. I don't really think this is worth it, but I'll let you see that patch.

> This param should be defined in BrowserContract
Done.
Attachment #671597 - Attachment is obsolete: true
Like I said, I don't love this patch, but wanted to look at it.
Attachment #671602 - Attachment is obsolete: true
Attachment #672955 - Flags: review?
Attachment #672956 - Flags: review?(mark.finkle)
Attachment #672954 - Flags: review?(lucasr.at.mozilla)
Attachment #672955 - Flags: review? → review?(lucasr.at.mozilla)
Attachment #672956 - Flags: review?(mark.finkle) → review+
Attachment #672955 - Attachment is obsolete: true
Attachment #672955 - Flags: review?(lucasr.at.mozilla)
Attachment #672988 - Flags: review?(lucasr.at.mozilla)
Attached patch TestsSplinter Review
Attachment #672950 - Attachment is obsolete: true
Attachment #672950 - Flags: review?(lucasr.at.mozilla)
Attachment #672989 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 672954 [details] [diff] [review]
Patch 1/3

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

r+ with the recommended changes.

::: mobile/android/base/db/BrowserDB.java
@@ +22,5 @@
>          public static String VISITS = "visits";
>          public static String KEYWORD = "keyword";
>      }
>  
> +    public enum ExpirePriority {

I still prefer ExpirationMode. Not a big deal though.

This should be moved to BrowserContract given that it's being used in BrowserProvider.

::: mobile/android/base/db/BrowserProvider.java.in
@@ +2138,5 @@
>          if (!values.containsKey(Bookmarks.DATE_CREATED)) {
>              values.put(Bookmarks.DATE_CREATED, now);
>          }
>  
> +        values.put(Bookmarks.DATE_MODIFIED, now);

What is this change about? Doesn't seem related or necessary.
Attachment #672954 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 672988 [details] [diff] [review]
Patch 3/3 - static sort order

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

Looks good.

::: mobile/android/base/db/BrowserContract.java.in
@@ +30,5 @@
>      public static final String PARAM_INSERT_IF_NEEDED = "insert_if_needed";
>      public static final String PARAM_INCREMENT_VISITS = "increment_visits";
>      public static final String PARAM_EXPIRE_PRIORITY = "priority";
>  
> +    static public String getDefaultSort(boolean includesBookmarks, boolean asc) {

Maybe getFrecencySortOrder?

@@ +39,5 @@
> +        if (includesBookmarks) {
> +            order.insert(0, "(CASE WHEN " + Combined.BOOKMARK_ID + " > -1 THEN 100 ELSE 0 END) + ");
> +        }
> +
> +        order.append(asc ? "ASC" : "DESC");

Prepend a space here, just in case? i.e. " ASC" and " DESC".
Attachment #672988 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 672989 [details] [diff] [review]
Tests

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

Nice! Thanks for writing the tests!

::: mobile/android/base/db/BrowserProvider.java.in
@@ +2235,5 @@
>                  selectionArgs, null, null, null);
>  
>          try {
> +            if (!values.containsKey(Bookmarks.DATE_MODIFIED)) {
> +                values.put(Bookmarks.DATE_MODIFIED,  System.currentTimeMillis());

What's this change about?

::: mobile/android/base/tests/testBrowserProvider.java.in
@@ +1616,5 @@
> +                                 new String[] { "http://www.test.org/" + i });
> +            }
> +
> +            Cursor c = mProvider.query(mHistoryUri, null, "", null, null);
> +            mAsserter.is(count, count, count + " history entries found");

I guess you mean c.getCount() somewhere here?

@@ +1627,5 @@
> +            createFakeHistory(0, count);
> +
> +            // expiring with a normal priority should not delete new entries
> +            Uri url = mHistoryOldUri;
> +            url = appendUriParam(url, "PARAM_EXPIRE_PRIORITY", "NORMAL");

Maybe it's simpler:
Uri url = appendUriParam(mHistoryOldUri, "PARAM_EXPIRE_PRIORITY", "NORMAL");

@@ +1634,5 @@
> +            mAsserter.is(c.getCount(), count, count + " history entries found");
> +
> +            // expiring with a aggressive priority should leave 500 entries
> +            url = mHistoryOldUri;
> +            url = appendUriParam(url, "PARAM_EXPIRE_PRIORITY", "AGGRESSIVE");

Same here.

@@ +1645,5 @@
> +            // insert a bunch of entries with an old time created/modified
> +            long time = 1000L * 60L * 60L * 24L * 30L * 3L;
> +            createFakeHistory(time, count);
> +
> +            // expiring with an agressive priority should remove old

I guess you mean "normal" here.

@@ +1648,5 @@
> +
> +            // expiring with an agressive priority should remove old
> +            // entries leaving at least 2000
> +            url = mHistoryOldUri;
> +            url = appendUriParam(url, "PARAM_EXPIRE_PRIORITY", "NORMAL");

Same here.

@@ +1662,5 @@
> +            c = mProvider.query(mHistoryUri, null, "", null, null);
> +            mAsserter.is(c.getCount(), 500, "500 history entries found");
> +
> +            // clear the history
> +            mProvider.delete(mHistoryUri, null, null);

This is not really necessary as we call ensureEmptyDatabase() after each test runnable.
Attachment #672989 - Flags: review?(lucasr.at.mozilla) → review+
Wes, did you read this comment from Lucas:

---
@@ -2068,19 +2134,17 @@ public class BrowserProvider extends Con
     long insertBookmark(Uri uri, ContentValues values) {
         // Generate values if not specified. Don't overwrite
         // if specified by caller.
         long now = System.currentTimeMillis();
         if (!values.containsKey(Bookmarks.DATE_CREATED)) {
             values.put(Bookmarks.DATE_CREATED, now);
         }
 
-        if (!values.containsKey(Bookmarks.DATE_MODIFIED)) {
-            values.put(Bookmarks.DATE_MODIFIED, now);
-        }
+        values.put(Bookmarks.DATE_MODIFIED, now);

What is this change about? Doesn't seem related or necessary.
---


and accidentally commit just the removal?

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ab4bbf4b815#l3.219

---
   3.210 @@ -2068,20 +2134,16 @@ public class BrowserProvider extends Con
   3.211      long insertBookmark(Uri uri, ContentValues values) {
   3.212          // Generate values if not specified. Don't overwrite
   3.213          // if specified by caller.
   3.214          long now = System.currentTimeMillis();
   3.215          if (!values.containsKey(Bookmarks.DATE_CREATED)) {
   3.216              values.put(Bookmarks.DATE_CREATED, now);
   3.217          }
   3.218  
   3.219 -        if (!values.containsKey(Bookmarks.DATE_MODIFIED)) {
   3.220 -            values.put(Bookmarks.DATE_MODIFIED, now);
   3.221 -        }
   3.222 -
   3.223          if (!values.containsKey(Bookmarks.GUID)) {
   3.224              values.put(Bookmarks.GUID, Utils.generateGuid());
   3.225          }
---

Please fix. (I don't think it's urgent enough for me to back it out, but don't let it get to Aurora!)

Also, your "add tests" commit touches non-test code, which is pretty confusing!

(Setting assignee etc. so the inbound sheriff doesn't have to.)
Assignee: nobody → wjohnston
Status: NEW → ASSIGNED
Flags: needinfo?(wjohnston)
Drive-by review of https://hg.mozilla.org/integration/mozilla-inbound/rev/55b904ef7211 :

There's something smelly about a singleton holding a reference to a context, based solely on who got to init it first.

Furthermore, this worries me:

$ fgrep -r "MemoryMonitor.getInstance" *
GeckoActivity.java.in:        MemoryMonitor.getInstance().onLowMemory();
GeckoActivity.java.in:        MemoryMonitor.getInstance().onTrimMemory(level);
GeckoApplication.java:        MemoryMonitor.getInstance().init(getApplicationContext());


Note that only GeckoApplication inits the MemoryMonitor, and we now have a precedent for other classes inheriting behavior that touches the MemoryMonitor singleton (potentially before it's been initialized). That's a recipe for accidentally introducing NPEs. Let's not do that.

I think a much better solution is to pass the context through from the intent handler, scoping it to the Runnable. Untested patch (it compiles!) attached.
Attachment #673518 - Flags: review?(wjohnston)
Attachment #673518 - Flags: feedback?(mark.finkle)
(In reply to Richard Newman [:rnewman] from comment #42)
> Wes, did you read this comment from Lucas:
> and accidentally commit just the removal?

Whoops. Yep. Just missed that I did this. I'll push a fix in the morning.

> Also, your "add tests" commit touches non-test code, which is pretty confusing!

We need that change for the tests to pass, and I'm not a fan of micro commits. To me that's less confusing.
Flags: needinfo?(wjohnston)
(In reply to Wesley Johnston (:wesj) from comment #44)

> Whoops. Yep. Just missed that I did this. I'll push a fix in the morning.

Thanks!

> > Also, your "add tests" commit touches non-test code, which is pretty confusing!
> 
> We need that change for the tests to pass, and I'm not a fan of micro
> commits. To me that's less confusing.

Yeah, I'd have rolled all four up into one :D

I keep my Git repos 'micro', and hg 'macro'…
Attachment #673518 - Flags: review?(wjohnston) → review+
Attachment #673518 - Flags: feedback?(mark.finkle) → feedback+
Depends on: 811144
You need to log in before you can comment on or make changes to this bug.