[DynAPI-notify] [ dynapi-Bugs-787137 ] Memory leak in DynElement.deleteChild()

[SourceForge.net <no...@so...>]

Bugs item #787137, was opened at 2003-08-11 20:36
Message generated for change (Comment added) made by doug_melvin
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=105757&aid=787137&group_id=5757

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: DynAPI 3 API
Group: None
Status: Open
Resolution: None
Priority: 5
Submitted By: Andrew Gillett (agillett)
Assigned to: Nobody/Anonymous (nobody)
Summary: Memory leak in DynElement.deleteChild()

Initial Comment:
The DynElement.deleteChild() function is implemented 
thus:

p.deleteChild = function(c) {
	c.removeFromParent();
   	c._delete();
};

Although the _delete() function is documented in 
quickref.dynelement.html, it is not implemented 
_anywhere_ in the DynAPI source code, except for 
being set to dynapi.functions.Null in event.js.

If you have a page that repeatedly adds and removes 
DynElements, it will leak memory.  If you replace the call
to _delete() with a call to _destroy(), the memory leak 
goes away:

p.deleteChild = function(c) {
	c.removeFromParent();
   	c._destroy();
};


----------------------------------------------------------------------

Comment By: Doug Melvin (doug_melvin)
Date: 2005-07-29 12:17

Message:
Logged In: YES 
user_id=184788

Confirmed: _delete() is depecriated code.
The correct call is _destroy()

Please implement in cvs copy and and close this bug.

Thanks (I'll check if i have cvs write access this evening)

The change:
Line 159 of event.js:
change from: c._delete();
to: c._destroy();

----------------------------------------------------------------------

Comment By: Raymond Irving (xwisdom)
Date: 2003-08-14 17:15

Message:
Logged In: YES 
user_id=696242

I believe the lastest snapshot should have fixed this problem. 
I did add _destroy() to the deleteChild() function and made 
some other changes that fixed the problem. 

Keep up the good work.


----------------------------------------------------------------------

Comment By: Andrew Gillett (agillett)
Date: 2003-08-14 16:48

Message:
Logged In: YES 
user_id=134108

Hmm, we could have: remove, delete, destroy and dispose.

> I see what's happening. IMO deleteChild() and removeChild
> means the same thing, correct?
No. To my way of thinking, 'delete' should be like the C++ 
keyword - once deleted an object no longer exists.  And why 
have both deleteChild and removeChild if they do the same 
thing?

IMO we should just add the _destroy() function to deleteChild 
and update the documentation accordingly.


----------------------------------------------------------------------

Comment By: Raymond Irving (xwisdom)
Date: 2003-08-14 06:02

Message:
Logged In: YES 
user_id=696242


Thanks for the feedback

Ok. Will keep the delete functions them _destroy() the layers 
as well.


----------------------------------------------------------------------

Comment By: L W (warp9pnt9)
Date: 2003-08-14 05:30

Message:
Logged In: YES 
user_id=706287

My view on the matter.

I think the naming convention would be clearer if the
function that removes children from parents was called
removeChild / removeFromParent, but not destroying the
layer.  I think this is how it works now.  I think the
function that removes the child from the parent AND destroys
the layer should be called deleteChild / deleteFromParent or
maybe destroyChild / destroyFromParent.  I think it's
confusing to add a new name "dispose" when it doesn't
intuitively correspond to an underlying "dispose" function.
 It's a little vague.

----------------------------------------------------------------------

Comment By: Raymond Irving (xwisdom)
Date: 2003-08-13 19:37

Message:
Logged In: YES 
user_id=696242


I see what's happening. IMO deleteChild() and removeChild 
means the same thing, correct? 

When a child layer is removed from the parent it's actually 
deleted from the parent, correct?  The layer is not completly 
destroyed as a reference to it still exists within DynObject.all 
collection. 

Calling _destroy() during a deleteChild() function will not only 
delete the child from its parent but it will also dispose of the 
child object.

I think we should change deleteChild/AllChildren/etc to 
disposeChild/AllChildren/etc wihich will clearly define what is 
been done to the object. The dispose functions will complete 
remove and destroy the object.

Are we all in agreement with changing delete to dispose? Or 
should we add the _destroy() function to deleteChild and 
keep the delete functions?  

(Note: If we keep the delete functions we'll have to 
document that it will completely destroy and remove the layer 
from the system.)



----------------------------------------------------------------------

Comment By: Andrew Gillett (agillett)
Date: 2003-08-13 17:31

Message:
Logged In: YES 
user_id=134108

> Are you using the lastest snapshot?
Yes, I check out the code directly from CVS.
> What browsers are experiencing this memory leak?
IE6 & Mozilla1.4, but that is not really relevant.

On 29/07/2003 12:26 AM (my time), in response to my post 
about a bug in deleteAllChildren(), you wrote:
>To fix the memory and layer problem use the following:
>
>p.deleteAllChildren = function() {
>	var c,l = this.children.length;
>	for(var i=0;i<l;i++) {
>		c=this.children[i];
>		if(c) {
>			c._destroy();
>			c._created = c.isChild = false;
>		};
>		delete this.children[i];
>	}
>	this.children.length = 0;
>};
>
This fix worked perfectly. The _destroy() method is called on 
each child of the layer.

However this is not what was put into CVS.  In the current 
version of event.js, deleteAllChildren() calls deleteFromParent
() which calls deleteChild() which calls _delete() instead of 
_destroy().

It is the call to _delete() that I have a problem with.  There 
is simply no such function.  In line 184 of event.js there is:

... = p._delete = p._destroy = dynapi.functions.Null;

but nowhere else in the entire source tree is _delete() 
defined.


Try the attached example.  On my Windows XP system, IE 
grows by about 6MB each time the "Memory Leak" button is 
pressed.  But if I replace the call to _delete() with a call to 
_destroy() in the deleteChild function, then IE doesn't leak at 
all.


----------------------------------------------------------------------

Comment By: Raymond Irving (xwisdom)
Date: 2003-08-13 06:53

Message:
Logged In: YES 
user_id=696242

Are you using the lastest snapshot?

What browsers are experiencing this memory leak?

You can get the lastest snapshot here:
http://dynapi.sourceforge.net/snapshot/?N=D


----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=105757&aid=787137&group_id=5757