#1727 closed defect (invalid)
removeChild leaks in IE
Reported by: | Michael Schall | Owned by: | liucougar |
---|---|---|---|
Priority: | high | Milestone: | |
Component: | DOM | Version: | 0.4 |
Keywords: | Cc: | ||
Blocked By: | Blocking: |
Description
I have not tested how much is leaked, however according to drip's site
http://outofhanwell.com/ieleak/index.php?title=Fixing_Leaks#Pseudo-Leaks
removeChild will not clean up correctly in IE. Should dojo.dom.removeNode use the suggested code?
Attachments (3)
Change History (30)
comment:1 Changed 16 years ago by
Milestone: | → 0.4.1 |
---|---|
Owner: | changed from anonymous to bill |
comment:2 Changed 16 years ago by
Owner: | changed from bill to Bryan Forbes |
---|
comment:3 Changed 16 years ago by
This is a much larger problem than I thought. I downloaded a better version of drip, sIEve referenced in another ticket from http://home.wanadoo.nl/jsrosman/. With this tool I found that removeChild does not destroy anything that is "removed". It is removed from the document ("orphaned" in sIEve), but still has references, so IE does not garbage collect the nodes. The nodes are collected when moving to a different page, but this is not good for single page apps that create/destroy a lot of widgets/nodes.
This should mean that most DOM "removeChild" calls in dojo "core" are moved to dojo.dom.removeNode, and the function should look something like:
dojo.dom.removeChildren = function(node){ var count = node.childNodes.length; while(node.hasChildNodes()){ dojo.dom.removeNode(node.firstChild); } return count; } dojo.dom.removeNode = function(node){ if(dojo.render.html.ie){ dojo.dom._discardElement(node); return node; } else { if(node && node.parentNode){ // return a ref to the removed child return node.parentNode.removeChild(node); } } } dojo.dom._discardElement = function (element) { var garbageBin = document.getElementById('IELeakGarbageBin'); if (!garbageBin) { garbageBin = document.createElement('DIV'); garbageBin.id = 'IELeakGarbageBin'; garbageBin.style.display = 'none'; document.body.appendChild(garbageBin); } // move the element to the garbage bin garbageBin.appendChild(element); garbageBin.innerHTML = ''; }
This may reference what is seen in #1757. By changing this, I removed many of my leaks. Replacing the removeChild with the new dojo.dom.removeNode in destroyRendering of HtmlWidget? removed of many more leaks. I still have a few to find, and will post more tomorrow. I'll also post an html file that shows the leaks if opened in sIEve.
Changed 16 years ago by
Attachment: | memLeak.htm added |
---|
Example of IE removeChild leak and possible workaround
comment:4 Changed 16 years ago by
I found that replaceChild leaks the same way as removeChild in IE. Adding a dojo.dom.replaceNode implemented as the following fixes the issue.
dojo.dom.replaceNode = function(node, newNode) { if(dojo.render.html.ie){ node.parentNode.insertBefore(newNode, node); return dojo.dom.removeNode(node); } else { return node.parentNode.replaceChild(newNode, node); } }
comment:5 Changed 16 years ago by
The dojo.widget._templateCache also leaks on unload in IE if the js <--> dom references are not cleaned up. I have fixed this with the code block below, but it may be better to do it in dojo.widget.manager.destroyAll or in the anonymous function in dojo.event.browser that calls dojo.widget.manager.destroyAll so an extra listener is not needed.
dojo.event.browser.addListener( window, "onunload", function() { for (var name in dojo.widget._templateCache) { if (dojo.widget._templateCache[name].node) { dojo.dom.removeNode(dojo.widget._templateCache[name].node); dojo.widget._templateCache[name].node = null; delete(dojo.widget._templateCache[name].node); } } });
comment:6 Changed 16 years ago by
Now we need to use the suggested functions deep in the widget system to remove the rest of the leaks that sIEve finds for us.
In the postInitialize function in DomWidget?.js we need to switch
var oldNode = sourceNodeRef.parentNode.replaceChild(this.domNode, sourceNodeRef);
to
dojo.dom.replaceNode(sourceNodeRef, this.domNode)
and in the destroyRendering function in HtmlWidget?.js we need to switch
this.domNode.parentNode.removeChild(this.domNode);
to
dojo.dom.removeNode(this.domNode);
comment:7 Changed 16 years ago by
Owner: | changed from Bryan Forbes to Adam Peller |
---|
Bryan doesn't have IE. Adam, can you do this one?
comment:8 Changed 16 years ago by
comment:9 Changed 16 years ago by
Status: | new → assigned |
---|
comment:12 Changed 16 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
comment:13 Changed 16 years ago by
comment:14 Changed 16 years ago by
comment:15 Changed 15 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
this does not work for nodes not in the main page (such as in an iframe)
what I am proposing is to not use that extra div, but make use of outerHTML (only available in IE)
so we can simplily do this:
element.outerHTML=''
comment:16 Changed 15 years ago by
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
comment:17 Changed 15 years ago by
Cc: | liucougar added |
---|---|
Resolution: | fixed |
Status: | closed → reopened |
We need to make sure the leak count is down to what it was in 0.4.1RC1 and verify with Mike. And before closing I suggest we modify the docs for removeNode and any of the other methods which rely on removeNode that the caller is responsible for calling destroyNode.
comment:18 Changed 15 years ago by
Cc: | liucougar removed |
---|---|
Owner: | changed from Adam Peller to liucougar |
Status: | reopened → new |
comment:20 Changed 15 years ago by
I think I have the leaks back under control... There is still one leak in my sample, but that is probably due to a leak from one of the specific widgets in the sample.
I had to change a few things:
dojo.dom.replaceNode could be reverted back to a simple dom call since removeNode is no longer keeping the array of node to be removed.
domWidget's postInitialize has the change liu or bill suggested
this.replacedNode = dojo.dom.replaceNode(sourceNodeRef, this.domNode);
and is correctly cleaning up what is setup within this file
destroyRendering: function(){ // summary: UI destructor. Destroy the dom nodes associated w/this widget. try{ dojo.dom.destroyNode(this.domNode); delete this.domNode; }catch(e){ /* squelch! */ } if(this.replacedNode){ try{ dojo.dom.destroyNode(this.replacedNode); }catch(e){ /* squelch! */ } } },
HtmlWidget?'s destroyRendering should not have been destroying the node, it should be calling it's super which will clean up the replacedNode as well.
destroyRendering: function(finalize){ try{ if(this.bgIframe){ this.bgIframe.remove(); delete this.bgIframe; } if(!finalize && this.domNode){ dojo.event.browser.clean(this.domNode); } dojo.widget.HtmlWidget.superclass.destroyRendering.call(this); }catch(e){ /* squelch! */ } },
I will attach the patch.
I still think that this route will cause leaks in developers code. Having an array of removed or replaced nodes and cleaning onunload similar to the event cleanup would "protect" the developer. At least with this patch the core to have no known leaks.
Changed 15 years ago by
Attachment: | memLeak.patch added |
---|
comment:21 Changed 15 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:22 Changed 15 years ago by
comment:23 Changed 15 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
*Any* time you are doing something DOM-wise that is specific to a browser, you should be putting said changes in the dojo.html module and NOT the dojo.dom module. dojo.dom is mixed into dojo.html the first time dojo.html is loaded; you'll notice that there are method overrides in dojo.html.common that are HTML-specific.
Please take any changes you've made to dojo.dom and move them into dojo.html.common.
comment:24 Changed 15 years ago by
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
we've gotta close 0.4.1, and there's enough chatter in this ticket already. I'm going to make the refactor a new ticket.
comment:26 Changed 15 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
I'm not sure if I need to open a new ticket for this but I'm still having problems with memory leaks.
Using siEve, When I run this code:
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd"> <html> <head> <meta http-equiv="content-type" content="text/html;charset=UTF-8" /> <title>Testcase Context Menu Memory Leak</title> <script type="text/javascript" src="dojo/dojo.js" ></script> <script language="JavaScript" type="text/javascript"> dojo.require("dojo.event.*"); dojo.hostenv.writeIncludes(); </script> <script language="JavaScript" type="text/javascript"> var amount = 0; function Test() { // create element var element = document.createElement('div'); document.body.appendChild(element); amount++; document.getElementById('amount').innerHTML = amount; // connect test function on onclick event dojo.event.connect(element, 'onclick', this, 'MyOnclickFunction'); // disconnect event dojo.event.disconnect(element, 'onclick', this, 'MyOnclickFunction'); // remove element var node = dojo.dom.removeNode(element); dojo.dom.destroyNode(node); } function StartTest() { task = window.setInterval("Test()", 100); document.getElementById('state').innerHTML = 'Running'; } function StopTest() { window.clearInterval(task); document.getElementById('state').innerHTML = 'Not running'; } function MyOnclickFunction() { return true; } </script> </head> <body style="font-family:Arial;font-size:13px;"> <p> <button name="stressTest" type="button" onclick="StartTest();">Start Test</button> <button name="stressTest" type="button" onclick="StopTest();">Stop Test</button> </p> <p><b>State:</b> <span id="state">Not running</span> / <b>Amount of elements created:</b> <span id="amount">0</span></p> </body> </html>
My memory usage just scrolls up and up. Using 0.4.3 I can see a bunch or orphaned divs. Using .9 the orphaned divs gp away but the memory usage is still climbing.
comment:27 Changed 15 years ago by
Resolution: | → invalid |
---|---|
Status: | reopened → closed |
Please write a testcase against 0.9 (the testcase above calls dojo.dom.destroyNode which isn't even available in 0.9), and then file a new bug, submit it, and then attach the test case to the bug using the "Attach File" button. Thanks!
very simple change so scheduling for 0.4.1