This project is archived and is in readonly mode.

#346 ✓invalid
Paul Spencer

$unlink infinite recursion

Reported by Paul Spencer | August 29th, 2008 @ 08:22 PM | in 2.0 (closed)

If you attempt to pass an object which is a DOM element to $merge, it results in an infinite loop if that element has child nodes because the child nodes have a parentNode reference back to the parent.

Comments and changes to this ticket

  • Paul Spencer

    Paul Spencer August 29th, 2008 @ 08:31 PM

    or with anything else that contains a circular reference in fact. I have a set of things that get passed around and they have a reference back to the set, causing the same problem.

  • Paul Spencer

    Paul Spencer October 1st, 2008 @ 04:59 PM

    • Title changed from “$merge/$unlink infinite loop with DOM elements” to “$unlink infinite recursion”
    • Tag changed from core, defect to 12, defect, setoptions, unlink

    I managed to reduce this to a simple set of code that causes the infinite recursion. It has nothing to do with DOM elements, just circular references.

    window.addEvent('load', function(){

    var a = {};
    var b = {};
    b.a = a;
    a.b = b;
    new C({a:a});
    
    

    }); var C = new Class({ Implements: [Options], options: {

       a: null
    
    

    }, initialize: function(options){

       this.setOptions(options);
    
    

    } });

  • Paul Spencer

    Paul Spencer October 1st, 2008 @ 05:01 PM

    sorry for the poor formatting before:

    
    window.addEvent('load', function(){
        var a = {};
        var b = {};
        b.a = a;
        a.b = b;
        new C({a:a});
    });
    var C = new Class({
       Implements: [Options],
       options: {
           a: null
       },
       initialize: function(options){
           this.setOptions(options);
       }
    });
    
  • magmoro
  • Paul Spencer

    Paul Spencer October 2nd, 2008 @ 12:01 PM

    Yes, poor architecture in my code probably.

    I had originally thought that this occurred when passing a DOM node as an option because the parentNode of the node has a childNodes array that refers to node creating this circular reference but I cannot reproduce this in a simple test so I expect that my node had a direct reference to an object rather than having used store/retrieve.

    I don't mind if this gets closed invalid or won't fix, but at least it will be here for others that run into this.

  • Tom Occhino

    Tom Occhino October 2nd, 2008 @ 03:09 PM

    • State changed from “new” to “invalid”

    Thanks for all the research Paul, if it's okay i'll close this as invalid because circular references (ass described by magmoro) really are bad practice. $unlink is not likely the only place that will cause you problems with a setup like that.

  • Paul Spencer

    Paul Spencer October 2nd, 2008 @ 03:24 PM

    No problem! I have been working around this with some ugly hacks around setOptions, I will try to go back and fix my architecture to avoid the problem altogether.

  • dnolen

    dnolen March 26th, 2009 @ 09:31 PM

    This ticket should probably be reopened as this affects object (not DOM) circular references which, in a moderately complex application is to be expected. We're running into this problem with our quite complex UI for MoMA.org. We don't hack around setOptions. We fix the problem at it's source: $unlink.

    The problem is simple to patch by maintaining a stack of visited objects in a recursive $unlink descent. As most object hierarchies really aren't that deep this isn't much of a performance hit.

    
    
    var visited = []; // FIX: for infinite unlink loops - David
    function $unlink(object){
    	var unlinked;
    	
      // prevent infinite loops in unlinking, push the object onto the visited stack
    	if(!visited.contains(object))
    	{
    	  visited.push(object);	  
    	}
    	else
    	{
    	  return object;
    	}
    	
    	switch ($type(object)){
    		case 'object':
    			unlinked = {};
    			for (var p in object) unlinked[p] = $unlink(object[p]);
    		break;
    		case 'hash':
    			unlinked = $unlink(object.getClean());
    		break;
    		case 'array':
    			unlinked = [];
    			for (var i = 0, l = object.length; i < l; i++) unlinked[i] = $unlink(object[i]);
    		break;
    		default: return object;
    	}
    	
    	// pop the object off the visited stack
    	visited.pop();
    	
    	return unlinked;
    };
    
    

    Thanks.

  • Paul Spencer

    Paul Spencer March 27th, 2009 @ 01:30 PM

    the proposed patch does not quite work as expected, I tried to drop it in as a replacement for the existing $unlink and get an error that visited.contains is not a function. Adding the patch to the page after loading mootools does work. To make this work as a patch for mootools proper, the visited array needs to be declared after the Array.implement code and $unlink needs to add a check if ($defined(visited)) around the two modifications so if $unlink is called before the visited array is declared it will work as before.

  • kasi

    kasi March 27th, 2009 @ 02:17 PM

    To get rid of the $unlink problem i do the following:

    
    Class.Mutators.Family = function(self,name){
            self.$family = {'name':name}
            return self;
        }
    
    MyClass = new Class({
        Family: 'MyClass',
        Extends: SomeClass,
        initialize:
        .....
    
    

    Because $family is used internally, it might break in future versions. Maybe a core developer could give some information if this will break in next version.

    KASI

  • Paul Spencer

    Paul Spencer April 1st, 2009 @ 03:19 AM

    I implemented KASI's suggestion in JxLib and it completely fixed my problem. Its a very clean 'mootools' solution to the problem. The reason it works is because $unlink only operates on things that return 'object', 'array' or 'hash' from $type and $type uses the $family attribute if available to determine the type. By providing a $family attribute, this effectively tells $unlink not to ... um ... unlink it :) The use of Class.Mutators.Family isn't strictly necessary as you could just use $family: 'MyClass' but the mutator centralizes the code making it easier to change if something in MooTools changes and makes the code look a little cleaner.

  • Marc Weber

    Marc Weber July 29th, 2010 @ 10:14 PM

    • Milestone order changed from “0” to “0”

    That's what I came up with based on dnolen suggestion:

    var path = [];
    function $unlink(object){
            var type = $type(object)
    
            var guard = type == 'object' || type == 'hash' || type == 'array';
    
            if (guard){
              var idx = 0;
              for (i=0; i < path.length; i++){
                    if (path[i] == object){
                      return path[i];
                    }
              }
              path.push(object);
            }
            var unlinked;
            switch (type){
                    case 'object':
                            unlinked = {};
                            for (var p in object) unlinked[p] = $unlink(object[p]);
                    break;
                    case 'hash':
                            unlinked = new Hash(object);
                    break;
                    case 'array':
                            unlinked = [];
                            for (var i = 0, l = object.length; i < l; i++) unlinked[i] = $unlink(object[i]);
                    break;
                    default: unlinked = object;
            }
            if (guard){
              path.pop();
            }
            return unlinked;
    };
    

    In contrast to the previous solution it returns unlinked object if a object is visited the second time. Neither does it push primitive types such as int, string.
    For now it seem to work for me.

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

Shared Ticket Bins

Referenced by

Pages