Bug 342296 Switch bookmark transaction system from JS to C++ to prevent leaks r=IanN
authorNeil Rashbrook <neil@parkwaycc.co.uk>
Wed, 02 Sep 2009 00:02:06 +0100
changeset 3487 de2502431ec30544ebf5942d068d39a802812938
parent 3486 7cd1d18a9c4671fb2c7b2650d13173f90d1a222a
child 3488 b83f71869a2506c6f8d48f7a680c645bf25d2dee
child 3621 a9f098f4a65d411c9c42667cb14fb29c81d72bfa
push idunknown
push userunknown
push dateunknown
reviewersIanN
bugs342296
Bug 342296 Switch bookmark transaction system from JS to C++ to prevent leaks r=IanN
suite/browser/public/nsIBookmarksService.idl
suite/browser/src/nsBookmarksService.cpp
suite/common/bookmarks/bookmarks.js
suite/common/bookmarks/bookmarksTree.xml
--- a/suite/browser/public/nsIBookmarksService.idl
+++ b/suite/browser/public/nsIBookmarksService.idl
@@ -38,20 +38,22 @@
  * ***** END LICENSE BLOCK ***** */
 
 /**
  * The Browser Bookmarks service
  */
 
 #include "nsISupports.idl"
 
+interface nsIRDFNode;
 interface nsIRDFResource;
+interface nsITransaction;
 interface nsITransactionManager;
 
-[scriptable, uuid(4342a6ac-1b43-4121-b606-4bdf62de71ff)]
+[scriptable, uuid(b3b8d09d-71b1-4654-b807-9288c2f16300)]
 interface nsIBookmarksService : nsISupports
 {
     const unsigned long BOOKMARK_DEFAULT_TYPE   = 0;
     const unsigned long BOOKMARK_SEARCH_TYPE    = 1;
     const unsigned long BOOKMARK_FIND_TYPE      = 2;
 
     const long SORT_DESCENDING = -1;
     const long SORT_ASCENDING = 1;
@@ -100,16 +102,21 @@ interface nsIBookmarksService : nsISuppo
     
     AString getLastCharset(in AUTF8String aURL); 
 
     string resolveKeyword(in wstring aName);
     
     void importSystemBookmarks(in nsIRDFResource aParentFolder);
 
     readonly attribute nsITransactionManager transactionManager;
+
+    nsITransaction createTransaction(in nsIRDFResource parent,
+                                     in nsIRDFNode item,
+                                     in unsigned long index,
+                                     in boolean remove);
 };
 
 %{C++
 
 // {E638D760-8687-11d2-B530-000000000000}
 #define NS_BOOKMARKS_SERVICE_CID \
 { 0xe638d760, 0x8687, 0x11d2, { 0xb5, 0x30, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0 } }
 
--- a/suite/browser/src/nsBookmarksService.cpp
+++ b/suite/browser/src/nsBookmarksService.cpp
@@ -1617,16 +1617,117 @@ ElementArray::Clear()
     while (--index >= 0)
     {
         ElementInfo* elementInfo = ElementAt(index);
         delete elementInfo;
     }
     nsAutoVoidArray::Clear();
 }
 
+/**
+ * The bookmark transaction knows how to assert and unassert arcs
+ */
+class BookmarkTransaction: public nsITransaction,
+                           public nsIInterfaceRequestor
+{
+  NS_DECL_ISUPPORTS
+  NS_DECL_NSITRANSACTION
+  NS_DECL_NSIINTERFACEREQUESTOR
+
+private:
+  nsBookmarksService* mBookmarksService;
+  nsCOMPtr<nsIRDFResource> mParent;
+  nsCOMPtr<nsIRDFNode> mItem;
+  PRUint32 mIndex;
+  PRBool mRemove;
+  nsresult performTransaction(PRBool aRemove, PRBool aRenumber);
+
+public:
+  BookmarkTransaction(nsBookmarksService* aBookmarksService,
+                      nsIRDFResource* aParent, nsIRDFNode* aItem,
+                      PRUint32 aIndex, PRBool aRemove)
+    : mBookmarksService(aBookmarksService),
+      mParent(aParent),
+      mItem(aItem),
+      mIndex(aIndex),
+      mRemove(aRemove) {}
+};
+
+NS_IMPL_ISUPPORTS2(BookmarkTransaction, nsITransaction, nsIInterfaceRequestor)
+
+nsresult
+BookmarkTransaction::performTransaction(PRBool aRemove, PRBool aRenumber)
+{
+  nsresult rv;
+  nsCOMPtr<nsIRDFContainer> container(do_CreateInstance("@mozilla.org/rdf/container;1", &rv));
+  if (NS_FAILED(rv))
+    return rv;
+
+  rv = container->Init(mBookmarksService, mParent);
+  if (NS_FAILED(rv))
+    return rv;
+
+  if (!aRemove)
+    return container->InsertElementAt(mItem, mIndex, aRenumber);
+
+  // We don't renumber the container when deleting bookmarks.
+  // This makes it easy to undo the transaction by restoring the same index.
+  return container->RemoveElement(mItem, PR_FALSE);
+}
+
+NS_IMETHODIMP
+BookmarkTransaction::DoTransaction()
+{
+  return performTransaction(mRemove, PR_TRUE);
+}
+
+NS_IMETHODIMP
+BookmarkTransaction::UndoTransaction()
+{
+  // We don't renumber the container when undoing transactions.
+  // This makes it easy to redo the transaction by restoring the same index.
+  return performTransaction(!mRemove, PR_FALSE);
+}
+
+NS_IMETHODIMP
+BookmarkTransaction::RedoTransaction()
+{
+  // We don't renumber the container when redoing transactions.
+  // This makes it easy to undo the transaction by restoring the same index.
+  return performTransaction(mRemove, PR_FALSE);
+}
+
+NS_IMETHODIMP
+BookmarkTransaction::GetIsTransient(PRBool *aIsTransient)
+{
+  NS_ENSURE_ARG_POINTER(aIsTransient);
+  *aIsTransient = PR_FALSE;
+  return NS_OK;
+}
+
+NS_IMETHODIMP
+BookmarkTransaction::Merge(nsITransaction* aTransaction, PRBool* aResult)
+{
+  NS_ENSURE_ARG_POINTER(aResult);
+  *aResult = PR_FALSE;
+  return NS_OK;
+}
+
+NS_IMETHODIMP
+BookmarkTransaction::GetInterface(REFNSIID aIID, void** aResult)
+{
+  NS_ENSURE_ARG_POINTER(aResult);
+  nsISupports *result = nsnull;
+  if (aIID.Equals(NS_GET_IID(nsIRDFNode))) {
+    NS_ADDREF(result = mItem);
+    *aResult = result;
+    return NS_OK;
+  }
+  return NS_NOINTERFACE;
+}
 
 ////////////////////////////////////////////////////////////////////////
 // nsBookmarksService implementation
 
 nsBookmarksService::nsBookmarksService() :
     mUpdateBatchNest(0),
     mDirty(PR_FALSE)
 
@@ -3974,16 +4075,33 @@ nsBookmarksService::GetTransactionManage
 {
     NS_ENSURE_ARG_POINTER(aTransactionManager);
 
     NS_ADDREF(*aTransactionManager = mTransactionManager);
 
     return NS_OK;
 }
 
+NS_IMETHODIMP
+nsBookmarksService::CreateTransaction(nsIRDFResource* aParent,
+                                      nsIRDFNode* aItem,
+                                      PRUint32 aIndex,
+                                      PRBool aRemove,
+                                      nsITransaction** aTransaction)
+{
+  NS_ENSURE_ARG_POINTER(aTransaction);
+  NS_ENSURE_ARG(aItem);
+  *aTransaction = new BookmarkTransaction(this, aParent, aItem, aIndex, aRemove);
+  if (!*aTransaction)
+    return NS_ERROR_OUT_OF_MEMORY;
+
+  NS_ADDREF(*aTransaction);
+  return NS_OK;
+}
+
 ////////////////////////////////////////////////////////////////////////
 // nsIRDFDataSource
 
 NS_IMETHODIMP
 nsBookmarksService::GetURI(char* *aURI)
 {
     *aURI = NS_strdup("rdf:bookmarks");
     if (! *aURI)
--- a/suite/common/bookmarks/bookmarks.js
+++ b/suite/common/bookmarks/bookmarks.js
@@ -121,18 +121,16 @@ function initServices()
   } catch (e) {}
 }
 
 function initBMService()
 {
   kBMSVCIID = Components.interfaces.nsIBookmarksService;
   BMDS  = RDF.GetDataSource("rdf:bookmarks");
   BMSVC = BMDS.QueryInterface(kBMSVCIID);
-  BookmarkTransaction.prototype.RDFC = RDFC;
-  BookmarkTransaction.prototype.BMDS = BMDS;
 }
 
 /**
  * XXX - 04/16/01
  *  ACK! massive command name collision problems are causing big issues
  *  in getting this stuff to work in the Navigator window. For sanity's 
  *  sake, we need to rename all the commands to be of the form cmd_bm_*
  *  otherwise there'll continue to be problems. For now, we're just 
@@ -670,37 +668,35 @@ var BookmarksCommand = {
         fileName = kFilePicker.file.path;
         if (!fileName) return;
       }
       else return;
     }
     catch (e) {
       return;
     }
-    rTarget = RDF.GetResource("NC:BookmarksRoot");
+    var rTarget = BookmarksUtils.getNewBookmarkFolder();
     RDFC.Init(BMDS, rTarget);
-    var countBefore = parseInt(BookmarksUtils.getProperty(rTarget, RDF_NS+"nextVal"));
+    var index = RDFC.GetCount();
     var args = [{ property: NC_NS+"URL", literal: fileName}];
     this.doBookmarksCommand(rTarget, NC_NS_CMD+"import", args);
-    var countAfter = parseInt(BookmarksUtils.getProperty(rTarget, RDF_NS+"nextVal"));
+    RDFC.Init(BMDS, rTarget); // changing the selection clobbers RDFC
+    var count = RDFC.GetCount();
+    if (index == count)
+      return;
 
-    var transaction = new BookmarkImportTransaction("import");
-    for (var index = countBefore; index < countAfter; index++) {
-      var nChildArc = RDFCU.IndexToOrdinalResource(index);
-      var rChild    = BMDS.GetTarget(rTarget, nChildArc, true);
-      transaction.item   .push(rChild);
-      transaction.parent .push(rTarget);
-      transaction.index  .push(index);
-      transaction.isValid.push(true);
+    var txmgr = BMSVC.transactionManager;
+    txmgr.beginBatch();
+    while (++index <= count) {
+      var rChild = RDFC.RemoveElementAt(index, true);
+      var txn = BMSVC.createTransaction(rTarget, rChild, index, false);
+      txmgr.doTransaction(txn);
     }
-    var isCancelled = !BookmarksUtils.any(transaction.isValid);
-    if (!isCancelled) {
-      BMSVC.transactionManager.doTransaction(transaction);
-      BookmarksUtils.flushDataSource();
-    }    
+    txmgr.endBatch();
+    BookmarksUtils.flushDataSource();
   },
 
   exportBookmarks: function ()
   {
     try {
       const kFilePickerContractID = "@mozilla.org/filepicker;1";
       const kFilePickerIID = Components.interfaces.nsIFilePicker;
       const kFilePicker = Components.classes[kFilePickerContractID].createInstance(kFilePickerIID);
@@ -1313,39 +1309,38 @@ var BookmarksUtils = {
         return true;
     }
     return false;
   },
 
   /////////////////////////////////////////////////////////////////////////////
   removeSelection: function (aAction, aSelection)
   {
-    var transaction     = new BookmarkRemoveTransaction(aAction);
-    transaction.item    = new Array(aSelection.length);
-    transaction.parent  = new Array(aSelection.length);
-    transaction.index   = new Array(aSelection.length);
-    transaction.isValid = BookmarksUtils.isSelectionValidForDeletion(aSelection);
+    var isValid = BookmarksUtils.isSelectionValidForDeletion(aSelection);
+    if (SOUND && !BookmarksUtils.all(isValid))
+      SOUND.beep();
+    if (!BookmarksUtils.any(isValid))
+      return false;
+
+    var txmgr = BMSVC.transactionManager;
+    txmgr.beginBatch();
     for (var i = 0; i < aSelection.length; ++i) {
-      transaction.item  [i] = aSelection.item  [i];
-      transaction.parent[i] = aSelection.parent[i];
-      if (transaction.isValid[i]) {
+      if (isValid[i]) {
         RDFC.Init(BMDS, aSelection.parent[i]);
-        transaction.index[i]  = RDFC.IndexOf(aSelection.item[i]);
+        var index = RDFC.IndexOf(aSelection.item[i]);
+        var txn = BMSVC.createTransaction(aSelection.parent[i], aSelection.item[i], index, true);
+        txmgr.doTransaction(txn);
       }
     }
-    if (SOUND && aAction != "move" && !BookmarksUtils.all(transaction.isValid))
-      SOUND.beep();
-    var isCancelled = !BookmarksUtils.any(transaction.isValid);
-    if (!isCancelled) {
-      BMSVC.transactionManager.doTransaction(transaction);
-      if (aAction != "move")
-        BookmarksUtils.notifyBookmarksChanged();
-        BookmarksUtils.flushDataSource();
+    txmgr.endBatch();
+    if (aAction != "move") {
+      BookmarksUtils.notifyBookmarksChanged();
+      BookmarksUtils.flushDataSource();
     }
-    return !isCancelled;
+    return true;
   },
       // If the current bookmark is the IE Favorites folder, we have a little
       // extra work to do - set the pref |browser.bookmarks.import_system_favorites|
       // to ensure that we don't re-import next time. 
       //if (aSelection[count].getAttribute("type") == (NC_NS + "IEFavoriteFolder")) {
       //  const kPrefSvcContractID = "@mozilla.org/preferences-service;1";
       //  const kPrefSvcIID = Components.interfaces.nsIPrefBranch;
       //  const kPrefSvc = Components.classes[kPrefSvcContractID].getService(kPrefSvcIID);
@@ -1356,60 +1351,61 @@ var BookmarksUtils = {
   {
     Components.classes["@mozilla.org/observer-service;1"]
               .getService(Components.interfaces.nsIObserverService)
               .notifyObservers(null, "bookmarks-changed", "");
   },
 
   insertSelection: function (aAction, aSelection, aTarget)
   {
-    var transaction     = new BookmarkInsertTransaction(aAction);
-    transaction.item    = new Array(aSelection.length);
-    transaction.parent  = new Array(aSelection.length);
-    transaction.index   = new Array(aSelection.length);
-    if (aAction != "move")
-      transaction.isValid = BookmarksUtils.isSelectionValidForInsertion(aSelection, aTarget, aAction);
-    else // isValid has already been determined in moveSelection
-      transaction.isValid = aSelection.isValid;
+    var isValid = BookmarksUtils.isSelectionValidForInsertion(aSelection, aTarget, aAction);
+    if (SOUND && !BookmarksUtils.all(isValid))
+      SOUND.beep();
+    if (!BookmarksUtils.any(isValid))
+      return false;
+
+    var txmgr = BMSVC.transactionManager;
+    txmgr.beginBatch();
     var index = aTarget.index;
-    for (var i=0; i<aSelection.length; ++i) {
-      var rSource = aSelection.item[i];
-      
-      if (transaction.isValid[i]) {
-        if (BMSVC.isBookmarkedResource(rSource))
-          rSource = BMSVC.cloneResource(rSource);
-        transaction.item  [i] = rSource;
-        transaction.parent[i] = aTarget.parent;
-        transaction.index [i] = index++;
-      } else {
-        transaction.item  [i] = rSource;
-        transaction.parent[i] = aSelection.parent[i];
-      } 
+    for (var i = 0; i < aSelection.length; ++i) {
+      if (isValid[i]) {
+        var txn = BMSVC.createTransaction(aTarget.parent, aSelection.item[i], index++, false);
+        txmgr.doTransaction(txn);
+      }
     }
-    if (SOUND && !BookmarksUtils.all(transaction.isValid))
-      SOUND.beep();
-    var isCancelled = !BookmarksUtils.any(transaction.isValid);
-    if (!isCancelled) {
-      BMSVC.transactionManager.doTransaction(transaction);
-      BookmarksUtils.notifyBookmarksChanged();
-      BookmarksUtils.flushDataSource();
-    }
-    return !isCancelled;
+    txmgr.endBatch();
+    BookmarksUtils.notifyBookmarksChanged();
+    BookmarksUtils.flushDataSource();
+    return true;
   },
 
   moveSelection: function (aAction, aSelection, aTarget)
   {
-    aSelection.isValid = BookmarksUtils.isSelectionValidForInsertion(aSelection, aTarget, "move");
-    var transaction = new BookmarkMoveTransaction(aAction, aSelection, aTarget);
-    var isCancelled = !BookmarksUtils.any(transaction.isValid);
-    if (!isCancelled) {
-      BMSVC.transactionManager.doTransaction(transaction);
-    } else if (SOUND)
+    var isValid = BookmarksUtils.isSelectionValidForInsertion(aSelection, aTarget, aAction);
+    if (SOUND && !BookmarksUtils.all(isValid))
       SOUND.beep();
-    return !isCancelled;
+    if (!BookmarksUtils.any(isValid))
+      return false;
+
+    var txmgr = BMSVC.transactionManager;
+    txmgr.beginBatch();
+    var index = aTarget.index;
+    for (var i = 0; i < aSelection.length; ++i) {
+      if (isValid[i]) {
+        RDFC.Init(BMDS, aSelection.parent[i]);
+        var deleteIndex = RDFC.IndexOf(aSelection.item[i]);
+        var deleteTxn = BMSVC.createTransaction(aSelection.parent[i], aSelection.item[i], deleteIndex, true);
+        txmgr.doTransaction(deleteTxn);
+        var insertTxn = BMSVC.createTransaction(aTarget.parent, aSelection.item[i], index++, false);
+        txmgr.doTransaction(insertTxn);
+      }
+    }
+    txmgr.endBatch();
+    BookmarksUtils.flushDataSource();
+    return true;
   }, 
 
   getXferDataFromSelection: function (aSelection)
   {
     if (aSelection.length == 0)
       return null;
     var dataSet = new TransferDataSet();
     var data, item, itemUrl, itemName, parent, name;
@@ -1646,204 +1642,8 @@ var BookmarksUtils = {
     if (!target)
       return;
 
     var rSource   = RDF.GetResource(aEvent.target.id);
     var selection = BookmarksUtils.getSelectionFromResource(rSource);
     BookmarksCommand.openBookmark(selection, target, aDS, aEvent)
   }
 }
-
-
-function BookmarkTransaction()
-{
-}
-
-BookmarkTransaction.prototype = {
-  BATCH_LIMIT : 8,
-  RDFC        : null,
-  BMDS        : null,
-
-  beginUpdateBatch: function()
-  {
-    if (this.item.length > this.BATCH_LIMIT) {
-      this.BMDS.beginUpdateBatch();
-    }
-  },
-
-  endUpdateBatch: function()
-  {
-    if (this.item.length > this.BATCH_LIMIT) {
-      this.BMDS.endUpdateBatch();
-    }
-  },
-
-  // nsITransaction method stubs
-  doTransaction: function() {},
-  undoTransaction: function() {},
-  redoTransaction: function() { this.doTransaction(); },
-  get isTransient() { return false; },
-  merge: function(aTransaction) { return false; },
-
-  // debugging helper
-  get wrappedJSObject() { return this; }
-}
-
-function BookmarkInsertTransaction (aAction)
-{
-  this.type    = "insert";
-  this.action  = aAction;
-  this.item    = null;
-  this.parent  = null;
-  this.index   = null;
-  this.isValid = null;
-}
-
-BookmarkInsertTransaction.prototype =
-{
-  __proto__: BookmarkTransaction.prototype,
-
-  doTransaction: function ()
-  {
-    this.beginUpdateBatch();
-    for (var i=0; i<this.item.length; ++i) {
-      if (this.isValid[i]) {
-        this.RDFC.Init(this.BMDS, this.parent[i]);
-        this.RDFC.InsertElementAt(this.item[i], this.index[i], true);
-      }
-    }
-    this.endUpdateBatch();
-  },
-
-  undoTransaction: function ()
-  {
-    this.beginUpdateBatch();
-    // XXXvarga Can't use |RDFC| here because it's being "reused" elsewhere.
-    var container = Components.classes[kRDFCContractID].createInstance(kRDFCIID);
-    for (var i=this.item.length-1; i>=0; i--) {
-      if (this.isValid[i]) {
-        container.Init(this.BMDS, this.parent[i]);
-        container.RemoveElementAt(this.index[i], true);
-      }
-    }
-    this.endUpdateBatch();
-  }
-
-}
-
-function BookmarkRemoveTransaction (aAction)
-{
-  this.type    = "remove";
-  this.action  = aAction;
-  this.item    = null;
-  this.parent  = null;
-  this.index   = null;
-  this.isValid = null;
-}
-
-BookmarkRemoveTransaction.prototype =
-{
-  __proto__: BookmarkTransaction.prototype,
-
-  doTransaction: function ()
-  {
-    this.beginUpdateBatch();
-    for (var i=0; i<this.item.length; ++i) {
-      if (this.isValid[i]) {
-        this.RDFC.Init(this.BMDS, this.parent[i]);
-        this.RDFC.RemoveElementAt(this.index[i], false);
-      }
-    }
-    this.endUpdateBatch();
-  },
-
-  undoTransaction: function ()
-  {
-    this.beginUpdateBatch();
-    for (var i=this.item.length-1; i>=0; i--) {
-      if (this.isValid[i]) {
-        this.RDFC.Init(this.BMDS, this.parent[i]);
-        this.RDFC.InsertElementAt(this.item[i], this.index[i], false);
-      }
-    }
-    this.endUpdateBatch();
-  }
-
-}
-
-function BookmarkMoveTransaction (aAction, aSelection, aTarget)
-{
-  this.type      = "move";
-  this.action    = aAction;
-  this.selection = aSelection;
-  this.target    = aTarget;
-  this.isValid   = aSelection.isValid;
-}
-
-BookmarkMoveTransaction.prototype =
-{
-  __proto__: BookmarkTransaction.prototype,
-
-  beginUpdateBatch: function()
-  {
-    if (this.selection.length > this.BATCH_LIMIT) {
-      this.BMDS.beginUpdateBatch();
-    }
-  },
-
-  endUpdateBatch: function()
-  {
-    if (this.selection.length > this.BATCH_LIMIT) {
-      this.BMDS.endUpdateBatch();
-    }
-  },
-
-  doTransaction: function ()
-  {
-    this.beginUpdateBatch();
-    BookmarksUtils.removeSelection("move", this.selection);
-    BookmarksUtils.insertSelection("move", this.selection, this.target);
-    this.endUpdateBatch();
-  },
-
-  redoTransaction     : function () {}
-
-}
-
-function BookmarkImportTransaction (aAction)
-{
-  this.type    = "import";
-  this.action  = aAction;
-  this.item    = [];
-  this.parent  = [];
-  this.index   = [];
-  this.isValid = [];
-}
-
-BookmarkImportTransaction.prototype =
-{
-  __proto__: BookmarkTransaction.prototype,
-
-  undoTransaction: function ()
-  {
-    this.beginUpdateBatch();
-    for (var i=this.item.length-1; i>=0; i--) {
-      if (this.isValid[i]) {
-        this.RDFC.Init(this.BMDS, this.parent[i]);
-        this.RDFC.RemoveElementAt(this.index[i], true);
-      }
-    }
-    this.endUpdateBatch();
-  },
-   
-  redoTransaction: function ()
-  {
-    this.beginUpdateBatch();
-    for (var i=0; i<this.item.length; ++i) {
-      if (this.isValid[i]) {
-        this.RDFC.Init(this.BMDS, this.parent[i]);
-        this.RDFC.InsertElementAt(this.item[i], this.index[i], true);
-      }
-    }
-    this.endUpdateBatch();
-  }
-
-}
--- a/suite/common/bookmarks/bookmarksTree.xml
+++ b/suite/common/bookmarks/bookmarksTree.xml
@@ -359,38 +359,31 @@
             }
 
             selection.selectEventsSuppressed = false;
           }
         ]]></body>
       </method>
 
       <field name="_itemToBeToggled">  []</field>
-      <field name="_parentToBeToggled">[]</field>
       <method name="preUpdateTreeSelection">
         <parameter name="aTxn"/>
         <body><![CDATA[
-          aTxn = aTxn.wrappedJSObject;
-          var type = aTxn.type;
-          // Skip transactions that aggregates nested "insert" or "remove" transactions.
-          if (type != "insert" && type != "remove")
-            return;
-          for (var i=0; i<aTxn.item.length; ++i) {
-            this._itemToBeToggled  .push(aTxn.item  [i]);
-            this._parentToBeToggled.push(aTxn.parent[i]);
-          }
+          // aTxn could be null, or possibly some third-party transaction?
+          if (aTxn instanceof Components.interfaces.nsIInterfaceRequestor)
+            this._itemToBeToggled.push(aTxn.getInterface(Components.interfaces.nsIRDFNode));
         ]]></body>
       </method>
 
       <method name="updateTreeSelection">
         <body><![CDATA[
           this.treeBoxObject.view.selection.selectEventsSuppressed = true;
           this.treeBoxObject.view.selection.clearSelection();
           for (var i=0; i<this._itemToBeToggled.length; ++i) {
-            index = this.treeBuilder.getIndexOfResource(this._itemToBeToggled[i]);
+            var index = this.treeBuilder.getIndexOfResource(this._itemToBeToggled[i]);
             if (index >=0)
               this.treeBoxObject.view.selection.toggleSelect(index);
           }
           this.treeBoxObject.view.selection.selectEventsSuppressed = false;
         ]]></body>
       </method>
 
       <method name="createTreeContextMenu">
@@ -686,35 +679,32 @@
       <!-- nsITransactionManager listener -->
       <field name="transactionListener"><![CDATA[
       ({
 
         mOuter: this,
 
         willDo: function (aTxmgr, aTxn) {
           this.mOuter._itemToBeToggled   = [];
-          this.mOuter._parentToBeToggled = [];
         },
 
         didDo: function (aTxmgr, aTxn) {
           this.mOuter.preUpdateTreeSelection(aTxn);
         },
 
         willUndo: function (aTxmgr, aTxn) {
           this.mOuter._itemToBeToggled   = [];
-          this.mOuter._parentToBeToggled = [];
         },
 
         didUndo: function (aTxmgr, aTxn) {
           this.mOuter.preUpdateTreeSelection(aTxn);
         },
 
         willRedo: function (aTxmgr, aTxn) {
           this.mOuter._itemToBeToggled   = [];
-          this.mOuter._parentToBeToggled = [];
         },
 
         didRedo: function (aTxmgr, aTxn) {
           this.mOuter.preUpdateTreeSelection(aTxn);
         },
 
         didMerge       : function (aTxmgr, aTxn) {},
         didBeginBatch  : function (aTxmgr, aTxn) {},