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

[bernhard.voelker@…]

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';

[Adam Peller]

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

[Adam Peller]

is an id for the thumb needed at all?

[bernhard.voelker@…]

I didn't try, but IMHO no.

[bill]

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.

[bill]

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

[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.