Opened 11 years ago

Closed 11 years ago

#7105 closed defect (fixed)

BorderContainer: destroy() leaks memory when BorderContainer contains a splitter

Reported by: guest Owned by: Adam Peller
Priority: high Milestone: 1.2
Component: Dijit Version: 1.1.1
Keywords: memory leak splitter BorderContainer Cc:
Blocked By: Blocking:

Description

I've created a custom widget that contains a BorderContainer? in the template. Within that template I added two ContentPanes?, top and center, and added a splitter to the top region. When I destroy my custom widget, the BorderContainer? does not clean up the splitter widget. I verified this by using sIEve. To ensure that it was the splitter, I added a destroy function to the BorderContainer?:

destroy: function(){
	for(region in this._splitters){
		var splitter = this._splitters[region];
		dijit.byNode(splitter).destroy();
		delete this._splitters[region];
		delete this._splitterThickness[region];
	}
	this.inherited(arguments);
},

After adding the destroy function, the BorderContainer? no longer showed a memory leak in sIEve.

My custom widget test code is attached.

Attachments (2)

dijit-test.zip (3.3 KB) - added by guest 11 years ago.
custom widget
7105.patch (1.8 KB) - added by Adam Peller 11 years ago.
slight variant on Jake's patch. destroy the splitter node and remove from the widget's children also so it can't get destroyed twice. it should be sufficient to delete the hash objects, even that may be superfluous. will test against Jake's example and check in when I'm awake.

Download all attachments as: .zip

Change History (14)

Changed 11 years ago by guest

Attachment: dijit-test.zip added

custom widget

comment:1 Changed 11 years ago by bill

Owner: set to Adam Peller

comment:2 Changed 11 years ago by Adam Peller

Status: newassigned

thanks, who filed this ticket?

comment:3 Changed 11 years ago by Adam Peller

_splitterThickness has simple arrays, so a single delete at the top level ought to be sufficient.

Bill, if the splitter widgets are all children of the BorderContainer?, why aren't they getting destroyed for us? Who is supposed to call destroyRecursive()?

comment:4 Changed 11 years ago by guest

Hi Peller,

I filled this ticket. (jake - jakegour AT hotmail.com)

comment:5 Changed 11 years ago by bill

Oh, the developer is supposed to call destroyRecursive()... Jake, you aren't doing that?

comment:6 Changed 11 years ago by guest

Hi guys,

I changed my destroy() call to destroyRecursive() and memory is cleaned up as expected. I didn't call this initially because the destroy function comments state that it will:

destroy internal widgets such as those used within a template.

After reviewing the dijit._Widget code I can see that destroyRecursive() is designed to clean up widgets that are created programmatically (like the dijit.layout._Splitter).

Thanks for looking into this,

Jake

comment:7 Changed 11 years ago by bill

Hmm, well now that you mention it, I guess destroy() should also remove those splitters. It seems like a low priority bug though since developers rarely call destroy(). They almost always call destroyRecursive(), because they almost always want to destroy the ContentPanes along with the BorderContainer.

comment:8 Changed 11 years ago by guest

I agree that this is a low priority bug.

From my understanding, widgets within a template are destroyed automatically because they get added to the _supportingWidgets array. I assumed that the BorderContainer? did this as well. If I had created by BorderContainer? programmatically and added ContentPanes? then I would have called destroyRecursive. I guess there is no harm in always calling destroyRecursive(). Then it doesn't matter whether it was created from a template or programmatically.

comment:9 Changed 11 years ago by bill

FYI, the special thing about BorderContainer is that the splitters are inside BorderContainer.containerNode, along with the ContentPanes. Usually supporting widgets are outside of containerNode (often specified as other nodes inside the template).

Changed 11 years ago by Adam Peller

Attachment: 7105.patch added

slight variant on Jake's patch. destroy the splitter node and remove from the widget's children also so it can't get destroyed twice. it should be sufficient to delete the hash objects, even that may be superfluous. will test against Jake's example and check in when I'm awake.

comment:10 Changed 11 years ago by bill

Summary: BorderContainer leaks memory when it contains a splitterBorderContainer: destroy() leaks memory when BorderContainer contains a splitter

See #7137.

comment:11 Changed 11 years ago by bill

Looks good to me; you should check this in.

I fixed getChildren() to skip the splitters (since from an API perspective they aren't children) so now this is more important to check in; otherwise they'll never get destroyed.

comment:12 Changed 11 years ago by Adam Peller

Resolution: fixed
Status: assignedclosed

(In [14391]) Add destroy() to clean up splitters. Fixes #7105 !strict

Note: See TracTickets for help on using tickets.