Menu: leak on destroy

[guest]

Easy repeat:

for (var i = 0; i < 10000000; i++) {
    var x = new dijit.Menu();
    x.destroy();
};

Tracked from a larger leak in menu usage; but I figure if the root menu leaks it will cause the others.

Uncertain if it's leaking in firefox, doesn't grow memory at a perceivable rate.

Leak from menus is massive if destroyRecursive isn't used when there are actual children; that may be another subject; but I mention it here only if relevant to tracking down the root cause.

I'm marking this a critical. Unless I'm doing something wrong, this is bad bad bad.

[Sam Foster]

I can confirm this in Firefox. In both the 0.9.0 release and current trunk, if you go to: ​http://archive.dojotoolkit.org/nightly/checkout/dijit/tests/test_Menu.html that has one top-level menu on the page, with a bunch of submenus. The toplevel one is submenu1. So, dijit.byId("submenu1").destroyRecursive() should cleanly unhook and remove everything, but doesnt - elements are left in the dom, and event handlers are left hooked up - right click now yields an error "Object cannot be created in this context"

dijit.byId("submenu1") is now gone, but dijit.byId("submenu2") and its friends are still there - but orphaned.

No clue as to the cause right now. But before you destroy anything, if you try dijit.byId("submenu1").getDescendants() that collection seems correct.

[guest]

Looking at code:
destroyDescendants in _Widget.js must be recursive. I tried a recursive version with getChildren() instead of getDescendants(), but dojo.query("> [widgetId]") (getChildren) returns the same result of dojo.query("[widgetId]") (getDescendants)
Regards,

Nicola

[guest]

Update: dijit.byId("submenu2").domNode isn't child of dijit.byId("submenu1").domNode[[BR]]

Nicola

[bill]

Ah right, all the menus are attached (directly) to document.body, to make positioning easier and also so the menus don't get hidden if they are inside a div with overflow: auto or overflow: hidden. But that appendChild() happens in startup() so it doesn't apply to the original test case, since the original testcase isn't even calling startup()

[guest]

Matt here - I submitted the original 4641 here, and worked with SFoster in identifying the (most likely distinct) pileup of MenuItems? at the root of the DOM.

IMHO both are serious problems; but very most likely distinct issues. The only common theme is that they decimate the viability of DropDownButtons? used dynamically; but that does not necessarily mean they have the same cause.

The latter - the pileup of menu items - has nothing to do with IE. A (more) core contributor should take this bug and either kill it, or put up the "menuitems piling up in DOM" as a distinct critical bug.

[bill]

I can take a look although I probably won't have time to address it for 1.0. If someone else wants to address it for 1.0 that would be good.

[Sam Foster]

Cant promise I can fix this, but I'll own it for now. This really needs to be 2 bugs.. but I'll look into it before splitting them out - maybe fixing one issue wil fix the other.

[bill]

Second issue filed as #4902. Leaving this bug open for original issue on IE of memory leak from

for (var i = 0; i < 10000000; i = i + 1) {
    var x = new dijit.Menu();
    x.destroy();
};

[bill]

test case for leak

[bill]

Seems like there are a number of issues here. One big one is a cleanup problem for the onkeypress event; I filed bug #4908 (with patch) for that.

[bill]

Probably will be fixed once #5382 is fixed, but I'll leave it open to test.

[Adam Peller]

#5382 was moved to 1.2

[bill]

Closing this, it's unlikely we'll workaround this IE bug given the age of this ticket and the fact that IE6 and IE7 are fast becoming extinct. I don't see any issue [anymore] on firefox, and although IE8 has a memory increase too, it's less than on IE6 and IE7.

sIeve isn't showing any memory leaks so I'm not sure what could be done anyway.

See also #5382.