Bug 777005 - Fix Bookmarks and history menus behave incorrectly due to non-node weak map keys (Port Bug 760940) r=Neil a=Callek
authorFrank Wein <bugzilla@mcsmurf.de>
Fri, 10 Aug 2012 13:54:00 -0400
changeset 12565 4519d9baf3f4725a50e99ef7533967858e13e37b
parent 12564 97c9fb2486dae008639c3ab6beaf6ec70769c4c0
child 12566 194320465afc78c4be0e79a2356717c73a029040
push id650
push userCallek@gmail.com
push dateWed, 22 Aug 2012 21:32:19 +0000
treeherdercomm-beta@194320465afc [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersNeil, Callek
bugs777005, 760940
Bug 777005 - Fix Bookmarks and history menus behave incorrectly due to non-node weak map keys (Port Bug 760940) r=Neil a=Callek
suite/common/history/treeView.js
suite/common/places/browserPlacesViews.js
suite/common/places/controller.js
suite/common/places/treeView.js
--- a/suite/common/history/treeView.js
+++ b/suite/common/history/treeView.js
@@ -121,20 +121,29 @@ PlacesTreeView.prototype = {
    * @return aNode's row if it's in the rows list or if aForceBuild is set, -1
    *         otherwise.
    */
   _getRowForNode:
   function PTV__getRowForNode(aNode, aForceBuild, aParentRow, aNodeIndex) {
     if (aNode == this._rootNode)
       throw "The root node is never visible";
 
-    let ancestors = PlacesUtils.nodeAncestors(aNode);
-    for (let ancestor in ancestors) {
+    // A node is removed form the view either if it has no parent or if its
+    // root-ancestor is not the root node (in which case that's the node
+    // for which nodeRemoved was called).
+    let ancestors = [x for each (x in PlacesUtils.nodeAncestors(aNode))];
+    if (ancestors.length == 0 ||
+        ancestors[ancestors.length - 1] != this._rootNode) {
+      throw new Error("Removed node passed to _getRowForNode");
+    }
+
+    // Ensure that the entire chain is open, otherwise that node is invisible.
+    for (let ancestor of ancestors) {
       if (!ancestor.containerOpen)
-        throw "Invisible node passed to _getRowForNode";
+        throw new Error("Invisible node passed to _getRowForNode");
     }
 
     // Non-plain containers are initially built with their contents.
     let parent = aNode.parent;
     let parentIsPlain = this._isPlainContainer(parent);
     if (!parentIsPlain) {
       if (parent == this._rootNode)
         return this._rows.indexOf(aNode);
@@ -816,17 +825,17 @@ PlacesTreeView.prototype = {
     if (this._result) {
       this._result.removeObserver(this);
       this._rootNode.containerOpen = false;
     }
 
     if (val) {
       this._result = val;
       this._rootNode = val.root;
-      this._cellProperties = new WeakMap();
+      this._cellProperties = new Map();
     }
     else if (this._result) {
       delete this._result;
       delete this._rootNode;
       delete this._cellProperties;
     }
 
     // If the tree is not set yet, setTree will call finishInit.
--- a/suite/common/places/browserPlacesViews.js
+++ b/suite/common/places/browserPlacesViews.js
@@ -63,17 +63,17 @@ PlacesViewBase.prototype = {
 
     if (this._rootElt.localName == "menupopup")
       this._rootElt._built = false;
 
     this._result = val;
     if (val) {
       this._resultNode = val.root;
       this._rootElt._placesNode = this._resultNode;
-      this._domNodes = new WeakMap();
+      this._domNodes = new Map();
       this._domNodes.set(this._resultNode, this._rootElt);
 
       // This calls _rebuild through invalidateContainer.
       this._resultNode.containerOpen = true;
     }
     else {
       this._resultNode = null;
       delete this._domNodes;
--- a/suite/common/places/controller.js
+++ b/suite/common/places/controller.js
@@ -92,17 +92,17 @@ InsertionPoint.prototype = {
  * Places Controller
  */
 
 function PlacesController(aView) {
   this._view = aView;
   XPCOMUtils.defineLazyServiceGetter(this, "clipboard",
                                      "@mozilla.org/widget/clipboard;1",
                                      "nsIClipboard");
-  this._cachedLivemarkInfoObjects = new WeakMap();
+  this._cachedLivemarkInfoObjects = new Map();
 }
 
 PlacesController.prototype = {
   /**
    * The places view.
    */
   _view: null,
 
--- a/suite/common/places/treeView.js
+++ b/suite/common/places/treeView.js
@@ -138,22 +138,31 @@ PlacesTreeView.prototype = {
    * container, this method will just return a calculated row value, without
    * making assumptions on existence of the node at that position.
    * @return aNode's row if it's in the rows list or if aForceBuild is set, -1
    *         otherwise.
    */
   _getRowForNode:
   function PTV__getRowForNode(aNode, aForceBuild, aParentRow, aNodeIndex) {
     if (aNode == this._rootNode)
-      throw "The root node is never visible";
+      throw new Error("The root node is never visible");
 
-    let ancestors = PlacesUtils.nodeAncestors(aNode);
-    for (let ancestor in ancestors) {
+    // A node is removed from the view either if it has no parent or if its
+    // root-ancestor is not the root node (in which case that's the node
+    // for which nodeRemoved was called).
+    let ancestors = [x for each (x in PlacesUtils.nodeAncestors(aNode))];
+    if (ancestors.length == 0 ||
+        ancestors[ancestors.length - 1] != this._rootNode) {
+      throw new Error("Removed node passed to _getRowForNode");
+    }
+
+    // Ensure that the entire chain is open, otherwise that node is invisible.
+    for (let ancestor of ancestors) {
       if (!ancestor.containerOpen)
-        throw "Invisible node passed to _getRowForNode";
+        throw new Error("Invisible node passed to _getRowForNode");
     }
 
     // Non-plain containers are initially built with their contents.
     let parent = aNode.parent;
     let parentIsPlain = this._isPlainContainer(parent);
     if (!parentIsPlain) {
       if (parent == this._rootNode)
         return this._rows.indexOf(aNode);
@@ -1066,17 +1075,17 @@ PlacesTreeView.prototype = {
     if (this._result) {
       this._result.removeObserver(this);
       this._rootNode.containerOpen = false;
     }
 
     if (val) {
       this._result = val;
       this._rootNode = val.root;
-      this._cellProperties = new WeakMap();
+      this._cellProperties = new Map();
     }
     else if (this._result) {
       delete this._result;
       delete this._rootNode;
       delete this._cellProperties;
     }
 
     // If the tree is not set yet, setTree will call finishInit.