Bug 402486 - "deleting lots of items from the history sidebar is slow, locks up browser until finished" [p=mak77@supereva.it (Marco Bonardo [mak77]) r=dietrich a1.9b3=schrep]

browser/components/places/content/controller.js

@@ -51,6 +51,17 @@ const RELOAD_ACTION_REMOVE = 2;
 // rows.
 const RELOAD_ACTION_MOVE = 3;
 
+// when removing a bunch of pages we split them in chunks to avoid passing
+// a too big array to RemovePages
+// 300 is the best choice with an history of about 150000 visits
+// smaller chunks could cause a Slow Script warning with a huge history
+const REMOVE_PAGES_CHUNKLEN = 300;
+// if we are removing less than this pages we will remove them one by one
+// since it will be reflected faster on the UI
+// 10 is a good compromise, since allows the user to delete a little amount of
+// urls for privacy reasons, but does not cause heavy disk access
+const REMOVE_PAGES_MAX_SINGLEREMOVES = 10;
+
 /**
  * Represents an insertion point within a container where we can insert
  * items. 
@@ -912,19 +923,44 @@ PlacesController.prototype = {
   },
 
   /**
-   * Removes the set of selected ranges from history. 
+   * Removes the set of selected ranges from history.
    */
   _removeRowsFromHistory: function PC__removeRowsFromHistory() {
     // Other containers are history queries, just delete from history
     // history deletes are not undoable.
     var nodes = this._view.getSelectionNodes();
+    var URIs = [];
+    var bhist = PlacesUtils.history.QueryInterface(Ci.nsIBrowserHistory);
+    var resultView = this._view.getResultView();
     for (var i = 0; i < nodes.length; ++i) {
       var node = nodes[i];
-      var bhist = PlacesUtils.history.QueryInterface(Ci.nsIBrowserHistory);
       if (PlacesUtils.nodeIsHost(node))
         bhist.removePagesFromHost(node.title, true);
-      else if (PlacesUtils.nodeIsURI(node))
+      else if (PlacesUtils.nodeIsURI(node)) {
-        bhist.removePage(PlacesUtils._uri(node.uri));
+        var uri = PlacesUtils._uri(node.uri);
+        // avoid trying to delete the same url twice
+        if (URIs.indexOf(uri) < 0) {
+          URIs.push(uri);
+        }
+      }
+    }
+
+    // if we have to delete a lot of urls RemovePage will be slow, it's better
+    // to delete them in bunch and rebuild the full treeView
+    if (URIs.length > REMOVE_PAGES_MAX_SINGLEREMOVES) {
+      // do removal in chunks to avoid passing a too big array to removePages
+      for (var i = 0; i < URIs.length; i += REMOVE_PAGES_CHUNKLEN) {
+        var URIslice = URIs.slice(i, Math.max(i + REMOVE_PAGES_CHUNKLEN, URIs.length));
+        // set DoBatchNotify only on the last chunk
+        bhist.removePages(URIslice, URIslice.length,
+                          (i + REMOVE_PAGES_CHUNKLEN) >= URIs.length);
+      }
+    }
+    else {
+      // if we have to delete fewer urls, removepage will allow us to avoid
+      // rebuilding the full treeView
+      for (var i = 0; i < URIs.length; ++i)
+        bhist.removePage(URIs[i]);
     }
   },
 

toolkit/components/places/public/nsIBrowserHistory.idl

@@ -42,11 +42,12 @@
 #include "nsISupports.idl"
 #include "nsIGlobalHistory2.idl"
 
-[scriptable, uuid(c43079c3-3d8d-4b7c-af14-0e30ab46865f)]
+[scriptable, uuid(ef807ae6-4ae9-4d1f-8ade-3ee75e24d18c)]
 interface nsIBrowserHistory : nsIGlobalHistory2
 {
-    /** 
+    /**
      * addPageWithDetails
+     *
      * Adds a page to history with specific time stamp information. This is used in
      * the History migrator. 
      */
@@ -54,23 +55,38 @@ interface nsIBrowserHistory : nsIGlobalHistory2
 
     /**
      * lastPageVisited
+     *
      * The last page that was visited in a top-level window.
      */
     readonly attribute AUTF8String lastPageVisited;
 
     /**
      * count
+     *
      * The number of entries in global history
      */
     readonly attribute PRUint32 count;
 
     /**
-     * remove a page from history
+     * removePage
+     *
+     * Remove a page from history
      */
     void removePage(in nsIURI aURI);
 
+    /**
+     * removePages
+     *
+     * Remove a bunch of pages from history
+     * Notice that this does not call observers for performance reasons,
+     * instead setting aDoBatchNotify true will send Begin/EndUpdateBatch
+     */
+    void removePages([array, size_is(aLength)] in nsIURI aURIs,
+                     in unsigned long aLength, in boolean aDoBatchNotify);
+
     /**
      * removePagesFromHost
+     *
      * Remove all pages from the given host.
      * If aEntireDomain is true, will assume aHost is a domain,
      * and remove all pages from the entire domain.
@@ -79,22 +95,24 @@ interface nsIBrowserHistory : nsIGlobalHistory2
 
     /**
      * removeAllPages
+     *
      * Remove all pages from global history
      */
     void removeAllPages();
 
     /**
      * hidePage
+     *
      * Hide the specified URL from being enumerated (and thus
      * displayed in the UI)
-     *
+     * If the page hasn't been visited yet, then it will be added
-     * if the page hasn't been visited yet, then it will be added
      * as if it was visited, and then marked as hidden
      */
     void hidePage(in nsIURI aURI);
 
     /**
      * markPageAsTyped
+     *
      * Designate the url as having been explicitly typed in by
      * the user, so it's okay to be an autocomplete result.
      */

toolkit/components/places/src/nsNavHistory.cpp

@@ -3089,96 +3089,107 @@ nsNavHistory::GetCount(PRUint32 *aCount)
 }
 
 
+// nsNavHistory::RemovePages
+//
+//    Removes a bunch of uris from history.
+//    Has better performance than RemovePage when deleting a lot of history.
+//    Notice that this function does not call the onDeleteURI observers,
+//    instead, if aDoBatchNotify is true, we call OnBegin/EndUpdateBatch.
+//    We don't do duplicates removal, URIs array should be cleaned-up before.
+
+NS_IMETHODIMP
+nsNavHistory::RemovePages(nsIURI **aURIs, PRUint32 aLength, PRBool aDoBatchNotify)
+{
+  // build a list of place ids to delete
+  nsCString deletePlaceIdsQueryString;
+  nsresult rv;
+  for (PRInt32 i = 0; i < aLength; i++) {
+    PRInt64 placeId;
+    rv = GetUrlIdFor(aURIs[i], &placeId, PR_FALSE);
+    NS_ENSURE_SUCCESS(rv, rv);
+    if (placeId != 0) {
+      if (!deletePlaceIdsQueryString.IsEmpty())
+        deletePlaceIdsQueryString.AppendLiteral(",");
+      deletePlaceIdsQueryString.AppendInt(placeId);
+    }
+  }
+
+  // early return if there is nothing to delete
+  if (deletePlaceIdsQueryString.IsEmpty())
+    return NS_OK;
+
+  mozStorageTransaction transaction(mDBConn, PR_FALSE);
+  nsCOMPtr<mozIStorageStatement> statement;
+
+  // delete all visits
+  rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
+      "DELETE FROM moz_historyvisits WHERE place_id IN (") +
+        deletePlaceIdsQueryString +
+        NS_LITERAL_CSTRING(")"));
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  // now that visits have been removed, run annotation expiration.
+  // this will remove all expire-able annotations for these URIs.
+  (void)mExpire.OnDeleteURI();
+
+  // if there are no more annotations, and the entry is not bookmarked
+  // then we can remove the moz_places entry.
+  // 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 WHERE id IN ("
+        "SELECT h.id FROM moz_places h WHERE h.id IN (") +
+        deletePlaceIdsQueryString +
+        NS_LITERAL_CSTRING(") AND "
+        "NOT EXISTS (SELECT b.id FROM moz_bookmarks b WHERE b.fk = h.id) AND "
+        "NOT EXISTS (SELECT a.id FROM moz_annos a WHERE a.place_id = h.id))"));
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  // if a moz_place was annotated or was a bookmark,
+  // we didn't delete it, but we did delete the moz_visits
+  // so we need to reset the frecency.  Note, we don't
+  // reset the visit_count, as we use that in our "on idle"
+  // query to figure out which places to recalculate frecency first.
+  rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
+      "UPDATE moz_places SET frecency = -1 WHERE id IN(") +
+        deletePlaceIdsQueryString +
+        NS_LITERAL_CSTRING(")"));
+  NS_ENSURE_SUCCESS(rv, rv);
+ 
+  // placeId could have a livemark item, so setting the frecency to -1
+  // would cause it to show up in the url bar autocomplete
+  // call FixInvalidFrecenciesForExcludedPlaces() to handle that scenario
+  // XXX this might be dog slow, further degrading delete perf.
+  rv = FixInvalidFrecenciesForExcludedPlaces();
+  NS_ENSURE_SUCCESS(rv, rv);
+ 
+  // XXX todo
+  // forcibly call the "on idle" timer here to do a little work
+  // but the rest will happen on idle.  
+  
+  rv = transaction.Commit();
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  // force a full refresh calling onEndUpdateBatch (will call Refresh())
+  if (aDoBatchNotify)
+    UpdateBatchScoper batch(*this); // sends Begin/EndUpdateBatch to observers
+
+  return NS_OK;
+}
+
+
 // nsNavHistory::RemovePage
 //
 //    Removes all visits and the main history entry for the given URI.
 //    Silently fails if we have no knowledge of the page.
 
-NS_IMETHODIMP
+ NS_IMETHODIMP
 nsNavHistory::RemovePage(nsIURI *aURI)
 {
-  PRInt64 placeId;
+  nsIURI** URIs = &aURI;
-  nsresult rv = GetUrlIdFor(aURI, &placeId, PR_TRUE);
+  nsresult rv = RemovePages(URIs, 1, PR_FALSE);
   NS_ENSURE_SUCCESS(rv, rv);
-
+  // call observers here for the removed url
-  mozStorageTransaction transaction(mDBConn, PR_FALSE);
-  nsCOMPtr<mozIStorageStatement> statement;
-
-  // delete all visits for this page
-  rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
-      "DELETE FROM moz_historyvisits WHERE place_id = ?1"),
-      getter_AddRefs(statement));
-  NS_ENSURE_SUCCESS(rv, rv);
-  rv = statement->BindInt64Parameter(0, placeId);
-  NS_ENSURE_SUCCESS(rv, rv);
-  rv = statement->Execute();
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  // now that visits have been removed, run annotation expiration.
-  // this will remove all expire-able annotations for this URI.
-  (void)mExpire.OnDeleteURI();
-
-  // does the uri have un-expirable annotations?
-  nsAnnotationService* annosvc = nsAnnotationService::GetAnnotationService();
-  NS_ENSURE_STATE(annosvc);
-  nsTArray<nsCString> annoNames;
-  rv = annosvc->GetAnnotationNamesTArray(placeId, &annoNames, PR_FALSE);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  // is the uri bookmarked?
-  nsNavBookmarks* bookmarksService = nsNavBookmarks::GetBookmarksService();
-  NS_ENSURE_STATE(bookmarksService);
-  PRBool bookmarked = PR_FALSE;
-  rv = bookmarksService->IsBookmarked(aURI, &bookmarked);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  // if there are no more annotations, and the entry is not bookmarked
-  // then we can remove the moz_places entry.
-  if (annoNames.Length() == 0 && !bookmarked) {
-    // Note that we do NOT delete favicons. Any unreferenced favicons will be
-    // deleted next time the browser is shut down.
-
-    // delete main history entries
-    rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
-        "DELETE FROM moz_places WHERE id = ?1"),
-        getter_AddRefs(statement));
-    NS_ENSURE_SUCCESS(rv, rv);
-    rv = statement->BindInt64Parameter(0, placeId);
-    NS_ENSURE_SUCCESS(rv, rv);
-    rv = statement->Execute();
-    NS_ENSURE_SUCCESS(rv, rv);
-  }
-  else {
-    // because the moz_place was annotated (or it was a bookmark), 
-    // we didn't delete it, but we did delete the moz_visits
-    // so we need to reset the frecency.  Note, we don't
-    // reset the visit_count, as we use that in our "on idle"
-    // query to figure out which places to recalcuate frecency first.
-    rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
-        "UPDATE moz_places SET frecency = -1 WHERE id = ?1"),
-        getter_AddRefs(statement));
-    NS_ENSURE_SUCCESS(rv, rv);
-    rv = statement->BindInt64Parameter(0, placeId);
-    NS_ENSURE_SUCCESS(rv, rv);
-    rv = statement->Execute();
-    NS_ENSURE_SUCCESS(rv, rv);
-
-    // placeId could have a livemark item, so setting the frecency to -1
-    // would cause it to show up in the url bar autocomplete
-    // call FixInvalidFrecenciesForExcludedPlaces() to handle that scenario
-    // XXX this might be dog slow, further degrading delete perf.
-    rv = FixInvalidFrecenciesForExcludedPlaces();
-    NS_ENSURE_SUCCESS(rv, rv);
-
-    // XXX todo
-    // forcibly call the "on idle" timer here to do a little work
-    // but the rest will happen on idle.
-  }
-
-  // Observers: Be sure to finish transaction before calling observers. Note also
-  // that we always call the observers even though we aren't sure something
-  // actually got deleted.
-  transaction.Commit();
   ENUMERATE_WEAKARRAY(mObservers, nsINavHistoryObserver, OnDeleteURI(aURI))
   return NS_OK;
 }

toolkit/components/places/tests/unit/test_browserhistory.js

@@ -23,6 +23,7 @@
  *  Darin Fisher <darin@meer.net>
  *  Dietrich Ayala <dietrich@mozilla.com>
  *  Dan Mills <thunder@mozilla.com>
+ *  Marco Bonardo <mak77@supereva.it>
  *
  * Alternatively, the contents of this file may be used under the terms of
  * either the GNU General Public License Version 2 or later (the "GPL"), or
@@ -45,6 +46,20 @@ try {
   do_throw("Could not get history service\n");
 } 
 
+// Get annotation service
+try {
+  var annosvc= Cc["@mozilla.org/browser/annotation-service;1"].getService(Ci.nsIAnnotationService);
+} catch(ex) {
+  do_throw("Could not get annotation service\n");
+} 
+
+// Get bookmark service
+try {
+  var bmsvc = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].getService(Ci.nsINavBookmarksService);
+} catch(ex) {
+  do_throw("Could not get nav-bookmarks-service\n");
+}
+
 // main
 function run_test() {
   var testURI = uri("http://mozilla.com");
@@ -83,6 +98,52 @@ function run_test() {
   do_check_eq(0, bhist.count);
   do_check_eq("", bhist.lastPageVisited);
 
+  /**
+   * remove a bunch of pages from history
+   */
+  var deletedPages = [];
+  deletedPages.push(uri("http://mirror1.mozilla.com"));
+  deletedPages.push(uri("http://mirror2.mozilla.com"));
+  deletedPages.push(uri("http://mirror3.mozilla.com"));
+  deletedPages.push(uri("http://mirror4.mozilla.com"));
+  deletedPages.push(uri("http://mirror5.mozilla.com"));
+  deletedPages.push(uri("http://mirror6.mozilla.com"));
+  deletedPages.push(uri("http://mirror7.mozilla.com"));
+  deletedPages.push(uri("http://mirror8.mozilla.com"));
+
+  try {
+    for (var i = 0; i < deletedPages.length ; ++i)
+      bhist.addPageWithDetails(deletedPages[i], "testURI" + (i+1), Date.now() * 1000);
+  } catch(ex) {
+    do_throw("addPageWithDetails failed");
+  }
+
+  // annotated and bookmarked items should not be removed from moz_places
+  var annoIndex = 1;
+  var annoName = "testAnno";
+  var annoValue = "foo";
+  var bookmarkIndex = 2;
+  var bookmarkName = "bar";  
+  annosvc.setPageAnnotation(deletedPages[annoIndex], annoName, annoValue, 0,
+                            Ci.nsIAnnotationService.EXPIRE_NEVER);
+  var bookmark = bmsvc.insertBookmark(bmsvc.bookmarksMenuFolder,
+      deletedPages[bookmarkIndex], bmsvc.DEFAULT_INDEX, bookmarkName);
+
+  try {
+    bhist.removePages(deletedPages, deletedPages.length, false);
+  } catch(ex) {
+    do_throw("removePages failed");
+  }
+  do_check_eq(0, bhist.count);
+  do_check_eq("", bhist.lastPageVisited);
+  // check that bookmark and annotation still exist
+  do_check_eq(bmsvc.getBookmarkURI(bookmark).spec, deletedPages[bookmarkIndex].spec);
+  do_check_eq(annosvc.getPageAnnotation(deletedPages[annoIndex], annoName), annoValue);
+  // remove annotation and bookmark
+  annosvc.removePageAnnotation(deletedPages[annoIndex], annoName);
+  bmsvc.removeItem(bookmark);
+  bhist.removeAllPages();
+
   /**
    * removePagesFromHost
    * Remove all pages from the given host.