Opened 11 years ago

Closed 11 years ago

Last modified 10 years ago

#7784 closed defect (fixed)

ContentPane: manually created widgets inside of ContentPane not destroyed

Reported by: Matt Sgarlata Owned by: bill
Priority: high Milestone: 1.3
Component: Dijit Version: 1.2beta
Keywords: Cc: development-staff@…, josh@…
Blocked By: Blocking:

Description

If you create a widget programmatically that resides inside a ContentPane?, when the contents of the ContentPane? are changed the widget is not destroyed correctly. This leads to memory leaks because the widget is never destroyed. Even calling the destroyDescendants method of the ContentPane? manually doesn't help.

At first I argued to myself that this was a design choice rather than an oversight. After all, you could have references to widgets in JavaScript?, so it wouldn't be fair to destroy the widget if there was still a reference to it somewhere. Now I don't think this is a good argument because when widgets are created with markup they *are* destroyed correctly, and in fact there were lots of commits related to this recently. As previously mentioned, if a widget is created programmatically there might be a reference to it somewhere in JavaScript?, however that is also true for widgets created with markup (just takes a call to dijit.byId to create the reference).

Considering I wrestled with this myself, I understand this could be somewhat controversial, but I strongly believe there should be a consistent approach for widgets created with markup and widgets created programmatically. Either both should be destroyed when the content pane contents are cleared, or neither should be destroyed. My personal preference is to destroy widgets in both circumstances so someone new to dojo doesn't end up with an app that leaks gobs and gobs of memory. If someone really needs to keep the widgets around longer, then IMO that should be an added configuration option.

Test case attached.

Attachments (5)

dojoBug7784Test.html (599 bytes) - added by Matt Sgarlata 11 years ago.
Test case
dojoBug7784Test2.html (3.2 KB) - added by ben hockey 11 years ago.
2nd test case with parsed and programatic widgets
7784.diff (767 bytes) - added by ben hockey 11 years ago.
_ContentSetter-destroyWidgets.patch (843 bytes) - added by dsnopek 11 years ago.
Adds a destroyWidgets() function to dojo.html._ContentSetter which only destroys widgets but doesn't empty the node.
ContentPane-destroyDescendants.patch (596 bytes) - added by dsnopek 11 years ago.
Modifies dijit.layout.ContentPane? to use the new _ContentSetter.destroyWidgets() function from that patch.

Download all attachments as: .zip

Change History (30)

Changed 11 years ago by Matt Sgarlata

Attachment: dojoBug7784Test.html added

Test case

comment:1 Changed 11 years ago by Matt Sgarlata

In the test case, the Set Content button programatically creates a button widget and puts it inside a content pane. To execute this test case, click on the Set Content button twice. The second time you click, there is a blowup because the widget was not correctly destroyed when the content pane's contents were cleared.

If you create a similar test case with widgets that are defined in markup, the test case does not fail. To do that, you would change the setContent method to something like

function setContent() {
   dijit.byId('contentPane').attr('content', '<div id="bar" dojoType="dijit.form.Button">Bar</div>');
}

comment:2 Changed 11 years ago by Matt Sgarlata

Here is a bit of code that fixes the problem for me. I just put it at the beginning of the _onUnloadHandler in ContentPane?.js. I am pretty sure you aren't going to like this approach, but I thought I would post it anyway ;) If you do like it, there is already a CCLA or whatever available for the company I work for, Spider Strategies.

dojo.query(" [widgetId]", this.containerNode).forEach(function(widgetNode) {
	var widget = dijit.byId(widgetNode.id);
	if (widget) {
		widget.destroyRecursive();
	}
});

comment:3 Changed 11 years ago by bill

severity: majornormal
Summary: Memory leak when programmatically created widget inside ContentPaneContentPane: manually created widgets inside of ContentPane not destroyed

Hmm, yes, this won't work. ContentPane keeps track of which widgets it contains, so if you add a widget manually to ContentPane.domNode, w/out ContentPane's knowledge, then ContentPane won't destroy it.

I think this is just the way our architecture works. Defining ContentPane.destroyDescendants() to just call !Widget. destroyDescendants() would fix the problem although then it wouldn't destroy things like Menu's and Dialogs, since their dom nodes are moved to be directly under <body>.

comment:4 Changed 11 years ago by Matt Sgarlata

IMO there is a big difference between a widget that lives inside a ContentPane? and one that does not. A menu widget doesn't exist in a ContentPane? because really that doesn't make any sense because the menu can extend outside the boundaries of the ContentPane?. A button widget that lives inside a ContentPane? is IMO not semantically different depending on whether it's created programmatically or with markup. It's not like I am using some sneaky back way to create the widget here, I am using the widget's constructor. Since ContentPanes? are part of the dijit core, I would expect that a widget created using its constructor would realize its inside a ContentPane? and register itself somehow so that it is destroyed correctly. I consider this a bare minimum, for a programmatically created widget to be given the same considerate destruction as a widget defined with markup.

That said, you do point out another case that I had not considered where it would be advantageous to tie the destruction of a widget (menu, for example) whose div originally resided inside a ContentPane? to the destruction of the contents of the pane. This is a broader case than the one I outlined and I fully support fixing it. Again, in a case like this I don't think it's unreasonable for the widget's constructor to take a look at all its ancestors via the DOM node's parentNode attribute and register itself for destruction with the ContentPane?. However, this may be an edge case. Also I could see an argument that destruction in this way is too aggressive. On the other hand I could see dojo books in the future instructing people to always put their menus inside ContentPanes? so that they are destroyed appropriately.

BTW, I admit a programmatically created button is a kind of silly edge case, but consider a programmatically created tree. That IMO is not at all an edge case.

comment:5 Changed 11 years ago by Matt Sgarlata

Poking around in _Widget it looks like there is a member variable this._supportingWidgets. It seems like whenever a widget is created, it should check to see if it is a child (in the DOM sense) of any other widgets, and if so register itself with that _supportingWidgets variable.

This wouldn't be such a big deal if dojo itself didn't keep references to widgets after they were created in a giant associative array. If dojo didn't do that, then the widgets would be free to be garbage collected by the browser so long as there were no JavaScript? references to them anymore. IMO because dojo is maintaing references to widgets, that also makes it responsible for making sure it is not retaining them past their natural lives.

comment:6 Changed 11 years ago by Les

This defect clearly is a regression introduced in release 1.2. It works fine in release 1.1.1, but it fails in 1.2. If possible, please change the milestone from tbd to 1.2.1.

I'm not able to upgrade to 1.2 because of this regression. If this issue cannot be resolved for 1.2.1, is there a plan to address it in release 1.3?

comment:7 Changed 11 years ago by Adam Peller

Owner: set to Sam Foster

comment:8 Changed 11 years ago by jstein

Has there been any progress on this defect? It looks like this ticket is related to #7885.

comment:9 Changed 11 years ago by ben hockey

it seems that the cause of this problem is an incorrect assumption made in destroyDescendants() on line 399 of dijit/layout/ContentPane.js - including the comment above that line...

    // calling empty destroys all child widgets as well as emptying the containerNode
    setter.empty();

the truth here is that setter.empty() only destroys all child _parsed_ widgets. if the widgets have not been parsed then they will not be destroyed. this explains why there is a different behavior for parsed widgets and programatic widgets.

the question is... what is the purpose of using setter.empty()?

i tried replacing

var setter = this._contentSetter; 
if(setter){
	// calling empty destroys all child widgets as well as emptying the containerNode
	setter.empty();
}else{
	this.inherited(arguments);
	dojo.html._emptyNode(this.containerNode);
}

with

this.inherited(arguments);
dojo.html._emptyNode(this.containerNode

and in the test case provided it works but since i don't know why setter.empty() seemed necessary then i'm not certain this takes everything into consideration. perhaps someone better qualified can comment but this definitely explains the root cause of the reason for different behaviors between parsed and programatic widgets.

comment:10 Changed 11 years ago by Josh Trutwin

Hmm, looking at dojo/html.js it looks like empty() should always call dojo.html._emptyNode() when it's done with the parseResults array, maybe the problem is that the node isn't being provided to the empty() function though so _emptyNode() essentially becomes a no-op.

Can you try:

var setter = this._contentSetter; 
if(setter){
	// calling empty destroys all child widgets as well as emptying the containerNode
	setter.empty(this.containerNode);
}else{
	this.inherited(arguments);
	dojo.html._emptyNode(this.containerNode);
}

(note, added this.containerNode as param to empty)

Josh

comment:11 in reply to:  10 Changed 11 years ago by ben hockey

Replying to trutwijd:

Hmm, looking at dojo/html.js it looks like empty() should always call dojo.html._emptyNode() when it's done with the parseResults array, maybe the problem is that the node isn't being provided to the empty() function though so _emptyNode() essentially becomes a no-op.

when setter.empty() is called, the node is emptied fine via dojo.html._emptyNode(this.node). the problem is that emptying the node is not enough on its own to destroy widgets. so, the contentSetter uses parseResults to keep track of which widgets have been parsed and destroys those widgets when empty() is called but contentSetter is not aware of any programatic widgets and so those still exist after a call to setter.empty().

if there is no dependency on setter.empty() then i would suggest that my solution above should be sufficient. however, if there is some dependency on setter.empty() then the code below works with this test case

var setter = this._contentSetter; 
if(setter){
	this.inherited(arguments);
	// calling empty destroys all child widgets as well as emptying the containerNode
	setter.empty();
}else{
	this.inherited(arguments);
	dojo.html._emptyNode(this.containerNode);
}

this.inherited(arguments) eventually finds it's children through dojo.query('[widgetId]') and since setter.empty() will empty the contents of the node, this.inherited(arguments) must be called before setter.empty(). it would still need to be determined if this has any consequence on setter.empty().

comment:12 Changed 11 years ago by Matt Sgarlata

Does this take care of the case that Bill pointed out, where "Menu's and Dialogs... [have] their dom nodes... moved to be directly under <body>"?

comment:13 Changed 11 years ago by ben hockey

Bill's comment still stands. widgets that have their dom nodes under <body> won't be destroyed unless ContentPane? knows about them through the parseResults array, ie the widgets were parsed. since you are creating the widgets programatically then i would suggest that you connect to a widget inside your ContentPane? through something like this

        dojo.connect(widgetInCP, 'destroy', null, function(){
            contextMenu.destroyRecursive();
            messageDialog.destroyRecursive();
        }); 

Changed 11 years ago by ben hockey

Attachment: dojoBug7784Test2.html added

2nd test case with parsed and programatic widgets

Changed 11 years ago by ben hockey

Attachment: 7784.diff added

comment:14 Changed 11 years ago by ben hockey

i've added another test case and a solution. the code works with both test cases and passes the unit tests.

comment:15 Changed 11 years ago by ben hockey

after looking at this further, so far, i have found 3 ways that a child can be added to a contentPane

  1. parsed from markup by dojo.parser
  2. parsed as through the contentPane's contentSetter via href or attf('content',...)
  3. added programmatically somehow

in the first case, the contentSetter is not created and so with the current code these widgets are destroyed via the call to this.inherited.

in the 2nd case, the contentSetter is created and knows about these widgets through the parseResults array.

in the 3rd case, the contentSetter is not created and these widgets are destroyed in the same way as the first case.

currently, any time the 2nd case co-exists with either of the other 2 cases, only the widgets created via the 2nd method are destroyed. this is the cause of the current problem.

the solution i proposed above takes this into consideration but ends up trying to destroy some of the widgets multiple times - see #6954. once #6954 is fixed then it may be possible that the final solution to this bug is something more like this:

var setter = this._contentSetter; 
if(setter){
	delete setter.parseResults;
}
this.inherited(arguments);
dojo.html._emptyNode(this.containerNode);

i'm currently trying to work on a test case to add to dijit/tests/layout/ContentPane.html but find myself bouncing back and forth between a number of problems relating to this ticket and #6954

comment:16 Changed 11 years ago by bill

Well, I think we'd be better off calling setter.empty() first and then calling this.inherited(); or dojo.html._emptyNode() to pick up an "stragglers" such as the manually created widgets (the subject of this ticket). That would get both Menus and manually created widgets w/o fear of double-deletion.

The only downside would be the slower performance?

comment:17 Changed 11 years ago by ben hockey

the performance is not so much the issue. setter.empty() already calls dojo.html._emptyNode() and since getDescendants() depends on the content of the node then once the node is empty a call to this.inherited() is not going to pick up the stragglers and you will be left with "manually created widgets inside of ContentPane? not destroyed" since getDescendants() can no longer find them.

currently, the only way to get the manually created widgets is via this.inherited() and that is why it must be called before any call (directly or indirectly) to dojo.html._emptyNode(). also, calling this.inherited() will destroy the widgets created via the setter, so the only cleanup needed for the setter after that is to delete setter.parseResults. note that a call to setter.empty() after a call to this.inherited() would result in double-deletion of some widgets. all of these considerations led me to the code in my previous comment.

comment:18 Changed 11 years ago by dsnopek

As an alternative, we could add a function to dojo.html._ContentSetter that only destroys the widgets but doesn't empty the node. Then we could do like bill suggests, where we have the _ContentSetter destroy its widgets, then do this.inherited() and finally dojo.html._emptyNode().

I'll attach a patch to dojo base that adds this function to _ContentSetter as well as a patch to dojo.layout.ContentPane? that uses it. If a different approach is desired, let me know, and I'll hack-up a new set of patches.

Changed 11 years ago by dsnopek

Adds a destroyWidgets() function to dojo.html._ContentSetter which only destroys widgets but doesn't empty the node.

Changed 11 years ago by dsnopek

Modifies dijit.layout.ContentPane? to use the new _ContentSetter.destroyWidgets() function from that patch.

comment:19 Changed 11 years ago by bill

Cc: josh@… added

comment:20 Changed 11 years ago by bill

Hi neonstalwart,

You are right that calling destroyDescendants() after empty() doesn't make any sense, because it's too late.

However, calling delete setter.parseResults; will leave "stragglers" like Menus, since this.inherited() won't find them.

I'm still not sure this is a bug. Note that ContentPane *will* destroy widgets created programatically as long as they are passed into ContentPane's constructor, or to a call to attr('content', ...). The issue is when widgets are directly inserted as children of ContentPane's dom node, bypassing ContentPane's API.

On the other hand, the 1.1 ContentPane did destroy those widgets.

comment:21 Changed 11 years ago by bill

Milestone: tbd1.3
Owner: changed from Sam Foster to bill
Status: newassigned

comment:22 Changed 11 years ago by bill

Resolution: fixed
Status: assignedclosed

Fixed in [15610]: Add code to ContentPane so it will destroy all widgets inside it's containerNode, even if they were inserted manually by the user. As before, ContentPane will also track things like Menus, even though the domNode is moved to <body>, provided that the Menu was added to the ContentPane via an href or an attr('content', ...).

Also fixed problem where nested ContentPanes wouldn't destroy properly because the outer ContentPane was calling destroy() on the inner ContentPane, and it's contents weren't being destroyed.

Finally, fixed issue with widgetsInTemplates getting destroyed twice by taking advantage of new getDescendants(true) API.

Fixes #7706, #7784, #7823. !strict

Patch is in conjunction with [15607]

comment:23 Changed 11 years ago by Matt Sgarlata

Resolution: fixed
Status: closedreopened

I just re-ran this test and I am not seeing this fix in dojo 1.2.2

comment:24 Changed 11 years ago by bill

Resolution: fixed
Status: reopenedclosed

It's in 1.3. (The fixed version is listed in the "Milestone" field at the top of the ticket.)

comment:25 Changed 10 years ago by bill

(In [16904]) 1. Refactor [15607] (refs #6954, #7550, #7706, #7784, #7823) so that getDescendants() works as in 1.2 again, returning all descendant widgets, even nested widgets that are defined in templates. Expose the new functionality from [15607], to find direct descendants only, into a new _Widget.getChildren() method, rather than as flag to getDescendants().

This is because:

  • [15607] gave getDescendants() a subtle API change: it no longer found widgets inside of a dijit.Declaration (since no containerNode was defined)
  • because of that, Form was broken in that it didn't find form widgets inside of a custom widgets, declared with dijit.Declaration or dojo.declare, which didn't define this.containerNode
  1. Also, rolling back #7819: ContentPane?: missing an addChild() method (refs #7819), because of the backwards compatibility issue for custom widgets that extend ContentPane? and also extend _Container/_Contained.

Summary:

  • getChildren() is now supported by all widgets, and _Container widgets have (as before) an optimized implementation of getChildren() that overrides the one in _Widget.
  • ContentPane? not supporting any _Container methods except getChildren(). Users can set a ContentPane? child by calling attr('value', ...) etc.

!strict

Note: See TracTickets for help on using tickets.