Bug 487040 - Crash [@ nsNavHistoryResult::OnItemAdded ] in mochitest-browser-chrome, r=dietrich,sdwilsh,smaug a=blocking191
authorMarco Bonardo <mbonardo@mozilla.com>
Sat, 18 Apr 2009 11:58:17 +0200
changeset 24935 8077dff341df
parent 24934 71a5ebd9a59c
child 24936 61f8eafbdf25
push id1268
push usermak77@bonardo.net
push dateMon, 20 Apr 2009 10:28:03 +0000
reviewersdietrich, sdwilsh, smaug, blocking191
bugs487040
milestone1.9.1b4pre
Bug 487040 - Crash [@ nsNavHistoryResult::OnItemAdded ] in mochitest-browser-chrome, r=dietrich,sdwilsh,smaug a=blocking191
browser/components/places/content/controller.js
browser/components/places/content/editBookmarkOverlay.js
browser/components/places/src/nsPlacesTransactionsService.js
browser/components/places/tests/browser/Makefile.in
browser/components/places/tests/browser/browser_410196_paste_into_tags.js
browser/components/places/tests/browser/browser_423515.js
browser/components/places/tests/browser/browser_425884.js
browser/components/places/tests/browser/browser_457473_no_copy_guid.js
browser/components/places/tests/browser/browser_library_search.js
browser/components/places/tests/unit/test_placesTxn.js
toolkit/components/places/src/nsNavHistoryResult.cpp
toolkit/components/places/src/nsNavHistoryResult.h
--- a/browser/components/places/content/controller.js
+++ b/browser/components/places/content/controller.js
@@ -1094,17 +1094,18 @@ PlacesController.prototype = {
     try {
       result.viewer = null;
       var nodes = this._view.getSelectionNodes();
 
       var xferable =  Cc["@mozilla.org/widget/transferable;1"].
                       createInstance(Ci.nsITransferable);
       var foundFolder = false, foundLink = false;
       var copiedFolders = [];
-      var placeString = mozURLString = htmlString = unicodeString = "";
+      var placeString, mozURLString, htmlString, unicodeString;
+      placeString = mozURLString = htmlString = unicodeString = "";
 
       for (var i = 0; i < nodes.length; ++i) {
         var node = nodes[i];
         if (this._shouldSkipNode(node, copiedFolders))
           continue;
         if (PlacesUtils.nodeIsFolder(node))
           copiedFolders.push(node);
         
--- a/browser/components/places/content/editBookmarkOverlay.js
+++ b/browser/components/places/content/editBookmarkOverlay.js
@@ -805,17 +805,18 @@ var gEditItemOverlay = {
    * @param aFolderId
    *        The identifier of the bookmarks folder.
    */
   _getFolderMenuItem:
   function EIO__getFolderMenuItem(aFolderId) {
     var menupopup = this._folderMenuList.menupopup;
 
     for (var i=0;  i < menupopup.childNodes.length; i++) {
-      if (menupopup.childNodes[i].folderId == aFolderId)
+      if (menupopup.childNodes[i].folderId &&
+          menupopup.childNodes[i].folderId == aFolderId)
         return menupopup.childNodes[i];
     }
 
     // 3 special folders + separator + folder-items-count limit
     if (menupopup.childNodes.length == 4 + MAX_FOLDER_ITEM_IN_MENU_LIST)
       menupopup.removeChild(menupopup.lastChild);
 
     return this._appendFolderItemToMenupopup(menupopup, aFolderId);
--- a/browser/components/places/src/nsPlacesTransactionsService.js
+++ b/browser/components/places/src/nsPlacesTransactionsService.js
@@ -17,16 +17,17 @@
  * The Initial Developer of the Original Code is Google Inc.
  *
  * Portions created by the Initial Developer are Copyright (C) 2005
  * the Initial Developer. All Rights Reserved.
  *
  * Contributor(s):
  *   Sungjoon Steve Won <stevewon@gmail.com> (Original Author)
  *   Asaf Romano <mano@mozilla.com>
+ *   Marco Bonarco <mak77@bonardo.net>
  *
  * 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
@@ -387,20 +388,22 @@ placesCreateFolderTransactions.prototype
     for (var i = 0; i < this._childItemsTransactions.length; ++i) {
       var txn = this._childItemsTransactions[i];
       txn.wrappedJSObject.container = this._id;
       txn.doTransaction();
     }
   },
 
   undoTransaction: function PCFT_undoTransaction() {
-    for (var i = 0; i < this._childItemsTransactions.length; ++i) {
+    // Undo transactions should always be done in reverse order.
+    for (var i = this._childItemsTransactions.length - 1; i >= 0 ; i--) {
       var txn = this._childItemsTransactions[i];
       txn.undoTransaction();
     }
+    // Remove item only after all child transactions have been reverted.
     PlacesUtils.bookmarks.removeFolder(this._id);
   }
 };
 
 function placesCreateItemTransactions(aURI, aContainer, aIndex, aTitle,
                                       aKeyword, aAnnotations,
                                       aChildTransactions) {
   this._uri = aURI;
@@ -431,21 +434,23 @@ placesCreateItemTransactions.prototype =
     for (var i = 0; i < this._childTransactions.length; ++i) {
       var txn = this._childTransactions[i];
       txn.wrappedJSObject.id = this._id;
       txn.doTransaction();
     }
   },
 
   undoTransaction: function PCIT_undoTransaction() {
-    PlacesUtils.bookmarks.removeItem(this._id);
-    for (var i = 0; i < this._childTransactions.length; ++i) {
+    // Undo transactions should always be done in reverse order.
+    for (var i = this._childTransactions.length - 1; i >= 0; i--) {
       var txn = this._childTransactions[i];
       txn.undoTransaction();
     }
+    // Remove item only after all child transactions have been reverted.
+    PlacesUtils.bookmarks.removeItem(this._id);
   }
 };
 
 function placesCreateSeparatorTransactions(aContainer, aIndex) {
   this._container = aContainer;
   this._index = typeof(aIndex) == "number" ? aIndex : -1;
   this._id = null;
   this.redoTransaction = this.doTransaction;
--- a/browser/components/places/tests/browser/Makefile.in
+++ b/browser/components/places/tests/browser/Makefile.in
@@ -38,26 +38,25 @@ DEPTH			= ../../../../..
 topsrcdir	= @top_srcdir@
 srcdir		= @srcdir@
 VPATH			= @srcdir@
 relativesrcdir  = browser/components/places/tests/browser
 
 include $(DEPTH)/config/autoconf.mk
 include $(topsrcdir)/config/rules.mk
 
-# Temporarly disabled to investigate crashes (bug 487040)
-# browser_bookmarksProperties.js
 _BROWSER_TEST_FILES = \
 	browser_0_library_left_pane_migration.js \
 	browser_425884.js \
 	browser_423515.js \
 	browser_410196_paste_into_tags.js \
 	browser_457473_no_copy_guid.js \
 	browser_sort_in_library.js \
 	browser_library_open_leak.js \
 	browser_library_panel_leak.js \
 	browser_library_search.js \
 	browser_history_sidebar_search.js \
+	browser_bookmarksProperties.js \
 	browser_forgetthissite_single.js \
 	$(NULL)
 
 libs:: $(_BROWSER_TEST_FILES)
 	$(INSTALL) $(foreach f,$^,"$f") $(DEPTH)/_tests/testing/mochitest/browser/$(relativesrcdir)
--- a/browser/components/places/tests/browser/browser_410196_paste_into_tags.js
+++ b/browser/components/places/tests/browser/browser_410196_paste_into_tags.js
@@ -86,17 +86,17 @@ function test() {
           ok(PUIU, "PlacesUIUtils in scope");
           ok(PO, "Places organizer in scope");
         },
 
         makeHistVisit: function() {
           // need to add a history object
           var testURI1 = PU._uri(MOZURISPEC);
           isnot(testURI1, null, "testURI is not null");
-          visitId = add_visit(testURI1);
+          var visitId = add_visit(testURI1);
           ok(visitId > 0, "A visit was added to the history");
           ok(gh.isVisited(testURI1), MOZURISPEC + " is a visited url.");
         },
 
         makeTag: function() {
           // create an initial tag to work with
           var bmId = add_bookmark(PlacesUtils._uri(TEST_URL));
           ok(bmId > 0, "A bookmark was added");
--- a/browser/components/places/tests/browser/browser_423515.js
+++ b/browser/components/places/tests/browser/browser_423515.js
@@ -141,29 +141,34 @@ function test() {
       // test toolbar shortcut node
       is(rootNode.childCount, this.folders.length,
         "populated data to the test root");
 
       function getRootChildNode(aId) {
         var node = PlacesUtils.getFolderContents(PlacesUtils.placesRootId, false, true).root;
         for (var i = 0; i < node.childCount; i++) {
           var child = node.getChild(i);
-          if (child.itemId == aId)
+          if (child.itemId == aId) {
+            node.containerOpen = false;
             return child;
+          }
         }
+        node.containerOpen = false;
+        throw("Unable to find child node");
+        return null;
       }
 
       for (var i = 0; i < this.folders.length; i++) {
         var id = this.folders[i];
 
         is(PlacesControllerDragHelper.canMoveContainer(id),
            false, "shouldn't be able to move special folder id");
 
-        //var node = PlacesUtils.getFolderContents(id, false, true).root;
         var node = getRootChildNode(id);
+        isnot(node, null, "Node found");
         is(PlacesControllerDragHelper.canMoveNode(node),
            false, "shouldn't be able to move special folder node");
 
         var shortcutId = this.shortcuts[id];
         var shortcutNode = rootNode.getChild(i);
 
         is(shortcutNode.itemId, shortcutId, "shortcut id and shortcut node item id match");
 
@@ -194,16 +199,18 @@ function test() {
 
       tagsNode.containerOpen = true;
       is(tagsNode.childCount, 1, "has new tag");
 
       var tagNode = tagsNode.getChild(0);
       
       is(PlacesControllerDragHelper.canMoveNode(tagNode),
          false, "should not be able to move tag container node");
+
+      tagsNode.containerOpen = false;
     }
   });
 
   // test that any child of a read-only node cannot be moved
   tests.push({
     populate: function() {
       this.id =
         PlacesUtils.bookmarks.createFolder(rootId, "foo", IDX);
--- a/browser/components/places/tests/browser/browser_425884.js
+++ b/browser/components/places/tests/browser/browser_425884.js
@@ -97,16 +97,20 @@ function test() {
   is(testRootNode.childCount, 1, "confirm undo removed the copied folder");
 
   // redo the transaction
   PlacesUIUtils.ptm.redoTransaction();
   is(testRootNode.childCount, 2, "confirm redo re-copied the folder");
   folderBNode = testRootNode.getChild(1);
   validate(folderBNode);
 
+  // Close containers, cleaning up their observers.
+  testRootNode.containerOpen = false;
+  toolbarNode.containerOpen = false;
+
   // clean up
   PlacesUIUtils.ptm.undoTransaction();
   PlacesUtils.bookmarks.removeItem(folderAId);
 }
 
 function populate(aFolderId) {
   var folderId = PlacesUtils.bookmarks.createFolder(aFolderId, "test folder", -1);
   PlacesUtils.bookmarks.insertBookmark(folderId, PlacesUtils._uri("http://foo"), -1, "test bookmark");
--- a/browser/components/places/tests/browser/browser_457473_no_copy_guid.js
+++ b/browser/components/places/tests/browser/browser_457473_no_copy_guid.js
@@ -110,16 +110,20 @@ function test() {
   ok(checkGUIDs(folderBNode, folderAGUIDs, false), "folder B GUIDs after undo/redo don't match folder A GUIDs"); // sanity check
 
   // XXXdietrich fails since GUIDs are created lazily. the anno
   // isn't present at the time the transaction is first executed,
   // and undo just undoes the original transaction - doesn't pull
   // in any new changes.
   //ok(checkGUIDs(folderBNode, folderBGUIDs, true, true), "folder B GUIDs after under/redo should match pre-undo/redo folder B GUIDs");
 
+  // Close containers, cleaning up their observers.
+  testRootNode.containerOpen = false;
+  toolbarNode.containerOpen = false;
+
   // clean up
   PlacesUIUtils.ptm.undoTransaction();
   PlacesUtils.bookmarks.removeItem(testRootId);
 }
 
 function getGUIDs(aNode) {
   asContainer(aNode);
   aNode.containerOpen = true;
--- a/browser/components/places/tests/browser/browser_library_search.js
+++ b/browser/components/places/tests/browser/browser_library_search.js
@@ -71,41 +71,41 @@ var testCases = [
     search(PlacesUIUtils.allBookmarksFolderId, "dummy", defScope);
     is(selectScope("scopeBarFolder"), false,
        "Folder scope should be disabled for All Bookmarks");
     resetSearch(defScope);
   },
 
   // History
   function () {
-    defScope = getDefaultScope(PlacesUIUtils.leftPaneQueries["History"]);
+    var defScope = getDefaultScope(PlacesUIUtils.leftPaneQueries["History"]);
     search(PlacesUIUtils.leftPaneQueries["History"], "dummy", defScope);
     is(selectScope("scopeBarFolder"), false,
        "Folder scope should be disabled for History");
     resetSearch(defScope);
   },
 
   // Toolbar folder
   function () {
-    defScope = getDefaultScope(bmsvc.toolbarFolder);
+    var defScope = getDefaultScope(bmsvc.toolbarFolder);
     search(bmsvc.toolbarFolder, "dummy", defScope);
     is(selectScope("scopeBarFolder"), true,
        "Folder scope should be enabled for toolbar folder");
     // Ensure that folder scope is still selected after resetting and searching
     // again.
     resetSearch("scopeBarFolder");
     search(bmsvc.toolbarFolder, "dummy", "scopeBarFolder");
   },
 
   // A regular non-root subfolder
   function () {
     var folderId = bmsvc.createFolder(bmsvc.toolbarFolder,
                                       "dummy folder",
                                       bmsvc.DEFAULT_INDEX);
-    defScope = getDefaultScope(folderId);
+    var defScope = getDefaultScope(folderId);
     search(folderId, "dummy", defScope);
     is(selectScope("scopeBarFolder"), true,
        "Folder scope should be enabled for regular subfolder");
     // Ensure that folder scope is still selected after resetting and searching
     // again.
     resetSearch("scopeBarFolder");
     search(folderId, "dummy", "scopeBarFolder");
     bmsvc.removeItem(folderId);
--- a/browser/components/places/tests/unit/test_placesTxn.js
+++ b/browser/components/places/tests/unit/test_placesTxn.js
@@ -163,17 +163,17 @@ function run_test() {
   txn1.undoTransaction();
   do_check_eq(observer._itemRemovedId, folderId);
   do_check_eq(observer._itemRemovedFolder, root);
   do_check_eq(observer._itemRemovedIndex, bmStartIndex);
 
   // Test creating an item
   // Create to Root
   var txn2 = ptSvc.createItem(uri("http://www.example.com"), root, bmStartIndex, "Testing1");
-  ptSvc.doTransaction(txn2); //Also testing doTransaction
+  ptSvc.doTransaction(txn2); // Also testing doTransaction
   var b = (bmsvc.getBookmarkIdsForURI(uri("http://www.example.com"), {}))[0];
   do_check_eq(observer._itemAddedId, b);
   do_check_eq(observer._itemAddedIndex, bmStartIndex);
   do_check_true(bmsvc.isBookmarked(uri("http://www.example.com")));
   txn2.undoTransaction();
   do_check_eq(observer._itemRemovedId, b);
   do_check_eq(observer._itemRemovedIndex, bmStartIndex);
   do_check_false(bmsvc.isBookmarked(uri("http://www.example.com")));
@@ -701,10 +701,54 @@ function run_test() {
   do_check_eq(bmsvc.getItemType(newBkmk3_1Id), bmsvc.TYPE_BOOKMARK);
   do_check_eq(bmsvc.getBookmarkURI(newBkmk3_1Id).spec, "http://www.mozilla.org/");
   do_check_eq(bmsvc.getFolderIdForItem(newBkmk3_2Id), newBkmk3Id);
   do_check_eq(bmsvc.getItemType(newBkmk3_2Id), bmsvc.TYPE_SEPARATOR);
   do_check_eq(bmsvc.getFolderIdForItem(newBkmk3_3Id), newBkmk3Id);
   do_check_eq(bmsvc.getItemType(newBkmk3_3Id), bmsvc.TYPE_FOLDER);
   do_check_eq(bmsvc.getItemTitle(newBkmk3_3Id), "folder");
 
+  // Test creating an item with child transactions.
+  var newDateAdded = Date.now() - 20000;
+  var childTxn = ptSvc.editItemDateAdded(null, newDateAdded);
+  var itemWChildTxn = ptSvc.createItem(uri("http://www.example.com"), root, bmStartIndex, "Testing1", null, null, [childTxn]);
+  try {
+    ptSvc.doTransaction(itemWChildTxn); // Also testing doTransaction
+    var itemId = (bmsvc.getBookmarkIdsForURI(uri("http://www.example.com"), {}))[0];
+    do_check_eq(observer._itemAddedId, itemId);
+    do_check_eq(newDateAdded, bmsvc.getItemDateAdded(itemId));
+    itemWChildTxn.undoTransaction();
+    do_check_eq(observer._itemRemovedId, itemId);
+    itemWChildTxn.redoTransaction();
+    do_check_true(bmsvc.isBookmarked(uri("http://www.example.com")));
+    var newId = (bmsvc.getBookmarkIdsForURI(uri("http://www.example.com"), {}))[0];
+    do_check_eq(newDateAdded, bmsvc.getItemDateAdded(newId));
+    do_check_eq(observer._itemAddedId, newId);
+    itemWChildTxn.undoTransaction();
+    do_check_eq(observer._itemRemovedId, newId);
+  } catch (ex) {
+    do_throw("Setting a child transaction in a createItem transaction did throw: " + ex);
+  }
+
+  // Create a folder with child item transactions.
+  var childItemTxn = ptSvc.createItem(uri("http://www.childItem.com"), root, bmStartIndex, "childItem");
+  var folderWChildItemTxn = ptSvc.createFolder("Folder", root, bmStartIndex, null, [childItemTxn]);
+  try {
+    ptSvc.doTransaction(folderWChildItemTxn);
+    var childItemId = (bmsvc.getBookmarkIdsForURI(uri("http://www.childItem.com"), {}))[0];
+    do_check_eq(observer._itemAddedId, childItemId);
+    do_check_eq(observer._itemAddedIndex, 0);
+    do_check_true(bmsvc.isBookmarked(uri("http://www.childItem.com")));
+    folderWChildItemTxn.undoTransaction();
+    do_check_false(bmsvc.isBookmarked(uri("http://www.childItem.com")));
+    folderWChildItemTxn.redoTransaction();
+    newchildItemId = (bmsvc.getBookmarkIdsForURI(uri("http://www.childItem.com"), {}))[0];
+    do_check_eq(observer._itemAddedIndex, 0);
+    do_check_eq(observer._itemAddedId, newchildItemId);
+    do_check_true(bmsvc.isBookmarked(uri("http://www.childItem.com")));
+    folderWChildItemTxn.undoTransaction();
+    do_check_false(bmsvc.isBookmarked(uri("http://www.childItem.com")));
+  } catch (ex) {
+    do_throw("Setting a child item transaction in a createFolder transaction did throw: " + ex);
+  }
+
   bmsvc.removeObserver(observer, false);
 }
--- a/toolkit/components/places/src/nsNavHistoryResult.cpp
+++ b/toolkit/components/places/src/nsNavHistoryResult.cpp
@@ -18,64 +18,51 @@
  * Google Inc.
  * Portions created by the Initial Developer are Copyright (C) 2005
  * the Initial Developer. All Rights Reserved.
  *
  * Contributor(s):
  *   Brett Wilson <brettw@gmail.com> (original author)
  *   Dietrich Ayala <dietrich@mozilla.com>
  *   Asaf Romano <mano@mozilla.com>
+ *   Marco Bonardo <mak77@bonardo.net>
  *
  * 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 ***** */
 
-
 #include <stdio.h>
 #include "nsNavHistory.h"
 #include "nsNavBookmarks.h"
-
-#include "nsIArray.h"
-#include "nsCOMPtr.h"
+#include "nsFaviconService.h"
+#include "nsITaggingService.h"
+#include "nsAnnotationService.h"
+
 #include "nsDebug.h"
-#include "nsFaviconService.h"
-#include "nsHashPropertyBag.h"
-#include "nsIComponentManager.h"
-#include "nsIDateTimeFormat.h"
-#include "nsIDOMElement.h"
-#include "nsILocalFile.h"
-#include "nsIDynamicContainer.h"
-#include "nsIServiceManager.h"
-#include "nsISupportsPrimitives.h"
-#ifdef MOZ_XUL
-#include "nsITreeColumns.h"
-#endif
-#include "nsIURI.h"
-#include "nsIURL.h"
-#include "nsIWritablePropertyBag.h"
-#include "nsITaggingService.h"
 #include "nsNetUtil.h"
 #include "nsPrintfCString.h"
-#include "nsPromiseFlatString.h"
 #include "nsString.h"
 #include "nsUnicharUtils.h"
 #include "prtime.h"
 #include "prprf.h"
+
+#include "nsIDynamicContainer.h"
+#include "nsHashPropertyBag.h"
+#include "nsIWritablePropertyBag.h"
 #include "mozStorageHelper.h"
-#include "nsAnnotationService.h"
 #include "nsCycleCollectionParticipant.h"
 
 // What we want is: NS_INTERFACE_MAP_ENTRY(self) for static IID accessors,
 // but some of our classes (like nsNavHistoryResult) have an ambiguous base
 // class of nsISupports which prevents this from working (the default macro
 // converts it to nsISupports, then addrefs it, then returns it). Therefore, we
 // expand the macro here and change it so that it works. Yuck.
 #define NS_INTERFACE_MAP_STATIC_AMBIGUOUS(_class) \
@@ -386,16 +373,23 @@ nsNavHistoryContainerResultNode::nsNavHi
   mContainerType(aContainerType),
   mExpanded(PR_FALSE),
   mChildrenReadOnly(aReadOnly),
   mOptions(aOptions),
   mDynamicContainerType(aDynamicContainerType)
 {
 }
 
+nsNavHistoryContainerResultNode::~nsNavHistoryContainerResultNode()
+{
+  // Explicitly clean up array of children of this container.  We must ensure
+  // all references are gone and all of their destructors are called.
+  mChildren.Clear();
+}
+
 // nsNavHistoryContainerResultNode::OnRemoving
 //
 //    Containers should notify their children that they are being removed
 //    when the container is being removed.
 
 void
 nsNavHistoryContainerResultNode::OnRemoving()
 {
@@ -1372,18 +1366,18 @@ nsNavHistoryContainerResultNode::InsertS
 //
 //  This checks if the item at aIndex is located correctly given the sorting
 //  move. If it's not, the item is moved, and the result view are notified.
 //
 //  Returns true if the item position has been changed, false otherwise.
 
 PRBool
 nsNavHistoryContainerResultNode::EnsureItemPosition(PRUint32 aIndex) {
-  NS_ASSERTION(aIndex >= 0 && aIndex < mChildren.Count(), "Invalid index");
-  if (aIndex < 0 || aIndex >= mChildren.Count())
+  NS_ASSERTION(aIndex >= 0 && aIndex < (PRUint32)mChildren.Count(), "Invalid index");
+  if (aIndex < 0 || aIndex >= (PRUint32)mChildren.Count())
     return PR_FALSE;
 
   SortComparator comparator = GetSortingComparator(GetSortType());
   if (!comparator)
     return PR_FALSE;
 
   nsCAutoString sortAnno;
   GetSortingAnnotation(sortAnno);
@@ -2060,16 +2054,26 @@ nsNavHistoryQueryResultNode::nsNavHistor
   NS_ASSERTION(aQueries.Count() > 0, "Must have at least one query");
 
   nsNavHistory* history = nsNavHistory::GetHistoryService();
   NS_ASSERTION(history, "History service missing");
   mLiveUpdate = history->GetUpdateRequirements(mQueries, mOptions,
                                                &mHasSearchTerms);
 }
 
+nsNavHistoryQueryResultNode::~nsNavHistoryQueryResultNode() {
+  // Remove this node from result's observers.  We don't need to be notified
+  // anymore.
+  if (mResult && mResult->mAllBookmarksObservers.IndexOf(this) !=
+                   mResult->mAllBookmarksObservers.NoIndex)
+    mResult->RemoveAllBookmarksObserver(this);
+  if (mResult && mResult->mHistoryObservers.IndexOf(this) !=
+                   mResult->mHistoryObservers.NoIndex)
+    mResult->RemoveHistoryObserver(this);
+}
 
 // nsNavHistoryQueryResultNode::CanExpand
 //
 //    Whoever made us may want non-expanding queries. However, we always
 //    expand when we are the root node, or else asking for non-expanding
 //    queries would be useless. A query node is not expandable if excludeItems=1
 //    or expandQueries=0.
 
@@ -2385,17 +2389,17 @@ nsNavHistoryQueryResultNode::FillChildre
       RecursiveSort(sortingAnnotation.get(), comparator);
     }
   }
 
   // if we are limiting our results remove items from the end of the
   // mChildren array after sorting. This is done for root node only.
   // note, if count < max results, we won't do anything.
   if (!mParent && mOptions->MaxResults()) {
-    while (mChildren.Count() > mOptions->MaxResults())
+    while ((PRUint32)mChildren.Count() > mOptions->MaxResults())
       mChildren.RemoveObjectAt(mChildren.Count() - 1);
   }
 
   nsNavHistoryResult* result = GetResult();
   NS_ENSURE_TRUE(result, NS_ERROR_FAILURE);
 
   if (mOptions->QueryType() == nsINavHistoryQueryOptions::QUERY_TYPE_HISTORY ||
       mOptions->QueryType() == nsINavHistoryQueryOptions::QUERY_TYPE_UNIFIED) {
@@ -2812,17 +2816,17 @@ nsNavHistoryQueryResultNode::OnDeleteURI
 }
 
 
 // nsNavHistoryQueryResultNode::OnClearHistory
 
 NS_IMETHODIMP
 nsNavHistoryQueryResultNode::OnClearHistory()
 {
-  Refresh();
+  (void)Refresh();
   return NS_OK;
 }
 
 
 // nsNavHistoryQueryResultNode::OnPageChanged
 //
 
 static void setFaviconCallback(
@@ -3220,17 +3224,17 @@ nsNavHistoryFolderResultNode::FillChildr
     GetSortingAnnotation(sortingAnnotation);
     RecursiveSort(sortingAnnotation.get(), comparator);
   }
 
   // if we are limiting our results remove items from the end of the
   // mChildren array after sorting. This is done for root node only.
   // note, if count < max results, we won't do anything.
   if (!mParent && mOptions->MaxResults()) {
-    while (mChildren.Count() > mOptions->MaxResults())
+    while ((PRUint32)mChildren.Count() > mOptions->MaxResults())
       mChildren.RemoveObjectAt(mChildren.Count() - 1);
   }
 
   // register with the result for updates
   nsNavHistoryResult* result = GetResult();
   NS_ENSURE_TRUE(result, NS_ERROR_FAILURE);
   result->AddBookmarkFolderObserver(this, mItemId);
   mIsRegisteredFolderObserver = PR_TRUE;
@@ -3272,18 +3276,19 @@ nsNavHistoryFolderResultNode::Refresh()
   ClearChildren(PR_TRUE);
 
   if (! mExpanded) {
     // when we are not expanded, we don't update, just invalidate and unhook
     return NS_OK; // no updates in tree state
   }
 
   // ignore errors from FillChildren, since we will still want to refresh
-  // the tree (there just might not be anything in it on error). Need to
-  // unregister as an observer because FillChildren will try to re-register us.
+  // the tree (there just might not be anything in it on error). ClearChildren
+  // has unregistered us as an observer since FillChildren will try to
+  // re-register us.
   (void)FillChildren();
 
   nsNavHistoryResult* result = GetResult();
   NS_ENSURE_TRUE(result, NS_ERROR_FAILURE);
   if (result->GetView())
     return result->GetView()->InvalidateContainer(
         static_cast<nsNavHistoryContainerResultNode*>(this));
   return NS_OK;
@@ -3319,17 +3324,17 @@ nsNavHistoryFolderResultNode::StartIncre
 
     // when a tree is attached also do incremental updates if our parent is
     // visible so that twisties are drawn correctly.
     if (mParent && result->GetView())
       return PR_TRUE;
   }
 
   // otherwise, we don't do incremental updates, invalidate and unregister
-  Refresh();
+  (void)Refresh();
   return PR_FALSE;
 }
 
 
 // nsNavHistoryFolderResultNode::ReindexRange
 //
 //    This function adds aDelta to all bookmark indices between the two
 //    endpoints, inclusive. It is used when items are added or removed from
@@ -3442,16 +3447,19 @@ nsNavHistoryFolderResultNode::OnItemAdde
   ReindexRange(aIndex, PR_INT32_MAX, 1);
 
   nsRefPtr<nsNavHistoryResultNode> node;
   if (itemType == nsINavBookmarksService::TYPE_BOOKMARK) {
     nsNavHistory* history = nsNavHistory::GetHistoryService();
     NS_ENSURE_TRUE(history, NS_ERROR_OUT_OF_MEMORY);
     rv = history->BookmarkIdToResultNode(aItemId, mOptions, getter_AddRefs(node));
     NS_ENSURE_SUCCESS(rv, rv);
+    // Correctly set batch status for new query nodes.
+    if (mResult && node->IsQuery())
+      node->GetAsQuery()->mBatchInProgress = mResult->mBatchInProgress;
   }
   else if (itemType == nsINavBookmarksService::TYPE_FOLDER ||
            itemType == nsINavBookmarksService::TYPE_DYNAMIC_CONTAINER) {
     rv = bookmarks->ResultNodeForContainer(aItemId, mOptions, getter_AddRefs(node));
     NS_ENSURE_SUCCESS(rv, rv);
   }
   else if (itemType == nsINavBookmarksService::TYPE_SEPARATOR) {
     node = new nsNavHistorySeparatorResultNode();
@@ -3721,17 +3729,19 @@ RemoveBookmarkFolderObserversCallback(ns
   delete aData;
   return PL_DHASH_REMOVE;
 }
 
 NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(nsNavHistoryResult)
   NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mRootNode)
   NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mView)
   tmp->mBookmarkFolderObservers.Enumerate(&RemoveBookmarkFolderObserversCallback, nsnull);
-NS_IMPL_CYCLE_COLLECTION_UNLINK_END 
+  NS_IMPL_CYCLE_COLLECTION_UNLINK_NSTARRAY(mAllBookmarksObservers)
+  NS_IMPL_CYCLE_COLLECTION_UNLINK_NSTARRAY(mHistoryObservers)
+NS_IMPL_CYCLE_COLLECTION_UNLINK_END
 
 static PLDHashOperator
 TraverseBookmarkFolderObservers(nsTrimInt64HashKey::KeyType aKey,
                                 nsNavHistoryResult::FolderObserverList*& aData,
                                 void* aUserArg)
 {
   nsCycleCollectionTraversalCallback* cb =
     static_cast<nsCycleCollectionTraversalCallback*>(aUserArg);
@@ -3744,16 +3754,18 @@ TraverseBookmarkFolderObservers(nsTrimIn
   }
   return PL_DHASH_NEXT;
 }
 
 NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(nsNavHistoryResult)
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR_AMBIGUOUS(mRootNode, nsINavHistoryContainerResultNode)
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mView)
   tmp->mBookmarkFolderObservers.Enumerate(&TraverseBookmarkFolderObservers, &cb);
+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSTARRAY_MEMBER(mAllBookmarksObservers, nsNavHistoryQueryResultNode)
+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSTARRAY_MEMBER(mHistoryObservers, nsNavHistoryQueryResultNode)
 NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
 
 NS_IMPL_CYCLE_COLLECTING_ADDREF(nsNavHistoryResult)
 NS_IMPL_CYCLE_COLLECTING_RELEASE(nsNavHistoryResult)
 
 NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(nsNavHistoryResult)
   NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsINavHistoryResult)
   NS_INTERFACE_MAP_STATIC_AMBIGUOUS(nsNavHistoryResult)
@@ -3837,16 +3849,21 @@ nsNavHistoryResult::NewHistoryResult(nsI
     return NS_ERROR_OUT_OF_MEMORY;
   NS_ADDREF(*result); // must happen before Init
   nsresult rv = (*result)->Init(aQueries, aQueryCount, aOptions);
   if (NS_FAILED(rv)) {
     NS_RELEASE(*result);
     *result = nsnull;
     return rv;
   }
+
+  // Correctly set mBatchInProgress for the result based on the root node value.
+  if (aRoot->IsQuery())
+    (*result)->mBatchInProgress = aRoot->GetAsQuery()->mBatchInProgress;
+
   return NS_OK;
 }
 
 
 // nsNavHistoryResult::PropertyBagFor
 //
 //    Given a pointer to a result node, this will give you the property bag
 //    corresponding to it. Each node exposes a property bag to be used to
@@ -3886,65 +3903,65 @@ nsNavHistoryResult::AddHistoryObserver(n
 {
   if (! mIsHistoryObserver) {
       nsNavHistory* history = nsNavHistory::GetHistoryService();
       NS_ASSERTION(history, "Can't create history service");
       history->AddObserver(this, PR_TRUE);
       mIsHistoryObserver = PR_TRUE;
   }
   if (mHistoryObservers.IndexOf(aNode) != mHistoryObservers.NoIndex) {
-    NS_WARNING("Attempting to register an observer twice!");
+    NS_WARNING("Attempting to register as a history observer twice!");
     return;
   }
   mHistoryObservers.AppendElement(aNode);
 }
 
 // nsNavHistoryResult::AddAllBookmarksObserver
 
 void
 nsNavHistoryResult::AddAllBookmarksObserver(nsNavHistoryQueryResultNode* aNode)
 {
-  if (! mIsAllBookmarksObserver) {
+  if (!mIsAllBookmarksObserver && !mIsBookmarkFolderObserver) {
     nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService();
     if (! bookmarks) {
       NS_NOTREACHED("Can't create bookmark service");
       return;
     }
     bookmarks->AddObserver(this, PR_TRUE);
     mIsAllBookmarksObserver = PR_TRUE;
   }
   if (mAllBookmarksObservers.IndexOf(aNode) != mAllBookmarksObservers.NoIndex) {
-    NS_WARNING("Attempting to register an observer twice!");
+    NS_WARNING("Attempting to register an all bookmarks observer twice!");
     return;
   }
   mAllBookmarksObservers.AppendElement(aNode);
 }
 
 
 // nsNavHistoryResult::AddBookmarkFolderObserver
 //
 //    Here, we also attach as a bookmark service observer if necessary
 
 void
 nsNavHistoryResult::AddBookmarkFolderObserver(nsNavHistoryFolderResultNode* aNode,
-                                        PRInt64 aFolder)
+                                              PRInt64 aFolder)
 {
-  if (! mIsBookmarkFolderObserver) {
+  if (!mIsBookmarkFolderObserver && !mIsAllBookmarksObserver) {
     nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService();
-    if (! bookmarks) {
+    if (!bookmarks) {
       NS_NOTREACHED("Can't create bookmark service");
       return;
     }
     bookmarks->AddObserver(this, PR_TRUE);
     mIsBookmarkFolderObserver = PR_TRUE;
   }
 
   FolderObserverList* list = BookmarkFolderObserversForId(aFolder, PR_TRUE);
   if (list->IndexOf(aNode) != list->NoIndex) {
-    NS_NOTREACHED("Attempting to register an observer twice!");
+    NS_NOTREACHED("Attempting to register as a folder observer twice!");
     return;
   }
   list->AppendElement(aNode);
 }
 
 
 // nsNavHistoryResult::RemoveHistoryObserver
 
@@ -4080,42 +4097,38 @@ nsNavHistoryResult::GetRoot(nsINavHistor
 
 
 // nsINavBookmarkObserver implementation
 
 // 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.
 #define ENUMERATE_BOOKMARK_FOLDER_OBSERVERS(_folderId, _functionCall) \
-  { \
+  PR_BEGIN_MACRO \
     FolderObserverList* _fol = BookmarkFolderObserversForId(_folderId, PR_FALSE); \
     if (_fol) { \
       FolderObserverList _listCopy(*_fol); \
       for (PRUint32 _fol_i = 0; _fol_i < _listCopy.Length(); _fol_i ++) { \
         if (_listCopy[_fol_i]) \
           _listCopy[_fol_i]->_functionCall; \
       } \
     } \
-  }
-#define ENUMERATE_ALL_BOOKMARKS_OBSERVERS(_functionCall) \
-  { \
-    nsTArray<nsNavHistoryQueryResultNode*> observerCopy(mAllBookmarksObservers); \
-    for (PRUint32 _obs_i = 0; _obs_i < observerCopy.Length(); _obs_i ++) { \
-      if (observerCopy[_obs_i]) \
-      observerCopy[_obs_i]->_functionCall; \
+  PR_END_MACRO
+#define ENUMERATE_QUERY_OBSERVERS(_functionCall, _observersList, _conditionCall) \
+  PR_BEGIN_MACRO \
+    QueryObserverList _listCopy(_observersList); \
+    for (PRUint32 _obs_i = 0; _obs_i < _listCopy.Length(); _obs_i ++) { \
+      if (_listCopy[_obs_i] && _listCopy[_obs_i]->_conditionCall) \
+        _listCopy[_obs_i]->_functionCall; \
     } \
-  }
+  PR_END_MACRO
+#define ENUMERATE_ALL_BOOKMARKS_OBSERVERS(_functionCall) \
+  ENUMERATE_QUERY_OBSERVERS(_functionCall, mAllBookmarksObservers, IsQuery())
 #define ENUMERATE_HISTORY_OBSERVERS(_functionCall) \
-  { \
-    nsTArray<nsNavHistoryQueryResultNode*> observerCopy(mHistoryObservers); \
-    for (PRUint32 _obs_i = 0; _obs_i < observerCopy.Length(); _obs_i ++) { \
-      if (observerCopy[_obs_i]) \
-      observerCopy[_obs_i]->_functionCall; \
-    } \
-  }
+  ENUMERATE_QUERY_OBSERVERS(_functionCall, mHistoryObservers, IsQuery())
 
 // nsNavHistoryResult::OnBeginUpdateBatch (nsINavBookmark/HistoryObserver)
 
 NS_IMETHODIMP
 nsNavHistoryResult::OnBeginUpdateBatch()
 {
   mBatchInProgress = PR_TRUE;
   ENUMERATE_HISTORY_OBSERVERS(OnBeginUpdateBatch());
@@ -4288,27 +4301,23 @@ nsNavHistoryResult::OnVisit(nsIURI* aURI
 
   if (!added && mRootNode->mExpanded) {
     // None of registered query observers has accepted our URI, this means,
     // that a matching query either was not expanded or it does not exist.
     PRUint32 resultType = mRootNode->mOptions->ResultType();
     if (resultType == nsINavHistoryQueryOptions::RESULTS_AS_DATE_QUERY ||
         resultType == nsINavHistoryQueryOptions::RESULTS_AS_DATE_SITE_QUERY ||
         resultType == nsINavHistoryQueryOptions::RESULTS_AS_SITE_QUERY)
-      mRootNode->GetAsQuery()->Refresh();
+      (void)mRootNode->GetAsQuery()->Refresh();
     else {
       // We are result of a folder node, then we should run through history
       // observers that are containers queries and refresh them.
       // We use a copy of the observers array since requerying could potentially
       // cause changes to the array.
-      nsTArray<nsNavHistoryQueryResultNode*> observerCopy(mHistoryObservers);
-      for (PRUint32 i = 0; i < observerCopy.Length(); i++) {
-        if (observerCopy[i] && observerCopy[i]->IsContainersQuery())
-          observerCopy[i]->Refresh();
-      }
+      ENUMERATE_QUERY_OBSERVERS(Refresh(), mHistoryObservers, IsContainersQuery());
     }
   }
 
   return NS_OK;
 }
 
 
 // nsNavHistoryResult::OnTitleChanged (nsINavHistoryObserver)
--- a/toolkit/components/places/src/nsNavHistoryResult.h
+++ b/toolkit/components/places/src/nsNavHistoryResult.h
@@ -46,17 +46,16 @@
 #define nsNavHistoryResult_h_
 
 #include "nsTArray.h"
 #include "nsInterfaceHashtable.h"
 #include "nsDataHashtable.h"
 #include "nsCycleCollectionParticipant.h"
 
 class nsNavHistory;
-class nsIDateTimeFormat;
 class nsIWritablePropertyBag;
 class nsNavHistoryQuery;
 class nsNavHistoryQueryOptions;
 
 class nsNavHistoryContainerResultNode;
 class nsNavHistoryFolderResultNode;
 class nsNavHistoryQueryResultNode;
 class nsNavHistoryVisitResultNode;
@@ -155,17 +154,17 @@ public:
 
   // returns the view. NOT-ADDREFED. May be NULL if there is no view
   nsINavHistoryResultViewer* GetView() const
     { return mView; }
 
 public:
   // two-stage init, use NewHistoryResult to construct
   nsNavHistoryResult(nsNavHistoryContainerResultNode* mRoot);
-  ~nsNavHistoryResult();
+  virtual ~nsNavHistoryResult();
   nsresult Init(nsINavHistoryQuery** aQueries,
                 PRUint32 aQueryCount,
                 nsNavHistoryQueryOptions *aOptions);
 
   nsRefPtr<nsNavHistoryContainerResultNode> mRootNode;
 
   nsCOMArray<nsINavHistoryQuery> mQueries;
   nsCOMPtr<nsNavHistoryQueryOptions> mOptions;
@@ -182,20 +181,22 @@ public:
   // property bags for all result nodes, see PropertyBagFor
   nsInterfaceHashtable<nsISupportsHashKey, nsIWritablePropertyBag> mPropertyBags;
 
   // node observers
   PRBool mIsHistoryObserver;
   PRBool mIsBookmarkFolderObserver;
   PRBool mIsAllBookmarksObserver;
 
-  nsTArray<nsNavHistoryQueryResultNode*> mHistoryObservers;
-  nsTArray<nsNavHistoryQueryResultNode*> mAllBookmarksObservers;
-  typedef nsTArray<nsRefPtr<nsNavHistoryFolderResultNode> > FolderObserverList;
-  nsDataHashtable<nsTrimInt64HashKey, FolderObserverList* > mBookmarkFolderObservers;
+  typedef nsTArray< nsRefPtr<nsNavHistoryQueryResultNode> > QueryObserverList;
+  QueryObserverList mHistoryObservers;
+  QueryObserverList mAllBookmarksObservers;
+
+  typedef nsTArray< nsRefPtr<nsNavHistoryFolderResultNode> > FolderObserverList;
+  nsDataHashtable<nsTrimInt64HashKey, FolderObserverList*> mBookmarkFolderObservers;
   FolderObserverList* BookmarkFolderObserversForId(PRInt64 aFolderId, PRBool aCreate);
 
   void RecursiveExpandCollapse(nsNavHistoryContainerResultNode* aContainer,
                                PRBool aExpand);
 
   void InvalidateTree();
   
   PRBool mBatchInProgress;
@@ -521,16 +522,18 @@ public:
     nsNavHistoryQueryOptions* aOptions);
   nsNavHistoryContainerResultNode(
     const nsACString& aURI, const nsACString& aTitle,
     PRTime aTime,
     const nsACString& aIconURI, PRUint32 aContainerType,
     PRBool aReadOnly, const nsACString& aDynamicContainerType,
     nsNavHistoryQueryOptions* aOptions);
 
+  virtual ~nsNavHistoryContainerResultNode();
+
   NS_DECLARE_STATIC_IID_ACCESSOR(NS_NAVHISTORYCONTAINERRESULTNODE_IID)
 
   NS_DECL_ISUPPORTS_INHERITED
   NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(nsNavHistoryContainerResultNode, nsNavHistoryResultNode)
   NS_FORWARD_COMMON_RESULTNODE_TO_BASE
   NS_IMETHOD GetType(PRUint32* type)
     { *type = mContainerType; return NS_OK; }
   NS_IMETHOD GetUri(nsACString& aURI)
@@ -689,16 +692,18 @@ public:
                               const nsCOMArray<nsNavHistoryQuery>& aQueries,
                               nsNavHistoryQueryOptions* aOptions);
   nsNavHistoryQueryResultNode(const nsACString& aTitle,
                               const nsACString& aIconURI,
                               PRTime aTime,
                               const nsCOMArray<nsNavHistoryQuery>& aQueries,
                               nsNavHistoryQueryOptions* aOptions);
 
+  virtual ~nsNavHistoryQueryResultNode();
+
   NS_DECL_ISUPPORTS_INHERITED
   NS_FORWARD_COMMON_RESULTNODE_TO_BASE
   NS_IMETHOD GetType(PRUint32* type)
     { *type = nsNavHistoryResultNode::RESULT_TYPE_QUERY; return NS_OK; }
   NS_IMETHOD GetUri(nsACString& aURI); // does special lazy creation
   NS_FORWARD_CONTAINERNODE_EXCEPT_HASCHILDREN_AND_READONLY
   NS_IMETHOD GetHasChildren(PRBool* aHasChildren);
   NS_IMETHOD GetChildrenReadOnly(PRBool *aChildrenReadOnly)