491983 - nsIBrowserHistory needs removeVisitsByTimeframe method

Status: VERIFIED FIXED

(Toolkit :: Places, defect)

Description

[Drew Willcoxon :adw]

removePagesByTimeframe exists, but bug 491883 points out the need for a way to remove visits -- not pages -- by timeframe.  An analogous method removeVisitsByTimeframe should be added.

Comment 1

[Drew Willcoxon :adw]

Attachment: WIP v1

Fly-bies appreciated.  I copied structure and some comments from RemovePagesByTimeframe and RemovePagesInternal.

What's up with having both nsIBrowserHistory and nsINavHistoryService?  Is it just that nsIBrowserHistory is old and nsINavHistoryService is new?  Should this new method go in nsINavHistoryService instead?  Everything in nsIBrowserHistory deals with pages, not visits, while nsINavHistoryService has addVisit.

Comment 2

[Drew Willcoxon :adw]

Since this toolkit bug blocks browser bug 491883, which blocks 3.5, requesting blocking.

Comment 3

[Drew Willcoxon :adw]

Attachment: for review v1

Please see comment 1 also.

Comment 4

[Dietrich Ayala (:dietrich)]

Comment on attachment 377087 [details] [diff] [review]
for review v1

first pass, looks generally good. some comments below, need to figure out if we need to do all this work inside the call.

>+NS_IMETHODIMP
>+nsNavHistory::RemoveVisitsByTimeframe(PRTime aBeginTime, PRTime aEndTime)
>+{
>+  NS_ASSERTION(NS_IsMainThread(), "This can only be called on the main thread");
>+
>+  mozStorageTransaction transaction(mDBConn, PR_FALSE);
>+
>+  // If we're removing all visits from a URI but the URI is bookmarked, we
>+  // shouldn't delete it. 

delete what? this comment is ambiguous

>+
>+  // Now that visits have been removed, run annotation expiration.  This will
>+  // remove all expire-able annotations for URIs we deleted.
>+  (void)mExpire.OnDeleteURI();

does this need to happen immediately? won't it happen next time annotation runs?

>+
>+  // If the entry has no visits, is not bookmarked, and is not a place: URI
>+  // then we can remove it from moz_places.  Note that we do NOT delete
>+  // favicons. Any unreferenced favicons will be deleted next time the browser
>+  // is shut down.
>+  rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
>+      "DELETE FROM moz_places_view "
>+      // visit_count should not be < 0, but be defensive.
>+      "WHERE visit_count <= 0 "
>+        "AND SUBSTR(url, 1, 6) != 'place:' "
>+        "AND NOT EXISTS ("
>+          "SELECT b.id "
>+          "FROM moz_bookmarks b "
>+          "WHERE moz_places_view.id = b.fk AND b.type = 1 "
>+          "LIMIT 1"
>+        ")"));
>+  NS_ENSURE_SUCCESS(rv, rv);

again, does this need to happen here? will the next expiration run take care of it?

>+  // If we have removed all visits to a livemark's child, we need to fix its
>+  // frecency, or it would appear in the url bar autocomplete.
>+  // XXX this might be dog slow, further degrading delete perf.
>+  rv = FixInvalidFrecenciesForExcludedPlaces();
>+  NS_ENSURE_SUCCESS(rv, rv);

ditto.

and finally, it looks as if a lot of this post-delete cleanup is shared with RemovePagesInternal. it should be unified into PostRemovalCleanup() or something like that, instead of duplicating the code.

>+
>+const NOW = Date.now() * 1000;
>+
>+const bmsvc = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
>+              getService(Ci.nsINavBookmarksService);
>+const histsvc = Cc["@mozilla.org/browser/nav-history-service;1"].
>+                getService(Ci.nsINavHistoryService);
>+
>+const dbConn = Cc["@mozilla.org/browser/nav-history-service;1"].
>+               getService(Ci.nsPIPlacesDatabase).
>+               DBConnection;
>+
>+var TEST_URI  = uri("http://example.com/");
>+var PLACE_URI = uri("place:queryType=0&sort=8&maxResults=10");

please put these up top, before they're used.

>+
>+///////////////////////////////////////////////////////////////////////////////
>+
>+/**
>+ * Returns true if the URI exists in moz_places and false otherwise.
>+ *
>+ * @param  aURI
>+ *         the URI of a place
>+ */
>+function existsInMozPlaces(aURI) {
>+  let sql = "SELECT id FROM moz_places_view WHERE url = :url";
>+  let stmt = dbConn.createStatement(sql);
>+  stmt.params.url = aURI.spec;
>+  var exists = stmt.executeStep();
>+  stmt.finalize();
>+
>+  return exists;
>+}

clarity nit: uriExistsInMozPlaces()

>+
>+/**
>+ * Returns the frecency of a URI.
>+ *
>+ * @param  aURI
>+ *         the URI of a place
>+ * @return the frecency of aURI
>+ */
>+function getFrecency(aURI) {
>+  let sql = "SELECT frecency FROM moz_places_view WHERE url = :url";
>+  let stmt = dbConn.createStatement(sql);
>+  stmt.params.url = aURI.spec;
>+  do_check_true(stmt.executeStep());
>+  let frecency = stmt.getInt32(0);
>+  stmt.finalize();
>+
>+  return frecency;
>+}

clarity nit: getFrecencyForURI();

>+
>+/**
>+ * Removes history and bookmarks.
>+ */
>+function cleanSlate() {
>+  histsvc.QueryInterface(Ci.nsIBrowserHistory).removeAllPages();
>+  remove_all_bookmarks();
>+}

clarity nit: name the function in a more descriptive manner

Comment 5

[Drew Willcoxon :adw]

Hmm, I think you might be right.  I don't know (at all) how expiration works.  A search for DELETE FROM moz_places(_view)? brings up nsNavHistoryExpire::EraseHistory and nsNavHistoryExpire::ExpireHistoryParanoid.  It looks like the former is eventually called on idle, but FindVisits, which determines which URIs to delete, doesn't gather URIs that have no visits, only URIs that have old visits...?

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsNavHistoryExpire.cpp#476

nsNavHistoryExpire::ExpireHistoryParanoid on the other hand removes URIs without visits and is eventually called on idle and on quit, so it looks like that does it?

I'm having a tough time following when everything gets deleted and when it doesn't.  In writing this patch I followed the lead of nsNavHistory::RemovePagesByTimeframe and RemovePagesInternal.  If it's the case that URIs and other things are expired every so often, is RemovePagesInternal also doing too much work?

And if URI deletion is done by expiration on idle, how would I go about testing that URIs no longer exist in the DB after I remove all visits?

Comment 6

[Shawn Wilsher :sdwilsh]

We also expire in nsPlacesDBFlush.js:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsPlacesDBFlush.js#419

Comment 7

[Marco Bonardo [:mak]]

for privacy is better to cleanup things user expect to be removed (do you recall how many bugs we had for "i removed this visit but the url is still in my db).

but i think removeVisitsByTimeFrame does not need to do all the work that removePagesByTimeFrame does, they are working on different sets of data.

Comment 8

[Marco Bonardo [:mak]]

PS: please don't rely on shutdown cleanup, we should get rid of it for Tshutdown, not rely on it.

Comment 9

[Drew Willcoxon :adw]

(In reply to comment #4)
> >+  // Now that visits have been removed, run annotation expiration.  This will
> >+  // remove all expire-able annotations for URIs we deleted.
> >+  (void)mExpire.OnDeleteURI();
> 
> does this need to happen immediately? won't it happen next time annotation
> runs?

nsNavHistory::RemovePagesInternal is the only place in the entire tree that calls nsNavHistoryExpire::OnDeleteURI apparently.  Not by any expiration or idle code.

> >+  // If the entry has no visits, is not bookmarked, and is not a place: URI
> >+  // then we can remove it from moz_places.  Note that we do NOT delete
> >+  // favicons. Any unreferenced favicons will be deleted next time the browser
> >+  // is shut down.
> >+  rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> >+      "DELETE FROM moz_places_view "
> >+      // visit_count should not be < 0, but be defensive.
> >+      "WHERE visit_count <= 0 "
> >+        "AND SUBSTR(url, 1, 6) != 'place:' "
> >+        "AND NOT EXISTS ("
> >+          "SELECT b.id "
> >+          "FROM moz_bookmarks b "
> >+          "WHERE moz_places_view.id = b.fk AND b.type = 1 "
> >+          "LIMIT 1"
> >+        ")"));
> >+  NS_ENSURE_SUCCESS(rv, rv);
> 
> again, does this need to happen here? will the next expiration run take care of
> it?

As I say in comment 5, nsNavHistoryExpire::ExpireHistoryParanoid, which is called on idle and on quit, runs a similar query, so it looks like this can go.

> >+  // If we have removed all visits to a livemark's child, we need to fix its
> >+  // frecency, or it would appear in the url bar autocomplete.
> >+  // XXX this might be dog slow, further degrading delete perf.
> >+  rv = FixInvalidFrecenciesForExcludedPlaces();
> >+  NS_ENSURE_SUCCESS(rv, rv);
> 
> ditto.

No, aside from RemovePagesInternal this is called only by nsNavHistoryExpire::ClearHistory, which is called by nsNavHistory::RemoveAllPages.

(In reply to comment #7)
> for privacy is better to cleanup things user expect to be removed (do you
> recall how many bugs we had for "i removed this visit but the url is still in
> my db).

Is this an argument against deleting things on idle instead of immediately?

> but i think removeVisitsByTimeFrame does not need to do all the work that
> removePagesByTimeFrame does, they are working on different sets of data.

That's only half true isn't it?  removeVisitsByTimeFrame does work on visits and not URIs directly like removePagesByTimeFrame does, but if all visits are removed, then the URI must go as well, and then you've got to do everything that removePagesByTimeFrame does.

Comment 10

[Marco Bonardo [:mak]]

(In reply to comment #9)
> (In reply to comment #7)
> > for privacy is better to cleanup things user expect to be removed (do you
> > recall how many bugs we had for "i removed this visit but the url is still in my db).
> 
> Is this an argument against deleting things on idle instead of immediately?

it is, removing pages from history is an high risk privacy task, many many times in the past i've seen exactly that report "i removed history but url is still in moz_places". But i'm pretty sure nobody will agree with me :)

> > but i think removeVisitsByTimeFrame does not need to do all the work that
> > removePagesByTimeFrame does, they are working on different sets of data.
> 
> That's only half true isn't it?  removeVisitsByTimeFrame does work on visits
> and not URIs directly like removePagesByTimeFrame does, but if all visits are
> removed, then the URI must go as well, and then you've got to do everything
> that removePagesByTimeFrame does.

Of course, it should work like that only if all visits for a page are removed. I think removePagesByTimeframe is a simplified version of this.

Comment 11

[Drew Willcoxon :adw]

(In reply to comment #4)
> and finally, it looks as if a lot of this post-delete cleanup is shared with
> RemovePagesInternal. it should be unified into PostRemovalCleanup() or
> something like that, instead of duplicating the code.

Practically all I can unify are the calls to FixInvalidFrecenciesForExcludedPlaces and transaction.Commit, since RemovePagesInternal acts on a list of URIs, not visits.  However, I've started rewriting RemoveVisitsByTimeframe to select all URIs that don't have visits outside the timeframe and then hand them off to RemovePagesInternal.  Of course this adds one more SQL query to the path.

Comment 12

[Drew Willcoxon :adw]

Attachment: for review v2

Does what comment 11 says: selects places whose visits fall entirely within the timespan and gives them to RemovePlacesInternal.  Let it deal with it.  I had to split up RemovePlacesInternal to do that.

Comment 13

[Dietrich Ayala (:dietrich)]

Comment on attachment 377827 [details] [diff] [review]
for review v2

>     /**
>+     * removeVisitsByTimeframe
>+     *
>+     * Removes all visits in a given timeframe.  Limits are included:
>+     * aBeginTime <= timeframe <= aEndTime.  Any place that becomes unvisited
>+     * as a result will also be deleted.
>+     *
>+     * Note that removal is performed in batch, so observers will not be
>+     * notified of individual places that are deleted.
>+     */
>+    void removeVisitsByTimeframe(in long long aBeginTime, in long long aEndTime);

for clarity, should explicitly say that observers will be notified of the batch events.

Comment 14

[Shawn Wilsher :sdwilsh]

Can we please have comments for methods java-doc style?  Pretty please?

Comment 15

[Dietrich Ayala (:dietrich)]

Comment on attachment 377827 [details] [diff] [review]
for review v2

>+// nsNavHistory::PreRemovePagesInternal
>+//
>+//    Resets the frecencies of a list of places that are about to be deleted.
>+//    This is an internal method used by RemovePagesInternal and
>+//    RemoveVisitsByTimeframe.  Takes a comma separated list of place IDs.
>+//    This list should not be empty!

please check the arg length, like RemovePagesInternal does.

should also note here that it doesn't execute in a transaction, so callers should make sure to do it.

>+// nsNavHistory::PostRemovePagesInternal
>+//
>+//    Performs cleanup on places that were just deleted, including deleting
>+//    any places that no longer have any visits.  This is an internal method
>+//    used by RemovePagesInternal and RemoveVisitsByTimeframe.
>+//    Takes a comma separated list of place IDs.  This list should not be empty!
>+
>+nsresult
>+nsNavHistory::PostRemovePagesInternal(const nsCString& aPlaceIdsQueryString)
>+{

ditto on checking aPlaceIdsQueryString

>+  // force a full refresh calling onEndUpdateBatch (will call Refresh())
>+  UpdateBatchScoper batch(*this); // sends Begin/EndUpdateBatch to observers
>+
>+  mozStorageTransaction transaction(mDBConn, PR_FALSE);
>+
>+  if (!deletePlaceIdsQueryString.IsEmpty())
>+    PreRemovePagesInternal(deletePlaceIdsQueryString);

should check result here

>+
>+  // Delete all visits within the timeframe.
>+  nsCOMPtr<mozIStorageStatement> deleteVisitsStmt;
>+  nsresult rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
>+      "DELETE FROM moz_historyvisits_view "
>+      "WHERE ?1 <= visit_date AND visit_date <= ?2"),
>+    getter_AddRefs(deleteVisitsStmt));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  rv = deleteVisitsStmt->BindInt64Parameter(0, aBeginTime);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  rv = deleteVisitsStmt->BindInt64Parameter(1, aEndTime);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  rv = deleteVisitsStmt->Execute();
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if (!deletePlaceIdsQueryString.IsEmpty())
>+    PostRemovePagesInternal(deletePlaceIdsQueryString);

ditto

hm, also, i think that i'd prefer naming the new methods descriptively, instead of prefixing them with Pre/Post.

Comment 16

[Drew Willcoxon :adw]

Dietrich, do you have an opinion on Shawn's comment 14 about javadoc-style comments?  I agree they're better, but I tried to blend in with the existing styles in these files (IDL and C++).

Comment 17

[Shawn Wilsher :sdwilsh]

bad past decisions should not constrain us on bettering our future code ;)

Comment 18

[Drew Willcoxon :adw]

(In reply to comment #17)
> bad past decisions should not constrain us on bettering our future code ;)

Amen to that, but for features facing end-user developers like comments in IDL, I think consistency is also a virtue.  I don't know how the two are balanced, which is why I asked for Dietrich's thoughts.

Comment 19

[Dietrich Ayala (:dietrich)]

Clarification: I assumed sdwilsh was referring to the non-IDL methods that you added.

I favor the javadoc comments. Anything that makes it easier to automate documentation for the long run is good in my book.

(In reply to comment #17)
> bad past decisions should not constrain us on bettering our future code ;)

The general rule across all of /mozilla is to respect the style of the code you're changing, so Drew's approach makes sense.

However, in this instance, I'm definitely in favor of making the docs of non-IDL methods machine parse-able.

Comment 20

[Drew Willcoxon :adw]

Attachment: for

Comment 21

[Drew Willcoxon :adw]

Attachment: for checkin [pushed]

Addresses comments above.

Comment 22

[Drew Willcoxon :adw]

Attachment: 1.9.1 for review v1

I followed what Shawn did in bug 487511 to add to 1.9.1's API.  I'm not a 100% clear on what I'm doing with all the XPCOM macros, but it looks right, and uh, it compiles and tests pass. :)  I searched for every place nsIBrowserHistory was mentioned and updated for the additions.

Comment 23

[Karl Tomlinson (:karlt)]

Comment on attachment 377894 [details] [diff] [review]
for checkin [pushed]

http://hg.mozilla.org/mozilla-central/rev/5056f3fa03c1

Comment 24

[Shawn Wilsher :sdwilsh]

Comment on attachment 377901 [details] [diff] [review]
1.9.1 for review v1

r=sdwilsh

Comment 25

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/36832738e877

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Marking as verified fixed based on the passed unit tests on trunk and 1.9.1.

Further we have to update the developer docs:
https://developer.mozilla.org/En/NsIBrowserHistory

Comment 27

[Eric Shepherd [:sheppy]]

Question: to write code that uses this and will work both on Gecko 1.9.1 and 1.9.2 (wherein I presume removeVisitsByTimeframe() will migrate into the main nsIBrowserHistory interface), do you need to do the QueryInterface on nsIBrowserHistory_MOZILLA_1_9_1_ADDITIONS then try calling against nsIBrowserHistory if that fails, or is there another preferred technique?

Comment 28

[Shawn Wilsher :sdwilsh]

you'd want to check if "nsIBrowserHistory_MOZILLA_1_9_1_ADDITIONS" in Components.classes, and then QI the variable to it if so.  All code after that point would work just fine.

Comment 29

[Eric Shepherd [:sheppy]]

I feel like an idiot, but I'm not sure exactly what to do there then.  The code I have right now looks like this:

const historyService = Components.classes["@mozilla.org/browser/nav-history-service;1"].getService(Components.interfaces.nsINavHistoryService);

historyService.QueryInterface(Components.interfaces.nsIBrowserHistory_MOZILLA_1_9_1_ADDITIONS).removeVisitsByTimeframe(startTime, endTime);

Not sure what needs to change there, but I know it's wrong. :)

Comment 30

[Dietrich Ayala (:dietrich)]

i think sdwilsh meant Components.interfaces. so something like this:

const Ci = Components.interfaces;

if (Ci["nsIBrowserHistory_MOZILLA_1_9_1_ADDITIONS"]) {
  historyService.QueryInterface(Ci.nsIBrowserHistory_MOZILLA_1_9_1_ADDITIONS);
  historyService.removeVisitsByTimeframe(startTime, endTime);
}

Comment 31

[Eric Shepherd [:sheppy]]

Wouldn't the removeVisitsByTimeframe() need to be outside the conditional there in order to be called in Gecko 1.9.2?

Comment 32

[Dietrich Ayala (:dietrich)]

oops, yes it would!

Comment 33

[Drew Willcoxon :adw]

(In reply to comment #28)
> you'd want to check if "nsIBrowserHistory_MOZILLA_1_9_1_ADDITIONS" in
> Components.classes

Shawn means Components.interfaces here.

Actually due to some special magic Shawn tells me about, you can simply QI to nsIBrowserHistory on both 1.9.1 and 1.9.2, like:

histsvc.QueryInterface(Ci.nsIBrowserHistory).removeVisitsByTimeframe(...);

It's cool!  Try it!  :D

Comment 34

[Shawn Wilsher :sdwilsh]

And really, the best way to do this is simply:

let bh = Components.classes["@mozilla.org/browser/nav-history-service;1"]
                   .getService(Components.interfaces.nsIBrowserHistory);
bh.removeVisitsByTimeframe(...);

Comment 35

[Eric Shepherd [:sheppy]]

In other words, there's no special behavior required at all from the perspective of the client of nsIBrowserHistory, then. :)

Comment 36

[Drew Willcoxon :adw]

(In reply to comment #35)
> In other words, there's no special behavior required at all from the
> perspective of the client of nsIBrowserHistory, then. :)

Yep.

For (my) future reference, here's the magic I referred to:

http://mxr.mozilla.org/mozilla1.9.1/source/toolkit/components/places/src/nsNavHistory.cpp#258

Comment 37

[Eric Shepherd [:sheppy]]

Awesome. That's very cool. I've added this method to https://developer.mozilla.org/En/NsIBrowserHistory (and learned new stuff about XPCOM while I was at it). Thanks for the info!