744961 - Add some form of history expiration

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 1

[Richard Newman [:rnewman]]

https://developer.mozilla.org/En/Places/History_Service_Design#Expiration
https://developer.mozilla.org/En/Places_Expiration

Comment 2

[Gian-Carlo Pascutto [:gcp]]

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

Comment 3

[Martijn Wargers (dead)]

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.

Comment 4

[Richard Newman [:rnewman]]

I wouldn't suggest that this should block release, but I'd like to see it fixed sooner rather than later, even during beta.

Comment 5

[Gian-Carlo Pascutto [:gcp]]

Is that 200M really browser.db? Are you sure you aren't seeing things like: bug 754575 ?

Comment 6

[Matt Brubeck (:mbrubeck)]

I'll take this.

Comment 7

[Richard Newman [:rnewman]]

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.

Comment 8

[Martijn Wargers (dead)]

(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.

Comment 9

[Gian-Carlo Pascutto [:gcp]]

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.

Comment 10

[Martijn Wargers (dead)]

Yes, I'm injecting random URLs during fuzz testing.

Comment 11

[Richard Newman [:rnewman]]

(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

Comment 12

[Richard Newman [:rnewman]]

Any update on this, gcp?

Comment 13

[Gian-Carlo Pascutto [: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.

Comment 14

[Richard Newman [:rnewman]]

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.

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

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.

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

Richard - Want to take this one?

Comment 17

[Richard Newman [:rnewman]]

(In reply to Mark Finkle (:mfinkle) from comment #16)
> Richard - Want to take this one?

Eventually! :D

Comment 18

[Richard Newman [:rnewman]]

Attachment: WIP, no tests. v1

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 :)

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 20

[Lucas Rocha (:lucasr)]

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.

Comment 21

[Richard Newman [:rnewman]]

(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.

Comment 22

[Richard Newman [:rnewman]]

Let's have a try build that will actually run tests.

https://tbpl.mozilla.org/?tree=Try&rev=fcbf98f585d6

Comment 23

[Wesley Johnston (:wesj)]

I'm gonna pick this up if you don't mind.

Comment 24

[Richard Newman [:rnewman]]

(In reply to Wesley Johnston (:wesj) from comment #23)
> I'm gonna pick this up if you don't mind.

Go for it!

Comment 25

[Wesley Johnston (:wesj)]

Attachment: Patch 1/2 - Expire methods

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?).

Comment 26

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2 - use it

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.

Comment 27

[Kartikaya Gupta (email:[e-mail])]

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 28

[Lucas Rocha (:lucasr)]

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)

Comment 29

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 671602 [details] [diff] [review]
Patch 2/2 - use it

Seems reasonable

Comment 30

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 31

[Wesley Johnston (:wesj)]

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.

Comment 32

[Wesley Johnston (:wesj)]

Attachment: Tests

Green on try: https://tbpl.mozilla.org/?tree=Try&rev=1a5c721893d5

Comment 33

[Wesley Johnston (:wesj)]

Attachment: Patch 1/3

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.

Comment 34

[Wesley Johnston (:wesj)]

Attachment: Patch 3/3 - Use a static method for sort order

Like I said, I don't love this patch, but wanted to look at it.

Comment 35

[Wesley Johnston (:wesj)]

Attachment: Patch 2/3 - Use the expiration methods

Comment 36

[Wesley Johnston (:wesj)]

Attachment: Patch 3/3 - static sort order

Comment 37

[Wesley Johnston (:wesj)]

Attachment: Tests

Comment 38

[Lucas Rocha (:lucasr)]

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.

Comment 39

[Lucas Rocha (:lucasr)]

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".

Comment 40

[Lucas Rocha (:lucasr)]

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.

Comment 41

[Wesley Johnston (:wesj)]

Landed these with the wrong bug number originally. Backed out and relanded:

https://hg.mozilla.org/integration/mozilla-inbound/rev/55b904ef7211
https://hg.mozilla.org/integration/mozilla-inbound/rev/65d560d91cb0
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d9a4a7b8818
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ab4bbf4b815

Comment 42

[Richard Newman [:rnewman]]

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.)

Comment 43

[Richard Newman [:rnewman]]

Attachment: Refinement for context handling. v1

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.

Comment 44

[Wesley Johnston (:wesj)]

(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.

Comment 45

[Richard Newman [:rnewman]]

(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'…

Comment 46

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a79caf5f26b3

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8ab4bbf4b815
https://hg.mozilla.org/mozilla-central/rev/5d9a4a7b8818
https://hg.mozilla.org/mozilla-central/rev/65d560d91cb0
https://hg.mozilla.org/mozilla-central/rev/55b904ef7211

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a79caf5f26b3

Comment 49

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d7e67c8d00e3

Comment 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d7e67c8d00e3