Bug 432706 - Deleting/adding/updating a large quantity of bookmarks/history is very slow due to updating observers (r=marco)

toolkit/components/places/src/nsNavHistoryResult.cpp

@@ -186,7 +186,7 @@ nsNavHistoryResultNode::GetTags(nsAString& aTags) {
   // then build it the first time this method called is called (and by that,
   // implicitly unset the void flag). Result observers may re-set the void flag
   // in order to force rebuilding of the tags string.
-  if (!mTags.IsVoid()) {
+  if (!mTags.IsVoid() && mParent) {
     aTags.Assign(mTags);
     return NS_OK;
   }
@@ -2964,6 +2964,7 @@ nsNavHistoryFolderResultNode::nsNavHistoryFolderResultNode(
   mContentsValid(PR_FALSE),
   mQueryItemId(-1),
   mIsRegisteredFolderObserver(PR_FALSE)
+, mBatchInProgress(PR_FALSE)
 {
   mItemId = aFolderId;
 }
@@ -3356,6 +3357,7 @@ nsNavHistoryFolderResultNode::FindChildById(PRInt64 aItemId,
 NS_IMETHODIMP
 nsNavHistoryFolderResultNode::OnBeginUpdateBatch()
 {
+  mBatchInProgress = PR_TRUE;
   return NS_OK;
 }
 
@@ -3365,6 +3367,13 @@ nsNavHistoryFolderResultNode::OnBeginUpdateBatch()
 NS_IMETHODIMP
 nsNavHistoryFolderResultNode::OnEndUpdateBatch()
 {
+  if (mBatchInProgress) {
+    mBatchInProgress = PR_FALSE;
+    nsresult rv = Refresh();
+    NS_ENSURE_SUCCESS(rv, rv);
+  }
+  else
+    NS_WARNING("EndUpdateBatch without a begin");
   return NS_OK;
 }
 
@@ -3378,6 +3387,9 @@ nsNavHistoryFolderResultNode::OnItemAdded(PRInt64 aItemId,
 {
   NS_ASSERTION(aParentFolder == mItemId, "Got wrong bookmark update");
 
+  if (mBatchInProgress)
+    return NS_OK;
+
   // here, try to do something reasonable if the bookmark service gives us
   // a bogus index.
   if (aIndex < 0) {
@@ -3457,6 +3469,9 @@ NS_IMETHODIMP
 nsNavHistoryFolderResultNode::OnItemRemoved(PRInt64 aItemId,
                                             PRInt64 aParentFolder, PRInt32 aIndex)
 {
+  if (mBatchInProgress)
+    return NS_OK;
+
   // We only care about notifications when a child changes. When the deleted
   // item is us, our parent should also be registered and will remove us from
   // its list.
@@ -3572,6 +3587,9 @@ nsNavHistoryFolderResultNode::OnItemChanged(PRInt64 aItemId,
                                             const nsACString& aProperty,
                                             PRBool aIsAnnotationProperty,
                                             const nsACString& aValue) {
+  if (mBatchInProgress)
+    return NS_OK;
+
   // The query-item's title is used for simple-query nodes
   if (mQueryItemId != -1) {
     PRBool isTitleChange = aProperty.EqualsLiteral("title");
@@ -3594,6 +3612,9 @@ NS_IMETHODIMP
 nsNavHistoryFolderResultNode::OnItemVisited(PRInt64 aItemId,
                                             PRInt64 aVisitId, PRTime aTime)
 {
+  if (mBatchInProgress)
+    return NS_OK;
+
   if (mOptions->ExcludeItems())
     return NS_OK; // don't update items when we aren't displaying them
   if (! StartIncrementalUpdate())
@@ -3645,6 +3666,10 @@ nsNavHistoryFolderResultNode::OnItemMoved(PRInt64 aItemId, PRInt64 aOldParent,
 {
   NS_ASSERTION(aOldParent == mItemId || aNewParent == mItemId,
                "Got a bookmark message that doesn't belong to us");
+
+  if (mBatchInProgress)
+    return NS_OK;
+
   if (! StartIncrementalUpdate())
     return NS_OK; // entire container was refreshed for us
 
@@ -4062,6 +4087,18 @@ nsNavHistoryResult::GetRoot(nsINavHistoryContainerResultNode** aRoot)
 
 // nsINavBookmarkObserver implementation
 
+PR_STATIC_CALLBACK(PLDHashOperator)
+FolderObserverEnumerator(nsTrimInt64HashKey::KeyType,
+                         nsNavHistoryResult::FolderObserverList *aList,
+                         void *aData)
+{
+  nsNavHistoryResult::FolderObserverList *list =
+    static_cast<nsNavHistoryResult::FolderObserverList *>(aData);
+  (void)list->AppendElements(*aList);
+  
+  return PL_DHASH_NEXT;
+}
+
 // Here, it is important that we create a COPY of the observer array. Some
 // observers will requery themselves, which may cause the observer array to
 // be modified or added to.
@@ -4076,6 +4113,15 @@ nsNavHistoryResult::GetRoot(nsINavHistoryContainerResultNode** aRoot)
       } \
     } \
   }
+#define ENUMERATE_ALL_BOOKMARK_FOLDER_OBSERVERS(_functionCall) \
+  { \
+    FolderObserverList _folders; \
+    mBookmarkFolderObservers.EnumerateRead(FolderObserverEnumerator, &_folders); \
+    for (PRUint32 _fol_i = 0; _fol_i < _folders.Length(); _fol_i++) { \
+      if (_folders[_fol_i]) \
+        _folders[_fol_i]->_functionCall; \
+    } \
+  }
 #define ENUMERATE_ALL_BOOKMARKS_OBSERVERS(_functionCall) \
   { \
     nsTArray<nsNavHistoryQueryResultNode*> observerCopy(mAllBookmarksObservers); \
@@ -4101,6 +4147,7 @@ nsNavHistoryResult::OnBeginUpdateBatch()
   mBatchInProgress = PR_TRUE;
   ENUMERATE_HISTORY_OBSERVERS(OnBeginUpdateBatch());
   ENUMERATE_ALL_BOOKMARKS_OBSERVERS(OnBeginUpdateBatch());
+  ENUMERATE_ALL_BOOKMARK_FOLDER_OBSERVERS(OnBeginUpdateBatch());
   return NS_OK;
 }
 
@@ -4114,6 +4161,7 @@ nsNavHistoryResult::OnEndUpdateBatch()
     mBatchInProgress = PR_FALSE;
     ENUMERATE_HISTORY_OBSERVERS(OnEndUpdateBatch());
     ENUMERATE_ALL_BOOKMARKS_OBSERVERS(OnEndUpdateBatch());
+    ENUMERATE_ALL_BOOKMARK_FOLDER_OBSERVERS(OnEndUpdateBatch());
   }
   else
     NS_WARNING("EndUpdateBatch without a begin");

toolkit/components/places/src/nsNavHistoryResult.h

@@ -808,6 +808,7 @@ public:
 private:
 
   PRBool mIsRegisteredFolderObserver;
+  PRBool mBatchInProgress;
 };
 
 // nsNavHistorySeparatorResultNode

toolkit/components/places/tests/bookmarks/test_432706_in_batch.js

@@ -0,0 +1,75 @@
+/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* vim:set ts=2 sw=2 sts=2 et: */
+/* ***** BEGIN LICENSE BLOCK *****
+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
+ *
+ * The contents of this file are subject to the Mozilla Public License Version
+ * 1.1 (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ * http://www.mozilla.org/MPL/
+ *
+ * Software distributed under the License is distributed on an "AS IS" basis,
+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
+ * for the specific language governing rights and limitations under the
+ * License.
+ *
+ * The Original Code is Places test code.
+ *
+ * The Initial Developer of the Original Code is Mozilla Corp.
+ * Portions created by the Initial Developer are Copyright (C) 2008
+ * the Initial Developer. All Rights Reserved.
+ *
+ * Contributor(s):
+ *  Dietrich Ayala <dietrich@mozilla.com>
+ *
+ * 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
+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
+ * in which case the provisions of the GPL or the LGPL are applicable instead
+ * of those above. If you wish to allow use of your version of this file only
+ * under the terms of either the GPL or the LGPL, and not to allow others to
+ * use your version of this file under the terms of the MPL, indicate your
+ * decision by deleting the provisions above and replace them with the notice
+ * and other provisions required by the GPL or the LGPL. If you do not delete
+ * the provisions above, a recipient may use your version of this file under
+ * the terms of any one of the MPL, the GPL or the LGPL.
+ *
+ * ***** END LICENSE BLOCK ***** */
+
+var bmsvc = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
+            getService(Ci.nsINavBookmarksService);
+var histsvc = Cc["@mozilla.org/browser/nav-history-service;1"].
+              getService(Ci.nsINavHistoryService);
+
+// main
+function run_test() {
+
+  // create a folder
+  var testFolder = bmsvc.createFolder(bmsvc.placesRoot, "test Folder",
+                                      bmsvc.DEFAULT_INDEX);
+
+  // get a node for the folder
+  var query = histsvc.getNewQuery();
+  query.setFolders([testFolder], 1);
+  var result = histsvc.executeQuery(query, histsvc.getNewQueryOptions());
+  var rootNode = result.root;
+  rootNode.containerOpen = true;
+
+  // start a batch
+  bmsvc.runInBatchMode({
+    runBatched: function(aUserData) {
+      // add a bookmark
+      var bookmarkId = bmsvc.insertBookmark(testFolder, uri("http://google.com/"),
+                                            bmsvc.DEFAULT_INDEX, "");
+
+      // confirm the node child count didn't change inside the batch
+      do_check_eq(rootNode.childCount, 0);
+    }
+  }, null);
+
+  // confirm that after the batch ended, the node and
+  // observer have been updated
+  do_check_eq(rootNode.childCount, 1);
+
+  rootNode.containerOpen = false;
+}

toolkit/components/places/tests/bookmarks/test_disconnected_nodes.js

@@ -0,0 +1,93 @@
+/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* vim:set ts=2 sw=2 sts=2 et: */
+/* ***** BEGIN LICENSE BLOCK *****
+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
+ *
+ * The contents of this file are subject to the Mozilla Public License Version
+ * 1.1 (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ * http://www.mozilla.org/MPL/
+ *
+ * Software distributed under the License is distributed on an "AS IS" basis,
+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
+ * for the specific language governing rights and limitations under the
+ * License.
+ *
+ * The Original Code is bug 471563 test code.
+ *
+ * The Initial Developer of the Original Code is
+ * Mozilla Corporation.
+ * Portions created by the Initial Developer are Copyright (C) 2008
+ * the Initial Developer. All Rights Reserved.
+ *
+ * Contributor(s):
+ *   Dietrich Ayala <dietrich@mozilla.com> (Original Author)
+ *
+ * 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
+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
+ * in which case the provisions of the GPL or the LGPL are applicable instead
+ * of those above. If you wish to allow use of your version of this file only
+ * under the terms of either the GPL or the LGPL, and not to allow others to
+ * use your version of this file under the terms of the MPL, indicate your
+ * decision by deleting the provisions above and replace them with the notice
+ * and other provisions required by the GPL or the LGPL. If you do not delete
+ * the provisions above, a recipient may use your version of this file under
+ * the terms of any one of the MPL, the GPL or the LGPL.
+ *
+ * ***** END LICENSE BLOCK ***** */
+
+// get services
+var histsvc = Cc["@mozilla.org/browser/nav-history-service;1"].
+              getService(Ci.nsINavHistoryService);
+var bmsvc = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
+            getService(Ci.nsINavBookmarksService);
+var tagssvc = Cc["@mozilla.org/browser/tagging-service;1"].
+              getService(Ci.nsITaggingService);
+
+function run_test() {
+  // get toolbar node
+  var options = histsvc.getNewQueryOptions();
+  var query = histsvc.getNewQuery();
+  query.setFolders([bmsvc.toolbarFolder], 1);
+  var result = histsvc.executeQuery(query, options);
+  var toolbarNode = result.root;
+  toolbarNode.containerOpen = true;
+
+  // add a bookmark
+  var bookmarkURI = uri("http://foo.com");
+  var bookmarkId = bmsvc.insertBookmark(bmsvc.toolbarFolder, bookmarkURI,
+                                        bmsvc.DEFAULT_INDEX, "");
+
+  // get the node for the new bookmark
+  var node = toolbarNode.getChild(toolbarNode.childCount-1);
+  do_check_eq(node.itemId, bookmarkId);
+
+  // confirm there's no tags via the .tags property
+  do_check_eq(node.tags, null); 
+
+  // add a tag
+  // NOTE: this executes in a batch, which causes |toolbarNode|
+  // to be refreshed, which disconnects the uri node |node|.
+  tagssvc.tagURI(bookmarkURI, ["foo"]);
+  do_check_eq(node.tags, "foo");
+
+  // add another tag, and check that the node updates itself.
+  tagssvc.tagURI(bookmarkURI, ["bar"]);
+  do_check_eq(node.tags, "bar, foo");
+
+  // change the bookmark title,
+  // confirm that the node updates itself.
+  // XXX disabled until bug 471563 is fixed.
+  //bmsvc.setItemTitle(node.itemId, "newtitle");
+  //do_check_eq(node.title, "newtitle");
+
+  // change the bookmark uri,
+  // confirm that the node updates itself.
+  // XXX disabled until bug 471563 is fixed.
+  //var newURI = uri("http://newuri");
+  //bmsvc.changeBookmarkURI(node.itemId, newURI);
+  //do_check_eq(node.uri, newURI.spec);
+
+  toolbarNode.containerOpen = false;
+}