Opened 7 years ago

Closed 7 years ago

Last modified 6 years ago

#15206 closed defect (fixed)

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

Reported by: nmweiler Owned by: bill
Priority: undecided Milestone: 1.7.4
Component: Dijit - Form Version: 1.7.1
Keywords: Cc: Douglas Hays, Colin Snover
Blocked By: Blocking:

Description

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.

Attachments (3)

DojoMemoryTest171.html (1.6 KB) - added by nmweiler 7 years ago.
Memory Error - Filtering select 1.7.1
DojoMemoryTest172.html (1.6 KB) - added by nmweiler 7 years ago.
Memory Error - Filtering select 1.7.2
DojoMemoryTest.html (1.7 KB) - added by bill 7 years ago.
modified version of test with manual "delete" button, showing that problem exists even if the onunload _destroyAll() call is removed

Download all attachments as: .zip

Change History (20)

Changed 7 years ago by nmweiler

Attachment: DojoMemoryTest171.html added

Memory Error - Filtering select 1.7.1

Changed 7 years ago by nmweiler

Attachment: DojoMemoryTest172.html added

Memory Error - Filtering select 1.7.2

comment:1 Changed 7 years ago by bill

Owner: set to nmweiler
Status: newpending

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.

comment:2 Changed 7 years ago by nmweiler

Status: pendingnew

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.

comment:3 in reply to:  1 Changed 7 years ago by nmweiler

Any updates? Were you able to recreate?

Thanks

comment:4 Changed 7 years ago by bill

Milestone: tbd1.8

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.

Changed 7 years ago by bill

Attachment: DojoMemoryTest.html added

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

comment:5 Changed 7 years ago by bill

Owner: changed from nmweiler to bill
Status: newassigned

comment:6 Changed 7 years ago by bill

Cc: Douglas Hays added
Component: DijitDijit - Form
Summary: Memory Leaks in IE8 - destroy on widget is not releasing widget resources.ComboBox: memory leak on destroy (IE8)

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

Version 0, edited 7 years ago by bill (next)

comment:7 Changed 7 years ago by bill

Resolution: fixed
Status: assignedclosed

(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.)
  1. Avoid closure issues on aspect.after(widget, "destroy", ..) call by putting callback function outside of the function calling aspect.after().
  1. Accessing ComboBoxMenu.containerNode.childNodes was also leaky. I guess it's asking for trouble returning a live collection from a function.
  1. The registry._destroyAll() call doesn't seem to be necessary on page unload, even for IE6, so removing it for now.

Fixes #15206 !strict.

comment:8 Changed 7 years ago by bill

#12484 is a duplicate of this ticket.

comment:9 Changed 7 years ago by bill

In [28640]:

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

comment:10 Changed 7 years ago by bill

Oops, above change ref'd #15216.

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

comment:12 Changed 7 years ago by bill

Cc: Colin Snover added

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.

comment:13 Changed 7 years ago by bill

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

comment:14 Changed 7 years ago by bill

In [29047]:

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

comment:15 Changed 7 years ago by bill

Milestone: 1.81.7.4

comment:16 Changed 6 years ago by bill

In [30670]:

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

comment:17 Changed 6 years ago by bill

In [31400]:

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

Note: See TracTickets for help on using tickets.