Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#10813 closed defect (fixed)

crash when deleting SplitContainer's sizer - same id as it's thumb

Reported by: bernhard.voelker@… Owned by: bill
Priority: high Milestone: 1.5
Component: Dijit Version: 1.3.0
Keywords: Cc: bernhard.voelker@…
Blocked By: Blocking:

Description

Problem

My application wants to remove a whole container hierarchy. It does this recursively by looping over all child elements. This fails: IE6 crashes and FF3.6 runs into an endless loop.

The 'cause' is that my recursion tries to continuously remove a dijit_layout_SplitterContainer_Splitter_<num> node.

A quick look into the DOM unveiled that the sizer has the same id attribute as it's child - the thumb.

The place in the code was found very quickly:

	_addSizer: function(index){
		index = index===undefined?this.sizers.length:index;

		// TODO: use a template for this!!!
		var sizer = dojo.doc.createElement('div');
		sizer.id=dijit.getUniqueId('dijit_layout_SplitterContainer_Splitter');
		this.sizers.splice(index,0,sizer);
		this.domNode.appendChild(sizer);

		sizer.className = this.isHorizontal ? 'dijitSplitContainerSizerH' : 'dijitSplitContainerSizerV';

		// add the thumb div
		var thumb = dojo.doc.createElement('div');
		thumb.className = 'thumb';
		thumb.id = sizer.id;
		sizer.appendChild(thumb);

		// FIXME: are you serious? why aren't we using mover start/stop combo?
		this.connect(sizer, "onmousedown", '_onSizerMouseDown');
		
		dojo.setSelectable(sizer, false);
	},

The offending line obviously is:

thumb.id = sizer.id;

This is still the case in trunk, revision 20689: http://trac.dojotoolkit.org/browser/dijit/trunk/layout/SplitContainer.js#L145

Fix

Appending "_thumb" to the thumbs id works:

thumb.id = sizer.id + '_thumb';

Change History (6)

comment:1 Changed 10 years ago by Adam Peller

Component: GeneralDijit
Owner: anonymous deleted

the FIXME comment a few lines below is curious also. there are many unresolved issue with this deprecated widget, but this one appears simple enough. thanks for this information

comment:2 Changed 10 years ago by Adam Peller

is an id for the thumb needed at all?

comment:3 Changed 10 years ago by bernhard.voelker@…

I didn't try, but IMHO no.

comment:4 Changed 10 years ago by bill

Milestone: tbd1.5
Owner: set to bill
Status: newassigned

Yah, looks like the id is unneeded too (the tests work w/out it), so I'll take it out.

(BTW the thumb isn't visible at all in tundra but it is in soria.)

I'm not sure why you have to do manual recursion to destroy the SplitContainer though, destroyRecursive() works for most widgets. Maybe it doesn't work for SplitContainer since that code is deprecated.

comment:5 Changed 10 years ago by bill

Resolution: fixed
Status: assignedclosed

(In [21505]) Remove unneeded id setting on SplitContainer's splitter's thumb, fixes #10813 !strict.

comment:6 in reply to:  4 Changed 10 years ago by bernhard.voelker@…

Replying to bill:

I'm not sure why you have to do manual recursion to destroy the SplitContainer though, destroyRecursive() works for most widgets.

"works for most widgets" sounds nice but is not sufficient. The problems are the nodes which are not supported. We suffered from major memory leaks when we tried that (it was back in 1.1, now we're using 1.3.1), so we had to write our own recursive deletion code.

Thanks for the commit.

Note: See TracTickets for help on using tickets.