[regression] Under IE, dom-construct.destroy fails in infinite loop when used with an <object> element.

[Patrick Ruzand]

IE9/Quirks or IE8 (all modes).

If an <object> element contains <param> children (like a flash or silverlight plugin), dom-construct.destroy yields to infinite loop.

The reason seems to be due to the removeNode() call that seems to do nothing when used on <param> nodes:

	function _destroy(/*DomNode*/ node, /*DomNode*/ parent){
		if(node.firstChild){
			_empty(node);
		}
		if(parent){
			// removeNode(false) doesn't leak in IE 6+, but removeChild() and removeNode(true) are known to leak under IE 8- while 9+ is TBD
			has("ie") && 'removeNode' in node ? node.removeNode(false) : parent.removeChild(node);
		}
	}

Because of this, the _empty() method endlessly loops (node.lastChild is always truthy):

		function(/*DomNode*/ node){
			try{
				node.innerHTML = ""; // really fast when it works
			}catch(e){ // IE can generate Unknown Error
				for(var c; c = node.lastChild;){ // intentional assignment
					_destroy(c, node); // destroy is better than removeChild so TABLE elements are removed in proper order
				}
			}
		} :

attached a testcase that demonstrates the problem. It contains 2 objects (a flash and a silverlight plugin) which both fail.

[Patrick Ruzand]

testcase

[bill]

I suppose this is a regression? 1.8 just called removeChild().

[Douglas Hays]

In IE quirks mode, OBJECT nodes have canHaveChildren=false and removeNode does nothing but lastChild is NOT null, and removeChild removes children.

[Douglas Hays]

SVG nodes can also leak in strict mode since _empty(node) executes
node.innerHTML = "";
which does nothing (innerHTML isn't supported so this just adds an additional attribute) and thus doesn't remove children.

[Douglas Hays]

possible fix for review

[Douglas Hays]

pruzand, can you please test/review the attached patch?

[bill]

I might be misunderstand this, but ISTM setting innerHTML won't work for SVG nodes on any browser. In other words, after your patch isn't _empty() still broken for SVG nodes on chrome and firefox?

The other thing I noticed is that maybe your "feature test" of node.canHaveChildren would be more straightforward as "innerHTML" in node?

[Patrick Ruzand]

doughays, your patch fixes the issue gfx was facing.

[Douglas Hays]

bill, it does seem that destroy(svgelement) should be leaking on all browsers since innerHTML="" is not clearing the children of SVG/strict parent elements.
As far as the feature test on IE, there are 4 conditions:

canHaveChildren  innerHTML   result
    undefined    undefined   must iterate child nodes
      false      readOnly    must iterate child nodes
      true       readOnly    must iterate child nodes
      true       readwrite   use innerHTML=""

The canHaveChildren feature test takes care of the first 2 conditions. The try/catch takes care of the second 2 conditions.

[Douglas Hays]

Presumably the _empty() method's non-IE code branch needs to be enhanced for SVG (i.e. non-HTMLElement subclasses). The 2 more obvious choices are to:
1) leave the has("ie") feature test and pass and extra node parameter to the helper _destroyEachChild function,

var _destroyEachChild = function(/*DomNode*/ node){
        for(var c; c = node.lastChild;){ // intentional assignment
                _destroy(c, node); // destroy is better than removeChild so TABLE subelements are removed in proper order
        }
};

var _empty = has("ie") ?
        function(/*DomNode*/ node){
                if(node.canHaveChildren){
                        try{
                                node.innerHTML = "";
                        }catch(e){
                                // IE can generate an exception when innerHTML is readOnly (e.g. TABLE (sub)elements in quirks mode)
                                _destroyEachChild(node);
                        }
                }else{
                        // SVG/strict elements don't support innerHTML/canHaveChildren, and OBJECT/APPLET elements in quirks node have canHaveChildren=false
                        _destroyEachChild(node);
                }
        } :
        function(/*DomNode*/ node){
                if(node.canHaveChildren){
                        node.innerHTML = "";
                }else{
                        _destroyEachChild(node); // SVG
                }
        };

2) remove the has("ie") test and leave try/catch in for all browsers as a safety net

function _empty(/*DomNode*/ node){
        function _destroyEachChild = function(){
                for(var c; c = node.lastChild;){ // intentional assignment
                        _destroy(c, node); // destroy is better than removeChild so TABLE subelements are removed in proper order
                }
        }
        if(node.canHaveChildren){
                try{
                        node.innerHTML = "";
                }catch(e){
                        // innerHTML is readOnly (e.g. TABLE (sub)elements in IE/quirks mode)
                        _destroyEachChild();
                }
        }else{
                // SVG/strict elements don't support innerHTML/canHaveChildren, and OBJECT/APPLET elements in IE/quirks node have canHaveChildren=false
                _destroyEachChild();
        }
}

I like option 2 since it's smaller and removes the ie feature test and has an added safety net if innerHTML fails on some future browser with some unknown non-HTMLElement tag.

[bill]

Option 2 makes sense to me too. You could even shave some bytes if you did like below, although I'm not sure if that's considered good programming or not. I guess it's OK.

function _empty(/*DomNode*/ node){
        if(node.canHaveChildren){
                try{
                        // fast path
                        node.innerHTML = "";
                        return;
                }catch(e){
                        // innerHTML is readOnly (e.g. TABLE (sub)elements in IE/quirks mode).  Fall through.
                }
        }
        // SVG/strict elements don't support innerHTML/canHaveChildren, and OBJECT/APPLET elements in IE/quirks node have canHaveChildren=false
        for(var c; c = node.lastChild;){ // intentional assignment
              _destroy(c, node); // destroy is better than removeChild so TABLE subelements are removed in proper order
        }
}

[Douglas Hays]

Thanks bill. I really like your compacted version. I don't see any downside for any browser.

[Douglas Hays]

In [31197]:

Fixes #16975. Remove leak when calling empty() on SVG elements (all browsers). On IE, use removeChild instead of removeNode on SVG elements since they don't inherit removeNode from HTMLElement and SVG/removeNode is incorrectly implemented. Backport thru 1.6. Add additional strict and quirks automated tests to make sure empty() and destroy() are working correctly with SVG and OBJECT nodes, and since these element's behavior seem dependent on the document's compatMode. !strict