Opened 11 years ago

Closed 11 years ago

#9252 closed defect (invalid)

supportingWidgets are destroyed using destroy, not destroyRecursive

Reported by: Ben Lowery Owned by: bill
Priority: high Milestone: 1.4
Component: Dijit Version: 1.3.0
Keywords: Cc:
Blocked By: Blocking:

Description

repro at http://blowery.org/js/widget-nesting.html

If you include a _Templated widget that uses widgetsInTemplate in another _Templated widget that uses widgetsInTemplate, the most nested widget will never be destroyed. This is because supportingWidgets are not destroyed using destroyRecursive, but just plain destroy.

Change History (9)

comment:1 Changed 11 years ago by bill

I'm not following what you mean by "supportingWidgets are not destroyed using destroyRecursive, but just plain destroy".

Since destroyRecursive() calls destroy(), what's the problem?

I looked over your test case and one issue w/the test itself is that you do a

var held = new Held({});

manually creating a supportingWidget, but that widget is never added to the _supportingWidgets array. Also, your test case needs a this.inherited() call in your unitialize method although that's probably not related to the problem.

comment:2 Changed 11 years ago by Ben Lowery

The problem is that destroyRecursive calls destroyDescendants, while plain destroy does not. If a widget in supportingWidgets is itself _Templated with widgetsInTemplate, the widgets it holds will never be destroyed.

comment:3 Changed 11 years ago by Ben Lowery

Sorry, that's not super clear either.

The problem is that by calling destroy for each _supportingWidget, each supporting widget never has destroyDescendants called, so any widgets the supporting widgets hold will never be destroyed. The overall graph looks like:

A is _Templated, holds B B is _Templated, holds C

When A is destroyed by calling destroyRecursive, B is destroyed by calling destroy. C's destroyRecursive (or destroy) is never called.

comment:4 Changed 11 years ago by bill

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

OIC, you are just saying that we should call destroyRecursive() on each widget in this._supportingWidgets, rather than destroy().

I think the only time that gets us into trouble is for something like a dojo.data instance, since IIRC it has a destroy() method but no destroyRecursive() method, but that just means that we need to check for a destroy() method if there is no destroyRecursive() method.

Also note that this won't be an issue in 2.0; see #5796.

Anyway, I'll make that change.

comment:5 Changed 11 years ago by bill

Actually, on second thought, I have to object to your description above. You said:

A is _Templated, holds B B is _Templated, holds C

If that were true, then A._supportWidgets contains B, and B._supportingWidgets contains C, right? Therefore A.destroy() calls B.destroy(), and B calls C.destroy(). There's no problem.

It looks like the actual thing happening in your test case is that B is not _Templated, and further, B._supportingWidgets does not contain C. So I'd say that your code is written incorrectly in that B.destroy() should be removing all internal widgets created by B, but it isn't.

comment:6 Changed 11 years ago by Ben Lowery

Right, the case I meant to describe is one in which C is not a _Templated, but does have descendants that are widgets.

So, with your description, what's the point of destroyDescendants if destroy is supposed to get rid of any internal widgets? Won't this lead to double destruction? What's the contract between destroy and destroyDescendants? What are callers supposed to call when tearing down a widget? My understanding was that it was destroyDescendants, but it's started to sound like I'm wrong.

comment:7 Changed 11 years ago by bill

destroyDescendants() is for contained widgets from the user's point of view, for example the stuff inside of this ContentPane:

<div dojoType=dijit.layout.ContentPane>
    <input dojoType=dijit.form.TextBox>
    <span>       <input dojoType=dijit.form.Textarea>     </span>
</div>

... or alternately, widgets the user has added via addChild/removeChild. More technically speaking, it's for widgets inside of containerNode.

I'm not sure markup like that even works in conjunction with widgetsInTemplate, hence the hesitation to change the destruction code.

comment:8 Changed 11 years ago by bill

PS: by "user" I mean "user of the widget", as opposed to "author of the widget".

comment:9 Changed 11 years ago by bill

Resolution: invalid
Status: assignedclosed

OK anyway, I don't think there's a bug here. All the private subwidgets need to be in the _supportingWidgets array, and every widget in that array is destroyed.

If we called destroyRecursive() on the widgets in the _supportingWidgets array that could actually lead to double-destroys, if some of the _supportingWidgets are children of the other _supportingWidgets.

Of course all this will have to change for 2.0 when we combine destroy() and destroyRecursive(), see #5796.

Note: See TracTickets for help on using tickets.