[regression] ComboBox: memory leak on destroy (IE8)

[nmweiler]

Issue in 1.7.x. Works fine in 1.6.1.

Appears that all the child widgets aren't properly being cleaned up in registry._destroyAll method when the dom nodes are getting removed. This causes IE8 to leak memory.

To Recreate-- Recreate with attached code samples. The drop down must be expanded for the memory leak to occur. Each time the drop down is expanded and page is submitted the memory for the IE process will continue to increase.

Additional observations-- Not running the addOnWindowUnload cleanup code in IE8 allows the browser to cleanup the references on it's own. Resulting in no memory leak.

if(has("ie")){
//change to if(dojo.isIE < 8)
		// Only run _destroyAll() for IE because we think it's only necessary in that case,
		// and because it causes problems on FF.  See bug #3531 for details.
		unload.addOnWindowUnload(function(){
			registry._destroyAll();
		});
	}

Problem was recreated on multiple WinXP machines with IE8 as well as a Windows 7 machine with IE8.

[nmweiler]

Memory Error - Filtering select 1.7.1

[nmweiler]

Memory Error - Filtering select 1.7.2

[bill]

Hmm, I can take a look. You said:

Appears that all the child widgets aren't properly being cleaned up in registry._destroyAll method when the dom nodes are getting removed.

What was your evidence for that?

Also, how were you testing IE's memory usage?

Thanks.

[nmweiler]

In this example, the memory leak appears to manifest itself only when the popup is created for the filtering select/combo box.

I believe I've further tracked the issue down to this code. When the dom node cleanup executes.

exports.destroy = function destroy(/*DOMNode|String*/node){
	node = dom.byId(node);
	try{
		var doc = node.ownerDocument;
		// cannot use _destroyContainer.ownerDocument since this can throw an exception on IE
		if(!_destroyContainer || _destroyDoc != doc){
			_destroyContainer = doc.createElement("div");
			_destroyDoc = doc;
		}
		_destroyContainer.appendChild(node.parentNode ? node.parentNode.removeChild(node) : node);
		// NOTE: see http://trac.dojotoolkit.org/ticket/2931. This may be a bug and not a feature
		_destroyContainer.innerHTML = "";
}catch(e){
		/* squelch */
	}
};

I verified IE's memory usage using task manager and also in Sysinternals Process Explorer. The memory consumed by the IE process just keeps growing with each execution of the code and is never released in IE8 as long as the browser remains open. (note: ie6 and 7 haven't been tested, memory is released in IE9). Using the attached examples, the test machines ie process starts at around 30MB. It increases by ~10-15MB with each execution, never releasing the memory. I've had this example use gigabytes of memory. Refreshing the browser or visiting a new page, does not not reduce the memory held by the IE process when this occurs.

Commenting this code out seems to allow the browser to cleanup and release the memory without issue on it's own, pretty much maintaining the original startup 30MB of memory. Running the example retrofitted to Dojo 1.61, also maintains memory at around the 30MB mark.

As IE 8's debugging tools are not the most helpful. This next part is an assumption. Since this only occurs when the popup widget is created and the dom cleanup code seems to be causing it. My assumption is that the popup widget isn't being properly destroyed before its dom node is destroyed. Or perhaps the destruction is not occurring in the correct order.

[nmweiler]

Any updates? Were you able to recreate?

Thanks

[bill]

Thanks for the report. I was able to recreate the leak using your test case and taskmgr, and confirmed your result that on IE8 the _destroyAll() is hurting rather than helping. Same results for themeTester.html. And according to [10043], the original intention was for the cleanup code to run on IE6 and IE7, so there's no history claiming that it's needed on IE8.

However, surprisingly, my tests show that also for IE6, it doesn't matter if that _destroyAll() call is there or not. So why is the code there at all?

I traced back some of the changes to #10206, and to #3531, which mentions that using sIEve ( ​http://home.wanadoo.nl/jsrosman/) on a box with IE7, you can see objects leaked by loading dijit/bench/benchTool.html. I traced the destroy-on-unload code all the way back to ​http://bugs.dojotoolkit.org/browser/dojo/dijit/trunk/util/manager.js?rev=7162#L77 but not sure where it came from before that.

The other thing is that your suggestion to remove the _destroyAll() call for IE8 seems like a workaround to a problem with calling FilteringSelect.destroy(), or more specifically _HasDropDown.destroy(). In other words, removing the destroy-on-unload code will likely leave a problem where the page manually calls destroy() on a widget like FilteringSelect. So probably some other changes need to be made to properly cleanup BackgroundIFrame's when a widget with a popup is destroyed.

[bill]

modified version of test with manual "delete" button, showing that problem exists even if the onunload _destroyAll() call is removed

[bill]

Tuns out most of the issues are in ComoBox (and its legion of supporting widgets).

[bill]

(In [28551]) Fix ComboBox memory leak on IE8 on page refresh:

1. Don't set dojo.store item as an expando property of the drop down list DOMNodes. (DOMNodes pointing to javascript objects causes leaks.)

2. Avoid closure issues on aspect.after(widget, "destroy", ..) call by putting callback function outside of the function calling aspect.after().

3. Accessing ComboBoxMenu.containerNode.childNodes was also leaky. I guess it's asking for trouble returning a live collection from a function.

4. The registry._destroyAll() call doesn't seem to be necessary on page unload, even for IE6, so removing it for now.

Fixes #15206 !strict.

[bill]

#12484 is a duplicate of this ticket.

[bill]

In [28640]:

fix typos from [28401], thanks Sudoh-san, refs #15206 !strict.

[bill]

Oops, above change ref'd #15216.

[Douglas Hays]

bill, can this be backported to 1.7.3 ? It seems to be a fairly widespread problem. I think parts of the fix should also apply to 1.6.

[bill]

The problem is that we are already in RC for 1.7.3 (so we aren't supposed to add any fixes except for newly found regressions), and also that I don't have any time to test a backport.

[bill]

In [29047]:

Backport ComboBox memory leak fix [28551] to 1.7 branch, refs #15206 !strict.

[bill]

In [30670]:

remove no longer used dojo/_base/unload dependency, refs #15206

[bill]

In [31400]:

remove no-longer used dependency, refs #15206 !strict