Opened 6 years ago

Closed 6 years ago

#16975 closed defect (fixed)

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

Reported by: Patrick Ruzand Owned by: Douglas Hays
Priority: blocker Milestone: 1.9
Component: HTML Version: 1.9.0b2
Keywords: Cc: Eugene Lazutkin
Blocked By: Blocking:

Description

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.

Attachments (2)

destroy_param.html (1.1 KB) - added by Patrick Ruzand 6 years ago.
testcase
16975.patch (6.1 KB) - added by Douglas Hays 6 years ago.
possible fix for review

Download all attachments as: .zip

Change History (15)

Changed 6 years ago by Patrick Ruzand

Attachment: destroy_param.html added

testcase

comment:1 Changed 6 years ago by Patrick Ruzand

Owner: changed from Eugene Lazutkin to Douglas Hays
Status: newassigned

comment:2 Changed 6 years ago by Patrick Ruzand

Cc: Eugene Lazutkin added

comment:3 Changed 6 years ago by bill

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

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

comment:4 Changed 6 years ago by Douglas Hays

Priority: undecidedblocker

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

comment:5 Changed 6 years ago by 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.

Changed 6 years ago by Douglas Hays

Attachment: 16975.patch added

possible fix for review

comment:6 Changed 6 years ago by Douglas Hays

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

comment:7 Changed 6 years ago by 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?

comment:8 Changed 6 years ago by Patrick Ruzand

doughays, your patch fixes the issue gfx was facing.

comment:9 Changed 6 years ago by 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.

comment:10 Changed 6 years ago by 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 if fails on some future browser.

Last edited 6 years ago by Douglas Hays (previous) (diff)

comment:11 Changed 6 years ago by 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
        }
}

comment:12 Changed 6 years ago by Douglas Hays

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

comment:13 Changed 6 years ago by Douglas Hays

Resolution: fixed
Status: assignedclosed

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

Note: See TracTickets for help on using tickets.