removeChild leaks in IE

[Michael Schall]

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?

[bill]

very simple change so scheduling for 0.4.1

[Michael Schall]

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.

[Michael Schall]

Example of IE removeChild leak and possible workaround

[Michael Schall]

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);	
	}
}

[Michael Schall]

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);
			}
		}
	});

[Michael Schall]

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);

[bill]

Bryan doesn't have IE. Adam, can you do this one?

[Adam Peller]

(In [6548]) References #1727. Going to do some more tests and chase down some more dom references before closing the ticket

[Michael Schall]

example of iframe leak

[Adam Peller]

[6589] References #1727. Thanks again, schallm. Will merge this all to trunk when complete.

[Adam Peller]

(In [6602]) plug another leak. References #1727

[Adam Peller]

Need to review the rest of the project for leaks. See #1927

[Adam Peller]

(In [6604]) Merge to trunk. Fixes #1727

[Adam Peller]

(In [6611]) Clarify removeNode API to use 'clobber' and return null if node was destroyed. References #1727

[Adam Peller]

(In [6614]) Merge to trunk: Clarify removeNode API to use 'clobber' and return null if node was destroyed. References #1727

[liucougar]

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=''

[liucougar]

(In [6707]) fixes #1727: improved mem leak fix in IE new API introduced: dojo.dom.destroyNode change removeNode to destroyNode in dojo code where appropriate

[Adam Peller]

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.

[Adam Peller]

...and put back Alex's dojo.event.browser.clean call

[Michael Schall]

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.

[liucougar]

(In [6759]) fixes #1727: merged patch from schallm added back dojo.event.browser.clean

[Adam Peller]

(In [6762]) fix typo, add missing ie leak warning. We'll revisit all this in 0.5. References #1727, [6760]

[Tom Trenka]

*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.

[Adam Peller]

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.

[(none)]

Milestone 0.4.1 deleted

[guest]

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.

[bill]

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!