#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)
Change History (20)
Changed 9 years ago by
Attachment: | DojoMemoryTest171.html added |
---|
Changed 9 years ago by
Attachment: | DojoMemoryTest172.html added |
---|
Memory Error - Filtering select 1.7.2
comment:1 follow-up: 3 Changed 9 years ago by
Owner: | set to nmweiler |
---|---|
Status: | new → pending |
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 9 years ago by
Status: | pending → new |
---|
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:4 Changed 9 years ago by
Milestone: | tbd → 1.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 9 years ago by
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 9 years ago by
Owner: | changed from nmweiler to bill |
---|---|
Status: | new → assigned |
comment:6 Changed 9 years ago by
Cc: | Douglas Hays added |
---|---|
Component: | Dijit → Dijit - 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 its legion of supporting widgets).
comment:7 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
(In [28551]) Fix ComboBox memory leak on IE8 on page refresh:
- Don't set dojo.store item as an expando property of the drop down list DOMNodes. (DOMNodes pointing to javascript objects causes leaks.)
- Avoid closure issues on aspect.after(widget, "destroy", ..) call by putting callback function outside of the function calling aspect.after().
- Accessing ComboBoxMenu.containerNode.childNodes was also leaky. I guess it's asking for trouble returning a live collection from a function.
- 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:11 Changed 9 years ago by
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 9 years ago by
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 9 years ago by
Summary: | ComboBox: memory leak on destroy (IE8) → [regression] ComboBox: memory leak on destroy (IE8) |
---|
comment:15 Changed 9 years ago by
Milestone: | 1.8 → 1.7.4 |
---|
Memory Error - Filtering select 1.7.1