Opened 12 years ago

Closed 8 years ago

#4641 closed defect (wontfix)

Menu: leak on destroy

Reported by: guest Owned by:
Priority: high Milestone: future
Component: Dijit Version: 0.9
Keywords: IE, Menu, Destroy Cc:
Blocked By: Blocking:

Description (last modified by bill)

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.

Attachments (1)

4641.html (948 bytes) - added by bill 12 years ago.
test case for leak

Download all attachments as: .zip

Change History (18)

comment:1 Changed 12 years ago by Sam Foster

Description: modified (diff)
Milestone: 1.0
severity: normalcritical

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.

comment:2 in reply to:  1 ; Changed 12 years ago by 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

comment:3 in reply to:  2 Changed 12 years ago by guest

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

Nicola

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

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

comment:6 Changed 12 years ago by bill

Component: GeneralDijit
Owner: anonymous deleted

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.

comment:7 Changed 12 years ago by Sam Foster

Owner: set to Sam Foster
Status: newassigned

comment:8 Changed 12 years ago by Sam Foster

Status: assignednew

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.

comment:9 Changed 12 years ago by Adam Peller

Summary: Chronic leak in dijit.Menu(destroy won't destroy) in IEChronic leak in dijit.Menu(destroy won't destroy)

comment:10 Changed 12 years ago by bill

Description: modified (diff)

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

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

Changed 12 years ago by bill

Attachment: 4641.html added

test case for leak

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

comment:12 Changed 12 years ago by bill

Milestone: 1.01.1

comment:13 Changed 12 years ago by bill

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

comment:14 Changed 12 years ago by Adam Peller

Milestone: 1.11.2

#5382 was moved to 1.2

comment:15 Changed 11 years ago by bill

Description: modified (diff)
Milestone: 1.2future
Owner: changed from Sam Foster to bill

comment:16 Changed 11 years ago by Sam Foster

Owner: bill deleted

comment:17 Changed 8 years ago by bill

Description: modified (diff)
Resolution: wontfix
Status: newclosed
Summary: Chronic leak in dijit.Menu(destroy won't destroy)Menu: leak on destroy

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.

Note: See TracTickets for help on using tickets.