Opened 11 years ago

Closed 11 years ago

#7709 closed defect (fixed)

ContentPane: destroying widgets twice

Reported by: hotheartboy Owned by: bill
Priority: high Milestone: 1.2
Component: Dijit Version: 1.2beta
Keywords: dijit.layout.ContentPan destory Cc: bill
Blocked By: Blocking:

Description (last modified by bill)

Sorry.I should tell that it happened in dijit.layout.ContentPane.

The code in dijit.layout.ContentPane._setContent():

setter = this._contentSetter = new dojo.html._ContentSetter();
setter.set( (dojo.isObject(cont) && cont.domNode) ? cont.domNode : cont );

the setter will parse widgets in the content and fill these widgets instances into the parseResults variable.

The code in dojo.html._ContentSetter._parse():

this.parseResults = dojo.parser.parse(rootNode, true);

The code in dojo.html._ContentSetter.empty():

dojo.forEach(this.parseResults, function(w) {

It causes that the ContentPane widget will call the included widgets's destroy() twice.One happened in the destroyDescendants() of the dijit._Widgets. The other one happened in the empty() of the dojo.html.

Thank you.

Change History (10)

comment:1 Changed 11 years ago by bill

Description: modified (diff)
Summary: The wrong about dijit.layout.ContentPanContentPane: destroying widgets twice

I'm confused... does the problem happen when the user calls ContentPane.setContent() because ContentPane.setContent() calls destroyRecursive() and then the forEach() in in dojo.html._ContentSetter.empty() executes?

comment:2 Changed 11 years ago by bill

Owner: set to Sam Foster

comment:3 in reply to:  2 Changed 11 years ago by hotheartboy

Replying to bill:

For example,there are two widgets named A and B. A includes B.When the widget A is destroyed,it calls destroyRecursive() then the destroy() in the widget B executes.

After the destroyRecursive() in the widget A had executed ,the widget A calls its destroy().The destroy() will call dojo.html._ContentSetter.empty().the empty() will call the destroy() of the widget B.But the destroy() of the widget B had executed in the destroyRecursive() of the widget A.So the destroy() of the widget B executes twice.

Thak you!

comment:4 Changed 11 years ago by bill

I think the fix here is just:

  1. _setContent() should only call destroyRecursive if this._contentSetter isn't defined... otherwise let this._contentSetter handle it
  1. ContentPane.destroyRecursive() should branch depending on whether or not this._contentSetter is defined. Call either destroyDescendants() or this._contentSetter.empty().

Was hoping for sfoster to double check / implement it though.

comment:5 Changed 11 years ago by bill

Owner: changed from Sam Foster to bill
Status: newassigned

OK I'm going to check in the above fix. hotheartboy if you can test the fix I'm about to checkin that would be good.

comment:6 Changed 11 years ago by bill

Resolution: fixed
Status: assignedclosed

(In [15332]) Fixes #7709: ContentPane? was destroying child widgets twice after the initial attr('content', ...) call, since destroyDescendants() was being called and dojo.html._ContentSetter was also destroying the widgets. !strict

comment:7 Changed 11 years ago by hotheartboy

Resolution: fixed
Status: closedreopened

Srroy!the wrong still exists.

please go to #7727

comment:8 Changed 11 years ago by bill

Also, can you attach a small test case (using the "attach file" button) to this ticket or #7727?

comment:9 in reply to:  8 Changed 11 years ago by hotheartboy

Replying to bill:

I'm sorry. I recheck the code and found "delete this.parseResults" in dojo.html.empty(). So you are right and it becomes fine.

Thank you very much!

comment:10 Changed 11 years ago by bill

Resolution: fixed
Status: reopenedclosed

OK cool. I'll close this ticket then (since widgets aren't being destroyed twice) but leave #7727 open since technically delete() is behaving incorrectly. Thanks for pointing out these bugs, and let us know if there's anything else wrong.

Also, note that if you sign a CLA you can contribute patches directly.

Note: See TracTickets for help on using tickets.