Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#15858 closed defect (fixed)

Tree: throw error: "this.domNode is null" when deleting ancestor of selected node

Reported by: Peter Jekel Owned by: bill
Priority: undecided Milestone: 1.9
Component: Dijit Version: 1.8.0
Keywords: Cc:
Blocked By: Blocking:

Description

A multi-parented tree throws an error "this.domNode is null" when a child is selected and the parent is than deleted.

See attachments and demo program for a detailed description.

Attachments (5)

tree_bug.html (2.8 KB) - added by Peter Jekel 7 years ago.
Program demonstrating the problem.
tree_bug.txt (3.2 KB) - added by Peter Jekel 7 years ago.
Detailed problem description and root cause + suggested fixes.
tree_bug_02.html (3.6 KB) - added by Peter Jekel 7 years ago.
Demostrate onSetChildItem() defect.
tree_bug_02.2.html (3.6 KB) - added by Peter Jekel 7 years ago.
setChildItems() defect analysis
tree_bug_02.txt (5.1 KB) - added by Peter Jekel 7 years ago.
setChildItems() root cause analysis

Download all attachments as: .zip

Change History (19)

Changed 7 years ago by Peter Jekel

Attachment: tree_bug.html added

Program demonstrating the problem.

Changed 7 years ago by Peter Jekel

Attachment: tree_bug.txt added

Detailed problem description and root cause + suggested fixes.

comment:1 Changed 7 years ago by bill

Summary: Dijit Tree throw error: "this.domNode is null"Tree: throw error: "this.domNode is null" when deleting ancestor of selected node

Thanks for the detailed test case and suggested fix. I'll take a look. Does this really have anything to do with multi-selection? From your description it just sounds like an issue with deleting an ancestor of the selected node.

See also #14443 and #6236.

comment:2 Changed 7 years ago by Peter Jekel

No, the issue is, IMHO, not related to multi-select. The issue is caused by the fact that whenever a parent node is deleted no check is peformed to see if any of its children is a selected item and therefore any selected child remains selected in the context of the _dndSelector. The problem only reveals itself, in the small demo I submitted, when parent C is placed on the tree more than once.

The easiest way of fixing the problem is to modify _dndSelector.getSelectedTreeNodes() as I suggested because it is going to be difficult to check each child when the parent is being deleted just because the node is being destoyed by calling node.destroyRecursive() which is handled by _widgetbase and thus outside the scope of the tree and _dndSelector. Just have _dndSelector.getSelectedTreeNode() verify if any selected node is still valid, if not, deleted it from the list of currently selected items.

I propose the same approach for the second issue I listed with the tree itself (setChildItems()). The fact that a node exists on the 'existingNodes' list does not guarentee the node is still valid when re-used. The current statement:

if(existingNodes[i] && !existingNodes[i].getParent()){
}

will always throw an error if the existingNode has not domNode (e.g. is destroyed), because a domNode is required when calling getParent().

Hope this explains it a little more.

Last edited 7 years ago by Peter Jekel (previous) (diff)

comment:3 Changed 7 years ago by bill

Ah right, makes sense, thanks.

OK, I will do that. I'll need to add an automated test case; I think it can be done without doh/robot, just by calling set("path", ...) before the store.deleteItem().

Anyway, thanks for tracking all that down.

Changed 7 years ago by Peter Jekel

Attachment: tree_bug_02.html added

Demostrate onSetChildItem() defect.

Changed 7 years ago by Peter Jekel

Attachment: tree_bug_02.2.html added

setChildItems() defect analysis

comment:4 Changed 7 years ago by Peter Jekel

Bill,

I have added one more demo program, tree_bug_02.html, to demonstrate the setChildItems() defect I listed as the secod issue in document tree_bug.txt. I also included the root cause analysis. Feel free if you want to handle this one as a separate defect.

Changed 7 years ago by Peter Jekel

Attachment: tree_bug_02.txt added

setChildItems() root cause analysis

comment:5 Changed 7 years ago by bill

In [29552]:

remove descendants of destroyed TreeNode? from selection, refs #15858 !strict.

comment:6 Changed 7 years ago by bill

About the second issue, there's code in Tree that does:

// Deregister mapping from item id --> this node

and I agree that code should deregister mapping from item id --> the descendants of "this node" too.

The thing that worries me about your test_bug_02.html test case though is that you are deleting C and then immediately adding back C-1, with no delay in between. Ideally this should work, but Tree has some funky code that delays processing of deletes via a this.defer() call:

// All the old children of this TreeNode are subject for destruction if
//              1) they aren't listed in the new children array (items)
//              2) they aren't immediately adopted by another node (DnD)
this.defer(function(){
        array.forEach(oldChildren, function(node){
                if(!node._destroyed && !node.getParent()){
                        // If node is in selection then remove it.
                        tree.dndController.removeTreeNode(node);

                        // Deregister mapping from item id --> this node
                        var id = model.getIdentity(node.item),
                                ary = tree._itemNodesMap[id];
                        if(ary.length == 1){
                                delete tree._itemNodesMap[id];
                        }else{
                                var index = array.indexOf(ary, node);
                                if(index != -1){
                                        ary.splice(index, 1);
                                }
                        }

                        // And finally we can destroy the node
                        node.destroyRecursive();
                }
        });
});

As the comment alludes to, the reason for the delay is to support DnD: dragging a node "C" from one parent to another is reported as a delete followed by an add, but we only want to process the store.remove() as a real deletion if it isn't followed by a store.put().

Also note that your detailed description mentions _onItemDelete() but that's essentially a deprecated method, since it's not used with dojo.store, but only with the deprecated dojo.data.

comment:7 Changed 7 years ago by bill

In [29553]:

remove descendants of destroyed TreeNode? from _itemNodesMap[], refs #15858 !strict.

comment:8 Changed 7 years ago by bill

Milestone: tbd1.9
Resolution: fixed
Status: newclosed

OK, so those two checkins are a bit different than what you suggested, but I think they fix the problems you described. Thanks for the detailed explanations and code analysis!

Originally I was going to checkin this fix to the 1.8/ branch, but it needs a little time to bake, so I'll just put it in trunk for now.

Like I said, there's still an issue with race conditions, which I filed as #15893.

comment:9 Changed 7 years ago by Peter Jekel

Bill,

The code you listed (introduced in 1.8) is, in my opinion, just a band-aid, to fix only the most obvious case and that is an immediate child was removed. It is a work-around for the underlying problem and does not handle any grand children that may have been removed (issue 2). This is also why it doesn't matter if the code snippet listed is deferred or not, you could wait for an hour but you still get the same error.

Both issues find their origin in the order in which the ItemFileWrite? store fires events and how the model handles them. Whenever an item is deleted from the store the store fires an onSet() event for the parent items "children" property BEFORE the onDelete() event is fired. From the store perspective this is perfectly fine as it is independent of any model.

The store model takes the initial onSet("children", ..) event and turn it into a onChildrenChange() event for the tree and subsequently fire a onDelete() event for the tree and thus the tree will receive the onChildrenChange() event BEFORE the onDelete() event.

Now when tree._onItemDelete() is called it will destroy the tree node and all its descendents BUT it only removes the deleted item from the _itemNodesMap[] table and NOT any of its destroyed descendents. As a result, any subsequent call to setChildItems() will operate on an out of sync _itemNodesMap[] table.

The following code will fix the problem and also render the earlier proposed fix/work-around for _dndSelector obsolete (every descendent is now also removed from the dndSelector). In addition it would make the 'band-aid' code you listed obsolete and will also resolve issue #15893

  _removeNode: function (node) {
    // summary:
    //    Remove a tree node and all of its descendants.
    // node:
    //    TreeNode instance
    // tag:
    //    private
    var identity = this.model.getIdentity(node.item);
    var nodeList = this._itemNodesMap[identity];
    var self     = this;

    function removeNodeMapping(node) {
      // summary:
      //    Remove node from the internal item -> nodes mapping table;
      // node:
      //    TreeNode instance
      // tag:
      //    protected
      var identity = self.model.getIdentity(node.item);
      var nodeList = self._itemNodesMap[identity];
      var index = array.indexOf(nodeList, node);
      if (index != -1) {
        nodeList.splice(index, 1);
      }
      if (!nodeList.length) {
        delete self._itemNodesMap[identity];
      }
      // Remove any paths for the node....
      if (node.isExpanded) {
        self._state(node, false);
      }
    }

    function removeChildren(node) {
      // summary:
      //    Remove all descendants of the node from the current selection and
      //    mapping table.
      // node:
      //    TreeNode instance
      // tag:
      //    protected
      array.forEach(node.getChildren(), function(childNode) {
        self.dndController.removeTreeNode(childNode);
        removeNodeMapping(childNode);
        removeChildren(childNode);    // Recursive...
      });
    }
    
    // Remove node and all its descendents from the mapping table
    // and dndSelector.
    this.dndController.removeTreeNode(node);
    removeNodeMapping(node);
    removeChildren(node);
    var parent = node.getParent();
    if(parent){
      parent.removeChild(node);
    }
    // Finally, destroy DOM node and its descendants
    node.destroyRecursive(); 
    
  },

  _onItemDelete: function(/*Item*/ item){
    // summary:
    //    Processes notification of a deletion of an item.
    // item:
    //    (Store) item.
    // tag:
    //    Private
    var identity = this.model.getIdentity(item);
    var nodes    = this._itemNodesMap[identity];
    var nodeList = [];

    // Clone nodes list so we won't iterate a list that may be altered 
    // by _removeNode().
    for(i=0; i<nodes.length;i++) {
      nodeList.push(nodes[i]);
    }
    if(nodeList){
      array.forEach(nodeList, this._removeNode, this);
    }
  },

For dijit 2.0 however, I would suggest to remove onChildrenChange() event from the store models and instead have the model only pass the onNew() and onDelete() events from the store as there is no added benefit to the onChildrenChange() event. It only complicates things by handling events out of order.

Hope this helps a little.

Version 3, edited 7 years ago by Peter Jekel (previous) (next) (diff)

comment:10 Changed 7 years ago by bill

Support for dojo.data (including ItemFileWriteStore ) is deprecated; the important thing is for dojo.store to work correctly. As far as I know, after my change, dojo.store is working perfectly (grandchildren, great-granchildren, whatever), excluding that timing issue I mentioned.

If you think something is still broken can you attach a test case using the Memory store (or any dojo.store)?

comment:11 Changed 7 years ago by Peter Jekel

I took a quick look at your 1.9 implementation and I have one final comment:

Your function remove() in setChildItems() must still call tree._state(node, false) in order to remove the path of the node and as a result remove it from the cookie.

comment:12 Changed 7 years ago by bill

That makes sense, thanks, I'll check that in too. Although, it won't help the case when a data store item is deleted when the Tree doesn't exist (ex: when the user doesn't have their browser open, or it's open to a different page).

comment:13 Changed 7 years ago by bill

Actually, I suppose that would fail in some obscure cases, like:

  1. Display Tree with data like below and expand all nodes:
A
    B
        C
            D
                 E
  1. Close node "B", so display looks like
A
     B

but cookie (listing expanded nodes) contains [A, C, D].

  1. Destroy and recreate Tree
  1. Delete node B

The above algorithm won't remove "C" or "D" from the cookie, although ideally it should.

So, probably a better algorithm is to loop through all elements in the cookie, removing any that contain the deleted node as part of their path.

comment:14 Changed 7 years ago by bill

In [29572]:

Cleanup Tree's persistence cookie when data store items are deleted. Not sure if it's worth it though, because it only works if the Tree exists and is listening to data store updates (specifically, monitoring the children of the parent of the deleted item). Refs #15858 !strict.

Note: See TracTickets for help on using tickets.