Opened 13 years ago

Closed 12 years ago

Last modified 10 years ago

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

memLeak.htm (1.2 KB) - added by Michael Schall 13 years ago.
Example of IE removeChild leak and possible workaround
dojoleak.zip (905 bytes) - added by Michael Schall 13 years ago.
example of iframe leak
memLeak.patch (2.1 KB) - added by Michael Schall 13 years ago.

Download all attachments as: .zip

Change History (30)

comment:1 Changed 13 years ago by bill

Milestone: 0.4.1
Owner: changed from anonymous to bill

very simple change so scheduling for 0.4.1

comment:2 Changed 13 years ago by bill

Owner: changed from bill to Bryan Forbes

comment:3 Changed 13 years ago by 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.

Changed 13 years ago by Michael Schall

Attachment: memLeak.htm added

Example of IE removeChild leak and possible workaround

comment:4 Changed 13 years ago by 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);	
	}
}

comment:5 Changed 13 years ago by 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);
			}
		}
	});

comment:6 Changed 13 years ago by 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);

comment:7 Changed 13 years ago by bill

Owner: changed from Bryan Forbes to Adam Peller

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

comment:8 Changed 13 years ago by Adam Peller

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

Changed 13 years ago by Michael Schall

Attachment: dojoleak.zip added

example of iframe leak

comment:9 Changed 13 years ago by Adam Peller

Status: newassigned

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

comment:10 Changed 13 years ago by Adam Peller

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

comment:11 Changed 13 years ago by Adam Peller

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

comment:12 Changed 13 years ago by Adam Peller

Resolution: fixed
Status: assignedclosed

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

comment:13 Changed 13 years ago by Adam Peller

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

comment:14 Changed 13 years ago by Adam Peller

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

comment:15 Changed 13 years ago by liucougar

Resolution: fixed
Status: closedreopened

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 13 years ago by liucougar

Resolution: fixed
Status: reopenedclosed

(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

comment:17 Changed 13 years ago by Adam Peller

Cc: liucougar added
Resolution: fixed
Status: closedreopened

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 13 years ago by Adam Peller

Cc: liucougar removed
Owner: changed from Adam Peller to liucougar
Status: reopenednew

comment:19 Changed 13 years ago by Adam Peller

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

comment:20 Changed 13 years ago by 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.

Changed 13 years ago by Michael Schall

Attachment: memLeak.patch added

comment:21 Changed 13 years ago by liucougar

Resolution: fixed
Status: newclosed

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

comment:22 Changed 13 years ago by Adam Peller

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

comment:23 Changed 13 years ago by Tom Trenka

Resolution: fixed
Status: closedreopened

*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 13 years ago by Adam Peller

Resolution: fixed
Status: reopenedclosed

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:25 Changed 12 years ago by (none)

Milestone: 0.4.1

Milestone 0.4.1 deleted

comment:26 Changed 12 years ago by guest

Resolution: fixed
Status: closedreopened

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 12 years ago by bill

Resolution: invalid
Status: reopenedclosed

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!

Note: See TracTickets for help on using tickets.