Opened 8 years ago
Closed 8 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)
Change History (15)
Changed 8 years ago by
Attachment: | destroy_param.html added |
---|
comment:1 Changed 8 years ago by
Owner: | changed from Eugene Lazutkin to Douglas Hays |
---|---|
Status: | new → assigned |
comment:2 Changed 8 years ago by
Cc: | Eugene Lazutkin added |
---|
comment:3 Changed 8 years ago by
Milestone: | tbd → 1.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 8 years ago by
Priority: | undecided → blocker |
---|
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 8 years ago by
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.
comment:7 Changed 8 years ago by
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:9 Changed 8 years ago by
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 8 years ago by
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.
comment:11 Changed 8 years ago by
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 8 years ago by
Thanks bill. I really like your compacted version. I don't see any downside for any browser.
testcase